Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Sylwester Nawrocki]

On 08/16/2013 03:28 PM, Pawel Moll wrote:
> On Fri, 2013-08-16 at 14:12 +0100, Jacek Anaszewski wrote:
>> diff --git a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>> new file mode 100644
>> index 0000000..e4f377d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>> @@ -0,0 +1,20 @@
>> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
>> +
>> +Required properties:
>> +
>> +  - compatible : should be "sharp,gp2ap020a00f"
>> +  - reg : the I2C address of the light sensor
>> +  - interrupt-parent : phandle to the parent interrupt controller
>> +  - interrupts : should be INT interrupt pin

This seems a bit misleading, perhaps:

interrupts : should be interrupt number the INT pin interrupt is routed to

?
> Any particular reason for the interrupt-parent being required? Couldn't
> it be inherited, as is happens in most cases?

I think this was intended for the cases when the sensor is connected
to a GPIO and the GPIO interrupt controller is used. For example on
the Exynos SoCs with each GPIO bank is associated separate interrupt
controller instance (see arch/arm/boot/dts/exynos4210-pinctrl.dtsi
for details). Thus one of those interrupt controllers need to be
selected by dts, rather than inheriting the GIC.

I'm not sure if "interrupt-parent" needs to be part of DT binding of
this sensor device. Perhaps it could be an optional property or should
it be dropped completely ? I guess it should be considered as part
of the interrupt controller binding ?

Thanks,
Sylwester

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Pawel Moll]

On Mon, 2013-08-19 at 11:55 +0100, Sylwester Nawrocki wrote:
> On 08/16/2013 03:28 PM, Pawel Moll wrote:
> > On Fri, 2013-08-16 at 14:12 +0100, Jacek Anaszewski wrote:
> >> diff --git a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
> >> new file mode 100644
> >> index 0000000..e4f377d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
> >> @@ -0,0 +1,20 @@
> >> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
> >> +
> >> +Required properties:
> >> +
> >> +  - compatible : should be "sharp,gp2ap020a00f"
> >> +  - reg : the I2C address of the light sensor
> >> +  - interrupt-parent : phandle to the parent interrupt controller
> >> +  - interrupts : should be INT interrupt pin
> 
> This seems a bit misleading, perhaps:
> 
> interrupts : should be interrupt number the INT pin interrupt is routed to
> 
> ?
> > Any particular reason for the interrupt-parent being required? Couldn't
> > it be inherited, as is happens in most cases?
> 
> I think this was intended for the cases when the sensor is connected
> to a GPIO and the GPIO interrupt controller is used. For example on
> the Exynos SoCs with each GPIO bank is associated separate interrupt
> controller instance (see arch/arm/boot/dts/exynos4210-pinctrl.dtsi
> for details). Thus one of those interrupt controllers need to be
> selected by dts, rather than inheriting the GIC.
> 
> I'm not sure if "interrupt-parent" needs to be part of DT binding of
> this sensor device. Perhaps it could be an optional property or should
> it be dropped completely ? I guess it should be considered as part
> of the interrupt controller binding ?

Yes, I believe so. At least this is the de-facto standard for existing
device bindings documentations. Hopefully the DT-schemas-being-forged
will clarify the matter once for all.

Pawel

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Stephen Warren]

On 08/16/2013 07:12 AM, Jacek Anaszewski wrote:
> This patch adds device tree binding documentation
> for the gp2ap020a00f proximity/als sensor.

> diff --git a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
> new file mode 100644
> index 0000000..e4f377d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
> @@ -0,0 +1,20 @@
> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
> +
> +Required properties:
> +
> +  - compatible : should be "sharp,gp2ap020a00f"
> +  - reg : the I2C address of the light sensor
> +  - interrupt-parent : phandle to the parent interrupt controller
> +  - interrupts : should be INT interrupt pin
> +  - vled-supply : VLED power supply, as covered
> +		  in Documentation/devicetree/bindings/regulator/regulator.txt

If this is a sensor for proximity and light, why does it need a
regulator for an LED?

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Jonathan Cameron]

Stephen Warren <swarren@wwwdotorg.org> wrote:
>On 08/16/2013 07:12 AM, Jacek Anaszewski wrote:
>> This patch adds device tree binding documentation
>> for the gp2ap020a00f proximity/als sensor.
>
>> diff --git
>a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>> new file mode 100644
>> index 0000000..e4f377d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>> @@ -0,0 +1,20 @@
>> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
>> +
>> +Required properties:
>> +
>> +  - compatible : should be "sharp,gp2ap020a00f"
>> +  - reg : the I2C address of the light sensor
>> +  - interrupt-parent : phandle to the parent interrupt controller
>> +  - interrupts : should be INT interrupt pin
>> +  - vled-supply : VLED power supply, as covered
>> +		  in Documentation/devicetree/bindings/regulator/regulator.txt
>
>If this is a sensor for proximity and light, why does it need a
>regulator for an LED?
This type of proximity sensor uses reflected light from an led right next to it to detect something is nearby. The devices typically either integrate the led directly or control an external led. 

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Stephen Warren]

On 08/19/2013 11:54 PM, Jonathan Cameron wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/16/2013 07:12 AM, Jacek Anaszewski wrote:
>>> This patch adds device tree binding documentation
>>> for the gp2ap020a00f proximity/als sensor.
>>
>>> diff --git a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>>> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
>>> +
>>> +Required properties:
>>> +
>>> +  - compatible : should be "sharp,gp2ap020a00f"
>>> +  - reg : the I2C address of the light sensor
>>> +  - interrupt-parent : phandle to the parent interrupt controller
>>> +  - interrupts : should be INT interrupt pin
>>> +  - vled-supply : VLED power supply, as covered
>>> +		  in Documentation/devicetree/bindings/regulator/regulator.txt
>>
>> If this is a sensor for proximity and light, why does it need a
>> regulator for an LED?
>
> This type of proximity sensor uses reflected light from an led right next to it to detect something is nearby. The devices typically either integrate the led directly or control an external led. 

OK, that makes sense. It might make sense to briefly describe this
aspect of the HW at the top of the binding file. Perhaps something like:

----------
* Sharp GP2AP020A00F I2C Proximity/ALS sensor

The proximity detector sensor requires an associated LED for its
operation. The power supply to this LED is also defined by this binding.
----------

?

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Jacek Anaszewski]

On 08/20/2013 06:20 PM, Stephen Warren wrote:
> On 08/19/2013 11:54 PM, Jonathan Cameron wrote:
>> Stephen Warren<swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>  wrote:
>>> On 08/16/2013 07:12 AM, Jacek Anaszewski wrote:
>>>> This patch adds device tree binding documentation
>>>> for the gp2ap020a00f proximity/als sensor.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>>>> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
>>>> +
>>>> +Required properties:
>>>> +
>>>> +  - compatible : should be "sharp,gp2ap020a00f"
>>>> +  - reg : the I2C address of the light sensor
>>>> +  - interrupt-parent : phandle to the parent interrupt controller
>>>> +  - interrupts : should be INT interrupt pin
>>>> +  - vled-supply : VLED power supply, as covered
>>>> +		  in Documentation/devicetree/bindings/regulator/regulator.txt
>>>
>>> If this is a sensor for proximity and light, why does it need a
>>> regulator for an LED?
>>
>> This type of proximity sensor uses reflected light from an led right next to it to detect something is nearby. The devices typically either integrate the led directly or control an external led.
>
> OK, that makes sense. It might make sense to briefly describe this
> aspect of the HW at the top of the binding file. Perhaps something like:
>
> ----------
> * Sharp GP2AP020A00F I2C Proximity/ALS sensor
>
> The proximity detector sensor requires an associated LED for its
> operation. The power supply to this LED is also defined by this binding.
> ----------
>
> ?

Actually the LED is built-in into the chip, and its anode is exposed
on the LEDA pin. On the board I have for my disposal the presence of
this voltage is driven by some circuit. It doesn't have to be the
case for all boards, as the presence of the voltage alone doesn't
drive the state of the diode - the relevant device register has to
be set to activate/deactivate it. Having this in mind we must prepare
for the cases where the voltage will be attached directly to the
LEDA pin without any intermediary driving circuit.

To handle this situation an empty regulator node should be assigned
to the vled-supply property. I am not sure whether this case should
be mentioned in the description. I will assume that it is obvious.

In view of the above I would change the description to:

----------
* Sharp GP2AP020A00F I2C Proximity/ALS sensor

The proximity detector sensor requires power supply for its
built-in LED. It is also defined by this binding.

----------

What is your opinion?

Thanks,
Jacek

Re: [PATCH/RFC v4 3/3] DT: Add documentation for gp2ap020a00f sensor

[Stephen Warren]

On 08/21/2013 02:29 AM, Jacek Anaszewski wrote:
> On 08/20/2013 06:20 PM, Stephen Warren wrote:
>> On 08/19/2013 11:54 PM, Jonathan Cameron wrote:
>>> Stephen Warren<swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>  wrote:
>>>> On 08/16/2013 07:12 AM, Jacek Anaszewski wrote:
>>>>> This patch adds device tree binding documentation
>>>>> for the gp2ap020a00f proximity/als sensor.
>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>>>>> +* Sharp GP2AP020A00F I2C Proximity/ALS sensor
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +  - compatible : should be "sharp,gp2ap020a00f"
>>>>> +  - reg : the I2C address of the light sensor
>>>>> +  - interrupt-parent : phandle to the parent interrupt controller
>>>>> +  - interrupts : should be INT interrupt pin
>>>>> +  - vled-supply : VLED power supply, as covered
>>>>> +          in
>>>>> Documentation/devicetree/bindings/regulator/regulator.txt
>>>>
>>>> If this is a sensor for proximity and light, why does it need a
>>>> regulator for an LED?
>>>
>>> This type of proximity sensor uses reflected light from an led right
>>> next to it to detect something is nearby. The devices typically
>>> either integrate the led directly or control an external led.
>>
>> OK, that makes sense. It might make sense to briefly describe this
>> aspect of the HW at the top of the binding file. Perhaps something like:
>>
>> ----------
>> * Sharp GP2AP020A00F I2C Proximity/ALS sensor
>>
>> The proximity detector sensor requires an associated LED for its
>> operation. The power supply to this LED is also defined by this binding.
>> ----------
>>
>> ?
> 
> Actually the LED is built-in into the chip, and its anode is exposed
> on the LEDA pin. On the board I have for my disposal the presence of
> this voltage is driven by some circuit. It doesn't have to be the
> case for all boards, as the presence of the voltage alone doesn't
> drive the state of the diode - the relevant device register has to
> be set to activate/deactivate it. Having this in mind we must prepare
> for the cases where the voltage will be attached directly to the
> LEDA pin without any intermediary driving circuit.
> 
> To handle this situation an empty regulator node should be assigned
> to the vled-supply property. I am not sure whether this case should
> be mentioned in the description. I will assume that it is obvious.

Historically, all regulator supplies have been mandatory; in other
words, if a device might ever need a voltage regulator, then the DT
property to define it was mandatory, and it could be implemented either
by a "real" regulator or a non-switchable fixed regulator.

More recently, there's been some work to support optional regulators,
such that the DT supply property can either point at a "real" regulator,
or simply be missing.

I'm not sure what the state of that change is in the regulator tree
though, although in terms of the DT binding, I guess you should be able
to make the property optional now if you want.

> In view of the above I would change the description to:
> 
> ----------
> * Sharp GP2AP020A00F I2C Proximity/ALS sensor
> 
> The proximity detector sensor requires power supply for its
> built-in LED. It is also defined by this binding.
> 
> ----------
> 
> What is your opinion?

That sounds good.