Date:   Sat, 10 Jul 2021 21:22:35 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Vincent Pelletier <plr.vincent@...il.com>
Cc:     Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
        Support Opensource <support.opensource@...semi.com>,
        Lee Jones <lee.jones@...aro.org>, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Opensource [Steve Twiss]" <stwiss.opensource@...semi.com>
Subject: Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

On 7/10/21 7:55 PM, Vincent Pelletier wrote:
> 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@...ck-us.net> 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)
> 

I am sure there are, but I don't know any pointers. Either case, commenting
out include files until it fails to compile is not a good idea.
The driver then may compile with one architecture but fail with another.

>>> +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.
> 

Please drop them. They can be added back as needed.

>>> +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.
> 
Sorry, you lost me there. Those functions use shift operations to move
the sign bit where it belongs, and the shift back retains the sign bit.
What is wrong with that ?

Also:

int main()
{
         unsigned int v1 = 247;
         int v2;
         int v3;

         v2 = (char)v1;
         v3 = (int)((char)v1);

         printf("%d %d %d\n", v1, v2, v3);

         return 0;
}

produces 247 -9 -9, so I don't fully follow what your (int)((char)tmp)
looks like. Besides, the outer typecast is not necessary.
In general,
	v2 = (char)v1;
is good enough since the char -> int conversion is automatic (and sign_extend32()
is indeed overkill for this situation).

Either case, please feel free to use 'char' if you like; I won't insist
on a change to int. However, please drop the "signed".

>>> +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 ?
> 
I don't know what exactly happens. Why don't you try by setting the
timeout to a really small value, one that _does_ result in this
situation ?

> 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.
> 
... but what I do know is that I don't understand why you insist having
the reinit_completion() _after_ the  wait call. The above doesn't explain
that. I see it as potentially racy, so if you want to keep the code as-is
I'll want to see a comment in the code explaining why it has to be done
this way, and how it is not racy.

Also: a return value of 0 from wait_for_completion_timeout()
already indicates a timeout. The subsequent regmap_read() to check
if the conversion is complete should not be necessary. If it does,
it really indicates a non-timeout problem. Are there situations
(other than the race condition I am concerned about) where
an interrupt can happen but DA9063_ADC_MAN is still set ?

If so, I think this needs a comment in the code, especially since there
is an extra i2c read which, after all, is costly. Also, this should
probably generate a different error code (-EIO, maybe), and
-ETIMEDOUT should be the result of wait_for_completion_timeout()
returning 0.

>>> +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.

One would not report the raw value, but convert it to m°C.

> Is this attribute used to give an insight as to how the chip was
> calibrated in-factory or otherwise good practice to expose ?
> 

It can be exposed as read-only value if it is a read-only
register/value. Ultimately it is your call if it is indeed read-only.
It still provides some value in that case, but not much.

Thanks,
Guenter

Powered by blists - more mailing lists