| Message ID | 20241203110019.1520071-20-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7124: Various fixes | expand |
On Tue, Dec 3, 2024 at 1:01 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > It can happen if a previous conversion was aborted the ADC pulls down > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling Interesting use of Unicode, but I suggest to avoid it when it can be avoided, i.e. using the notation of #RDY_N might be appropriate as that is how usually the HW people refer to the active low signals. > the irq might immediately fire (depending on the irq controller's controller > capabilities) and even with a rdy-gpio isn't identified as an unrelated > one. > > To cure that problem check for a pending event before the measurement is > started and clear it if needed. ... > + data = kzalloc(data_read_len + 1, GFP_KERNEL); Yes, I know that's not needed right now, but would make code robust against changes. I'm talking about using __free() here. > + if (!data) > + return -ENOMEM; > + > + spi_message_init(&m); > + if (sigma_delta->info->has_registers) { > + unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA; > + > + data[0] = data_reg << sigma_delta->info->addr_shift; > + data[0] |= sigma_delta->info->read_mask; > + data[0] |= sigma_delta->comm; > + t[0].tx_buf = data; > + spi_message_add_tail(&t[0], &m); > + } > + > + /* > + * The first transferred byte is part of the real data register, > + * so this doesn't need to be 0xff. In the remaining > + * `data_read_len - 1` bytes are less than $num_resetclks ones. > + */ > + t[1].tx_buf = data + 1; > + data[1] = 0x00; > + memset(data + 2, 0xff, data_read_len - 1); > + spi_message_add_tail(&t[1], &m); > + ret = spi_sync_locked(sigma_delta->spi, &m); > + > + kfree(data); > + > + return ret; With the above this will become just as return spi_sync_locked(...);
Hello Andy, On Tue, Dec 03, 2024 at 03:10:30PM +0200, Andy Shevchenko wrote: > On Tue, Dec 3, 2024 at 1:01 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > > > It can happen if a previous conversion was aborted the ADC pulls down > > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling > > Interesting use of Unicode, but I suggest to avoid it when it can be > avoided, i.e. > using the notation of #RDY_N might be appropriate as that is how > usually the HW people refer to the active low signals. Usage of ̅R̅D̅Y has the advantage to match the reference manual and data sheet. So I tend to keep it. > > the irq might immediately fire (depending on the irq controller's > > controller That matches the way I spelled it. Do you mean s/'s//? > > capabilities) and even with a rdy-gpio isn't identified as an unrelated > > one. > > > > To cure that problem check for a pending event before the measurement is > > started and clear it if needed. > > ... > > > + data = kzalloc(data_read_len + 1, GFP_KERNEL); > > Yes, I know that's not needed right now, but would make code robust > against changes. I'm talking about using __free() here. Given that I expect this commit to be backported to stable and $ git show stable/linux-4.19.y:include/linux/cleanup.h fatal: path 'include/linux/cleanup.h' exists on disk, but not in 'stable/linux-4.19.y' I'd not do that *now* and in this commit. But in general I agree. Best regards and thanks for your feedback, Uwe
On Tue, Dec 3, 2024 at 6:16 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Tue, Dec 03, 2024 at 03:10:30PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 3, 2024 at 1:01 PM Uwe Kleine-König > > <u.kleine-koenig@baylibre.com> wrote: > > > > > > It can happen if a previous conversion was aborted the ADC pulls down > > > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling > > > > Interesting use of Unicode, but I suggest to avoid it when it can be > > avoided, i.e. > > using the notation of #RDY_N might be appropriate as that is how > > usually the HW people refer to the active low signals. > > Usage of ̅R̅D̅Y has the advantage to match the reference manual and data > sheet. So I tend to keep it. Not sure it's strictly the same. The above has two dashes on top (actually misaligned a bit) of two letters out of three, this is quite confusing (as to me to an electrical engineer) and I hardly believe it's the same in the datasheet (however nowadays everything is possible with (ab)use of Unicode). > > > the irq might immediately fire (depending on the irq controller's > > > > controller > > That matches the way I spelled it. Do you mean s/'s//? Yes. > > > capabilities) and even with a rdy-gpio isn't identified as an unrelated > > > one. > > > > > > To cure that problem check for a pending event before the measurement is > > > started and clear it if needed. ... > > > + data = kzalloc(data_read_len + 1, GFP_KERNEL); > > > > Yes, I know that's not needed right now, but would make code robust > > against changes. I'm talking about using __free() here. > > Given that I expect this commit to be backported to stable and > > $ git show stable/linux-4.19.y:include/linux/cleanup.h > fatal: path 'include/linux/cleanup.h' exists on disk, but not in 'stable/linux-4.19.y' > > I'd not do that *now* and in this commit. But in general I agree. Good!
On Tue, Dec 03, 2024 at 07:47:53PM +0200, Andy Shevchenko wrote: > On Tue, Dec 3, 2024 at 6:16 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > On Tue, Dec 03, 2024 at 03:10:30PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 3, 2024 at 1:01 PM Uwe Kleine-König > > > <u.kleine-koenig@baylibre.com> wrote: > > > > > > > > It can happen if a previous conversion was aborted the ADC pulls down > > > > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling > > > > > > Interesting use of Unicode, but I suggest to avoid it when it can be > > > avoided, i.e. > > > using the notation of #RDY_N might be appropriate as that is how > > > usually the HW people refer to the active low signals. > > > > Usage of ̅R̅D̅Y has the advantage to match the reference manual and data > > sheet. So I tend to keep it. > > Not sure it's strictly the same. The above has two dashes on top > (actually misaligned a bit) of two letters out of three, this is quite > confusing (as to me to an electrical engineer) and I hardly believe > it's the same in the datasheet (however nowadays everything is > possible with (ab)use of Unicode). I think this is "only" a misrepresentation on your end. Sometimes it happens for me, too. A forced redraw helps then. I think that's a bug in the unicode render engine. In gitk it looks completely wrong. Syntactically it's correct however, the sequence is: \xcc\x85R\xcc\x85D\xcc\x85Y, where "\xcc\x85" is the UTF-8 representation of the "combining overline" code point (0x305). That makes me remember the times when having a non-ASCII char in your name was a problem: $ git log v6.13-rc1 | grep -P -o 'Kleine-K.*?nig' | sort | uniq -c 8 Kleine-König 1 Kleine-K=C3=B6nig 1 Kleine-K=F6nig 1 Kleine-K?nig 10 Kleine-Knig 156 Kleine-Koenig 8 Kleine-Konig 12862 Kleine-König 1 Kleine-K <u.kleine-koenig If we don't start using these, it will never be repaired ... Best regards Uwe
On Tue, Dec 3, 2024 at 8:47 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Tue, Dec 03, 2024 at 07:47:53PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 3, 2024 at 6:16 PM Uwe Kleine-König > > <u.kleine-koenig@baylibre.com> wrote: > > > On Tue, Dec 03, 2024 at 03:10:30PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 3, 2024 at 1:01 PM Uwe Kleine-König > > > > <u.kleine-koenig@baylibre.com> wrote: > > > > > > > > > > It can happen if a previous conversion was aborted the ADC pulls down > > > > > the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling > > > > > > > > Interesting use of Unicode, but I suggest to avoid it when it can be > > > > avoided, i.e. > > > > using the notation of #RDY_N might be appropriate as that is how > > > > usually the HW people refer to the active low signals. > > > > > > Usage of ̅R̅D̅Y has the advantage to match the reference manual and data > > > sheet. So I tend to keep it. > > > > Not sure it's strictly the same. The above has two dashes on top > > (actually misaligned a bit) of two letters out of three, this is quite > > confusing (as to me to an electrical engineer) and I hardly believe > > it's the same in the datasheet (however nowadays everything is > > possible with (ab)use of Unicode). > > I think this is "only" a misrepresentation on your end. I think it depends on all possible compositions of the fonts, glyphs and unicode libraries, now since I looked at the lore.kernel.org (via the same browser!) I see a different appearance of this, i.e. it now has one dash on top of both R and D and one (still misaligned) on top of R on top of the first dash, the thickness of them is also different there. > Sometimes it > happens for me, too. A forced redraw helps then. I think that's a bug in > the unicode render engine. In gitk it looks completely wrong. > Syntactically it's correct however, the sequence is: > \xcc\x85R\xcc\x85D\xcc\x85Y, where "\xcc\x85" is the UTF-8 > representation of the "combining overline" code point (0x305). With all this said, please, change it to a less confusing (dependent to external libraries/tools) way. > That makes me remember the times when having a non-ASCII char in your > name was a problem: Your name in UTF-8 looks nice to me, the below is different character set mis-conversions, but it's not the same as we are talking here about. > $ git log v6.13-rc1 | grep -P -o 'Kleine-K.*?nig' | sort | uniq -c > 8 Kleine-König > 1 Kleine-K=C3=B6nig > 1 Kleine-K=F6nig > 1 Kleine-K?nig > 10 Kleine-Knig > 156 Kleine-Koenig > 8 Kleine-Konig > 12862 Kleine-König > 1 Kleine-K <u.kleine-koenig > > If we don't start using these, it will never be repaired ... Sometimes it's better not to start at all... :-)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index 9891346c2d73..308e24dbfd16 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -29,8 +29,11 @@ #define AD_SD_COMM_CHAN_MASK 0x3 #define AD_SD_REG_COMM 0x00 +#define AD_SD_REG_STATUS 0x00 #define AD_SD_REG_DATA 0x03 +#define AD_SD_REG_STATUS_RDY 0x80 + /** * ad_sd_set_comm() - Set communications register * @@ -222,6 +225,86 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta) enable_irq(sigma_delta->irq_line); } +#define AD_SD_CLEAR_DATA_BUFLEN 9 + +/* Called with `sigma_delta->bus_locked == true` only. */ +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta) +{ + bool pending_event; + unsigned int data_read_len = BITS_TO_BYTES(sigma_delta->info->num_resetclks); + u8 *data; + struct spi_transfer t[] = { + { + .len = 1, + }, { + .len = data_read_len, + } + }; + struct spi_message m; + int ret; + + /* + * Read RDY pin (if possible) or status register to check if there is an + * old event. + */ + if (sigma_delta->rdy_gpiod) { + pending_event = gpiod_get_value(sigma_delta->rdy_gpiod); + } else { + unsigned status_reg; + + ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg); + if (ret) + return ret; + + pending_event = !(status_reg & AD_SD_REG_STATUS_RDY); + } + + if (!pending_event) + return 0; + + /* + * In general the size of the data register is unknown. It varies from + * device to device, might be one byte longer if CONTROL.DATA_STATUS is + * set and even varies on some devices depending on which input is + * selected. So send one byte to start reading the data register and + * then just clock for some bytes with DIN (aka MOSI) high to not + * confuse the register access state machine after the data register was + * completely read. Note however that the sequence length must be + * shorter than the reset procedure. + */ + + data = kzalloc(data_read_len + 1, GFP_KERNEL); + if (!data) + return -ENOMEM; + + spi_message_init(&m); + if (sigma_delta->info->has_registers) { + unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA; + + data[0] = data_reg << sigma_delta->info->addr_shift; + data[0] |= sigma_delta->info->read_mask; + data[0] |= sigma_delta->comm; + t[0].tx_buf = data; + spi_message_add_tail(&t[0], &m); + } + + /* + * The first transferred byte is part of the real data register, + * so this doesn't need to be 0xff. In the remaining + * `data_read_len - 1` bytes are less than $num_resetclks ones. + */ + t[1].tx_buf = data + 1; + data[1] = 0x00; + memset(data + 2, 0xff, data_read_len - 1); + spi_message_add_tail(&t[1], &m); + + ret = spi_sync_locked(sigma_delta->spi, &m); + + kfree(data); + + return ret; +} + int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, unsigned int mode, unsigned int channel) { @@ -237,6 +320,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ret = ad_sigma_delta_set_mode(sigma_delta, mode); if (ret < 0) goto out; @@ -310,6 +397,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); ad_sd_enable_irq(sigma_delta); @@ -406,6 +497,10 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev) sigma_delta->bus_locked = true; sigma_delta->keep_cs_asserted = true; + ret = ad_sigma_delta_clear_pending_event(sigma_delta); + if (ret) + return ret; + ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS); if (ret) goto err_unlock;
It can happen if a previous conversion was aborted the ADC pulls down the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling the irq might immediately fire (depending on the irq controller's capabilities) and even with a rdy-gpio isn't identified as an unrelated one. To cure that problem check for a pending event before the measurement is started and clear it if needed. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad_sigma_delta.c | 95 ++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)