Re: [PATCH] Device tree binding for Avago APDS990X light sensor

[Sakari Ailus]

On Wed, Dec 27, 2017 at 07:50:42PM +0100, Filip MatijeviÄ wrote:
> Hi Sakari,
> 
> and thank you for your input - I've added a few comments below.
> 
> On 12/27/2017 07:00 PM, Sakari Ailus wrote:
> > Hi Pavel,
> > 
> > Thanks for the patch. Please see my comments below.
> > 
> > On Wed, Dec 27, 2017 at 10:18:28AM +0100, Pavel Machek wrote:
> >> From: Filip MatijeviÄ <filip.matijevic.pz@xxxxxxxxx>
> >>
> >> This prepares binding for light sensor used in Nokia N9. 
> >>
> >> Signed-off-by: Filip MatijeviÄ <filip.matijevic.pz@xxxxxxxxx>
> >> Signed-off-by: Pavel machek <pavel@xxxxxx>
> >>
> >> ---
> >>
> >> Patches to convert APDS990X driver to device tree and to switch to iio
> >> are available.
> >>
> >> diff --git a/Documentation/devicetree/bindings/misc/avago-apds990x.txt b/Documentation/devicetree/bindings/misc/avago-apds990x.txt
> >> new file mode 100644
> >> index 0000000..e038146
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/misc/avago-apds990x.txt
> >> @@ -0,0 +1,39 @@
> >> +Avago APDS990X driver
> >> +
> >> +Required properties:
> >> +- compatible: "avago,apds990x"
> >> +- reg: address on the I2C bus
> >> +- interrupts: external interrupt line number
> >> +- Vdd-supply: power supply for VDD
> >> +- Vled-supply: power supply for LEDA
> > 
> > AFAIK the custom is to use lower case letters for regulator supplies.
> 
> I've just used the same notation as in current driver. Datasheet calls
> those VDD (with DD being in subscript) and LEDA. I see no problem in
> changing those to vdd-supply and vled-supply if no one else objects.
> 
> > 
> >> +- ga: Glass attenuation
> >> +- cf1: Clear channel factor 1
> >> +- irf1: IR channel factor 1
> >> +- cf2: Clear channel factor 2
> >> +- irf2: IR channel factor 2
> >> +- df: Device factor
> >> +- pdrive: IR current, one of APDS_IRLED_CURR_XXXmA values
> >> +- ppcount: Proximity pulse count
> > 
> > Are these device specific? If so, please add the vendor prefix to them.
> 
> AFAIK yes - same as before if no one else objects, these will be changed.
> 
> > 
> > I might not use short abbreviations such as "df" either. I wonder what
> > others think.
> 
> These are also come from current driver - some of these comes from the
> datasheet itself, but not all. For example "ga" and "df" are in there
> (so I I would leave those alone), but "irf1" is called "B", "cf2" is
> called "C" and "irf2" is called "D" (I guess the missing "cf1" should be
> "A", but there is no such thing in the datasheet).
> IMHO using some other names might just add to the confusion.

Ack, datasheet names are fine.

You could also use a single property with all device specific coefficients
in a pre-defined order.

I don't have a strong opinion either way.

-- 
Regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx