Re: [PATCH v2 1/9] dt-bindings: media: Add Renesas CEU bindings

[Laurent Pinchart]

Hi Jacopo,

On Wednesday, 3 January 2018 10:49:52 EET jacopo mondi wrote:
> On Tue, Jan 02, 2018 at 01:45:30PM +0200, Laurent Pinchart wrote:
> > On Thursday, 28 December 2017 16:01:13 EET Jacopo Mondi wrote:
> >> Add bindings documentation for Renesas Capture Engine Unit (CEU).
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >> ---
> >> 
> >>  .../devicetree/bindings/media/renesas,ceu.txt      | 85 +++++++++++++++
> >>  1 file changed, 85 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/media/renesas,ceu.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt
> >> b/Documentation/devicetree/bindings/media/renesas,ceu.txt new file mode
> >> 100644
> >> index 0000000..f45628e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt
> >> @@ -0,0 +1,85 @@
> >>+Renesas Capture Engine Unit (CEU)
> >> +----------------------------------------------
> >> +
> >> +The Capture Engine Unit is the image capture interface found on Renesas
> >> +RZ chip series and on SH Mobile ones.
> > 
> > "ones" sound a bit weird. How about "... found in the Renesas SH Mobil and
> > RZ SoCs." ?
> > 
> >> +The interface supports a single parallel input with data bus width up
> >> to
> >> +8/16 bits.
> > 
> > What do you mean by "up to 8/16 bits" ?
> 
> The input bus width can be 8 or 16 bit.

Then how about writing it "The CEU supports a single parallel input with a 
data bus width of 8 or 16 bits." ?

> On a general note: I always assumed DT bindings should describe the
> hardware capabilities. In this case the hardware supports 8 or 16 bits
> as input width, but the driver only cares about the 8 bits case. Which
> one should I describe here?

You should describe the hardware.

> I will fix all of yours and Geert's remarks in V3.

[snip]

-- 
Regards,

Laurent Pinchart