Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC

From: Guenter Roeck <linux@roeck-us.net>
To: Ruslan Zalata <rz@fabmicro.ru>
Cc: Jean Delvare <jdelvare@suse.com>, Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Samuel Holland <samuel@sholland.org>,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev
Subject: Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC
Date: Thu, 28 Apr 2022 23:03:58 -0700	[thread overview]
Message-ID: <59e91f45-7263-eb41-4b47-db217af54910@roeck-us.net> (raw)
In-Reply-To: <e4d1a6c8-1afd-671e-76bf-b5bde9dc282f@roeck-us.net>

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