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
