| Message ID | 1467406855-9677-1-git-send-email-alexandre.belloni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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 = <®_vcc3v0>; > + #io-channel-cells = <1>; > + }; > -- > 2.8.1 >
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 = <®_vcc3v0>; >> + #io-channel-cells = <1>; >> + }; >> -- >> 2.8.1 >>
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@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. 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
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@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. > > 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
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@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. > > > > 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.
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.
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
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@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. 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
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@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. > > 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.
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
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 = <®_vcc3v0>; + #io-channel-cells = <1>; + };
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