Re: [PATCH v5 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Fri, 24 Jun 2016 13:54:09 +0800
"Wu, Songjun" <songjun.wu@atmel.com> wrote:

> Hi Rob,
> 
> Thank you for your comments.
> 
> On 6/20/2016 21:25, Rob Herring wrote:
> > On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:  
> >> DT binding documentation for ISC driver.
> >>
> >> Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
> >> ---
> >>
> >> Changes in v5:
> >> - Add clock names.
> >>
> >> Changes in v4:
> >> - Remove the isc clock nodes.
> >>
> >> Changes in v3:
> >> - Remove the 'atmel,sensor-preferred'.
> >> - Modify the isc clock node according to the Rob's remarks.
> >>
> >> Changes in v2:
> >> - Remove the unit address of the endpoint.
> >> - Add the unit address to the clock node.
> >> - Avoid using underscores in node names.
> >> - Drop the "0x" in the unit address of the i2c node.
> >> - Modify the description of 'atmel,sensor-preferred'.
> >> - Add the description for the ISC internal clock.
> >>
> >>  .../devicetree/bindings/media/atmel-isc.txt        | 64 ++++++++++++++++++++++
> >>  1 file changed, 64 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> new file mode 100644
> >> index 0000000..9558a77
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> @@ -0,0 +1,64 @@
> >> +Atmel Image Sensor Controller (ISC)
> >> +----------------------------------------------
> >> +
> >> +Required properties for ISC:
> >> +- compatible
> >> +	Must be "atmel,sama5d2-isc".
> >> +- reg
> >> +	Physical base address and length of the registers set for the device.
> >> +- interrupts
> >> +	Should contain IRQ line for the ISC.
> >> +- clocks
> >> +	List of clock specifiers, corresponding to entries in
> >> +	the clock-names property;
> >> +	Please refer to clock-bindings.txt.
> >> +- clock-names
> >> +	Required elements: "hclock".  
> >
> > What about the 2 other clocks in the example?
> >  
> The other clocks is optional, not required.
> Do you have any suggestion?

Just add a second look at the sama5d2 and iscck gck are not optional.
What we call optional clocks are clocks that are not necessarily
required depending on the IP revision or external clocks that are board
dependent. This is not the case here: iscck and gck are internal to the
SoC, and are always available. Whether you want to use them or not is a
different question, and should not be encoded in the clocks/clock-names
properties.

To sum-up, define those 3 clocks as required and document their names.