Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver | Date | Sun, 11 Jul 2021 08:14:15 -0700 |
| |
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
| |