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

[Guenter Roeck <linux@roeck-us.net>]

On 7/11/21 4:39 AM, Vincent Pelletier wrote:
> Hello,
> 
> On Sat, 10 Jul 2021 21:22:35 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> 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.
> 
> On the riscv machine I am writing this driver for (and the only one I
> have with this chip), I get:
>    $ gcc test.c
>    $ ./a.out
>    247 247 247
>    $ file a.out
>    a.out: ELF 64-bit LSB pie executable, UCB RISC-V, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-riscv64-lp64d.so.1, BuildID[sha1]=0a146933fa8f9ab982a7aedb91b6e43b1bd8c668, for GNU/Linux 4.15.0, not stripped
> 
> It turns out that "char", without specifiers, is unsigned in the riscv
> ABI:
>    https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#c-type-representations
> 
> And indeed with:
>    v2 = (signed char)v1;
>    v3 = (int)((signed char)v1);
> I get the expected output:
>    247 -9 -9
> 
> This means I will be leaving a (signed char) in the code, and I am
> unsure if it needs anything else:
> - someone eventually dropping the apparently useless qualifier will
>    break the code on riscv, so a comment would be good
> - OTOH, if this is an ABI-level specificity and not something unique to
>    this driver, then such comment would surely be needed in a lot of
>    places, which would just get in the way.
> 
> What is your opinion ?
> 

If char, as it appears to be, is not well defined in C, it should be
avoided to use it to express a number which could be negative.
Fortunately this seems to be the only place at least in hwmon drivers
where someone insistss using a char to express such a value.

>>> 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.
> 
> Sorry that I gave you this impression, as this is definitely not my
> intention.
> I am rather trying to understand why moving {,re}init_completion() just
> before the wait call is enough to fix the issue, as I am under the
> impression that I may need to do more:
> The hardware IRQ could have been received before the DA9063_ADC_MAN
> write, and I guess the threaded handler can be delayed. So what is
> preventing the interrupt handler from running right between
> {,re}init_completion() and the wait ?
> 
> I'm leaning towards masking the interrupt when outside
> da9063_adc_manual_read:
> - acquire measure lock
> - if ADC is not ready, return some error (-EIO ? -EAGAIN ? -EBUSY ?)
>    as there does not seem to be a way to cancel an already triggered
>    conversion, so no way to prevent an interrupt triggering at an
>    unexpected time
> - clear any pending ADC IRQ
> - unmask ADC IRQ
> - clear completion
> - trigger measure
> - wait for completion
> - if timeout, return -ETIMEDOUT
> - decode measure
> - mask ADC IRQ
> - release measure lock
> 
> (plus a few gotos to cleanup code, and register read/write error
> propagation)
> This looks race-free to me, at the cost of a 3 extra register writes.
> 

Seems to me that da9063 is similar to da9052 and da9055, except for
conversion constants, and that it should be sufficient to follow the
guidance from that code.

Guenter