lkml.org 
[lkml]   [2021]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3 2/3] hwmon: da9063: HWMON driver
    Date
    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

    \
     
     \ /
      Last update: 2021-07-11 06:23    [W:2.827 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site