diff mbox

Device tree binding for Avago APDS990X light sensor

Message ID 20171227091828.GA3307@amd (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Machek Dec. 27, 2017, 9:18 a.m. UTC
From: Filip Matijević <filip.matijevic.pz@gmail.com>

This prepares binding for light sensor used in Nokia N9. 

Signed-off-by: Filip Matijević <filip.matijevic.pz@gmail.com>
Signed-off-by: Pavel machek <pavel@ucw.cz>

---

Patches to convert APDS990X driver to device tree and to switch to iio
are available.

Comments

Sakari Ailus Dec. 27, 2017, 6 p.m. UTC | #1
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@gmail.com>
> 
> This prepares binding for light sensor used in Nokia N9. 
> 
> Signed-off-by: Filip Matijević <filip.matijevic.pz@gmail.com>
> Signed-off-by: Pavel machek <pavel@ucw.cz>
> 
> ---
> 
> 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.

> +- 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.

I might not use short abbreviations such as "df" either. I wonder what
others think.

> +
> +Example (Nokia N9):
> +
> +	als_ps@39 {
> +		compatible = "avago,apds990x";
> +		reg = <0x39>;
> +
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <19 10>; /* gpio_83, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_LOW */
> +
> +		Vdd-supply = <&vaux1>;
> +		Vled-supply = <&vbat>;
> +
> +		ga	= <168834>;
> +		cf1	= <4096>;
> +		irf1	= <7824>;
> +		cf2	= <877>;
> +		irf2	= <1575>;
> +		df	= <52>;
> +
> +		pdrive	= <0x2>; /* APDS_IRLED_CURR_25mA */
> +		ppcount	= <5>;
> +	};
>
Filip Matijević Dec. 27, 2017, 6:50 p.m. UTC | #2
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@gmail.com>
>>
>> This prepares binding for light sensor used in Nokia N9. 
>>
>> Signed-off-by: Filip Matijević <filip.matijevic.pz@gmail.com>
>> Signed-off-by: Pavel machek <pavel@ucw.cz>
>>
>> ---
>>
>> 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.

> 
>> +
>> +Example (Nokia N9):
>> +
>> +	als_ps@39 {
>> +		compatible = "avago,apds990x";
>> +		reg = <0x39>;
>> +
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <19 10>; /* gpio_83, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_LOW */
>> +
>> +		Vdd-supply = <&vaux1>;
>> +		Vled-supply = <&vbat>;
>> +
>> +		ga	= <168834>;
>> +		cf1	= <4096>;
>> +		irf1	= <7824>;
>> +		cf2	= <877>;
>> +		irf2	= <1575>;
>> +		df	= <52>;
>> +
>> +		pdrive	= <0x2>; /* APDS_IRLED_CURR_25mA */
>> +		ppcount	= <5>;
>> +	};
>>
> 

Best regards,
Filip
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Dec. 27, 2017, 8:01 p.m. UTC | #3
Hi!

> > +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.
> 
> > +- 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.

Well, whole binding is "vendor specific". Does it make sense to add
prefix in such case?

> I might not use short abbreviations such as "df" either. I wonder what
> others think.

I see.

Thanks,
									Pavel
Sakari Ailus Dec. 27, 2017, 9:15 p.m. UTC | #4
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@gmail.com>
> >>
> >> This prepares binding for light sensor used in Nokia N9. 
> >>
> >> Signed-off-by: Filip Matijević <filip.matijevic.pz@gmail.com>
> >> Signed-off-by: Pavel machek <pavel@ucw.cz>
> >>
> >> ---
> >>
> >> 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.
Sakari Ailus Dec. 27, 2017, 9:16 p.m. UTC | #5
On Wed, Dec 27, 2017 at 09:01:47PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > +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.
> > 
> > > +- 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.
> 
> Well, whole binding is "vendor specific". Does it make sense to add
> prefix in such case?

Yes, it does. If you later find one or more of these are generic, you could
remove the vendor prefix. I doubt that'll happen though, these seem very
device specific parameters.
diff mbox

Patch

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
+- 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
+
+Example (Nokia N9):
+
+	als_ps@39 {
+		compatible = "avago,apds990x";
+		reg = <0x39>;
+
+		interrupt-parent = <&gpio3>;
+		interrupts = <19 10>; /* gpio_83, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_LOW */
+
+		Vdd-supply = <&vaux1>;
+		Vled-supply = <&vbat>;
+
+		ga	= <168834>;
+		cf1	= <4096>;
+		irf1	= <7824>;
+		cf2	= <877>;
+		irf2	= <1575>;
+		df	= <52>;
+
+		pdrive	= <0x2>; /* APDS_IRLED_CURR_25mA */
+		ppcount	= <5>;
+	};