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

[Vincent Pelletier]

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 ?

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

> 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 ?

Not as far as I know: only the ADC triggers this interrupt, and only
this driver should trigger an ADC conversion.
The chip can trigger the ADC internally, but these should not trigger
the IRQ according to the chip's documentation.

> If so, I think this needs a comment in the code, especially since there
> is an extra i2c read which, after all, is costly.

I was curious about the cost, so I checked with regmap events in
debug/tracing (hopefully this is representative enough). Here is the
breakdown (as of patchset v3, so without the IRQ masking scheme I am
considering):
- writing to DA9063_ADC_MAN to select the channel and initiate the
  measure (3 reads, 1 write): 3.5ms
- ADC measure, based on the time between the end of DA9063_ADC_MAN
  write and when the GPIO driver masks its interrupt line: 1.6ms
- clearing the IRQ in the DA9063 (3 reads, 1 write): 2ms
- reading DA9063_ADC_MAN back (2 reads): 1ms
- reading the conversion result (2 reads): 1ms
Total (including scheduling to and from the threaded interrupt
handler): 9.3ms

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1