Re: [PATCH RFC] clk: Introduce userspace clock driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Fri, May 10, 2013 at 10:31:31AM -0700, Soren Brinkmann wrote:

> +Example:
> +	usclk: usclk {
> +		compatible = "clk-userspace";
> +		clocks = <&foo 15>, <&bar>;
> +		clock-count = <2>;
> +	};

This is clearly *very* Linux specific so needs to be a linux vendor
thing (everything should have a namespaced name anyway).  It's not at
all obvious to me that this should be in device tree, though, since it's
not hardware description but a detail of how the OS is currently
implemented.

For your use case should these things be exposed by the FPGA device
asking for that rather than by having the clocks available separately?
Or is this part of the DT blob that's loaded incrementally along with
the FPGA (which does make things more interesting of course...).

> + * Expose clock controls through sysfs to userspace.

> + * By writing 0/1 to 'enable' the clock can be disabled/enabled. Reading
> + * that file returns the current state - 0 = disabled, 1 = enabled.

> + * Reading 'set_rate' returns the current clock frequency in Hz. Writing
> + * the file requests setting a new frequency in Hz.

This needs to be covered in Documentation/ABI since it's adding new
sysfs stuff.

> +	if (enable)
> +		ret = clk_prepare_enable(pdata->clk);
> +	else
> +		clk_disable_unprepare(pdata->clk);

> +	if (ret)
> +		return -EBUSY;

Why not pass back the actual error?

> +	pdata = kzalloc(clock_count * sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;

devm?

> +late_initcall(usclk_setup);

Why not just a regular driver?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]