Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver | Date | Sat, 10 Jul 2021 21:22:35 -0700 |
| |
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@roeck-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
| |