于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <[email protected]> 写到:
>Hi,
>
>On Thu, Apr 28, 2022 at 09:09:03PM +0000, Ruslan Zalata wrote:
>> Some Allwinner SoCs like A13, A20 or T2 are equipped with two-channel
>> low rate (6 bit) ADC that is often used for extra keys. There's a driver
>> for that already implementing standard input device, but it has these
>> limitations: 1) it cannot be used for general ADC data equisition, and
>> 2) it uses only one LRADC channel of two available.
>>
>> This driver provides basic hwmon interface to both channels of LRADC on
>> such Allwinner SoCs.
>>
>> Signed-off-by: Ruslan Zalata <[email protected]>
>> ---
>> MAINTAINERS | 6 +
>> drivers/hwmon/Kconfig | 13 ++
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
>> 4 files changed, 300 insertions(+)
>> create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5e8c2f61176..d9c71e94133 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18861,6 +18861,12 @@ S: Maintained
>> F: Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
>> F: drivers/input/keyboard/sun4i-lradc-keys.c
>>
>> +SUN4I LOW RES ADC HWMON DRIVER
>> +M: Ruslan Zalata <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <[email protected]>
>> L: [email protected]
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 68a8a27ab3b..86776488a81 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1691,6 +1691,19 @@ config SENSORS_SIS5595
>> This driver can also be built as a module. If so, the module
>> will be called sis5595.
>>
>> +config SENSORS_SUN4I_LRADC
>> + tristate "Allwinner A13/A20 LRADC hwmon"
>> + depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
>> + help
>> + Say y here to support the LRADC found in Allwinner A13/A20 SoCs.
>> + Both channels are supported.
>> +
>> + This driver can also be built as module. If so, the module
>> + will be called sun4i-lradc-hwmon.
>> +
>> + This option is not compatible with KEYBOARD_SUN4I_LRADC, one
>> + of these must be used at a time.
>
>How do you plan on enforcing that?
>
>I guess a better path forward would be to either register an hwmon
>device in the original driver, or convert that driver to iio and use
>iio-hwmon.
I think this driver should be use IIO, and then try to probe an IIO input
if possible.
>
>Maxime
On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
>
>
> 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <[email protected]> 写到:
> >Hi,
> >
> >On Thu, Apr 28, 2022 at 09:09:03PM +0000, Ruslan Zalata wrote:
> >> Some Allwinner SoCs like A13, A20 or T2 are equipped with two-channel
> >> low rate (6 bit) ADC that is often used for extra keys. There's a driver
> >> for that already implementing standard input device, but it has these
> >> limitations: 1) it cannot be used for general ADC data equisition, and
> >> 2) it uses only one LRADC channel of two available.
> >>
> >> This driver provides basic hwmon interface to both channels of LRADC on
> >> such Allwinner SoCs.
> >>
> >> Signed-off-by: Ruslan Zalata <[email protected]>
> >> ---
> >> MAINTAINERS | 6 +
> >> drivers/hwmon/Kconfig | 13 ++
> >> drivers/hwmon/Makefile | 1 +
> >> drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
> >> 4 files changed, 300 insertions(+)
> >> create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 5e8c2f61176..d9c71e94133 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -18861,6 +18861,12 @@ S: Maintained
> >> F: Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
> >> F: drivers/input/keyboard/sun4i-lradc-keys.c
> >>
> >> +SUN4I LOW RES ADC HWMON DRIVER
> >> +M: Ruslan Zalata <[email protected]>
> >> +L: [email protected]
> >> +S: Maintained
> >> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> >> +
> >> SUNDANCE NETWORK DRIVER
> >> M: Denis Kirjanov <[email protected]>
> >> L: [email protected]
> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >> index 68a8a27ab3b..86776488a81 100644
> >> --- a/drivers/hwmon/Kconfig
> >> +++ b/drivers/hwmon/Kconfig
> >> @@ -1691,6 +1691,19 @@ config SENSORS_SIS5595
> >> This driver can also be built as a module. If so, the module
> >> will be called sis5595.
> >>
> >> +config SENSORS_SUN4I_LRADC
> >> + tristate "Allwinner A13/A20 LRADC hwmon"
> >> + depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
> >> + help
> >> + Say y here to support the LRADC found in Allwinner A13/A20 SoCs.
> >> + Both channels are supported.
> >> +
> >> + This driver can also be built as module. If so, the module
> >> + will be called sun4i-lradc-hwmon.
> >> +
> >> + This option is not compatible with KEYBOARD_SUN4I_LRADC, one
> >> + of these must be used at a time.
> >
> >How do you plan on enforcing that?
> >
> >I guess a better path forward would be to either register an hwmon
> >device in the original driver, or convert that driver to iio and use
> >iio-hwmon.
>
> I think this driver should be use IIO, and then try to probe an IIO input
> if possible.
It's been a while, but if I remember well we couldn't use IIO for that
driver because it's not generating interrupts all the time but only when
it goes over a given threshold:
https://lore.kernel.org/all/[email protected]/
I'm not sure if it's still relevant, so we might just need to add an
hwmon driver to the existing driver
Maxime
On 5/2/22 04:21, Maxime Ripard wrote:
> On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
>>
>>
>> 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <[email protected]> 写到:
>>> Hi,
>>>
>>> On Thu, Apr 28, 2022 at 09:09:03PM +0000, Ruslan Zalata wrote:
>>>> Some Allwinner SoCs like A13, A20 or T2 are equipped with two-channel
>>>> low rate (6 bit) ADC that is often used for extra keys. There's a driver
>>>> for that already implementing standard input device, but it has these
>>>> limitations: 1) it cannot be used for general ADC data equisition, and
>>>> 2) it uses only one LRADC channel of two available.
>>>>
>>>> This driver provides basic hwmon interface to both channels of LRADC on
>>>> such Allwinner SoCs.
>>>>
>>>> Signed-off-by: Ruslan Zalata <[email protected]>
>>>> ---
>>>> MAINTAINERS | 6 +
>>>> drivers/hwmon/Kconfig | 13 ++
>>>> drivers/hwmon/Makefile | 1 +
>>>> drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
>>>> 4 files changed, 300 insertions(+)
>>>> create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 5e8c2f61176..d9c71e94133 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -18861,6 +18861,12 @@ S: Maintained
>>>> F: Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
>>>> F: drivers/input/keyboard/sun4i-lradc-keys.c
>>>>
>>>> +SUN4I LOW RES ADC HWMON DRIVER
>>>> +M: Ruslan Zalata <[email protected]>
>>>> +L: [email protected]
>>>> +S: Maintained
>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>> SUNDANCE NETWORK DRIVER
>>>> M: Denis Kirjanov <[email protected]>
>>>> L: [email protected]
>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> index 68a8a27ab3b..86776488a81 100644
>>>> --- a/drivers/hwmon/Kconfig
>>>> +++ b/drivers/hwmon/Kconfig
>>>> @@ -1691,6 +1691,19 @@ config SENSORS_SIS5595
>>>> This driver can also be built as a module. If so, the module
>>>> will be called sis5595.
>>>>
>>>> +config SENSORS_SUN4I_LRADC
>>>> + tristate "Allwinner A13/A20 LRADC hwmon"
>>>> + depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
>>>> + help
>>>> + Say y here to support the LRADC found in Allwinner A13/A20 SoCs.
>>>> + Both channels are supported.
>>>> +
>>>> + This driver can also be built as module. If so, the module
>>>> + will be called sun4i-lradc-hwmon.
>>>> +
>>>> + This option is not compatible with KEYBOARD_SUN4I_LRADC, one
>>>> + of these must be used at a time.
>>>
>>> How do you plan on enforcing that?
>>>
>>> I guess a better path forward would be to either register an hwmon
>>> device in the original driver, or convert that driver to iio and use
>>> iio-hwmon.
>>
>> I think this driver should be use IIO, and then try to probe an IIO input
>> if possible.
>
> It's been a while, but if I remember well we couldn't use IIO for that
> driver because it's not generating interrupts all the time but only when
> it goes over a given threshold:
>
> https://lore.kernel.org/all/[email protected]/
>
> I'm not sure if it's still relevant, so we might just need to add an
> hwmon driver to the existing driver
>
So now we have conflicting claims that the hwmon driver would need
to implement continuous interrupts because the chip otherwise doesn't
continuously measure ADC input, and that implementing an IIO driver
isn't possible or doesn't make sense because the chip would not support
generating continuous interrupts. Which one is it ? Am I missing something ?
Guenter
Hi Guenter,
LRADC does generate continuous interrupts as long as input voltage is
below LevelB threshold. The max possible LevelB is 0x3C which in case of
A20 SoC is close to 1.90V and that's what my driver sets LevelB to.
Perhaps this needs to be documented more thoroughly.
It is possible to implement this same driver for IIO subsystem, but I
would prefer to keep it in hwmon along with many other simple ADC
drivers used for temp and battery status monitoring.
If we talk about IIO, it will be necessary to implement serialization of
reads and updates which brings up some complexity I would try to avoid
at the moment. :)
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-05-03 07:02, Guenter Roeck wrote:
> On 5/2/22 04:21, Maxime Ripard wrote:
>> On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
>>>
>>>
>>> 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <[email protected]>
>>> 写到:
>>>> Hi,
>>>>
>>>> On Thu, Apr 28, 2022 at 09:09:03PM +0000, Ruslan Zalata wrote:
>>>>> Some Allwinner SoCs like A13, A20 or T2 are equipped with
>>>>> two-channel
>>>>> low rate (6 bit) ADC that is often used for extra keys. There's a
>>>>> driver
>>>>> for that already implementing standard input device, but it has
>>>>> these
>>>>> limitations: 1) it cannot be used for general ADC data equisition,
>>>>> and
>>>>> 2) it uses only one LRADC channel of two available.
>>>>>
>>>>> This driver provides basic hwmon interface to both channels of
>>>>> LRADC on
>>>>> such Allwinner SoCs.
>>>>>
>>>>> Signed-off-by: Ruslan Zalata <[email protected]>
>>>>> ---
>>>>> MAINTAINERS | 6 +
>>>>> drivers/hwmon/Kconfig | 13 ++
>>>>> drivers/hwmon/Makefile | 1 +
>>>>> drivers/hwmon/sun4i-lradc-hwmon.c | 280
>>>>> ++++++++++++++++++++++++++++++
>>>>> 4 files changed, 300 insertions(+)
>>>>> create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 5e8c2f61176..d9c71e94133 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -18861,6 +18861,12 @@ S: Maintained
>>>>>
>>>>> F: Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
>>>>> F: drivers/input/keyboard/sun4i-lradc-keys.c
>>>>> +SUN4I LOW RES ADC HWMON DRIVER
>>>>> +M: Ruslan Zalata <[email protected]>
>>>>> +L: [email protected]
>>>>> +S: Maintained
>>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>>> +
>>>>> SUNDANCE NETWORK DRIVER
>>>>> M: Denis Kirjanov <[email protected]>
>>>>> L: [email protected]
>>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>>> index 68a8a27ab3b..86776488a81 100644
>>>>> --- a/drivers/hwmon/Kconfig
>>>>> +++ b/drivers/hwmon/Kconfig
>>>>> @@ -1691,6 +1691,19 @@ config SENSORS_SIS5595
>>>>> This driver can also be built as a module. If so, the module
>>>>> will be called sis5595.
>>>>> +config SENSORS_SUN4I_LRADC
>>>>> + tristate "Allwinner A13/A20 LRADC hwmon"
>>>>> + depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
>>>>> + help
>>>>> + Say y here to support the LRADC found in Allwinner A13/A20
>>>>> SoCs.
>>>>> + Both channels are supported.
>>>>> +
>>>>> + This driver can also be built as module. If so, the module
>>>>> + will be called sun4i-lradc-hwmon.
>>>>> +
>>>>> + This option is not compatible with KEYBOARD_SUN4I_LRADC, one
>>>>> + of these must be used at a time.
>>>>
>>>> How do you plan on enforcing that?
>>>>
>>>> I guess a better path forward would be to either register an hwmon
>>>> device in the original driver, or convert that driver to iio and use
>>>> iio-hwmon.
>>>
>>> I think this driver should be use IIO, and then try to probe an IIO
>>> input
>>> if possible.
>>
>> It's been a while, but if I remember well we couldn't use IIO for that
>> driver because it's not generating interrupts all the time but only
>> when
>> it goes over a given threshold:
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> I'm not sure if it's still relevant, so we might just need to add an
>> hwmon driver to the existing driver
>>
>
> So now we have conflicting claims that the hwmon driver would need
> to implement continuous interrupts because the chip otherwise doesn't
> continuously measure ADC input, and that implementing an IIO driver
> isn't possible or doesn't make sense because the chip would not support
> generating continuous interrupts. Which one is it ? Am I missing
> something ?
>
> Guenter
On Tue, May 03, 2022 at 08:26:18PM +0500, Ruslan Zalata wrote:
> LRADC does generate continuous interrupts as long as input voltage is below
> LevelB threshold. The max possible LevelB is 0x3C which in case of A20 SoC
> is close to 1.90V and that's what my driver sets LevelB to. Perhaps this
> needs to be documented more thoroughly.
>
> It is possible to implement this same driver for IIO subsystem, but I would
> prefer to keep it in hwmon along with many other simple ADC drivers used for
> temp and battery status monitoring.
If you can get it work reliably enough, I think IIO+iio-hwmon is still
the way to go
The main issue here is that drivers that are decided at compile time are
kind of a pain as soon as you try to install a generic distro.
At the hardware level, I'd assume you would either use the LRADC as an
actual ADC, or use it to drive buttons, right?
So, you would have to change the device tree accordingly anyway, to
either list buttons and their associated voltages or use it to probe
whatever signal to have there.
So I don't think a new device tree binding is such a deal breaker since
you have to describe it differently anyway.
Since that would be a completely different use-case, the IIO driver
doesn't have to support input right away, it can be done later if
needed.
And you could have the two drivers compiled at the same time.
Maxime
Hi Maxime,
> At the hardware level, I'd assume you would either use the LRADC as an
> actual ADC, or use it to drive buttons, right?
Yes, exactly.
> So I don't think a new device tree binding is such a deal breaker since
> you have to describe it differently anyway.
...
> Since that would be a completely different use-case, the IIO driver
> doesn't have to support input right away, it can be done later if
> needed.
>
> And you could have the two drivers compiled at the same time.
As I got you right, you propose do add new bindings, say
"allwinner,sun4i-a10-lradc-hwmon" and "allwinner,sun8i-a83t-lradc-hwmon"
for new driver, which will allow two drivers (hwmon and keyboard) be
compiled and loaded at same time, only that one listed in DT will be
instantiated. If two are listed at same time, one of the calls to
devm_request_irq() will return with an error preventing second driver to
be probed (some error message would be necessary to let user know what's
going on). If this is ok, I will implement it.
I think moving this driver to IIO framework is overkill. We use LRADC to
monitor battery temp and state (voltage) and that's what HWMON was made
for. It's simple, easy and elegant. IIO, on the other hand, is for data
acquisition and is much more complex beast. Can we stay with HWMON,
please ? :)
I looked through the code for a number of iio/adc drivers and I could
see that all of them initiate ADC conversion inside read(), then wait
for completion and return single sample. For me this very flawed
approach because a) much more overhead/load on the system, b) initiating
conversion may (and will) take more time than a single consequent
conversion, c) samples will be read in irregular periods of time, hence
acquired data will not be consistent for any further processing (like
FFT). So, this whole IIO framework is no way better than HWMON, yet more
complex. At least for ADCs. :-)
A better approach for an IIO/ADC driver would be to implement some
serialization mechanism to let reads go in sync with updates (IRQs),
with buffering, guaranteeing no same sample is read twice and no sample
is lost. The read() would return next available sample from buffer with
nearly zero overhead or sleep till data is available.
And the best way is to extend IIO framework to support ring buffers
mechanism like the one proposed by Analog Devices, but that's a way
different story. Link:
https://events.static.linuxfound.org/sites/events/files/slides/iio_high_speed.pdf
Regards,
Ruslan.
Fabmicro, LLC.