Re: [PATCH v2] staging: Add Xilinx Clocking Wizard driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sören,

Thank you for the patch.

On Wednesday 01 October 2014 14:02:32 Soren Brinkmann wrote:
> Add a driver for the Xilinx Clocking Wizard soft IP. The clocking wizard
> provides an AXI interface to dynamically reconfigure the clocking
> resources of Xilinx FPGAs.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
> Hi Greg, Dan,
> 
> I fixed the things Dan pointed out, please take this v2 instead of the
> original patch.
> 
> Also, don't get caught up too much with style issues around the code that
> registers the clocks. To support set_rate() opeartions this will have to
> change anyway. It doesn't make a lot of sense to spend much time on those
> parts.
> 
> 	Thanks,
> 	Sören
> 
> v2:
>  - implement dev_pm_ops
>  - don't use array for clock inputs
>  - add more information in error output
>  - fix style issues
>  - fix error path
> ---
>  drivers/staging/Kconfig                            |   2 +
>  drivers/staging/Makefile                           |   1 +
>  drivers/staging/clocking-wizard/Kconfig            |   9 +
>  drivers/staging/clocking-wizard/Makefile           |   1 +
>  drivers/staging/clocking-wizard/TODO               |  12 +
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 336 ++++++++++++++++++
>  drivers/staging/clocking-wizard/dt-binding.txt     |  30 ++
>  7 files changed, 391 insertions(+)
>  create mode 100644 drivers/staging/clocking-wizard/Kconfig
>  create mode 100644 drivers/staging/clocking-wizard/Makefile
>  create mode 100644 drivers/staging/clocking-wizard/TODO
>  create mode 100644 drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>  create mode 100644 drivers/staging/clocking-wizard/dt-binding.txt

[snip]

> diff --git a/drivers/staging/clocking-wizard/dt-binding.txt
> b/drivers/staging/clocking-wizard/dt-binding.txt new file mode 100644
> index 000000000000..cc3cc5e91440
> --- /dev/null
> +++ b/drivers/staging/clocking-wizard/dt-binding.txt
> @@ -0,0 +1,30 @@
> +Binding for Xilinx Clocking Wizard IP Core
> +
> +This binding uses the common clock binding[1]. Details about the devices
> can be +found in the product guide[2].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Clocking Wizard Product Guide
> +http://www.xilinx.com/support/documentation/ip_documentation/clk_wiz/v5_1/p
> g065-clk-wiz.pdf +
> +Required properties:
> + - compatible: Must be 'xlnx,clocking-wizard'
> + - reg: Base and size of the cores register space
> + - clocks: Handle to input clock
> + - clock-names: Tuple containing 'clk_in1' and 's_axi_aclk'
> + - clock-output-names: Names for the output clocks
> +
> +Optional properties:
> + - speed-grade: Speed grade of the device

You should document what values are allowed.

> +
> +Example:
> +	clock-generator@40040000 {
> +		reg = <0x40040000 0x1000>;
> +		compatible = "xlnx,clocking-wizard";
> +		speed-grade = <(-1)>;

Can't we make the speed grade positive ? It looks to me that the - is nowadays 
a dash and not a minus sign, given that higher absolute values of the speed 
grade mean faster devices. The days when the speed grate represented the 
propagation time of signals in ns is long gone I suppose.

> +		clock-names = "clk_in1", "s_axi_aclk";
> +		clocks = <&clkc 15>, <&clkc 15>;
> +		clock-output-names = "clk_out0", "clk_out1", "clk_out2",
> +				     "clk_out3", "clk_out4", "clk_out5",
> +				     "clk_out6", "clk_out7";
> +	};

-- 
Regards,

Laurent Pinchart