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

[Guenter Roeck]

On 7/11/21 4:39 AM, Vincent Pelletier wrote:
> Hello,
>
> On Sat, 10 Jul 2021 21:22:35 -0700, Guenter Roeck <linux@xxxxxxxxxxxx> 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