Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Hello,

Thanks a lot for this new review (and sorry for the previous
very-incomplete send, unfortunate keyboard shortcut and sleepy fingers).

On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> Unnecessary include.
[...]
> I don't immediately see where this include is needed. Is this a
> leftover ?
[...]
> Same here.

Are there ways to systematically tell which includes are useless
besides commenting them out all and uncommenting until it compiles ?
(if that is even a good idea)

> > +enum da9063_adc {
> > +	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
> > +	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
> > +	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
> > +	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
> > +	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
> > +	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
> > +	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
> > +	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
> > +	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3  
> 
> Many of the above defines are not used. Do you plan a follow-up commit
> to use them ? Otherwise please drop unused defines.

I'm not sure (would like to, but for this I think I need to add
devicetree controls, and I am not sure how this should look like), so in
doubt I will drop them from this patch set.

There are also #defines in this patchset related to ADCIN channels,
which are hence unused. Should I also drop these ? In my (short)
experience, there seem to regularly be unused #defines in headers, so I
left them be.

> > +struct da9063_hwmon {
> > +	struct da9063 *da9063;
> > +	struct mutex hwmon_mutex;
> > +	struct completion adc_ready;
> > +	signed char tjunc_offset;  
> 
> I am curious: 'char' implies 'signed'. Any reason for using 'signed' ?

We are again getting into my "erring on the status-quo side" as this
comes from the original patchset. My reading of this is that using a
char for holding an integer is somewhat unusual (as opposed to a holding
character) and the non-essential "signed" would signal that there is
something maybe a bit unusual going on here.

But this all becomes moot with your next point:

> Also, note that on most architectures the resulting code is more complex
> when using 'char' instead of 'int'. This is seen easily by compiling the
> driver for arm64: Replacing the above 'signed char' with 'int' reduces
> code size by 32 bytes.

This is reaching outside of the parts of C that I am comfortable in:
what is the correct way to sign-extend an 8-bits value into an int ?

In regmap_read() fills "int *value" with the read bytes, not
sign-extended (which looks sane):
	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
	dev_warn(&pdev->dev, "da9063_hwmon_probe offset=%d\n", tmp);
->
[Jul11 01:53] da9063-hwmon da9063-hwmon: da9063_hwmon_probe offset=247

My naïve "(int)((char)tmp)" produces 247, instead of -9.
"(int)hwmon->tjunc_offset" does sign-extend, but going through an
intermediate variable looks overcomplex to me (for a tiny definition of
"overcomplex").
I see sign_extend*() functions but seeing their bitshift arguments I
feel these may not be intended for such no-shift-needed use.

> > +static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
[...]
> > +	ret = wait_for_completion_timeout(&hwmon->adc_ready,
> > +					  msecs_to_jiffies(100));
> > +	reinit_completion(&hwmon->adc_ready);  
> 
> This is unusual. Normally I see init_completion() or reinit_completion()
> ahead of calls to wait functions.
> 
> If a request timed out and an interrupt happened after the timeout,
> the next request would return immediately with the previous result,
> since complete() would be called on the re-initialized completion
> handler. That doesn't seem to be correct to me.

To confirm my comprehension: the issue is that if somehow the irq
handler fires outside a conversion request, it will mark adf_ready as
completed, so wait_for_completion_timeout() will immediately return.
The follow-up consequences being that the ADC, having just been asked
to do a new conversion, will still be busy, leading to a spurious
ETIMEDOUT.
Is this correct ?

With this in mind, could the time from regmap_update_bits() to
{,re}init_completion() be longer than the time the IRQ could take to
trigger ? In which case adc_ready would be marked as completed, then it
would be cleared, and wait_for_completion_timeout() would reach its
timeout despite the conversion being already over.

> > +static int da9063_hwmon_probe(struct platform_device *pdev)
[...]
> > +	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
> > +	if (ret < 0) {
> > +		tmp = 0;
> > +		dev_warn(&pdev->dev,
> > +			 "Temperature trimming value cannot be read (defaulting to 0)\n");
> > +	}
> > +	hwmon->tjunc_offset = (signed char) tmp;  
> 
> Nit: Unnecessary space after typecast (checkpatch --strict would tell you).
> 
> Also, I am curious: The temperature offset is a standard hwmon attribute.
> Is it an oversight to not report it, or is it on purpose ?

It was an oversight, but now that I know about it I am not sure this
should be used: the offset is in chip-internal ADC units, so userland
cannot make use of it for temperature measurement unless the raw ADC
output is also exposed.
Is this attribute used to give an insight as to how the chip was
calibrated in-factory or otherwise good practice to expose ?

I can of course expose it and apply the same formula as for the
temperature attribute, to get the expected m°C unit.

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1