| Message ID | 1388604610-20380-2-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, On Wed, Jan 01, 2014 at 08:30:07PM +0100, Hans de Goede wrote: > +Required properties: > + - compatible: "allwinner,sun4i-lradc-keys" > + - reg: mmio address range of the chip > + - interrupts: interrupt to which the chip is connected > + - allwinner,chan0-step: step in mV between keys must be 150 or 200 > + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_ codes I think this should be "linux,chan0-keycodes". Thanks.
Hi, On 01/01/2014 09:56 PM, Dmitry Torokhov wrote: > Hi Hans, > > On Wed, Jan 01, 2014 at 08:30:07PM +0100, Hans de Goede wrote: >> +Required properties: >> + - compatible: "allwinner,sun4i-lradc-keys" >> + - reg: mmio address range of the chip >> + - interrupts: interrupt to which the chip is connected >> + - allwinner,chan0-step: step in mV between keys must be 150 or 200 >> + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_ codes > > I think this should be "linux,chan0-keycodes". Right, because the codes are Linux specific, will fix in v2. Regards, Hans
Hi Hans, Dmitry, Am Donnerstag, 2. Januar 2014, 10:37:47 schrieb Hans de Goede: > Hi, > > On 01/01/2014 09:56 PM, Dmitry Torokhov wrote: > > Hi Hans, > > > > On Wed, Jan 01, 2014 at 08:30:07PM +0100, Hans de Goede wrote: > >> +Required properties: > >> + - compatible: "allwinner,sun4i-lradc-keys" > >> + - reg: mmio address range of the chip > >> + - interrupts: interrupt to which the chip is connected > >> + - allwinner,chan0-step: step in mV between keys must be 150 or 200 > >> + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_ > >> codes> > > I think this should be "linux,chan0-keycodes". > > Right, because the codes are Linux specific, will fix in v2. but the property with its "chan0-" thingy would be allwinner-specific if I'm not mistaken. Also, instead of inventing yet another vendor-specific property, why not re-use a button binding similar to gpio-keys like: lradc: lradc@01c22800 { compatible = "allwinner,sun4i-lradc-keys"; reg = <0x01c22800 0x100>; interrupts = <31>; allwinner,chan0-step = <200>; #address-cells = <1>; #size-cells = <0>; button@0 { reg = <0>; /* your channel index from above */ linux,code = <115>; /* already used as dt-property */ }; button@1 { reg = <1>; linux,code = <114>; }; ... }; But I may be on the wrong track here, so I've included the devicetree-people for help, which I guess should've been included from the beginning. Heiko
Hi, On 01/02/2014 12:59 PM, Heiko Stübner wrote: > Hi Hans, Dmitry, > > Am Donnerstag, 2. Januar 2014, 10:37:47 schrieb Hans de Goede: >> Hi, >> >> On 01/01/2014 09:56 PM, Dmitry Torokhov wrote: >>> Hi Hans, >>> >>> On Wed, Jan 01, 2014 at 08:30:07PM +0100, Hans de Goede wrote: >>>> +Required properties: >>>> + - compatible: "allwinner,sun4i-lradc-keys" >>>> + - reg: mmio address range of the chip >>>> + - interrupts: interrupt to which the chip is connected >>>> + - allwinner,chan0-step: step in mV between keys must be 150 or 200 >>>> + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_ >>>> codes> >>> I think this should be "linux,chan0-keycodes". >> >> Right, because the codes are Linux specific, will fix in v2. > > but the property with its "chan0-" thingy would be allwinner-specific if I'm > not mistaken. Correct, but denoting that this is linux only is more important, so as to avoid namespace collisions. > > Also, instead of inventing yet another vendor-specific property, why not re-use > a button binding similar to gpio-keys like: > > lradc: lradc@01c22800 { > compatible = "allwinner,sun4i-lradc-keys"; > reg = <0x01c22800 0x100>; > interrupts = <31>; > allwinner,chan0-step = <200>; > > #address-cells = <1>; > #size-cells = <0>; > > button@0 { > reg = <0>; /* your channel index from above */ > linux,code = <115>; /* already used as dt-property */ > }; > > button@1 { > reg = <1>; > linux,code = <114>; > }; Ugh no. Having a vendor specific property which is KISS certainly beats this, both wrt ease of writing dts files as well as wrt the dts parsing code in the driver. Regards, Hans
On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > >Also, instead of inventing yet another vendor-specific property, why not re-use > >a button binding similar to gpio-keys like: > > > > lradc: lradc@01c22800 { > > compatible = "allwinner,sun4i-lradc-keys"; > > reg = <0x01c22800 0x100>; > > interrupts = <31>; > > allwinner,chan0-step = <200>; > > > > #address-cells = <1>; > > #size-cells = <0>; > > > > button@0 { > > reg = <0>; /* your channel index from above */ > > linux,code = <115>; /* already used as dt-property */ > > }; > > > > button@1 { > > reg = <1>; > > linux,code = <114>; > > }; > > Ugh no. Having a vendor specific property which is KISS certainly > beats this, both wrt ease of writing dts files as well as wrt the > dts parsing code in the driver. I'd agree with Heiko here. This is pretty much the same construct that's already in use in other input drivers, like gpio-keys. This is also something that can really easily be made generic, since this is something that is rather common. Speaking of which. I believe this should actually come in two different drivers: - The ADC driver itself, using IIO - A generic button handler driver on top of IIO. The fact that on most board this adc is used for buttons doesn't make any difference, it's actually a hardware designer choice, we should support that choice, but we should also be able to use it just as an ADC. Carlo Caione already started to work on an IIO driver for the LRADC: https://github.com/carlocaione/linux/tree/sunxi-lradc maybe you can take over his work. I also wonder wether it would be possible in that case to use reg as the "button" voltage, to get rid of both the chan0-step property, and those big fat arrays in the driver. Maxime
On Thu, Jan 02, 2014 at 09:20:22PM +0100, Maxime Ripard wrote: > On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > > >Also, instead of inventing yet another vendor-specific property, why not re-use > > >a button binding similar to gpio-keys like: > > > > > > lradc: lradc@01c22800 { > > > compatible = "allwinner,sun4i-lradc-keys"; > > > reg = <0x01c22800 0x100>; > > > interrupts = <31>; > > > allwinner,chan0-step = <200>; > > > > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > button@0 { > > > reg = <0>; /* your channel index from above */ > > > linux,code = <115>; /* already used as dt-property */ > > > }; > > > > > > button@1 { > > > reg = <1>; > > > linux,code = <114>; > > > }; > > > > Ugh no. Having a vendor specific property which is KISS certainly > > beats this, both wrt ease of writing dts files as well as wrt the > > dts parsing code in the driver. > > I'd agree with Heiko here. This is pretty much the same construct > that's already in use in other input drivers, like gpio-keys. > > This is also something that can really easily be made generic, since > this is something that is rather common. Except that button definition from gpio-keys does not use 'reg' property but rather gpio. I'd rather we did not cram non-applicable attributes into that definition just to make it "reusable" like that. I'd be OK with having similar (but not claiming to be the same) mappings though. Thanks.
Hi, On 01/02/2014 09:20 PM, Maxime Ripard wrote: > On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: >>> Also, instead of inventing yet another vendor-specific property, why not re-use >>> a button binding similar to gpio-keys like: >>> >>> lradc: lradc@01c22800 { >>> compatible = "allwinner,sun4i-lradc-keys"; >>> reg = <0x01c22800 0x100>; >>> interrupts = <31>; >>> allwinner,chan0-step = <200>; >>> >>> #address-cells = <1>; >>> #size-cells = <0>; >>> >>> button@0 { >>> reg = <0>; /* your channel index from above */ >>> linux,code = <115>; /* already used as dt-property */ >>> }; >>> >>> button@1 { >>> reg = <1>; >>> linux,code = <114>; >>> }; >> >> Ugh no. Having a vendor specific property which is KISS certainly >> beats this, both wrt ease of writing dts files as well as wrt the >> dts parsing code in the driver. > > I'd agree with Heiko here. This is pretty much the same construct > that's already in use in other input drivers, like gpio-keys. In the gpio case there is a 1 on 1 relation between a single hw entity (the gpio-pin) and a single keycode. Here there is 1 hw entity which maps to an array of key-codes, certainly using an array rather then a much more complicated construct is the correct data-structure to represent this. > > This is also something that can really easily be made generic, since > this is something that is rather common. > > Speaking of which. I believe this should actually come in two > different drivers: > - The ADC driver itself, using IIO > - A generic button handler driver on top of IIO. > > The fact that on most board this adc is used for buttons doesn't make > any difference, it's actually a hardware designer choice, we should > support that choice, but we should also be able to use it just as an > ADC. No, this is not a generic adc, as mentioned in the commit msg, this adc is specifically designed to be used this way. The adc won't start sampling data, and won't generate any interrupts until a button is pressed. That is until the input voltage drops below 2/3 of Vref, this is checked through a built-in analog comparator, which hooks into the control logic. It has button down and button up interrupts, and can detect long presses (unused) and generate a second type of down interrupt for those. This really is an input device, which happens to use an adc. > Carlo Caione already started to work on an IIO driver for the LRADC: > https://github.com/carlocaione/linux/tree/sunxi-lradc > maybe you can take over his work. That won't work because the adc won't sample if the input gets above 2/3 of Vref. There may be some other mode which does not do that, but that is not clearly documented. Even if an IIO driver turns out to be doable, I strongly believe that having a separate input driver for this is best, since this device was designed to be used as such. Building input on top of IIO would mean polling the adc, while with my driver it actually generates button down / up interrupts without any polling being involved. And no boards I know of are using this as a generic analog input, where as many boards are using it as designed. Regards, Hans
On Thu, Jan 02, 2014 at 12:38:31PM -0800, Dmitry Torokhov wrote: > On Thu, Jan 02, 2014 at 09:20:22PM +0100, Maxime Ripard wrote: > > On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > > > >Also, instead of inventing yet another vendor-specific property, why not re-use > > > >a button binding similar to gpio-keys like: > > > > > > > > lradc: lradc@01c22800 { > > > > compatible = "allwinner,sun4i-lradc-keys"; > > > > reg = <0x01c22800 0x100>; > > > > interrupts = <31>; > > > > allwinner,chan0-step = <200>; > > > > > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > > > > > button@0 { > > > > reg = <0>; /* your channel index from above */ > > > > linux,code = <115>; /* already used as dt-property */ > > > > }; > > > > > > > > button@1 { > > > > reg = <1>; > > > > linux,code = <114>; > > > > }; > > > > > > Ugh no. Having a vendor specific property which is KISS certainly > > > beats this, both wrt ease of writing dts files as well as wrt the > > > dts parsing code in the driver. > > > > I'd agree with Heiko here. This is pretty much the same construct > > that's already in use in other input drivers, like gpio-keys. > > > > This is also something that can really easily be made generic, since > > this is something that is rather common. > > Except that button definition from gpio-keys does not use 'reg' property > but rather gpio. I'd rather we did not cram non-applicable attributes > into that definition just to make it "reusable" like that. > > I'd be OK with having similar (but not claiming to be the same) mappings > though. Yes, this is what I was meaning. Sorry if it was not clear enough. Maxime
On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote: > Hi, > > On 01/02/2014 09:20 PM, Maxime Ripard wrote: > >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > >>>Also, instead of inventing yet another vendor-specific property, why not re-use > >>>a button binding similar to gpio-keys like: > >>> > >>> lradc: lradc@01c22800 { > >>> compatible = "allwinner,sun4i-lradc-keys"; > >>> reg = <0x01c22800 0x100>; > >>> interrupts = <31>; > >>> allwinner,chan0-step = <200>; > >>> > >>> #address-cells = <1>; > >>> #size-cells = <0>; > >>> > >>> button@0 { > >>> reg = <0>; /* your channel index from above */ > >>> linux,code = <115>; /* already used as dt-property */ > >>> }; > >>> > >>> button@1 { > >>> reg = <1>; > >>> linux,code = <114>; > >>> }; > >> > >>Ugh no. Having a vendor specific property which is KISS certainly > >>beats this, both wrt ease of writing dts files as well as wrt the > >>dts parsing code in the driver. > > > >I'd agree with Heiko here. This is pretty much the same construct > >that's already in use in other input drivers, like gpio-keys. > > In the gpio case there is a 1 on 1 relation between a single hw > entity (the gpio-pin) and a single keycode. Here there is 1 hw entity > which maps to an array of key-codes, certainly using an array rather > then a much more complicated construct is the correct data-structure > to represent this. You can build an array in your driver out of this very easily, it's 10 lines in your probe. And you gain from this something that is more generic, can be shared by other similar drivers and is consistent with what is already in use. > >This is also something that can really easily be made generic, > >since this is something that is rather common. > > > >Speaking of which. I believe this should actually come in two > >different drivers: > > - The ADC driver itself, using IIO > > - A generic button handler driver on top of IIO. > > > > The fact that on most board this adc is used for buttons doesn't make > > any difference, it's actually a hardware designer choice, we should > > support that choice, but we should also be able to use it just as an > > ADC. > > No, this is not a generic adc, as mentioned in the commit msg, this > adc is specifically designed to be used this way. > > The adc won't start sampling data, and won't generate any interrupts > until a button is pressed. That is until the input voltage drops below > 2/3 of Vref, this is checked through a built-in analog comparator, which > hooks into the control logic. > > It has button down and button up interrupts, and can detect long > presses (unused) and generate a second type of down interrupt for those. > > This really is an input device, which happens to use an adc. Hmm, yes, ok. > >Carlo Caione already started to work on an IIO driver for the LRADC: > >https://github.com/carlocaione/linux/tree/sunxi-lradc > >maybe you can take over his work. > > That won't work because the adc won't sample if the input gets above > 2/3 of Vref. There may be some other mode which does not do that, but > that is not clearly documented. > > Even if an IIO driver turns out to be doable, I strongly believe that > having a separate input driver for this is best, since this device > was designed to be used as such. Building input on top of IIO would > mean polling the adc, while with my driver it actually generates > button down / up interrupts without any polling being involved. Not really. iio_channel_read calls the read_raw function (in that case) of your driver. If the read_raw function in your driver wants to poll the device, fine, but most of the time, it will just block waiting for an interrupt to come and return the data to the caller, which is obviously the saner behaviour, and you don't actually end up polling the device. Which is pretty much the architecture you're using already, just with an intermediate layer in between. Maxime
On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote: > On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote: > > Hi, > > > > On 01/02/2014 09:20 PM, Maxime Ripard wrote: > > >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > > >>>Also, instead of inventing yet another vendor-specific property, why not re-use > > >>>a button binding similar to gpio-keys like: > > >>> > > >>> lradc: lradc@01c22800 { > > >>> compatible = "allwinner,sun4i-lradc-keys"; > > >>> reg = <0x01c22800 0x100>; > > >>> interrupts = <31>; > > >>> allwinner,chan0-step = <200>; > > >>> > > >>> #address-cells = <1>; > > >>> #size-cells = <0>; > > >>> > > >>> button@0 { > > >>> reg = <0>; /* your channel index from above */ > > >>> linux,code = <115>; /* already used as dt-property */ > > >>> }; > > >>> > > >>> button@1 { > > >>> reg = <1>; > > >>> linux,code = <114>; > > >>> }; > > >> > > >>Ugh no. Having a vendor specific property which is KISS certainly > > >>beats this, both wrt ease of writing dts files as well as wrt the > > >>dts parsing code in the driver. > > > > > >I'd agree with Heiko here. This is pretty much the same construct > > >that's already in use in other input drivers, like gpio-keys. > > > > In the gpio case there is a 1 on 1 relation between a single hw > > entity (the gpio-pin) and a single keycode. Here there is 1 hw entity > > which maps to an array of key-codes, certainly using an array rather > > then a much more complicated construct is the correct data-structure > > to represent this. > > You can build an array in your driver out of this very easily, it's 10 > lines in your probe. And you gain from this something that is more > generic, can be shared by other similar drivers and is consistent with > what is already in use. How will it be shared? Surely not code-wise, but basically in spirit only. It seems to me that the originally proposed binding is simple and concise and works well for the driver. > > > >This is also something that can really easily be made generic, > > >since this is something that is rather common. > > > > > >Speaking of which. I believe this should actually come in two > > >different drivers: > > > - The ADC driver itself, using IIO > > > - A generic button handler driver on top of IIO. > > > > > > The fact that on most board this adc is used for buttons doesn't make > > > any difference, it's actually a hardware designer choice, we should > > > support that choice, but we should also be able to use it just as an > > > ADC. > > > > No, this is not a generic adc, as mentioned in the commit msg, this > > adc is specifically designed to be used this way. > > > > The adc won't start sampling data, and won't generate any interrupts > > until a button is pressed. That is until the input voltage drops below > > 2/3 of Vref, this is checked through a built-in analog comparator, which > > hooks into the control logic. > > > > It has button down and button up interrupts, and can detect long > > presses (unused) and generate a second type of down interrupt for those. > > > > This really is an input device, which happens to use an adc. > > Hmm, yes, ok. > > > >Carlo Caione already started to work on an IIO driver for the LRADC: > > >https://github.com/carlocaione/linux/tree/sunxi-lradc > > >maybe you can take over his work. > > > > That won't work because the adc won't sample if the input gets above > > 2/3 of Vref. There may be some other mode which does not do that, but > > that is not clearly documented. > > > > Even if an IIO driver turns out to be doable, I strongly believe that > > having a separate input driver for this is best, since this device > > was designed to be used as such. Building input on top of IIO would > > mean polling the adc, while with my driver it actually generates > > button down / up interrupts without any polling being involved. > > Not really. iio_channel_read calls the read_raw function (in that > case) of your driver. If the read_raw function in your driver wants to > poll the device, fine, but most of the time, it will just block > waiting for an interrupt to come and return the data to the caller, > which is obviously the saner behaviour, and you don't actually end up > polling the device. Which is pretty much the architecture you're using > already, just with an intermediate layer in between. What is the benefit of the IIO layer if device can't really be used as IIO? I am all for moving as many generic devices as we can to IIO but we should recognize that sometimes the device is not an IIO device. Thanks.
Am Freitag, 3. Januar 2014, 10:23:50 schrieb Dmitry Torokhov: > On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote: > > On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 01/02/2014 09:20 PM, Maxime Ripard wrote: > > > >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > > > >>>Also, instead of inventing yet another vendor-specific property, why > > > >>>not re-use> > >>> > > > >>>a button binding similar to gpio-keys like: > > > >>> lradc: lradc@01c22800 { > > > >>> > > > >>> compatible = "allwinner,sun4i-lradc-keys"; > > > >>> reg = <0x01c22800 0x100>; > > > >>> interrupts = <31>; > > > >>> allwinner,chan0-step = <200>; > > > >>> > > > >>> #address-cells = <1>; > > > >>> #size-cells = <0>; > > > >>> > > > >>> button@0 { > > > >>> > > > >>> reg = <0>; /* your channel index from above */ > > > >>> linux,code = <115>; /* already used as dt-property */ > > > >>> > > > >>> }; > > > >>> > > > >>> button@1 { > > > >>> > > > >>> reg = <1>; > > > >>> linux,code = <114>; > > > >>> > > > >>> }; > > > >> > > > >>Ugh no. Having a vendor specific property which is KISS certainly > > > >>beats this, both wrt ease of writing dts files as well as wrt the > > > >>dts parsing code in the driver. > > > > > > > >I'd agree with Heiko here. This is pretty much the same construct > > > >that's already in use in other input drivers, like gpio-keys. > > > > > > In the gpio case there is a 1 on 1 relation between a single hw > > > entity (the gpio-pin) and a single keycode. Here there is 1 hw entity > > > which maps to an array of key-codes, certainly using an array rather > > > then a much more complicated construct is the correct data-structure > > > to represent this. > > > > You can build an array in your driver out of this very easily, it's 10 > > lines in your probe. And you gain from this something that is more > > generic, can be shared by other similar drivers and is consistent with > > what is already in use. > > How will it be shared? Surely not code-wise, but basically in spirit > only. It seems to me that the originally proposed binding is simple and > concise and works well for the driver. I don't think "binding [...] works well for the driver" is the correct direction. From my understanding the binding should describe the hardware in an os-agnostic way (so "linux,foo" properties should stay the exception) and not the data structures used in the driver. The driver itself then implements the binding to convert the binding-data into a structure it wants to use. The sharing would, as you suggested, be in spirit and in the use of already established dt-properties without introducing more non-standard ones. Heiko
On Fri, Jan 03, 2014 at 10:23:50AM -0800, Dmitry Torokhov wrote: > On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote: > > On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 01/02/2014 09:20 PM, Maxime Ripard wrote: > > > >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote: > > > >>>Also, instead of inventing yet another vendor-specific property, why not re-use > > > >>>a button binding similar to gpio-keys like: > > > >>> > > > >>> lradc: lradc@01c22800 { > > > >>> compatible = "allwinner,sun4i-lradc-keys"; > > > >>> reg = <0x01c22800 0x100>; > > > >>> interrupts = <31>; > > > >>> allwinner,chan0-step = <200>; > > > >>> > > > >>> #address-cells = <1>; > > > >>> #size-cells = <0>; > > > >>> > > > >>> button@0 { > > > >>> reg = <0>; /* your channel index from above */ > > > >>> linux,code = <115>; /* already used as dt-property */ > > > >>> }; > > > >>> > > > >>> button@1 { > > > >>> reg = <1>; > > > >>> linux,code = <114>; > > > >>> }; > > > >> > > > >>Ugh no. Having a vendor specific property which is KISS certainly > > > >>beats this, both wrt ease of writing dts files as well as wrt the > > > >>dts parsing code in the driver. > > > > > > > >I'd agree with Heiko here. This is pretty much the same construct > > > >that's already in use in other input drivers, like gpio-keys. > > > > > > In the gpio case there is a 1 on 1 relation between a single hw > > > entity (the gpio-pin) and a single keycode. Here there is 1 hw entity > > > which maps to an array of key-codes, certainly using an array rather > > > then a much more complicated construct is the correct data-structure > > > to represent this. > > > > You can build an array in your driver out of this very easily, it's 10 > > lines in your probe. And you gain from this something that is more > > generic, can be shared by other similar drivers and is consistent with > > what is already in use. > > How will it be shared? Surely not code-wise, but basically in spirit > only. It seems to me that the originally proposed binding is simple and > concise and works well for the driver. See Heiko's answer, but I do believe the code can be shared as well if needs be. > > > >This is also something that can really easily be made generic, > > > >since this is something that is rather common. > > > > > > > >Speaking of which. I believe this should actually come in two > > > >different drivers: > > > > - The ADC driver itself, using IIO > > > > - A generic button handler driver on top of IIO. > > > > > > > > The fact that on most board this adc is used for buttons doesn't make > > > > any difference, it's actually a hardware designer choice, we should > > > > support that choice, but we should also be able to use it just as an > > > > ADC. > > > > > > No, this is not a generic adc, as mentioned in the commit msg, this > > > adc is specifically designed to be used this way. > > > > > > The adc won't start sampling data, and won't generate any interrupts > > > until a button is pressed. That is until the input voltage drops below > > > 2/3 of Vref, this is checked through a built-in analog comparator, which > > > hooks into the control logic. > > > > > > It has button down and button up interrupts, and can detect long > > > presses (unused) and generate a second type of down interrupt for those. > > > > > > This really is an input device, which happens to use an adc. > > > > Hmm, yes, ok. > > > > > >Carlo Caione already started to work on an IIO driver for the LRADC: > > > >https://github.com/carlocaione/linux/tree/sunxi-lradc > > > >maybe you can take over his work. > > > > > > That won't work because the adc won't sample if the input gets above > > > 2/3 of Vref. There may be some other mode which does not do that, but > > > that is not clearly documented. > > > > > > Even if an IIO driver turns out to be doable, I strongly believe that > > > having a separate input driver for this is best, since this device > > > was designed to be used as such. Building input on top of IIO would > > > mean polling the adc, while with my driver it actually generates > > > button down / up interrupts without any polling being involved. > > > > Not really. iio_channel_read calls the read_raw function (in that > > case) of your driver. If the read_raw function in your driver wants to > > poll the device, fine, but most of the time, it will just block > > waiting for an interrupt to come and return the data to the caller, > > which is obviously the saner behaviour, and you don't actually end up > > polling the device. Which is pretty much the architecture you're using > > already, just with an intermediate layer in between. > > What is the benefit of the IIO layer if device can't really be used as > IIO? I am all for moving as many generic devices as we can to IIO but we > should recognize that sometimes the device is not an IIO device. Yes, I've agreed to that. I was just clarifying Hans' statement. Maxime
diff --git a/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt b/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt new file mode 100644 index 0000000..94960da --- /dev/null +++ b/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt @@ -0,0 +1,20 @@ +Allwinner sun4i low res adc attached tablet keys +------------------------------------------------ + +Required properties: + - compatible: "allwinner,sun4i-lradc-keys" + - reg: mmio address range of the chip + - interrupts: interrupt to which the chip is connected + - allwinner,chan0-step: step in mV between keys must be 150 or 200 + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_ codes + +Example: + + lradc: lradc@01c22800 { + compatible = "allwinner,sun4i-lradc-keys"; + reg = <0x01c22800 0x100>; + interrupts = <31>; + allwinner,chan0-step = <200>; + /* KEY_VOLUMEUP VOLUMEDOWN MENU ENTER HOME */ + allwinner,chan0-keycodes = <115 114 139 28 102>; + }; diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index bb174c1..d95e6e4 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -544,6 +544,16 @@ config KEYBOARD_STMPE To compile this driver as a module, choose M here: the module will be called stmpe-keypad. +config KEYBOARD_SUN4I_LRADC + tristate "Allwinner sun4i low res adc attached tablet keys support" + depends on ARCH_SUNXI + help + This selects support for the Allwinner low res adc attached tablet + keys found on Allwinner sunxi SoCs. + + To compile this driver as a module, choose M here: the + module will be called sun4i-lradc-keys. + config KEYBOARD_DAVINCI tristate "TI DaVinci Key Scan" depends on ARCH_DAVINCI_DM365 diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index a699b61..f3265bd 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o obj-$(CONFIG_KEYBOARD_SPEAR) += spear-keyboard.o obj-$(CONFIG_KEYBOARD_STMPE) += stmpe-keypad.o obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o +obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) += sun4i-lradc-keys.o obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c new file mode 100644 index 0000000..9ef4d10 --- /dev/null +++ b/drivers/input/keyboard/sun4i-lradc-keys.c @@ -0,0 +1,243 @@ +/* + * Allwinner sun4i low res adc attached tablet keys driver + * + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Allwinnner sunxi SoCs have a lradc which is specifically designed to have + * various (tablet) keys (ie home, back, search, etc). attached to it using + * a resistor network. This driver is for the keys on such boards. + * + * There are 2 channels, currently this driver only supports chan0 since there + * are no boards known to use chan1. The devicetree properties are already + * prefixed with chan0 as preparation for chan1 support in the future. + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#define LRADC_CTRL 0x00 +#define LRADC_INTC 0x04 +#define LRADC_INTS 0x08 +#define LRADC_DATA0 0x0c +#define LRADC_DATA1 0x10 + +/* LRADC_CTRL bits */ +#define FIRST_CONVERT_DLY(x) ((x) << 24) /* 8 bits */ +#define CHAN_SELECT(x) ((x) << 22) /* 2 bits */ +#define CONTINUE_TIME_SEL(x) ((x) << 16) /* 4 bits */ +#define KEY_MODE_SEL(x) ((x) << 12) /* 2 bits */ +#define LEVELA_B_CNT(x) ((x) << 8) /* 4 bits */ +#define HOLD_EN(x) ((x) << 6) +#define LEVELB_VOL(x) ((x) << 4) /* 2 bits */ +#define SAMPLE_RATE(x) ((x) << 2) /* 2 bits */ +#define ENABLE(x) ((x) << 0) + +/* LRADC_INTC and LRADC_INTS bits */ +#define CHAN1_KEYUP_IRQ BIT(12) +#define CHAN1_ALRDY_HOLD_IRQ BIT(11) +#define CHAN1_HOLD_IRQ BIT(10) +#define CHAN1_KEYDOWN_IRQ BIT(9) +#define CHAN1_DATA_IRQ BIT(8) +#define CHAN0_KEYUP_IRQ BIT(4) +#define CHAN0_ALRDY_HOLD_IRQ BIT(3) +#define CHAN0_HOLD_IRQ BIT(2) +#define CHAN0_KEYDOWN_IRQ BIT(1) +#define CHAN0_DATA_IRQ BIT(0) + +#define MAX_KEYS 13 + +/* Lookup table to map the adc val to a keycode index for 150 mv step size */ +static const u8 adc_val_to_key_index_step150[64] = { + 0, 0, 0, + 1, 1, 1, 1, 1, + 2, 2, 2, 2, 2, + 3, 3, 3, 3, + 4, 4, 4, 4, 4, + 5, 5, 5, 5, 5, + 6, 6, 6, 6, 6, + 7, 7, 7, 7, + 8, 8, 8, 8, 8, + 9, 9, 9, 9, 9, + 10, 10, 10, 10, + 11, 11, 11, 11, + 12, 12, 12, 12, 12, 12, 12, 12, 12, 12 +}; + +/* Lookup table to map the adc val to a keycode index for 200 mv step size */ +static const u8 adc_val_to_key_index_step200[64] = { + 0, 0, 0, 0, 0, 0, 0, 0, + 1, 1, 1, 1, 1, 1, 1, + 2, 2, 2, 2, 2, 2, 2, + 3, 3, 3, 3, 3, 3, + 4, 4, 4, 4, 4, 4, + 5, 5, 5, 5, 5, 5, + 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, + 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7 +}; + +struct sun4i_lradc_data { + struct device *dev; + struct input_dev *input; + void __iomem *base; + u32 chan0_step; + u32 chan0_keycode; + u32 chan0_keycodes[MAX_KEYS]; +}; + +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id) +{ + struct sun4i_lradc_data *lradc = dev_id; + u32 ints, val; + + ints = readl(lradc->base + LRADC_INTS); + + /* + * lradc supports only one keypress at a time, release does not give + * any info as to which key was released, so we cache the keycode. + */ + if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) { + val = readl(lradc->base + LRADC_DATA0); + if (lradc->chan0_step == 150) + val = adc_val_to_key_index_step150[val]; + else + val = adc_val_to_key_index_step200[val]; + + lradc->chan0_keycode = lradc->chan0_keycodes[val]; + input_report_key(lradc->input, lradc->chan0_keycode, 1); + } + + if (ints & CHAN0_KEYUP_IRQ) { + input_report_key(lradc->input, lradc->chan0_keycode, 0); + lradc->chan0_keycode = 0; + } + + input_sync(lradc->input); + + writel(ints, lradc->base + LRADC_INTS); + + return IRQ_HANDLED; +} + +static int sun4i_lradc_open(struct input_dev *dev) +{ + struct sun4i_lradc_data *lradc = input_get_drvdata(dev); + + /* + * Set sample time to 16 ms / 62.5 Hz. Wait 2 * 16 ms for key to + * stabilize on press, wait (1 + 1) * 16 ms for key release + */ + writel(FIRST_CONVERT_DLY(2) | LEVELA_B_CNT(1) | HOLD_EN(1) | + SAMPLE_RATE(2) | ENABLE(1), lradc->base + LRADC_CTRL); + + writel(CHAN0_KEYUP_IRQ | CHAN0_KEYDOWN_IRQ, lradc->base + LRADC_INTC); + + return 0; +} + +static void sun4i_lradc_close(struct input_dev *dev) +{ + struct sun4i_lradc_data *lradc = input_get_drvdata(dev); + + /* Disable lradc, leave other settings unchanged */ + writel(FIRST_CONVERT_DLY(2) | LEVELA_B_CNT(1) | HOLD_EN(1) | + SAMPLE_RATE(2), lradc->base + LRADC_CTRL); + writel(0, lradc->base + LRADC_INTC); +} + +static int sun4i_lradc_probe(struct platform_device *pdev) +{ + struct sun4i_lradc_data *lradc; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + int i, ret; + + lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), GFP_KERNEL); + if (!lradc) + return -ENOMEM; + + ret = of_property_read_u32(np, "allwinner,chan0-step", + &lradc->chan0_step); + if (ret || (lradc->chan0_step != 150 && lradc->chan0_step != 200)) { + dev_err(dev, "Invalid allwinner,chan0-step dt-property\n"); + return -EINVAL; + } + + for (i = 0; i < MAX_KEYS; i++) + of_property_read_u32_index(np, "allwinner,chan0-keycodes", + i, &lradc->chan0_keycodes[i]); + + lradc->dev = dev; + lradc->input = devm_input_allocate_device(dev); + if (!lradc->input) + return -ENOMEM; + + lradc->input->name = pdev->name; + lradc->input->phys = "sun4i_lradc/input0"; + lradc->input->open = sun4i_lradc_open; + lradc->input->close = sun4i_lradc_close; + lradc->input->id.bustype = BUS_HOST; + lradc->input->id.vendor = 0x0001; + lradc->input->id.product = 0x0001; + lradc->input->id.version = 0x0100; + lradc->input->evbit[0] = BIT(EV_SYN) | BIT(EV_KEY); + for (i = 0; i < MAX_KEYS; i++) + set_bit(lradc->chan0_keycodes[i], lradc->input->keybit); + input_set_drvdata(lradc->input, lradc); + + lradc->base = devm_ioremap_resource(dev, + platform_get_resource(pdev, IORESOURCE_MEM, 0)); + if (IS_ERR(lradc->base)) + return PTR_ERR(lradc->base); + + ret = devm_request_irq(dev, platform_get_irq(pdev, 0), sun4i_lradc_irq, + 0, "sun4i-lradc-keys", lradc); + if (ret) + return ret; + + ret = input_register_device(lradc->input); + if (ret) + return ret; + + platform_set_drvdata(pdev, lradc); + return 0; +} + +static const struct of_device_id sun4i_lradc_of_match[] = { + { .compatible = "allwinner,sun4i-lradc-keys", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match); + +static struct platform_driver sun4i_lradc_driver = { + .driver = { + .owner = THIS_MODULE, + .name = "sun4i-lradc-keys", + .of_match_table = of_match_ptr(sun4i_lradc_of_match), + }, + .probe = sun4i_lradc_probe, +}; + +module_platform_driver(sun4i_lradc_driver); + +MODULE_DESCRIPTION("Allwinner sun4i low res adc attached tablet keys driver"); +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); +MODULE_LICENSE("GPL");
Allwinnner sunxi SoCs have a low resolution adc (called lradc) which is specifically designed to have various (tablet) keys (ie home, back, search, etc). attached to it using a resistor network. This adds a driver for this. There are 2 channels, currently this driver only supports chan0 since there are no boards known to use chan1. The devicetree properties are already prefixed with chan0 as preparation for chan1 support in the future. This has been tested on an olimex a10s-olinuxino-micro, a13-olinuxino, and a20-olinuxino-micro. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../devicetree/bindings/input/sun4i-lradc-keys.txt | 20 ++ drivers/input/keyboard/Kconfig | 10 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/sun4i-lradc-keys.c | 243 +++++++++++++++++++++ 4 files changed, 274 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt create mode 100644 drivers/input/keyboard/sun4i-lradc-keys.c