[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

Document the bindings for the Allwinner LRADC.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org

 .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
new file mode 100644
index 000000000000..c75a6067b8a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
@@ -0,0 +1,19 @@
+Allwinner sun4i Low Resolution ADC
+----------------------------------
+
+Required properties:
+ - compatible: "allwinner,sun4i-a10-lradc"
+ - reg: mmio address range of the chip
+ - interrupts: interrupt to which the chip is connected
+ - vref-supply: powersupply for the lradc reference voltage
+ - #io-channel-cells = <1>; As ADC has multiple outputs
+
+Example:
+
+	lradc: lradc@01c22800 {
+		compatible = "allwinner,sun4i-a10-lradc";
+		reg = <0x01c22800 0x100>;
+		interrupts = <31>;
+		vref-supply = <&reg_vcc3v0>;
+		#io-channel-cells = <1>;
+	};
-- 
2.8.1

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Chen-Yu Tsai]

Hi,

On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Document the bindings for the Allwinner LRADC.

We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
block.

Any plans to reconcile the different bindings?

Thanks
ChenYu

>
> Signed-off-by: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>
>  .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
> new file mode 100644
> index 000000000000..c75a6067b8a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
> @@ -0,0 +1,19 @@
> +Allwinner sun4i Low Resolution ADC
> +----------------------------------
> +
> +Required properties:
> + - compatible: "allwinner,sun4i-a10-lradc"
> + - reg: mmio address range of the chip
> + - interrupts: interrupt to which the chip is connected
> + - vref-supply: powersupply for the lradc reference voltage
> + - #io-channel-cells = <1>; As ADC has multiple outputs
> +
> +Example:
> +
> +       lradc: lradc@01c22800 {
> +               compatible = "allwinner,sun4i-a10-lradc";
> +               reg = <0x01c22800 0x100>;
> +               interrupts = <31>;
> +               vref-supply = <&reg_vcc3v0>;
> +               #io-channel-cells = <1>;
> +       };
> --
> 2.8.1
>

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 11:12, Chen-Yu Tsai wrote:
> Hi,
>
> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> Document the bindings for the Allwinner LRADC.
>
> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> block.

Right, this block is used on many tablets and some dev boards to
provide buttons (as in the hid type) and the block is designed for
this purpose, giving an irq when the adc level crosses a certain
threshold.

Sure it can be used in a more generic way, but that is not its
primary goal.

So any generic purpose adc driver must not break the current
use-case (which is already used in mainline kernel dts files
in plenty of cases).

I believe that the best way to deal with this is to add an
"allwinner,general-purpose-mode" flag to the existing binding
(as well as document general purpose mode in the existing
binding rather then in a new binding doc).

That seems to be the right thing to do purely looking at this
from a dt binding pov.

For the implementation of this we can simpy have 2 drivers,
then both drivers can check the flag and if present return
-ENODEV from the existing input driver and likewise if not
present return -ENODEV from the iio driver.

We may actually use a similar solution for the touchscreen
controller which can also be alternatively used as a generic
purpose adc.

Regards,

Hans



> Any plans to reconcile the different bindings?
>
> Thanks
> ChenYu
>
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> ---
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>>
>>  .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>> new file mode 100644
>> index 000000000000..c75a6067b8a5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>> @@ -0,0 +1,19 @@
>> +Allwinner sun4i Low Resolution ADC
>> +----------------------------------
>> +
>> +Required properties:
>> + - compatible: "allwinner,sun4i-a10-lradc"
>> + - reg: mmio address range of the chip
>> + - interrupts: interrupt to which the chip is connected
>> + - vref-supply: powersupply for the lradc reference voltage
>> + - #io-channel-cells = <1>; As ADC has multiple outputs
>> +
>> +Example:
>> +
>> +       lradc: lradc@01c22800 {
>> +               compatible = "allwinner,sun4i-a10-lradc";
>> +               reg = <0x01c22800 0x100>;
>> +               interrupts = <31>;
>> +               vref-supply = <&reg_vcc3v0>;
>> +               #io-channel-cells = <1>;
>> +       };
>> --
>> 2.8.1
>>

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-07-16 11:12, Chen-Yu Tsai wrote:
> >Hi,
> >
> >On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> ><alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >>Document the bindings for the Allwinner LRADC.
> >
> >We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> >and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> >block.
> 
> Right, this block is used on many tablets and some dev boards to
> provide buttons (as in the hid type) and the block is designed for
> this purpose, giving an irq when the adc level crosses a certain
> threshold.
> 
> Sure it can be used in a more generic way, but that is not its
> primary goal.

We've always had a different view on this, but it's a detail :)

> So any generic purpose adc driver must not break the current
> use-case (which is already used in mainline kernel dts files
> in plenty of cases).

Yep.

> I believe that the best way to deal with this is to add an
> "allwinner,general-purpose-mode" flag to the existing binding
> (as well as document general purpose mode in the existing
> binding rather then in a new binding doc).
> 
> That seems to be the right thing to do purely looking at this
> from a dt binding pov.

There's a way simpler solution: if there's no child nodes, it's meant
to be used as an ADC, otherwise, as input.

The logic will have to be a bit more complex than that, since there's
two channels, and you could only require one for the buttons, leaving
the other one available as an ADC.

But that doesn't require any new property.

> For the implementation of this we can simpy have 2 drivers,
> then both drivers can check the flag and if present return
> -ENODEV from the existing input driver and likewise if not
> present return -ENODEV from the iio driver.
> 
> We may actually use a similar solution for the touchscreen
> controller which can also be alternatively used as a generic
> purpose adc.

There's no need to keep both drivers as long as we keep the features
and bindings.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 13:02, Maxime Ripard wrote:
> On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 02-07-16 11:12, Chen-Yu Tsai wrote:
>>> Hi,
>>>
>>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>>> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>> Document the bindings for the Allwinner LRADC.
>>>
>>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>>> block.
>>
>> Right, this block is used on many tablets and some dev boards to
>> provide buttons (as in the hid type) and the block is designed for
>> this purpose, giving an irq when the adc level crosses a certain
>> threshold.
>>
>> Sure it can be used in a more generic way, but that is not its
>> primary goal.
>
> We've always had a different view on this, but it's a detail :)
>
>> So any generic purpose adc driver must not break the current
>> use-case (which is already used in mainline kernel dts files
>> in plenty of cases).
>
> Yep.
>
>> I believe that the best way to deal with this is to add an
>> "allwinner,general-purpose-mode" flag to the existing binding
>> (as well as document general purpose mode in the existing
>> binding rather then in a new binding doc).
>>
>> That seems to be the right thing to do purely looking at this
>> from a dt binding pov.
>
> There's a way simpler solution: if there's no child nodes, it's meant
> to be used as an ADC, otherwise, as input.
>
> The logic will have to be a bit more complex than that, since there's
> two channels, and you could only require one for the buttons, leaving
> the other one available as an ADC.
>
> But that doesn't require any new property.

True, if there are no button nodes, then go general-purpose will
work too.

>> For the implementation of this we can simpy have 2 drivers,
>> then both drivers can check the flag and if present return
>> -ENODEV from the existing input driver and likewise if not
>> present return -ENODEV from the iio driver.
>>
>> We may actually use a similar solution for the touchscreen
>> controller which can also be alternatively used as a generic
>> purpose adc.
>
> There's no need to keep both drivers as long as we keep the features
> and bindings.

That is also true.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 13:45:18 +0200, Hans de Goede wrote :
> Hi,
> 
> On 02-07-16 13:02, Maxime Ripard wrote:
> > On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 02-07-16 11:12, Chen-Yu Tsai wrote:
> > > > Hi,
> > > > 
> > > > On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> > > > <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > > > > Document the bindings for the Allwinner LRADC.
> > > > 
> > > > We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> > > > and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> > > > block.
> > > 
> > > Right, this block is used on many tablets and some dev boards to
> > > provide buttons (as in the hid type) and the block is designed for
> > > this purpose, giving an irq when the adc level crosses a certain
> > > threshold.
> > > 
> > > Sure it can be used in a more generic way, but that is not its
> > > primary goal.
> > 
> > We've always had a different view on this, but it's a detail :)
> > 
> > > So any generic purpose adc driver must not break the current
> > > use-case (which is already used in mainline kernel dts files
> > > in plenty of cases).
> > 
> > Yep.
> > 
> > > I believe that the best way to deal with this is to add an
> > > "allwinner,general-purpose-mode" flag to the existing binding
> > > (as well as document general purpose mode in the existing
> > > binding rather then in a new binding doc).
> > > 
> > > That seems to be the right thing to do purely looking at this
> > > from a dt binding pov.
> > 
> > There's a way simpler solution: if there's no child nodes, it's meant
> > to be used as an ADC, otherwise, as input.
> > 
> > The logic will have to be a bit more complex than that, since there's
> > two channels, and you could only require one for the buttons, leaving
> > the other one available as an ADC.
> > 
> > But that doesn't require any new property.
> 
> True, if there are no button nodes, then go general-purpose will
> work too.
> 

Well, I'd still argue that we need two different compatibles because
encoding the usage of an IP in its compatible string is less than ideal.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On Sat, Jul 02, 2016 at 03:32:43PM +0200, Alexandre Belloni wrote:
> > > > I believe that the best way to deal with this is to add an
> > > > "allwinner,general-purpose-mode" flag to the existing binding
> > > > (as well as document general purpose mode in the existing
> > > > binding rather then in a new binding doc).
> > > > 
> > > > That seems to be the right thing to do purely looking at this
> > > > from a dt binding pov.
> > > 
> > > There's a way simpler solution: if there's no child nodes, it's meant
> > > to be used as an ADC, otherwise, as input.
> > > 
> > > The logic will have to be a bit more complex than that, since there's
> > > two channels, and you could only require one for the buttons, leaving
> > > the other one available as an ADC.
> > > 
> > > But that doesn't require any new property.
> > 
> > True, if there are no button nodes, then go general-purpose will
> > work too.
> 
> Well, I'd still argue that we need two different compatibles because
> encoding the usage of an IP in its compatible string is less than ideal.

That's true, but it's what we have.

So feel free to add a new compatible if you want, but we'll have to
support the old one anyway.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
> Hi,
> 
> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > Document the bindings for the Allwinner LRADC.
> 
> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> block.
> 
> Any plans to reconcile the different bindings?
> 

Yes, I already submitted an adc-keys driver that can work with any ADC:
https://lkml.org/lkml/2016/7/1/670

I agree that because it is not yet handling interrupts and is polling
the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
that but it require significant work in iio.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 15:35, Alexandre Belloni wrote:
> On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
>> Hi,
>>
>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Document the bindings for the Allwinner LRADC.
>>
>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>> block.
>>
>> Any plans to reconcile the different bindings?
>>
>
> Yes, I already submitted an adc-keys driver that can work with any ADC:
> https://lkml.org/lkml/2016/7/1/670
>
> I agree that because it is not yet handling interrupts and is polling
> the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> that but it require significant work in iio.

And it also seems to break dt compatibility. Note I'm not against
making an exception for this and breaking the dt compat, but until
the polling is fixed we should not replace sun4i-lradc-keys.

If I understand you correctly then you want to use a new generic
"sun4i-lradc" compatible. If you do that then we can just build both
drivers for now and use the right compatible depending on how the
board uses the lradc for now.

Regards,

Hans

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 21:46:43 +0200, Hans de Goede wrote :
> Hi,
> 
> On 02-07-16 15:35, Alexandre Belloni wrote:
> > On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
> > > Hi,
> > > 
> > > On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> > > <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > > > Document the bindings for the Allwinner LRADC.
> > > 
> > > We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> > > and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> > > block.
> > > 
> > > Any plans to reconcile the different bindings?
> > > 
> > 
> > Yes, I already submitted an adc-keys driver that can work with any ADC:
> > https://lkml.org/lkml/2016/7/1/670
> > 
> > I agree that because it is not yet handling interrupts and is polling
> > the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> > that but it require significant work in iio.
> 
> And it also seems to break dt compatibility. Note I'm not against
> making an exception for this and breaking the dt compat, but until
> the polling is fixed we should not replace sun4i-lradc-keys.
> 
> If I understand you correctly then you want to use a new generic
> "sun4i-lradc" compatible. If you do that then we can just build both
> drivers for now and use the right compatible depending on how the
> board uses the lradc for now.
> 

Well, I never said we have to remove the previous compatible, just that
it was probably not the best one. I also didn't send a patch to remove
the previous driver and they can indeed coexist nicely for now.

Anyway, if we want to remove the sun4i-lradc-keys driver and keep DT
compatibility, we'll have to write a small stub driver. It isn't the
easiest task but it is doable.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Jonathan Cameron]

On 02/07/16 14:35, Alexandre Belloni wrote:
> On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
>> Hi,
>>
>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>>> Document the bindings for the Allwinner LRADC.
>>
>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>> block.
>>
>> Any plans to reconcile the different bindings?
>>
> 
> Yes, I already submitted an adc-keys driver that can work with any ADC:
> https://lkml.org/lkml/2016/7/1/670
> 
> I agree that because it is not yet handling interrupts and is polling
> the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> that but it require significant work in iio.
> 
As I'm having fun confusing the two drivers submitted for different
ADCs on the A10 this morning - here is the relevant bit of text I stuck
in my review for the other driver...

 For the key detection you have already observed that IIO needs some
 additions to be able to have consumers of what we term 'events' e.g. threshold
 interrupts.
 
 Looking at the lradc-keys driver in tree, it looks like we only really have
 really simple threshold interrups - configured to detect a very low voltage?
 + only one per channel.
 
 So not too nasty a case, but you are right some work is needed in IIO as
 we simply don't have a means of passing these on as yet or configuring them
 from in kernel consumers.
 If we take the easy route and don't demux incoming events then it shouldn't
 be too hard to add (demux can follow later).  Hence any client device can try
 to enable events it wants, but may get events that other client devices wanted
 as well.
 
 Config interface should be much the same as the write support for channels.
 Data flow marginally harder, but pretty much a list of callbacks within
 iio_push_event.
 
 Not trivial, but not too tricky either.
 
 The events subsystem has a few 'limitations' we need to address long term
 but as this is in kernel interface only, we can do this now and fix stuff
 up in future without any ABI breakage. (limitations are things like only
 one event of a given type and direction per channel - main challenge on
 that is finding a way of doing it without abi breakage).
 
 Anyhow, sounds fun - wish I had the time to do it myself!
Jonathan