Re: [PATCH] backlight: arcxcnn: devicetree bindings for ArticSand devices

[Lee Jones <lee.jones@linaro.org>]

On Fri, 09 Sep 2016, Olimpiu Dejeu wrote:

> On Tue, Aug 23, 2016 at 2:20 PM, Rob Herring <robh@kernel.org> wrote:
> 
> > On Mon, Aug 22, 2016 at 03:11:24PM -0400, Olimpiu Dejeu wrote:
> > > This is the device tree bindings documentation file
> > >
> > > Signed-off-by: Olimpiu Dejeu <olimpiu@arcticsand.com>
> > >
> > > ---
> > >  .../bindings/video/backlight/arcxcnn.txt           | 29
> > ++++++++++++++++++++++
> >
> >
> Check your directory location. Things have moved.
> >
> > Not sure what you mean. This seems to be the right place. Please advise.

What kernel version are you on?

Documentation/devicetree/bindings/leds/backlight

> > >  1 file changed, 29 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/video/backlight/
> > arcxcnn.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/video/backlight/arcxcnn.txt
> > b/Documentation/devicetree/bindings/video/backlight/arcxcnn.txt
> > > new file mode 100644
> > > index 0000000..9cd7315
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/video/backlight/arcxcnn.txt
> > > @@ -0,0 +1,29 @@
> > > +arcxcnn bindings
> > > +
> > > +Required properties:
> > > +  - compatible: "arc,arcxcnn", "arc,arc2c0608"
> >
> > One per line please.
> >
> > Is arcxcnn a specific chip? If not drop it. No wildcards in compatible
> > strings.
> >
> > Wild-cards can be dropped but there could be a large family of similar
> chips that would be supported with this one driver. How would one add
> support for a range of chips, some of which don't exist yet?

If they do not exist, then do not mention them in this document.

Every other chip needs to be named individually.

[...]

> > > +  - pwm-period: PWM period value. Set only PWM input mode used (u32)
> > > +  - prg-addr: Register address of ROM area to be updated (u32) low 8
> > bits used
> > > +  - prg-val: Register value to be updated (u32) low 8 bits used
> >
> > What is this for? This should be a specific property or properties to do
> > explicit things, not a generic fill registers/rom with magic values.
> >
> > We need to expose some registers for test without exposing underlying
> functionality. Do you suggest not documenting this in the bindings yet
> keeping the code in the driver? We followed the pattern from the lp855x_bl
> driver.

If you are supplying addresses, use the reg property.

> > > +
> > > +Example:
> > > +
> > > +     /* ARC2C0608 */
> > > +     backlight@30 {
> > > +             compatible = "arc,arc2c0608";
> > > +             reg = <0x30>;
> > > +
> > > +             init-brt = <123>;
> > > +
> > > +             /* LED0+1 string enabled */
> > > +             prg_06h {
> > > +                     prg-addr = <0x06>;
> > > +                     prg-val = <0x83>;
> > > +             };
> > > +
> > > +     };
> > >
> >

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog