* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-04 19:33 Jamie Cuthbert
0 siblings, 0 replies; 43+ messages in thread
From: Jamie Cuthbert @ 2022-05-04 19:33 UTC (permalink / raw)
To: rz
Cc: jdelvare, jernej.skrabec, linux-arm-kernel, linux-hwmon,
linux-kernel, linux-sunxi, linux, samuel, wens
Sent from my iPhone
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-04 19:33 Jamie Cuthbert
0 siblings, 0 replies; 43+ messages in thread
From: Jamie Cuthbert @ 2022-05-04 19:33 UTC (permalink / raw)
To: rz
Cc: jdelvare, jernej.skrabec, linux-arm-kernel, linux-hwmon,
linux-kernel, linux-sunxi, linux, samuel, wens
Sent from my iPhone
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-03 21:37 ` Ruslan Zalata
@ 2022-05-04 14:12 ` Maxime Ripard
-1 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-04 14:12 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3735 bytes --]
On Wed, May 04, 2022 at 02:37:32AM +0500, Ruslan Zalata wrote:
> 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.
Compatibles are meant to describe the hardware and remain OS-agnostic,
while hwmon is linux-specific so we should probably drop the hwmon from
the compatible. But otherwise, yes.
> 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.
Yeah, that's that *you* use it for. If someone wants to use it for some
other use, what's going to happen? Will we create a third driver for the
exact same controller? That's not reasonable.
> IIO, on the other hand, is for data acquisition and is much more
> complex beast. Can we stay with HWMON, please ? :)
But it's generic, and you can plug hwmon on top of it. So you don't lose
any feature, but it also doesn't prevent any one else to use it for some
slightly different usecase either.
> 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,
To monitor a battery? What kind of polling interval do you expect on this?
> b) initiating conversion may (and will) take more time than a single
> consequent conversion,
Ditto, I'd expect to have a request in the order of seconds for a
battery, so the setup time will be negligible.
> 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.
Like this:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html ?
> 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
Those suggestions were to handle more than a ~100 kilosamples per
seconds order of magnitude. How many samples per seconds do you expect
to get out from this ADC?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-04 14:12 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-04 14:12 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 3735 bytes --]
On Wed, May 04, 2022 at 02:37:32AM +0500, Ruslan Zalata wrote:
> 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.
Compatibles are meant to describe the hardware and remain OS-agnostic,
while hwmon is linux-specific so we should probably drop the hwmon from
the compatible. But otherwise, yes.
> 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.
Yeah, that's that *you* use it for. If someone wants to use it for some
other use, what's going to happen? Will we create a third driver for the
exact same controller? That's not reasonable.
> IIO, on the other hand, is for data acquisition and is much more
> complex beast. Can we stay with HWMON, please ? :)
But it's generic, and you can plug hwmon on top of it. So you don't lose
any feature, but it also doesn't prevent any one else to use it for some
slightly different usecase either.
> 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,
To monitor a battery? What kind of polling interval do you expect on this?
> b) initiating conversion may (and will) take more time than a single
> consequent conversion,
Ditto, I'd expect to have a request in the order of seconds for a
battery, so the setup time will be negligible.
> 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.
Like this:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html ?
> 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
Those suggestions were to handle more than a ~100 kilosamples per
seconds order of magnitude. How many samples per seconds do you expect
to get out from this ADC?
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-03 16:14 ` Maxime Ripard
@ 2022-05-03 21:37 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 21:37 UTC (permalink / raw)
To: Maxime Ripard
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 21:37 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 21:37 UTC (permalink / raw)
To: Maxime Ripard
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-03 15:26 ` Ruslan Zalata
@ 2022-05-03 16:14 ` Maxime Ripard
-1 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-03 16:14 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 16:14 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-03 16:14 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 1408 bytes --]
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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-03 8:50 ` Ruslan Zalata
@ 2022-05-03 16:07 ` Maxime Ripard
-1 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-03 16:07 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
Hi,
On Tue, May 03, 2022 at 01:50:39PM +0500, Ruslan Zalata wrote:
> > 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.
>
> My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as I
> began hacking the driver I quickly realized that it would be a mess since
> keyboard and hwmon belong to two different subsystems.
That's not really an issue in itself. There's plenty of drivers in Linux
that register into two frameworks.
> Besides we would need to invent a way to control which way the driver
> works (new bindings?).
I got confused there, and thought you were adding temperature reading
that is in another ADC on those SoCs, sorry. Yeah, that doesn't make
much sense to have both in the same drivers here.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 16:07 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-03 16:07 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]
Hi,
On Tue, May 03, 2022 at 01:50:39PM +0500, Ruslan Zalata wrote:
> > 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.
>
> My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as I
> began hacking the driver I quickly realized that it would be a mess since
> keyboard and hwmon belong to two different subsystems.
That's not really an issue in itself. There's plenty of drivers in Linux
that register into two frameworks.
> Besides we would need to invent a way to control which way the driver
> works (new bindings?).
I got confused there, and thought you were adding temperature reading
that is in another ADC on those SoCs, sorry. Yeah, that doesn't make
much sense to have both in the same drivers here.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-03 2:02 ` Guenter Roeck
@ 2022-05-03 15:26 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 15:26 UTC (permalink / raw)
To: Guenter Roeck
Cc: Maxime Ripard, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
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 <maxime@cerno.tech>
>>> 写到:
>>>> 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 <rz@fabmicro.ru>
>>>>> ---
>>>>> 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 <rz@fabmicro.ru>
>>>>> +L: linux-hwmon@vger.kernel.org
>>>>> +S: Maintained
>>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>>> +
>>>>> SUNDANCE NETWORK DRIVER
>>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>>> L: netdev@vger.kernel.org
>>>>> 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/52C5E9F1.9010700@redhat.com/
>>
>> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 15:26 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 15:26 UTC (permalink / raw)
To: Guenter Roeck
Cc: Maxime Ripard, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
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 <maxime@cerno.tech>
>>> 写到:
>>>> 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 <rz@fabmicro.ru>
>>>>> ---
>>>>> 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 <rz@fabmicro.ru>
>>>>> +L: linux-hwmon@vger.kernel.org
>>>>> +S: Maintained
>>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>>> +
>>>>> SUNDANCE NETWORK DRIVER
>>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>>> L: netdev@vger.kernel.org
>>>>> 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/52C5E9F1.9010700@redhat.com/
>>
>> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 11:00 ` Maxime Ripard
@ 2022-05-03 8:50 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 8:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
Hi Maxime,
> 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.
My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as
I began hacking the driver I quickly realized that it would be a mess
since keyboard and hwmon belong to two different subsystems. Besides we
would need to invent a way to control which way the driver works (new
bindings?). So I decided that a new independent small driver would be a
better solution.
---
Regards,
Ruslan.
Fabmicro, LLC.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 8:50 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-03 8:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
Hi Maxime,
> 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.
My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as
I began hacking the driver I quickly realized that it would be a mess
since keyboard and hwmon belong to two different subsystems. Besides we
would need to invent a way to control which way the driver works (new
bindings?). So I decided that a new independent small driver would be a
better solution.
---
Regards,
Ruslan.
Fabmicro, LLC.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 11:21 ` Maxime Ripard
@ 2022-05-03 2:02 ` Guenter Roeck
-1 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-03 2:02 UTC (permalink / raw)
To: Maxime Ripard, Icenowy Zheng
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
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 <maxime@cerno.tech> 写到:
>>> 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 <rz@fabmicro.ru>
>>>> ---
>>>> 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 <rz@fabmicro.ru>
>>>> +L: linux-hwmon@vger.kernel.org
>>>> +S: Maintained
>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>> SUNDANCE NETWORK DRIVER
>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>> L: netdev@vger.kernel.org
>>>> 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/52C5E9F1.9010700@redhat.com/
>
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-03 2:02 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-03 2:02 UTC (permalink / raw)
To: Maxime Ripard, Icenowy Zheng
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
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 <maxime@cerno.tech> 写到:
>>> 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 <rz@fabmicro.ru>
>>>> ---
>>>> 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 <rz@fabmicro.ru>
>>>> +L: linux-hwmon@vger.kernel.org
>>>> +S: Maintained
>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>> SUNDANCE NETWORK DRIVER
>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>> L: netdev@vger.kernel.org
>>>> 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/52C5E9F1.9010700@redhat.com/
>
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-29 6:03 ` Guenter Roeck
@ 2022-05-02 23:47 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-02 23:47 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
Hi Guenter,
I'm sorry to disappoint you, but continuous mode for LRADC works only
for key presses (significant voltage change), it does not work for raw
data. Here's an excerpt from the manual that supports my discovery
(punctuation and grammar preserved):
> The LRADC have three mode, Normal Mode、Single Mode and Continue Mode.
> Normal mode is that the LRADC will
> report the result data of each convert all the time when the key is
> down. Single Mode is that the LRADC
> will only report the first convert result data when the key is down.
> Continue Mode is that the LRADC will
> report one of 8*(N+1) (N is program can set) sample convert result data
> when key is down.
In other words, all three modes require key down event (voltage change)
and IRQ is the only way to get continuous raw data updates from LRADC.
I've experimented quite a lot with this for past few days, there are no
changes to values in ADC data regs except in DATA IRQ mode.
Regarding variant structure. Vref is not the only difference between
different implementations of LRADC in different Allwinner SoCs. For
example, A83T has only one channel LRADC instead of two channels in
A10/A13/A20. Some other SoCs may have even more differences. I
introduced hwmon_chip_info structure into variant to encapsulate such
differences in one place. Patch version 3 will follow. Thank you.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 11:03, Guenter Roeck wrote:
> On 4/28/22 22:32, Guenter Roeck wrote:
>> On 4/28/22 17:28, Ruslan Zalata wrote:
>>> Thank you Guenter for your valuable time.
>>>
>>> I have added update_interval option (it's in ms units, right?) and
>>> fixed all other issues you pointed to. Will test it on real hardware
>>> and send third version of the patch for review.
>>>
>>> Regarding IRQ. Alternatively the driver would need to sit and poll
>>> conversion ready bit in a loop which might cause a much worse load on
>>> system, is not it ? Anyway, the real problem with this piece of
>>> hardware is that there's no "conversion ready bit" provided, the only
>>> way to know data ready status is to receive an interrupt.
>>>
>>
>> Not necessarily. The data does not have to be "current", after all,
>> if the hardware is able to continuously convert. If not, the question
>> is how long a conversion takes. If it doesn't take too long, it would
>> be better to initiate a conversion and then wait for the completion.
>>
>>> I think it still needs a semaphore/seqlock to synchronize conversions
>>> and reads. I.e. two consequent reads should not return same old
>>> value. Although it's not an issue in my case, but could be a problem
>>> for others.
>>>
>> Why ? That happens for almost all hwmon devices. They will all report
>> the most recent conversion value. Some of them can take seconds
>> to complete a new conversion, so the reported value is always "old"
>> for a given defition of old (ie any time smaller than a conversion
>> interval).
>>
>> Sigh. Looks like I'll have to dig up the documentation and read about
>> the ADC myself.
>>
>
> I did, for both A13 and A20. The ADC supports continuous mode. That
> means it can be configured accordingly, and reading the ADC value
> just returns the most recent conversion value. There is absolutely
> no need to keep reading the conversion values using interrupts.
>
> Also,
>
> +struct lradc_variant {
> + u32 bits;
> + u32 resolution;
> + u32 vref;
> +};
>
> is unnecessary because the values are the same for both supported
> chips.
> That means that defines can and should be used. Yes, I can see that
> A83T uses a different voltage, but even that doesn't need a structure -
> the voltage can be set in struct sun4i_lradc_data if/when needed,
> and the resolution and number of bits is still the same.
>
> Guenter
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 23:47 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-02 23:47 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
Hi Guenter,
I'm sorry to disappoint you, but continuous mode for LRADC works only
for key presses (significant voltage change), it does not work for raw
data. Here's an excerpt from the manual that supports my discovery
(punctuation and grammar preserved):
> The LRADC have three mode, Normal Mode、Single Mode and Continue Mode.
> Normal mode is that the LRADC will
> report the result data of each convert all the time when the key is
> down. Single Mode is that the LRADC
> will only report the first convert result data when the key is down.
> Continue Mode is that the LRADC will
> report one of 8*(N+1) (N is program can set) sample convert result data
> when key is down.
In other words, all three modes require key down event (voltage change)
and IRQ is the only way to get continuous raw data updates from LRADC.
I've experimented quite a lot with this for past few days, there are no
changes to values in ADC data regs except in DATA IRQ mode.
Regarding variant structure. Vref is not the only difference between
different implementations of LRADC in different Allwinner SoCs. For
example, A83T has only one channel LRADC instead of two channels in
A10/A13/A20. Some other SoCs may have even more differences. I
introduced hwmon_chip_info structure into variant to encapsulate such
differences in one place. Patch version 3 will follow. Thank you.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 11:03, Guenter Roeck wrote:
> On 4/28/22 22:32, Guenter Roeck wrote:
>> On 4/28/22 17:28, Ruslan Zalata wrote:
>>> Thank you Guenter for your valuable time.
>>>
>>> I have added update_interval option (it's in ms units, right?) and
>>> fixed all other issues you pointed to. Will test it on real hardware
>>> and send third version of the patch for review.
>>>
>>> Regarding IRQ. Alternatively the driver would need to sit and poll
>>> conversion ready bit in a loop which might cause a much worse load on
>>> system, is not it ? Anyway, the real problem with this piece of
>>> hardware is that there's no "conversion ready bit" provided, the only
>>> way to know data ready status is to receive an interrupt.
>>>
>>
>> Not necessarily. The data does not have to be "current", after all,
>> if the hardware is able to continuously convert. If not, the question
>> is how long a conversion takes. If it doesn't take too long, it would
>> be better to initiate a conversion and then wait for the completion.
>>
>>> I think it still needs a semaphore/seqlock to synchronize conversions
>>> and reads. I.e. two consequent reads should not return same old
>>> value. Although it's not an issue in my case, but could be a problem
>>> for others.
>>>
>> Why ? That happens for almost all hwmon devices. They will all report
>> the most recent conversion value. Some of them can take seconds
>> to complete a new conversion, so the reported value is always "old"
>> for a given defition of old (ie any time smaller than a conversion
>> interval).
>>
>> Sigh. Looks like I'll have to dig up the documentation and read about
>> the ADC myself.
>>
>
> I did, for both A13 and A20. The ADC supports continuous mode. That
> means it can be configured accordingly, and reading the ADC value
> just returns the most recent conversion value. There is absolutely
> no need to keep reading the conversion values using interrupts.
>
> Also,
>
> +struct lradc_variant {
> + u32 bits;
> + u32 resolution;
> + u32 vref;
> +};
>
> is unnecessary because the values are the same for both supported
> chips.
> That means that defines can and should be used. Yes, I can see that
> A83T uses a different voltage, but even that doesn't need a structure -
> the voltage can be set in struct sun4i_lradc_data if/when needed,
> and the resolution and number of bits is still the same.
>
> Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-29 13:24 ` Jonathan Cameron
@ 2022-05-02 23:20 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-02 23:20 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
Hi Jonathan,
Thank you for pointing to DEFINE_SIMPLE_DEV_PM_OPS macros, will use it.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 18:24, Jonathan Cameron wrote:
> On Thu, 28 Apr 2022 14:30:06 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On 4/28/22 14:09, 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
>>
>> acquisition
>>
>> > 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 <rz@fabmicro.ru>
>>
>> Ok, next phase of review.
>>
>
> One thing noticed whilst randomly glancing at this patch.
>
>
>> > +#ifdef CONFIG_PM
>> > +static int sun4i_lradc_resume(struct device *dev)
>> > +{
>> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > + return sun4i_lradc_start(lradc);
>> > +}
>> > +
>> > +static int sun4i_lradc_suspend(struct device *dev)
>> > +{
>> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > + sun4i_lradc_stop(lradc);
>> > + return 0;
>> > +}
>> > +
>> > +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
>> > +#else
>> > +#define SUN4I_LRADC_DEV_PM_OPS NULL
>> > +#endif /* CONFIG_PM */
>> > +
>> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
>
> We have much better infrastructure for this these days.
>
> Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
> and pm_sleep_ptr()
>
> Those two in combination will let you get rid of all the ifdef stuff
> here by letting the compiler remove the unused code automatically.
>
>> > + .suspend = sun4i_lradc_suspend,
>> > + .resume = sun4i_lradc_resume,
>> > +};
>> > +
>> > +static const struct of_device_id sun4i_lradc_of_match[] = {
>> > + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
>> > + { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> > +
>> > +static struct platform_driver sun4i_lradc_driver = {
>> > + .driver = {
>> > + .name = "sun4i-lradc-hwmon",
>> > + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> > + .pm = SUN4I_LRADC_DEV_PM_OPS,
>> > + },
>> > + .probe = sun4i_lradc_probe,
>> > +};
>> > +
>> > +module_platform_driver(sun4i_lradc_driver);
>> > +
>> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> > +MODULE_LICENSE("GPL");
>> > +No empty line at end, please
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 23:20 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-05-02 23:20 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
Hi Jonathan,
Thank you for pointing to DEFINE_SIMPLE_DEV_PM_OPS macros, will use it.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 18:24, Jonathan Cameron wrote:
> On Thu, 28 Apr 2022 14:30:06 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On 4/28/22 14:09, 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
>>
>> acquisition
>>
>> > 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 <rz@fabmicro.ru>
>>
>> Ok, next phase of review.
>>
>
> One thing noticed whilst randomly glancing at this patch.
>
>
>> > +#ifdef CONFIG_PM
>> > +static int sun4i_lradc_resume(struct device *dev)
>> > +{
>> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > + return sun4i_lradc_start(lradc);
>> > +}
>> > +
>> > +static int sun4i_lradc_suspend(struct device *dev)
>> > +{
>> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > + sun4i_lradc_stop(lradc);
>> > + return 0;
>> > +}
>> > +
>> > +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
>> > +#else
>> > +#define SUN4I_LRADC_DEV_PM_OPS NULL
>> > +#endif /* CONFIG_PM */
>> > +
>> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
>
> We have much better infrastructure for this these days.
>
> Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
> and pm_sleep_ptr()
>
> Those two in combination will let you get rid of all the ifdef stuff
> here by letting the compiler remove the unused code automatically.
>
>> > + .suspend = sun4i_lradc_suspend,
>> > + .resume = sun4i_lradc_resume,
>> > +};
>> > +
>> > +static const struct of_device_id sun4i_lradc_of_match[] = {
>> > + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
>> > + { /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> > +
>> > +static struct platform_driver sun4i_lradc_driver = {
>> > + .driver = {
>> > + .name = "sun4i-lradc-hwmon",
>> > + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> > + .pm = SUN4I_LRADC_DEV_PM_OPS,
>> > + },
>> > + .probe = sun4i_lradc_probe,
>> > +};
>> > +
>> > +module_platform_driver(sun4i_lradc_driver);
>> > +
>> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> > +MODULE_LICENSE("GPL");
>> > +No empty line at end, please
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 13:39 ` Maxime Ripard
@ 2022-05-02 16:21 ` Guenter Roeck
-1 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-02 16:21 UTC (permalink / raw)
To: Maxime Ripard
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
On 5/2/22 06:39, Maxime Ripard wrote:
> On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
>> On 5/2/22 04:00, Maxime Ripard wrote:
>>> 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 <rz@fabmicro.ru>
>>>> ---
>>>> 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 <rz@fabmicro.ru>
>>>> +L: linux-hwmon@vger.kernel.org
>>>> +S: Maintained
>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>> SUNDANCE NETWORK DRIVER
>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>> L: netdev@vger.kernel.org
>>>> 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?
>>>
>> depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
>
> Right, but that just doesn't fly for any generic distro / build-system.
>
That is correct. Alternative might be to use devicetree bindings, which
presumably will be needed anyway to tell the driver(s) what to bind to.
Guenter
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 16:21 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-02 16:21 UTC (permalink / raw)
To: Maxime Ripard
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
On 5/2/22 06:39, Maxime Ripard wrote:
> On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
>> On 5/2/22 04:00, Maxime Ripard wrote:
>>> 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 <rz@fabmicro.ru>
>>>> ---
>>>> 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 <rz@fabmicro.ru>
>>>> +L: linux-hwmon@vger.kernel.org
>>>> +S: Maintained
>>>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>> SUNDANCE NETWORK DRIVER
>>>> M: Denis Kirjanov <kda@linux-powerpc.org>
>>>> L: netdev@vger.kernel.org
>>>> 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?
>>>
>> depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
>
> Right, but that just doesn't fly for any generic distro / build-system.
>
That is correct. Alternative might be to use devicetree bindings, which
presumably will be needed anyway to tell the driver(s) what to bind to.
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 13:31 ` Guenter Roeck
@ 2022-05-02 13:39 ` Maxime Ripard
-1 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 13:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]
On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
> On 5/2/22 04:00, Maxime Ripard wrote:
> > 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 <rz@fabmicro.ru>
> > > ---
> > > 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 <rz@fabmicro.ru>
> > > +L: linux-hwmon@vger.kernel.org
> > > +S: Maintained
> > > +F: drivers/hwmon/sun4i-lradc-hwmon.c
> > > +
> > > SUNDANCE NETWORK DRIVER
> > > M: Denis Kirjanov <kda@linux-powerpc.org>
> > > L: netdev@vger.kernel.org
> > > 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?
> >
> depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
Right, but that just doesn't fly for any generic distro / build-system.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 13:39 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 13:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 2789 bytes --]
On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
> On 5/2/22 04:00, Maxime Ripard wrote:
> > 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 <rz@fabmicro.ru>
> > > ---
> > > 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 <rz@fabmicro.ru>
> > > +L: linux-hwmon@vger.kernel.org
> > > +S: Maintained
> > > +F: drivers/hwmon/sun4i-lradc-hwmon.c
> > > +
> > > SUNDANCE NETWORK DRIVER
> > > M: Denis Kirjanov <kda@linux-powerpc.org>
> > > L: netdev@vger.kernel.org
> > > 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?
> >
> depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
Right, but that just doesn't fly for any generic distro / build-system.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 11:00 ` Maxime Ripard
@ 2022-05-02 13:31 ` Guenter Roeck
-1 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-02 13:31 UTC (permalink / raw)
To: Maxime Ripard, Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 5/2/22 04:00, Maxime Ripard wrote:
> 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 <rz@fabmicro.ru>
>> ---
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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?
>
depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
^^^^^^^^^^^^^^^^^^^^^
> 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.
>
Both is fine with me.
Guenter
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 13:31 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-05-02 13:31 UTC (permalink / raw)
To: Maxime Ripard, Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 5/2/22 04:00, Maxime Ripard wrote:
> 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 <rz@fabmicro.ru>
>> ---
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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?
>
depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
^^^^^^^^^^^^^^^^^^^^^
> 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.
>
Both is fine with me.
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 11:15 ` Icenowy Zheng
@ 2022-05-02 11:21 ` Maxime Ripard
-1 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 11:21 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Ruslan Zalata, Guenter Roeck, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3270 bytes --]
On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
>
>
> 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@cerno.tech> 写到:
> >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 <rz@fabmicro.ru>
> >> ---
> >> 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 <rz@fabmicro.ru>
> >> +L: linux-hwmon@vger.kernel.org
> >> +S: Maintained
> >> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> >> +
> >> SUNDANCE NETWORK DRIVER
> >> M: Denis Kirjanov <kda@linux-powerpc.org>
> >> L: netdev@vger.kernel.org
> >> 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/52C5E9F1.9010700@redhat.com/
I'm not sure if it's still relevant, so we might just need to add an
hwmon driver to the existing driver
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 11:21 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 11:21 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Ruslan Zalata, Guenter Roeck, Jean Delvare, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 3270 bytes --]
On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
>
>
> 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@cerno.tech> 写到:
> >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 <rz@fabmicro.ru>
> >> ---
> >> 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 <rz@fabmicro.ru>
> >> +L: linux-hwmon@vger.kernel.org
> >> +S: Maintained
> >> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> >> +
> >> SUNDANCE NETWORK DRIVER
> >> M: Denis Kirjanov <kda@linux-powerpc.org>
> >> L: netdev@vger.kernel.org
> >> 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/52C5E9F1.9010700@redhat.com/
I'm not sure if it's still relevant, so we might just need to add an
hwmon driver to the existing driver
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-05-02 11:00 ` Maxime Ripard
@ 2022-05-02 11:15 ` Icenowy Zheng
-1 siblings, 0 replies; 43+ messages in thread
From: Icenowy Zheng @ 2022-05-02 11:15 UTC (permalink / raw)
To: Maxime Ripard, Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@cerno.tech> 写到:
>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 <rz@fabmicro.ru>
>> ---
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 11:15 ` Icenowy Zheng
0 siblings, 0 replies; 43+ messages in thread
From: Icenowy Zheng @ 2022-05-02 11:15 UTC (permalink / raw)
To: Maxime Ripard, Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@cerno.tech> 写到:
>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 <rz@fabmicro.ru>
>> ---
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-28 21:09 Ruslan Zalata
@ 2022-05-02 11:00 ` Maxime Ripard
2022-05-02 11:00 ` Maxime Ripard
1 sibling, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 11:00 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
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 <rz@fabmicro.ru>
> ---
> 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 <rz@fabmicro.ru>
> +L: linux-hwmon@vger.kernel.org
> +S: Maintained
> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> +
> SUNDANCE NETWORK DRIVER
> M: Denis Kirjanov <kda@linux-powerpc.org>
> L: netdev@vger.kernel.org
> 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.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-05-02 11:00 ` Maxime Ripard
0 siblings, 0 replies; 43+ messages in thread
From: Maxime Ripard @ 2022-05-02 11:00 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
[-- Attachment #1.1: Type: text/plain, Size: 2469 bytes --]
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 <rz@fabmicro.ru>
> ---
> 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 <rz@fabmicro.ru>
> +L: linux-hwmon@vger.kernel.org
> +S: Maintained
> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> +
> SUNDANCE NETWORK DRIVER
> M: Denis Kirjanov <kda@linux-powerpc.org>
> L: netdev@vger.kernel.org
> 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.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-28 21:30 ` Guenter Roeck
@ 2022-04-29 13:24 ` Jonathan Cameron
-1 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2022-04-29 13:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
On Thu, 28 Apr 2022 14:30:06 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On 4/28/22 14:09, 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
>
> acquisition
>
> > 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 <rz@fabmicro.ru>
>
> Ok, next phase of review.
>
One thing noticed whilst randomly glancing at this patch.
> > +#ifdef CONFIG_PM
> > +static int sun4i_lradc_resume(struct device *dev)
> > +{
> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > + return sun4i_lradc_start(lradc);
> > +}
> > +
> > +static int sun4i_lradc_suspend(struct device *dev)
> > +{
> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > + sun4i_lradc_stop(lradc);
> > + return 0;
> > +}
> > +
> > +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
> > +#else
> > +#define SUN4I_LRADC_DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
We have much better infrastructure for this these days.
Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
and pm_sleep_ptr()
Those two in combination will let you get rid of all the ifdef stuff
here by letting the compiler remove the unused code automatically.
> > + .suspend = sun4i_lradc_suspend,
> > + .resume = sun4i_lradc_resume,
> > +};
> > +
> > +static const struct of_device_id sun4i_lradc_of_match[] = {
> > + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> > +
> > +static struct platform_driver sun4i_lradc_driver = {
> > + .driver = {
> > + .name = "sun4i-lradc-hwmon",
> > + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
> > + .pm = SUN4I_LRADC_DEV_PM_OPS,
> > + },
> > + .probe = sun4i_lradc_probe,
> > +};
> > +
> > +module_platform_driver(sun4i_lradc_driver);
> > +
> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> > +MODULE_LICENSE("GPL");
> > +No empty line at end, please
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-29 13:24 ` Jonathan Cameron
0 siblings, 0 replies; 43+ messages in thread
From: Jonathan Cameron @ 2022-04-29 13:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
On Thu, 28 Apr 2022 14:30:06 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On 4/28/22 14:09, 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
>
> acquisition
>
> > 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 <rz@fabmicro.ru>
>
> Ok, next phase of review.
>
One thing noticed whilst randomly glancing at this patch.
> > +#ifdef CONFIG_PM
> > +static int sun4i_lradc_resume(struct device *dev)
> > +{
> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > + return sun4i_lradc_start(lradc);
> > +}
> > +
> > +static int sun4i_lradc_suspend(struct device *dev)
> > +{
> > + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > + sun4i_lradc_stop(lradc);
> > + return 0;
> > +}
> > +
> > +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
> > +#else
> > +#define SUN4I_LRADC_DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
We have much better infrastructure for this these days.
Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
and pm_sleep_ptr()
Those two in combination will let you get rid of all the ifdef stuff
here by letting the compiler remove the unused code automatically.
> > + .suspend = sun4i_lradc_suspend,
> > + .resume = sun4i_lradc_resume,
> > +};
> > +
> > +static const struct of_device_id sun4i_lradc_of_match[] = {
> > + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> > +
> > +static struct platform_driver sun4i_lradc_driver = {
> > + .driver = {
> > + .name = "sun4i-lradc-hwmon",
> > + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
> > + .pm = SUN4I_LRADC_DEV_PM_OPS,
> > + },
> > + .probe = sun4i_lradc_probe,
> > +};
> > +
> > +module_platform_driver(sun4i_lradc_driver);
> > +
> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> > +MODULE_LICENSE("GPL");
> > +No empty line at end, please
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-29 5:32 ` Guenter Roeck
@ 2022-04-29 6:03 ` Guenter Roeck
-1 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-29 6:03 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 22:32, Guenter Roeck wrote:
> On 4/28/22 17:28, Ruslan Zalata wrote:
>> Thank you Guenter for your valuable time.
>>
>> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
>>
>> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
>>
>
> Not necessarily. The data does not have to be "current", after all,
> if the hardware is able to continuously convert. If not, the question
> is how long a conversion takes. If it doesn't take too long, it would
> be better to initiate a conversion and then wait for the completion.
>
>> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
>>
> Why ? That happens for almost all hwmon devices. They will all report
> the most recent conversion value. Some of them can take seconds
> to complete a new conversion, so the reported value is always "old"
> for a given defition of old (ie any time smaller than a conversion
> interval).
>
> Sigh. Looks like I'll have to dig up the documentation and read about
> the ADC myself.
>
I did, for both A13 and A20. The ADC supports continuous mode. That
means it can be configured accordingly, and reading the ADC value
just returns the most recent conversion value. There is absolutely
no need to keep reading the conversion values using interrupts.
Also,
+struct lradc_variant {
+ u32 bits;
+ u32 resolution;
+ u32 vref;
+};
is unnecessary because the values are the same for both supported chips.
That means that defines can and should be used. Yes, I can see that
A83T uses a different voltage, but even that doesn't need a structure -
the voltage can be set in struct sun4i_lradc_data if/when needed,
and the resolution and number of bits is still the same.
Guenter
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-29 6:03 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-29 6:03 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 22:32, Guenter Roeck wrote:
> On 4/28/22 17:28, Ruslan Zalata wrote:
>> Thank you Guenter for your valuable time.
>>
>> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
>>
>> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
>>
>
> Not necessarily. The data does not have to be "current", after all,
> if the hardware is able to continuously convert. If not, the question
> is how long a conversion takes. If it doesn't take too long, it would
> be better to initiate a conversion and then wait for the completion.
>
>> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
>>
> Why ? That happens for almost all hwmon devices. They will all report
> the most recent conversion value. Some of them can take seconds
> to complete a new conversion, so the reported value is always "old"
> for a given defition of old (ie any time smaller than a conversion
> interval).
>
> Sigh. Looks like I'll have to dig up the documentation and read about
> the ADC myself.
>
I did, for both A13 and A20. The ADC supports continuous mode. That
means it can be configured accordingly, and reading the ADC value
just returns the most recent conversion value. There is absolutely
no need to keep reading the conversion values using interrupts.
Also,
+struct lradc_variant {
+ u32 bits;
+ u32 resolution;
+ u32 vref;
+};
is unnecessary because the values are the same for both supported chips.
That means that defines can and should be used. Yes, I can see that
A83T uses a different voltage, but even that doesn't need a structure -
the voltage can be set in struct sun4i_lradc_data if/when needed,
and the resolution and number of bits is still the same.
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-29 0:28 ` Ruslan Zalata
@ 2022-04-29 5:32 ` Guenter Roeck
-1 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-29 5:32 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 17:28, Ruslan Zalata wrote:
> Thank you Guenter for your valuable time.
>
> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
>
> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
>
Not necessarily. The data does not have to be "current", after all,
if the hardware is able to continuously convert. If not, the question
is how long a conversion takes. If it doesn't take too long, it would
be better to initiate a conversion and then wait for the completion.
> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
>
Why ? That happens for almost all hwmon devices. They will all report
the most recent conversion value. Some of them can take seconds
to complete a new conversion, so the reported value is always "old"
for a given defition of old (ie any time smaller than a conversion
interval).
Sigh. Looks like I'll have to dig up the documentation and read about
the ADC myself.
Guenter
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-29 5:32 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-29 5:32 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 17:28, Ruslan Zalata wrote:
> Thank you Guenter for your valuable time.
>
> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
>
> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
>
Not necessarily. The data does not have to be "current", after all,
if the hardware is able to continuously convert. If not, the question
is how long a conversion takes. If it doesn't take too long, it would
be better to initiate a conversion and then wait for the completion.
> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
>
Why ? That happens for almost all hwmon devices. They will all report
the most recent conversion value. Some of them can take seconds
to complete a new conversion, so the reported value is always "old"
for a given defition of old (ie any time smaller than a conversion
interval).
Sigh. Looks like I'll have to dig up the documentation and read about
the ADC myself.
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-28 21:30 ` Guenter Roeck
@ 2022-04-29 0:28 ` Ruslan Zalata
-1 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-04-29 0:28 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
Thank you Guenter for your valuable time.
I have added update_interval option (it's in ms units, right?) and fixed
all other issues you pointed to. Will test it on real hardware and send
third version of the patch for review.
Regarding IRQ. Alternatively the driver would need to sit and poll
conversion ready bit in a loop which might cause a much worse load on
system, is not it ? Anyway, the real problem with this piece of hardware
is that there's no "conversion ready bit" provided, the only way to know
data ready status is to receive an interrupt.
I think it still needs a semaphore/seqlock to synchronize conversions
and reads. I.e. two consequent reads should not return same old value.
Although it's not an issue in my case, but could be a problem for
others.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 02:30, Guenter Roeck wrote:
> On 4/28/22 14:09, 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
>
> acquisition
>
>> 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 <rz@fabmicro.ru>
>
> Ok, next phase of review.
>
>
>> ---
>> MAINTAINERS | 6 +
>> drivers/hwmon/Kconfig | 13 ++
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/sun4i-lradc-hwmon.c | 280
>> ++++++++++++++++++++++++++++++
>
> Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
> and don't forget to add it to Documentation/hwmon/index.rst.
>
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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
>
> only one of these
>
>> + of these must be used at a time.
>> +
>> config SENSORS_SY7636A
>> tristate "Silergy SY7636A"
>> help
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 8a03289e2aa..3e5dc902acf 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
>> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
>> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
>> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
>> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
>> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
>> obj-$(CONFIG_SENSORS_TC74) += tc74.o
>> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c
>> b/drivers/hwmon/sun4i-lradc-hwmon.c
>> new file mode 100644
>> index 00000000000..8d5268b037b
>> --- /dev/null
>> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
>> + *
>> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
>> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>
> Should not be needed.
>
>> +
>> +#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_KEY_EN(x) ((x) << 7)
>> +#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)
>> +
>> +struct lradc_variant {
>> + u32 bits;
>> + u32 resolution;
>> + u32 vref;
>> +};
>> +
>> +struct sun4i_lradc_data {
>> + struct device *dev;
>> + void __iomem *base;
>> + const struct lradc_variant *variant;
>> + struct mutex update_lock; /* atomic read and data updates */
>> + u32 in[2];
>> +};
>> +
>> +struct lradc_variant variant_sun4i_a10_lradc = {
>> + .bits = 0x3f,
>> + .resolution = 63,
>> + .vref = 2000000, /* Vref = 2.0V from SoC datasheet */
>> +};
>> +
>> +static int sun4i_lradc_read(struct device *dev, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> + int in;
>> +
>> + if (IS_ERR(lradc))
>> + return PTR_ERR(lradc);
>
> That won't happen. Unnecessary check.
>
>> +
>> + mutex_lock(&lradc->update_lock);
>> + in = lradc->in[channel];
>
>
> Pointless. Just use
>
>> + mutex_unlock(&lradc->update_lock);
>> +
>> + switch (attr) {
>> + case hwmon_in_input:
>> + *val = in;
>
> *val = lradc->in[channel];
>
> here. The operation is atomic.
>
>> + break;
>> +
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + return 0;
>> +}
>> +
>> +umode_t sun4i_lradc_is_visible(const void *data, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + return 0444;
>> +}
>> +
>> +static const u32 sun4i_lradc_chip_config[] = {
>> + HWMON_I_INPUT,
>
> This is wrong. Chip wide attributes are named HWMON_C_XXX.
> This generated a reset_history attribute which isn't really
> supported or handled. Chip attributes should only be specified
> if actually used. Drop this.
>
>> + 0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_chip = {
>> + .type = hwmon_chip,
>> + .config = sun4i_lradc_chip_config,
>> +};
>> +
>> +static const u32 sun4i_lradc_input_config[] = {
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + 0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_input = {
>> + .type = hwmon_in,
>> + .config = sun4i_lradc_input_config,
>> +};
>> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
>> + &sun4i_lradc_chip,
>> + &sun4i_lradc_input,
>> + NULL
>> +};
>> +
> Use the HWMON_CHANNEL_INFO macro instead.
>
>> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
>> + .is_visible = sun4i_lradc_is_visible,
>> + .read = sun4i_lradc_read,
>> +};
>> +
>> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
>> + .ops = &sun4i_lradc_hwmon_ops,
>> + .info = sun4i_lradc_info,
>> +};
>> +
>> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_id;
>> + u32 ints = readl(lradc->base + LRADC_INTS);
>> +
>> + mutex_lock(&lradc->update_lock);
>> +
>> + if (ints & CHAN0_DATA_IRQ)
>> + lradc->in[0] = (readl(lradc->base + LRADC_DATA0) &
>> lradc->variant->bits) *
>> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV
>> */
>> +
>> + if (ints & CHAN1_DATA_IRQ)
>> + lradc->in[1] = (readl(lradc->base + LRADC_DATA1) &
>> lradc->variant->bits) *
>> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV
>> */
>> +
>> + mutex_unlock(&lradc->update_lock);
>> +
>> + writel(ints, lradc->base + LRADC_INTS);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>
> You don't explain why the interrupt handler is needed. It is
> undesirable
> to do this for hwnon devices because it results in extra load on the
> system.
> The ADC should be read only when needed and not continuously. If that
> is not
> possible for some reason it needs to be explained in detail.
>
>> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
>> +{
>> + writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
>> + HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
>> + lradc->base + LRADC_CTRL);
>> +
>
> If the update rate is configurable, it might make sense to support
> the update_interval attribute.
>
>> + writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
>> +
>> + return 0;
>> +}
>> +
>> +static void sun4i_lradc_stop(void *data)
>> +{
>> + struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
>> +
>> + 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 *hwmon_dev;
>> + struct resource *res;
>> + int hwmon_irq, error;
>> +
>> + lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data),
>> GFP_KERNEL);
>> + if (!lradc)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(dev, lradc);
>> +
>> + lradc->variant = of_device_get_match_data(&pdev->dev);
>> + if (!lradc->variant)
>> + return -EINVAL;
>> +
>> + lradc->dev = dev;
>> +
>
> I don't immediately see where this is used.
>
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (IS_ERR(res))
>> + return PTR_ERR(res);
>> +
>> + lradc->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(lradc->base))
>> + return PTR_ERR(lradc->base);
>> +
>> + hwmon_irq = platform_get_irq(pdev, 0);
>> + if (hwmon_irq < 0)
>> + return hwmon_irq;
>> +
>> + error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0,
>> "sun4i-lradc-hwmon", lradc);
>> + if (error) {
>> + dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
>> + return error;
>> + }
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon",
>> lradc,
>> + &sun4i_lradc_chip_info, NULL);
>> +
>> + if (IS_ERR(hwmon_dev)) {
>> + error = PTR_ERR(hwmon_dev);
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
>> + if (error)
>> + return error;
>> +
>> + error = sun4i_lradc_start(lradc);
>> +
>> + return error;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int sun4i_lradc_resume(struct device *dev)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> + return sun4i_lradc_start(lradc);
>> +}
>> +
>> +static int sun4i_lradc_suspend(struct device *dev)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> + sun4i_lradc_stop(lradc);
>> + return 0;
>> +}
>> +
>> +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
>> +#else
>> +#define SUN4I_LRADC_DEV_PM_OPS NULL
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
>> + .suspend = sun4i_lradc_suspend,
>> + .resume = sun4i_lradc_resume,
>> +};
>> +
>> +static const struct of_device_id sun4i_lradc_of_match[] = {
>> + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data =
>> &variant_sun4i_a10_lradc},
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> +
>> +static struct platform_driver sun4i_lradc_driver = {
>> + .driver = {
>> + .name = "sun4i-lradc-hwmon",
>> + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> + .pm = SUN4I_LRADC_DEV_PM_OPS,
>> + },
>> + .probe = sun4i_lradc_probe,
>> +};
>> +
>> +module_platform_driver(sun4i_lradc_driver);
>> +
>> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> +MODULE_LICENSE("GPL");
>> +No empty line at end, please
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-29 0:28 ` Ruslan Zalata
0 siblings, 0 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-04-29 0:28 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
Thank you Guenter for your valuable time.
I have added update_interval option (it's in ms units, right?) and fixed
all other issues you pointed to. Will test it on real hardware and send
third version of the patch for review.
Regarding IRQ. Alternatively the driver would need to sit and poll
conversion ready bit in a loop which might cause a much worse load on
system, is not it ? Anyway, the real problem with this piece of hardware
is that there's no "conversion ready bit" provided, the only way to know
data ready status is to receive an interrupt.
I think it still needs a semaphore/seqlock to synchronize conversions
and reads. I.e. two consequent reads should not return same old value.
Although it's not an issue in my case, but could be a problem for
others.
---
Regards,
Ruslan.
Fabmicro, LLC.
On 2022-04-29 02:30, Guenter Roeck wrote:
> On 4/28/22 14:09, 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
>
> acquisition
>
>> 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 <rz@fabmicro.ru>
>
> Ok, next phase of review.
>
>
>> ---
>> MAINTAINERS | 6 +
>> drivers/hwmon/Kconfig | 13 ++
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/sun4i-lradc-hwmon.c | 280
>> ++++++++++++++++++++++++++++++
>
> Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
> and don't forget to add it to Documentation/hwmon/index.rst.
>
>> 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 <rz@fabmicro.ru>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>> SUNDANCE NETWORK DRIVER
>> M: Denis Kirjanov <kda@linux-powerpc.org>
>> L: netdev@vger.kernel.org
>> 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
>
> only one of these
>
>> + of these must be used at a time.
>> +
>> config SENSORS_SY7636A
>> tristate "Silergy SY7636A"
>> help
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 8a03289e2aa..3e5dc902acf 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
>> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
>> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
>> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
>> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
>> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
>> obj-$(CONFIG_SENSORS_TC74) += tc74.o
>> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c
>> b/drivers/hwmon/sun4i-lradc-hwmon.c
>> new file mode 100644
>> index 00000000000..8d5268b037b
>> --- /dev/null
>> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
>> + *
>> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
>> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>
> Should not be needed.
>
>> +
>> +#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_KEY_EN(x) ((x) << 7)
>> +#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)
>> +
>> +struct lradc_variant {
>> + u32 bits;
>> + u32 resolution;
>> + u32 vref;
>> +};
>> +
>> +struct sun4i_lradc_data {
>> + struct device *dev;
>> + void __iomem *base;
>> + const struct lradc_variant *variant;
>> + struct mutex update_lock; /* atomic read and data updates */
>> + u32 in[2];
>> +};
>> +
>> +struct lradc_variant variant_sun4i_a10_lradc = {
>> + .bits = 0x3f,
>> + .resolution = 63,
>> + .vref = 2000000, /* Vref = 2.0V from SoC datasheet */
>> +};
>> +
>> +static int sun4i_lradc_read(struct device *dev, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> + int in;
>> +
>> + if (IS_ERR(lradc))
>> + return PTR_ERR(lradc);
>
> That won't happen. Unnecessary check.
>
>> +
>> + mutex_lock(&lradc->update_lock);
>> + in = lradc->in[channel];
>
>
> Pointless. Just use
>
>> + mutex_unlock(&lradc->update_lock);
>> +
>> + switch (attr) {
>> + case hwmon_in_input:
>> + *val = in;
>
> *val = lradc->in[channel];
>
> here. The operation is atomic.
>
>> + break;
>> +
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + return 0;
>> +}
>> +
>> +umode_t sun4i_lradc_is_visible(const void *data, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + return 0444;
>> +}
>> +
>> +static const u32 sun4i_lradc_chip_config[] = {
>> + HWMON_I_INPUT,
>
> This is wrong. Chip wide attributes are named HWMON_C_XXX.
> This generated a reset_history attribute which isn't really
> supported or handled. Chip attributes should only be specified
> if actually used. Drop this.
>
>> + 0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_chip = {
>> + .type = hwmon_chip,
>> + .config = sun4i_lradc_chip_config,
>> +};
>> +
>> +static const u32 sun4i_lradc_input_config[] = {
>> + HWMON_I_INPUT,
>> + HWMON_I_INPUT,
>> + 0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_input = {
>> + .type = hwmon_in,
>> + .config = sun4i_lradc_input_config,
>> +};
>> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
>> + &sun4i_lradc_chip,
>> + &sun4i_lradc_input,
>> + NULL
>> +};
>> +
> Use the HWMON_CHANNEL_INFO macro instead.
>
>> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
>> + .is_visible = sun4i_lradc_is_visible,
>> + .read = sun4i_lradc_read,
>> +};
>> +
>> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
>> + .ops = &sun4i_lradc_hwmon_ops,
>> + .info = sun4i_lradc_info,
>> +};
>> +
>> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_id;
>> + u32 ints = readl(lradc->base + LRADC_INTS);
>> +
>> + mutex_lock(&lradc->update_lock);
>> +
>> + if (ints & CHAN0_DATA_IRQ)
>> + lradc->in[0] = (readl(lradc->base + LRADC_DATA0) &
>> lradc->variant->bits) *
>> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV
>> */
>> +
>> + if (ints & CHAN1_DATA_IRQ)
>> + lradc->in[1] = (readl(lradc->base + LRADC_DATA1) &
>> lradc->variant->bits) *
>> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV
>> */
>> +
>> + mutex_unlock(&lradc->update_lock);
>> +
>> + writel(ints, lradc->base + LRADC_INTS);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>
> You don't explain why the interrupt handler is needed. It is
> undesirable
> to do this for hwnon devices because it results in extra load on the
> system.
> The ADC should be read only when needed and not continuously. If that
> is not
> possible for some reason it needs to be explained in detail.
>
>> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
>> +{
>> + writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
>> + HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
>> + lradc->base + LRADC_CTRL);
>> +
>
> If the update rate is configurable, it might make sense to support
> the update_interval attribute.
>
>> + writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
>> +
>> + return 0;
>> +}
>> +
>> +static void sun4i_lradc_stop(void *data)
>> +{
>> + struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
>> +
>> + 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 *hwmon_dev;
>> + struct resource *res;
>> + int hwmon_irq, error;
>> +
>> + lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data),
>> GFP_KERNEL);
>> + if (!lradc)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(dev, lradc);
>> +
>> + lradc->variant = of_device_get_match_data(&pdev->dev);
>> + if (!lradc->variant)
>> + return -EINVAL;
>> +
>> + lradc->dev = dev;
>> +
>
> I don't immediately see where this is used.
>
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (IS_ERR(res))
>> + return PTR_ERR(res);
>> +
>> + lradc->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(lradc->base))
>> + return PTR_ERR(lradc->base);
>> +
>> + hwmon_irq = platform_get_irq(pdev, 0);
>> + if (hwmon_irq < 0)
>> + return hwmon_irq;
>> +
>> + error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0,
>> "sun4i-lradc-hwmon", lradc);
>> + if (error) {
>> + dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
>> + return error;
>> + }
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon",
>> lradc,
>> + &sun4i_lradc_chip_info, NULL);
>> +
>> + if (IS_ERR(hwmon_dev)) {
>> + error = PTR_ERR(hwmon_dev);
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
>> + if (error)
>> + return error;
>> +
>> + error = sun4i_lradc_start(lradc);
>> +
>> + return error;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int sun4i_lradc_resume(struct device *dev)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> + return sun4i_lradc_start(lradc);
>> +}
>> +
>> +static int sun4i_lradc_suspend(struct device *dev)
>> +{
>> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> + sun4i_lradc_stop(lradc);
>> + return 0;
>> +}
>> +
>> +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
>> +#else
>> +#define SUN4I_LRADC_DEV_PM_OPS NULL
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
>> + .suspend = sun4i_lradc_suspend,
>> + .resume = sun4i_lradc_resume,
>> +};
>> +
>> +static const struct of_device_id sun4i_lradc_of_match[] = {
>> + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data =
>> &variant_sun4i_a10_lradc},
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> +
>> +static struct platform_driver sun4i_lradc_driver = {
>> + .driver = {
>> + .name = "sun4i-lradc-hwmon",
>> + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> + .pm = SUN4I_LRADC_DEV_PM_OPS,
>> + },
>> + .probe = sun4i_lradc_probe,
>> +};
>> +
>> +module_platform_driver(sun4i_lradc_driver);
>> +
>> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> +MODULE_LICENSE("GPL");
>> +No empty line at end, please
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
2022-04-28 21:09 Ruslan Zalata
@ 2022-04-28 21:30 ` Guenter Roeck
2022-05-02 11:00 ` Maxime Ripard
1 sibling, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-28 21:30 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 14:09, 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
acquisition
> 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 <rz@fabmicro.ru>
Ok, next phase of review.
> ---
> MAINTAINERS | 6 +
> drivers/hwmon/Kconfig | 13 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
and don't forget to add it to Documentation/hwmon/index.rst.
> 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 <rz@fabmicro.ru>
> +L: linux-hwmon@vger.kernel.org
> +S: Maintained
> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> +
> SUNDANCE NETWORK DRIVER
> M: Denis Kirjanov <kda@linux-powerpc.org>
> L: netdev@vger.kernel.org
> 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
only one of these
> + of these must be used at a time.
> +
> config SENSORS_SY7636A
> tristate "Silergy SY7636A"
> help
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8a03289e2aa..3e5dc902acf 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
> obj-$(CONFIG_SENSORS_TC74) += tc74.o
> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c b/drivers/hwmon/sun4i-lradc-hwmon.c
> new file mode 100644
> index 00000000000..8d5268b037b
> --- /dev/null
> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
> + *
> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Should not be needed.
> +
> +#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_KEY_EN(x) ((x) << 7)
> +#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)
> +
> +struct lradc_variant {
> + u32 bits;
> + u32 resolution;
> + u32 vref;
> +};
> +
> +struct sun4i_lradc_data {
> + struct device *dev;
> + void __iomem *base;
> + const struct lradc_variant *variant;
> + struct mutex update_lock; /* atomic read and data updates */
> + u32 in[2];
> +};
> +
> +struct lradc_variant variant_sun4i_a10_lradc = {
> + .bits = 0x3f,
> + .resolution = 63,
> + .vref = 2000000, /* Vref = 2.0V from SoC datasheet */
> +};
> +
> +static int sun4i_lradc_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> + int in;
> +
> + if (IS_ERR(lradc))
> + return PTR_ERR(lradc);
That won't happen. Unnecessary check.
> +
> + mutex_lock(&lradc->update_lock);
> + in = lradc->in[channel];
Pointless. Just use
> + mutex_unlock(&lradc->update_lock);
> +
> + switch (attr) {
> + case hwmon_in_input:
> + *val = in;
*val = lradc->in[channel];
here. The operation is atomic.
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +umode_t sun4i_lradc_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const u32 sun4i_lradc_chip_config[] = {
> + HWMON_I_INPUT,
This is wrong. Chip wide attributes are named HWMON_C_XXX.
This generated a reset_history attribute which isn't really
supported or handled. Chip attributes should only be specified
if actually used. Drop this.
> + 0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_chip = {
> + .type = hwmon_chip,
> + .config = sun4i_lradc_chip_config,
> +};
> +
> +static const u32 sun4i_lradc_input_config[] = {
> + HWMON_I_INPUT,
> + HWMON_I_INPUT,
> + 0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_input = {
> + .type = hwmon_in,
> + .config = sun4i_lradc_input_config,
> +};
> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
> + &sun4i_lradc_chip,
> + &sun4i_lradc_input,
> + NULL
> +};
> +
Use the HWMON_CHANNEL_INFO macro instead.
> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
> + .is_visible = sun4i_lradc_is_visible,
> + .read = sun4i_lradc_read,
> +};
> +
> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
> + .ops = &sun4i_lradc_hwmon_ops,
> + .info = sun4i_lradc_info,
> +};
> +
> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> +{
> + struct sun4i_lradc_data *lradc = dev_id;
> + u32 ints = readl(lradc->base + LRADC_INTS);
> +
> + mutex_lock(&lradc->update_lock);
> +
> + if (ints & CHAN0_DATA_IRQ)
> + lradc->in[0] = (readl(lradc->base + LRADC_DATA0) & lradc->variant->bits) *
> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> + if (ints & CHAN1_DATA_IRQ)
> + lradc->in[1] = (readl(lradc->base + LRADC_DATA1) & lradc->variant->bits) *
> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> + mutex_unlock(&lradc->update_lock);
> +
> + writel(ints, lradc->base + LRADC_INTS);
> +
> + return IRQ_HANDLED;
> +}
> +
You don't explain why the interrupt handler is needed. It is undesirable
to do this for hwnon devices because it results in extra load on the system.
The ADC should be read only when needed and not continuously. If that is not
possible for some reason it needs to be explained in detail.
> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
> +{
> + writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
> + HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
> + lradc->base + LRADC_CTRL);
> +
If the update rate is configurable, it might make sense to support
the update_interval attribute.
> + writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
> +
> + return 0;
> +}
> +
> +static void sun4i_lradc_stop(void *data)
> +{
> + struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
> +
> + 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 *hwmon_dev;
> + struct resource *res;
> + int hwmon_irq, error;
> +
> + lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), GFP_KERNEL);
> + if (!lradc)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, lradc);
> +
> + lradc->variant = of_device_get_match_data(&pdev->dev);
> + if (!lradc->variant)
> + return -EINVAL;
> +
> + lradc->dev = dev;
> +
I don't immediately see where this is used.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> +
> + lradc->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(lradc->base))
> + return PTR_ERR(lradc->base);
> +
> + hwmon_irq = platform_get_irq(pdev, 0);
> + if (hwmon_irq < 0)
> + return hwmon_irq;
> +
> + error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0, "sun4i-lradc-hwmon", lradc);
> + if (error) {
> + dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
> + return error;
> + }
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon", lradc,
> + &sun4i_lradc_chip_info, NULL);
> +
> + if (IS_ERR(hwmon_dev)) {
> + error = PTR_ERR(hwmon_dev);
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
> + if (error)
> + return error;
> +
> + error = sun4i_lradc_start(lradc);
> +
> + return error;
> +}
> +
> +#ifdef CONFIG_PM
> +static int sun4i_lradc_resume(struct device *dev)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> + return sun4i_lradc_start(lradc);
> +}
> +
> +static int sun4i_lradc_suspend(struct device *dev)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> + sun4i_lradc_stop(lradc);
> + return 0;
> +}
> +
> +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
> +#else
> +#define SUN4I_LRADC_DEV_PM_OPS NULL
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
> + .suspend = sun4i_lradc_suspend,
> + .resume = sun4i_lradc_resume,
> +};
> +
> +static const struct of_device_id sun4i_lradc_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> +
> +static struct platform_driver sun4i_lradc_driver = {
> + .driver = {
> + .name = "sun4i-lradc-hwmon",
> + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
> + .pm = SUN4I_LRADC_DEV_PM_OPS,
> + },
> + .probe = sun4i_lradc_probe,
> +};
> +
> +module_platform_driver(sun4i_lradc_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> +MODULE_LICENSE("GPL");
> +No empty line at end, please
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-28 21:30 ` Guenter Roeck
0 siblings, 0 replies; 43+ messages in thread
From: Guenter Roeck @ 2022-04-28 21:30 UTC (permalink / raw)
To: Ruslan Zalata
Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi
On 4/28/22 14:09, 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
acquisition
> 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 <rz@fabmicro.ru>
Ok, next phase of review.
> ---
> MAINTAINERS | 6 +
> drivers/hwmon/Kconfig | 13 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
and don't forget to add it to Documentation/hwmon/index.rst.
> 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 <rz@fabmicro.ru>
> +L: linux-hwmon@vger.kernel.org
> +S: Maintained
> +F: drivers/hwmon/sun4i-lradc-hwmon.c
> +
> SUNDANCE NETWORK DRIVER
> M: Denis Kirjanov <kda@linux-powerpc.org>
> L: netdev@vger.kernel.org
> 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
only one of these
> + of these must be used at a time.
> +
> config SENSORS_SY7636A
> tristate "Silergy SY7636A"
> help
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8a03289e2aa..3e5dc902acf 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
> obj-$(CONFIG_SENSORS_TC74) += tc74.o
> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c b/drivers/hwmon/sun4i-lradc-hwmon.c
> new file mode 100644
> index 00000000000..8d5268b037b
> --- /dev/null
> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
> + *
> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Should not be needed.
> +
> +#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_KEY_EN(x) ((x) << 7)
> +#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)
> +
> +struct lradc_variant {
> + u32 bits;
> + u32 resolution;
> + u32 vref;
> +};
> +
> +struct sun4i_lradc_data {
> + struct device *dev;
> + void __iomem *base;
> + const struct lradc_variant *variant;
> + struct mutex update_lock; /* atomic read and data updates */
> + u32 in[2];
> +};
> +
> +struct lradc_variant variant_sun4i_a10_lradc = {
> + .bits = 0x3f,
> + .resolution = 63,
> + .vref = 2000000, /* Vref = 2.0V from SoC datasheet */
> +};
> +
> +static int sun4i_lradc_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> + int in;
> +
> + if (IS_ERR(lradc))
> + return PTR_ERR(lradc);
That won't happen. Unnecessary check.
> +
> + mutex_lock(&lradc->update_lock);
> + in = lradc->in[channel];
Pointless. Just use
> + mutex_unlock(&lradc->update_lock);
> +
> + switch (attr) {
> + case hwmon_in_input:
> + *val = in;
*val = lradc->in[channel];
here. The operation is atomic.
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +umode_t sun4i_lradc_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const u32 sun4i_lradc_chip_config[] = {
> + HWMON_I_INPUT,
This is wrong. Chip wide attributes are named HWMON_C_XXX.
This generated a reset_history attribute which isn't really
supported or handled. Chip attributes should only be specified
if actually used. Drop this.
> + 0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_chip = {
> + .type = hwmon_chip,
> + .config = sun4i_lradc_chip_config,
> +};
> +
> +static const u32 sun4i_lradc_input_config[] = {
> + HWMON_I_INPUT,
> + HWMON_I_INPUT,
> + 0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_input = {
> + .type = hwmon_in,
> + .config = sun4i_lradc_input_config,
> +};
> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
> + &sun4i_lradc_chip,
> + &sun4i_lradc_input,
> + NULL
> +};
> +
Use the HWMON_CHANNEL_INFO macro instead.
> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
> + .is_visible = sun4i_lradc_is_visible,
> + .read = sun4i_lradc_read,
> +};
> +
> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
> + .ops = &sun4i_lradc_hwmon_ops,
> + .info = sun4i_lradc_info,
> +};
> +
> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> +{
> + struct sun4i_lradc_data *lradc = dev_id;
> + u32 ints = readl(lradc->base + LRADC_INTS);
> +
> + mutex_lock(&lradc->update_lock);
> +
> + if (ints & CHAN0_DATA_IRQ)
> + lradc->in[0] = (readl(lradc->base + LRADC_DATA0) & lradc->variant->bits) *
> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> + if (ints & CHAN1_DATA_IRQ)
> + lradc->in[1] = (readl(lradc->base + LRADC_DATA1) & lradc->variant->bits) *
> + lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> + mutex_unlock(&lradc->update_lock);
> +
> + writel(ints, lradc->base + LRADC_INTS);
> +
> + return IRQ_HANDLED;
> +}
> +
You don't explain why the interrupt handler is needed. It is undesirable
to do this for hwnon devices because it results in extra load on the system.
The ADC should be read only when needed and not continuously. If that is not
possible for some reason it needs to be explained in detail.
> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
> +{
> + writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
> + HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
> + lradc->base + LRADC_CTRL);
> +
If the update rate is configurable, it might make sense to support
the update_interval attribute.
> + writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
> +
> + return 0;
> +}
> +
> +static void sun4i_lradc_stop(void *data)
> +{
> + struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
> +
> + 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 *hwmon_dev;
> + struct resource *res;
> + int hwmon_irq, error;
> +
> + lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), GFP_KERNEL);
> + if (!lradc)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, lradc);
> +
> + lradc->variant = of_device_get_match_data(&pdev->dev);
> + if (!lradc->variant)
> + return -EINVAL;
> +
> + lradc->dev = dev;
> +
I don't immediately see where this is used.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> +
> + lradc->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(lradc->base))
> + return PTR_ERR(lradc->base);
> +
> + hwmon_irq = platform_get_irq(pdev, 0);
> + if (hwmon_irq < 0)
> + return hwmon_irq;
> +
> + error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0, "sun4i-lradc-hwmon", lradc);
> + if (error) {
> + dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
> + return error;
> + }
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon", lradc,
> + &sun4i_lradc_chip_info, NULL);
> +
> + if (IS_ERR(hwmon_dev)) {
> + error = PTR_ERR(hwmon_dev);
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
> + if (error)
> + return error;
> +
> + error = sun4i_lradc_start(lradc);
> +
> + return error;
> +}
> +
> +#ifdef CONFIG_PM
> +static int sun4i_lradc_resume(struct device *dev)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> + return sun4i_lradc_start(lradc);
> +}
> +
> +static int sun4i_lradc_suspend(struct device *dev)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> + sun4i_lradc_stop(lradc);
> + return 0;
> +}
> +
> +#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
> +#else
> +#define SUN4I_LRADC_DEV_PM_OPS NULL
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
> + .suspend = sun4i_lradc_suspend,
> + .resume = sun4i_lradc_resume,
> +};
> +
> +static const struct of_device_id sun4i_lradc_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> +
> +static struct platform_driver sun4i_lradc_driver = {
> + .driver = {
> + .name = "sun4i-lradc-hwmon",
> + .of_match_table = of_match_ptr(sun4i_lradc_of_match),
> + .pm = SUN4I_LRADC_DEV_PM_OPS,
> + },
> + .probe = sun4i_lradc_probe,
> +};
> +
> +module_platform_driver(sun4i_lradc_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> +MODULE_LICENSE("GPL");
> +No empty line at end, please
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
@ 2022-04-28 21:09 Ruslan Zalata
2022-04-28 21:30 ` Guenter Roeck
2022-05-02 11:00 ` Maxime Ripard
0 siblings, 2 replies; 43+ messages in thread
From: Ruslan Zalata @ 2022-04-28 21:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-sunxi
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 <rz@fabmicro.ru>
---
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 <rz@fabmicro.ru>
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: drivers/hwmon/sun4i-lradc-hwmon.c
+
SUNDANCE NETWORK DRIVER
M: Denis Kirjanov <kda@linux-powerpc.org>
L: netdev@vger.kernel.org
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.
+
config SENSORS_SY7636A
tristate "Silergy SY7636A"
help
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8a03289e2aa..3e5dc902acf 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
obj-$(CONFIG_SENSORS_STTS751) += stts751.o
+obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
obj-$(CONFIG_SENSORS_TC74) += tc74.o
diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c b/drivers/hwmon/sun4i-lradc-hwmon.c
new file mode 100644
index 00000000000..8d5268b037b
--- /dev/null
+++ b/drivers/hwmon/sun4i-lradc-hwmon.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Allwinner sun4i (A13/A20) LRADC hwmon driver
+ *
+ * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
+ * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.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_KEY_EN(x) ((x) << 7)
+#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)
+
+struct lradc_variant {
+ u32 bits;
+ u32 resolution;
+ u32 vref;
+};
+
+struct sun4i_lradc_data {
+ struct device *dev;
+ void __iomem *base;
+ const struct lradc_variant *variant;
+ struct mutex update_lock; /* atomic read and data updates */
+ u32 in[2];
+};
+
+struct lradc_variant variant_sun4i_a10_lradc = {
+ .bits = 0x3f,
+ .resolution = 63,
+ .vref = 2000000, /* Vref = 2.0V from SoC datasheet */
+};
+
+static int sun4i_lradc_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+ int in;
+
+ if (IS_ERR(lradc))
+ return PTR_ERR(lradc);
+
+ mutex_lock(&lradc->update_lock);
+ in = lradc->in[channel];
+ mutex_unlock(&lradc->update_lock);
+
+ switch (attr) {
+ case hwmon_in_input:
+ *val = in;
+ break;
+
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+umode_t sun4i_lradc_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return 0444;
+}
+
+static const u32 sun4i_lradc_chip_config[] = {
+ HWMON_I_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info sun4i_lradc_chip = {
+ .type = hwmon_chip,
+ .config = sun4i_lradc_chip_config,
+};
+
+static const u32 sun4i_lradc_input_config[] = {
+ HWMON_I_INPUT,
+ HWMON_I_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info sun4i_lradc_input = {
+ .type = hwmon_in,
+ .config = sun4i_lradc_input_config,
+};
+
+static const struct hwmon_channel_info *sun4i_lradc_info[] = {
+ &sun4i_lradc_chip,
+ &sun4i_lradc_input,
+ NULL
+};
+
+static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
+ .is_visible = sun4i_lradc_is_visible,
+ .read = sun4i_lradc_read,
+};
+
+static const struct hwmon_chip_info sun4i_lradc_chip_info = {
+ .ops = &sun4i_lradc_hwmon_ops,
+ .info = sun4i_lradc_info,
+};
+
+static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
+{
+ struct sun4i_lradc_data *lradc = dev_id;
+ u32 ints = readl(lradc->base + LRADC_INTS);
+
+ mutex_lock(&lradc->update_lock);
+
+ if (ints & CHAN0_DATA_IRQ)
+ lradc->in[0] = (readl(lradc->base + LRADC_DATA0) & lradc->variant->bits) *
+ lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
+
+ if (ints & CHAN1_DATA_IRQ)
+ lradc->in[1] = (readl(lradc->base + LRADC_DATA1) & lradc->variant->bits) *
+ lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
+
+ mutex_unlock(&lradc->update_lock);
+
+ writel(ints, lradc->base + LRADC_INTS);
+
+ return IRQ_HANDLED;
+}
+
+static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
+{
+ writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
+ HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
+ lradc->base + LRADC_CTRL);
+
+ writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
+
+ return 0;
+}
+
+static void sun4i_lradc_stop(void *data)
+{
+ struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
+
+ 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 *hwmon_dev;
+ struct resource *res;
+ int hwmon_irq, error;
+
+ lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), GFP_KERNEL);
+ if (!lradc)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, lradc);
+
+ lradc->variant = of_device_get_match_data(&pdev->dev);
+ if (!lradc->variant)
+ return -EINVAL;
+
+ lradc->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR(res))
+ return PTR_ERR(res);
+
+ lradc->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(lradc->base))
+ return PTR_ERR(lradc->base);
+
+ hwmon_irq = platform_get_irq(pdev, 0);
+ if (hwmon_irq < 0)
+ return hwmon_irq;
+
+ error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0, "sun4i-lradc-hwmon", lradc);
+ if (error) {
+ dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
+ return error;
+ }
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon", lradc,
+ &sun4i_lradc_chip_info, NULL);
+
+ if (IS_ERR(hwmon_dev)) {
+ error = PTR_ERR(hwmon_dev);
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
+ if (error)
+ return error;
+
+ error = sun4i_lradc_start(lradc);
+
+ return error;
+}
+
+#ifdef CONFIG_PM
+static int sun4i_lradc_resume(struct device *dev)
+{
+ struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+ return sun4i_lradc_start(lradc);
+}
+
+static int sun4i_lradc_suspend(struct device *dev)
+{
+ struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+ sun4i_lradc_stop(lradc);
+ return 0;
+}
+
+#define SUN4I_LRADC_DEV_PM_OPS (&sun4i_lradc_dev_pm_ops)
+#else
+#define SUN4I_LRADC_DEV_PM_OPS NULL
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
+ .suspend = sun4i_lradc_suspend,
+ .resume = sun4i_lradc_resume,
+};
+
+static const struct of_device_id sun4i_lradc_of_match[] = {
+ { .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
+
+static struct platform_driver sun4i_lradc_driver = {
+ .driver = {
+ .name = "sun4i-lradc-hwmon",
+ .of_match_table = of_match_ptr(sun4i_lradc_of_match),
+ .pm = SUN4I_LRADC_DEV_PM_OPS,
+ },
+ .probe = sun4i_lradc_probe,
+};
+
+module_platform_driver(sun4i_lradc_driver);
+
+MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
+MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
+MODULE_LICENSE("GPL");
+
--
2.36.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
end of thread, other threads:[~2022-05-04 19:33 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-04 19:33 [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC Jamie Cuthbert
-- strict thread matches above, loose matches on Subject: below --
2022-05-04 19:33 Jamie Cuthbert
2022-04-28 21:09 Ruslan Zalata
2022-04-28 21:30 ` Guenter Roeck
2022-04-28 21:30 ` Guenter Roeck
2022-04-29 0:28 ` Ruslan Zalata
2022-04-29 0:28 ` Ruslan Zalata
2022-04-29 5:32 ` Guenter Roeck
2022-04-29 5:32 ` Guenter Roeck
2022-04-29 6:03 ` Guenter Roeck
2022-04-29 6:03 ` Guenter Roeck
2022-05-02 23:47 ` Ruslan Zalata
2022-05-02 23:47 ` Ruslan Zalata
2022-04-29 13:24 ` Jonathan Cameron
2022-04-29 13:24 ` Jonathan Cameron
2022-05-02 23:20 ` Ruslan Zalata
2022-05-02 23:20 ` Ruslan Zalata
2022-05-02 11:00 ` Maxime Ripard
2022-05-02 11:00 ` Maxime Ripard
2022-05-02 11:15 ` Icenowy Zheng
2022-05-02 11:15 ` Icenowy Zheng
2022-05-02 11:21 ` Maxime Ripard
2022-05-02 11:21 ` Maxime Ripard
2022-05-03 2:02 ` Guenter Roeck
2022-05-03 2:02 ` Guenter Roeck
2022-05-03 15:26 ` Ruslan Zalata
2022-05-03 15:26 ` Ruslan Zalata
2022-05-03 16:14 ` Maxime Ripard
2022-05-03 16:14 ` Maxime Ripard
2022-05-03 21:37 ` Ruslan Zalata
2022-05-03 21:37 ` Ruslan Zalata
2022-05-04 14:12 ` Maxime Ripard
2022-05-04 14:12 ` Maxime Ripard
2022-05-02 13:31 ` Guenter Roeck
2022-05-02 13:31 ` Guenter Roeck
2022-05-02 13:39 ` Maxime Ripard
2022-05-02 13:39 ` Maxime Ripard
2022-05-02 16:21 ` Guenter Roeck
2022-05-02 16:21 ` Guenter Roeck
2022-05-03 8:50 ` Ruslan Zalata
2022-05-03 8:50 ` Ruslan Zalata
2022-05-03 16:07 ` Maxime Ripard
2022-05-03 16:07 ` Maxime Ripard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.