| Message ID | 20240206065818.2016910-1-mike.looijmans@topic.nl |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3,1/2] dt-bindings: iio: adc: ti-ads1298: Add bindings | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: > Skeleton driver for the TI ADS1298 medical ADC. This device is > typically used for ECG and similar measurements. Supports data > acquisition at configurable scale and sampling frequency. Thanks for an update, I have only a few style comments and a single one about comparison (see below). If you are going to address them as suggested, feel free to add Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> to the next version. ... > +/* Registers */ > +#define ADS1298_REG_ID 0x00 > +#define ADS1298_MASK_ID_FAMILY GENMASK(7, 3) > +#define ADS1298_MASK_ID_CHANNELS GENMASK(2, 0) > +#define ADS1298_ID_FAMILY_ADS129X 0x90 > +#define ADS1298_ID_FAMILY_ADS129XR 0xd0 + Blank line? (And so on for all registers that have bitfields defined) > +#define ADS1298_REG_CONFIG1 0x01 > +#define ADS1298_MASK_CONFIG1_HR BIT(7) > +#define ADS1298_MASK_CONFIG1_DR GENMASK(2, 0) > +#define ADS1298_SHIFT_DR_HR 6 > +#define ADS1298_SHIFT_DR_LP 7 > +#define ADS1298_LOWEST_DR 0x06 ... > + factor = (rate >> ADS1298_SHIFT_DR_HR) / val; > + if (factor >= 128) I just realized that this comparison is probably better in a form if (factor >= ADS1298_MASK_CONFIG1_HR) as it points out why this is a special case in comparison to 'if (factor)' below. What do you think? > + cfg = ADS1298_LOWEST_DR; > + else if (factor) > + cfg = ADS1298_MASK_CONFIG1_HR | ilog2(factor); /* Use HR mode */ > + else > + cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */ ... > + *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR)); Outer parentheses are redundant. ... > + wasbusy = --priv->rdata_xfer_busy; Why preincrement? How would it be different from postincrement? > + if (wasbusy) { To me more robust code would look like if (wasbusy < 1) return; ... if (wasbusy > 1) ... > + /* > + * DRDY interrupt occurred before SPI completion. Start a new > + * SPI transaction now to retrieve the data that wasn't latched > + * into the ADS1298 chip's transfer buffer yet. > + */ > + spi_async(priv->spi, &priv->rdata_msg); > + /* > + * If more than one DRDY took place, there was an overrun. Since > + * the sample is already lost, reset the counter to 1 so that > + * we will wait for a DRDY interrupt after this SPI transaction. > + */ > + if (wasbusy > 1) > + priv->rdata_xfer_busy = 1; > + } ... > + /* > + * for a single transfer mode we're kept in direct_mode until For > + * completion, avoiding a race with buffered IO. > + */ ... > + wasbusy = priv->rdata_xfer_busy++; So, it starts from negative? > + /* When no SPI transfer in transit, start one now */ > + if (!wasbusy) To be compatible with above perhaps if (wasbusy < 1) also makes it more robust (all negative numbers will be considered the same. > + spi_async(priv->spi, &priv->rdata_msg); ... > + struct device *dev = &priv->spi->dev; > + int ret; > + unsigned int val; Better ordering is so called reversed xmas tree (longest lines first): struct device *dev = &priv->spi->dev; unsigned int val; int ret; ... > + /* Device initializes into RDATAC mode, which we don't want. */ No period at the end of one-line comments (be consistent in the style over comments of the same class). ... > + dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val), > + (unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS))); Castings in printf() is a big red flag usually (it's rarely we need them). Why is it here? ... > + /* VREF can be supplied externally. Otherwise use internal reference */ Better to use comma as it's one-line comment. Or make it multi-line with respective periods. ... > + ret = devm_add_action_or_reset(dev, ads1298_reg_disable, > + priv->reg_vref); Can be one line. > + if (ret) > + return ret; ... > + ret = devm_add_action_or_reset(dev, ads1298_reg_disable, > + priv->reg_avdd); One line. > + if (ret) > + return ret; ... > + if (reset_gpio) { > + /* Minimum reset pulsewidth is 2 clock cycles */ > + udelay(ADS1298_CLOCKS_TO_USECS(2)); > + gpiod_set_value(reset_gpio, 0); I would rewrite it as /* Minimum reset pulsewidth is 2 clock cycles */ gpiod_set_value(reset_gpio, 1); udelay(ADS1298_CLOCKS_TO_USECS(2)); gpiod_set_value(reset_gpio, 0); to be sure we have a reset done correctly, and the comment will make more sense. > + } else { > + ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET); > + if (ret) > + return dev_err_probe(dev, ret, "RESET failed\n"); > + } > + /* Wait 18 clock cycles for reset command to complete */ > + udelay(ADS1298_CLOCKS_TO_USECS(18));
On 06-02-2024 13:57, Andy Shevchenko wrote: > On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: >> Skeleton driver for the TI ADS1298 medical ADC. This device is >> typically used for ECG and similar measurements. Supports data >> acquisition at configurable scale and sampling frequency. > Thanks for an update, I have only a few style comments and a single one about > comparison (see below). If you are going to address them as suggested, feel > free to add > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > to the next version. > > ... Thanks for reviewing... >> +/* Registers */ >> +#define ADS1298_REG_ID 0x00 >> +#define ADS1298_MASK_ID_FAMILY GENMASK(7, 3) >> +#define ADS1298_MASK_ID_CHANNELS GENMASK(2, 0) >> +#define ADS1298_ID_FAMILY_ADS129X 0x90 >> +#define ADS1298_ID_FAMILY_ADS129XR 0xd0 > + Blank line? (And so on for all registers that have bitfields defined) Makes sense... Looks too cluttered as it is now. > >> +#define ADS1298_REG_CONFIG1 0x01 >> +#define ADS1298_MASK_CONFIG1_HR BIT(7) >> +#define ADS1298_MASK_CONFIG1_DR GENMASK(2, 0) >> +#define ADS1298_SHIFT_DR_HR 6 >> +#define ADS1298_SHIFT_DR_LP 7 >> +#define ADS1298_LOWEST_DR 0x06 > ... > >> + factor = (rate >> ADS1298_SHIFT_DR_HR) / val; >> + if (factor >= 128) > I just realized that this comparison is probably better in a form > > if (factor >= ADS1298_MASK_CONFIG1_HR) > > as it points out why this is a special case in comparison to 'if (factor)' > below. What do you think? The "HR" bit sets the device to high-res mode (which apparently doubles the internal sample rate). But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the max oversampling factor. > ... > >> + wasbusy = --priv->rdata_xfer_busy; > Why preincrement? How would it be different from postincrement? Maybe better write this as: --priv->rdata_xfer_busy; wasbusy = priv->rdata_xfer_busy; I want the value after decrementing it. >> + if (wasbusy) { > To me more robust code would look like > > if (wasbusy < 1) > return; > ... > if (wasbusy > 1) > ... wasbusy could have been unsigned. This code will only ever execute with rdata_xfer_busy > 0 (or the SPI driver called our completion callback without us calling spi_async first) > >> + /* >> + * DRDY interrupt occurred before SPI completion. Start a new >> + * SPI transaction now to retrieve the data that wasn't latched >> + * into the ADS1298 chip's transfer buffer yet. >> + */ >> + spi_async(priv->spi, &priv->rdata_msg); >> + /* >> + * If more than one DRDY took place, there was an overrun. Since >> + * the sample is already lost, reset the counter to 1 so that >> + * we will wait for a DRDY interrupt after this SPI transaction. >> + */ >> + if (wasbusy > 1) >> + priv->rdata_xfer_busy = 1; >> + } > ... > >> + /* >> + * for a single transfer mode we're kept in direct_mode until > For > >> + * completion, avoiding a race with buffered IO. >> + */ > ... > >> + wasbusy = priv->rdata_xfer_busy++; > So, it starts from negative? > >> + /* When no SPI transfer in transit, start one now */ >> + if (!wasbusy) > To be compatible with above perhaps > > if (wasbusy < 1) > > also makes it more robust (all negative numbers will be considered the same. > >> + spi_async(priv->spi, &priv->rdata_msg); The "rdata_xfer_busy" starts at 0. Increments when a DRDY occurs. Decrements when SPI completion is reported. So the meaning of "rdata_xfer_busy" is: 0 = Waiting for DRDY interrupt 1 = SPI transfer in progress 2 = DRDY occured during SPI transfer, should start another on completion >2 = Multiple DRDY during SPI transfer, overflow, we lost rdata_xfer_busy - 2 samples > ... > > >> + dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val), >> + (unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS))); > Castings in printf() is a big red flag usually (it's rarely we need them). > Why is it here? Compiler complains that the expression is "unsigned long". Probably because of ADS1298_MASK_ID_CHANNELS being so. > ... > >> + if (reset_gpio) { >> + /* Minimum reset pulsewidth is 2 clock cycles */ >> + udelay(ADS1298_CLOCKS_TO_USECS(2)); >> + gpiod_set_value(reset_gpio, 0); > I would rewrite it as > > /* Minimum reset pulsewidth is 2 clock cycles */ > gpiod_set_value(reset_gpio, 1); > udelay(ADS1298_CLOCKS_TO_USECS(2)); > gpiod_set_value(reset_gpio, 0); > > to be sure we have a reset done correctly, and the comment will make more > sense. If used, the reset must be asserted *before* the voltages and clocks are activated. This would obfuscate that, and add a redundant call to set_value. I did forget to use "cansleep" here, will add that.
On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: > On 06-02-2024 13:57, Andy Shevchenko wrote: > > On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: ... > > > + factor = (rate >> ADS1298_SHIFT_DR_HR) / val; > > > + if (factor >= 128) > > I just realized that this comparison is probably better in a form > > > > if (factor >= ADS1298_MASK_CONFIG1_HR) > > > > as it points out why this is a special case in comparison to 'if (factor)' > > below. What do you think? > > The "HR" bit sets the device to high-res mode (which apparently doubles the > internal sample rate). > > But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the max > oversampling factor. Use BIT(..._DR_LP) and we are done here. ... > > > + wasbusy = --priv->rdata_xfer_busy; > > Why preincrement? How would it be different from postincrement? > > Maybe better write this as: > > --priv->rdata_xfer_busy; > > wasbusy = priv->rdata_xfer_busy; > > I want the value after decrementing it. Yes, looks more obvious. > > > + if (wasbusy) { > > To me more robust code would look like > > > > if (wasbusy < 1) > > return; > > ... > > if (wasbusy > 1) > > ... > > wasbusy could have been unsigned. > > This code will only ever execute with rdata_xfer_busy > 0 (or the SPI driver > called our completion callback without us calling spi_async first) You never know what may go wrong in the future :-) That said, I prefer robust code against non-robust. ... > > > + wasbusy = priv->rdata_xfer_busy++; > > So, it starts from negative? > > > > > + /* When no SPI transfer in transit, start one now */ > > > + if (!wasbusy) > > To be compatible with above perhaps > > > > if (wasbusy < 1) > > > > also makes it more robust (all negative numbers will be considered the same. > > > > > + spi_async(priv->spi, &priv->rdata_msg); > > The "rdata_xfer_busy" starts at 0. > > Increments when a DRDY occurs. > > Decrements when SPI completion is reported. > > So the meaning of "rdata_xfer_busy" is: > > 0 = Waiting for DRDY interrupt > > 1 = SPI transfer in progress > > 2 = DRDY occured during SPI transfer, should start another on completion > > >2 = Multiple DRDY during SPI transfer, overflow, we lost rdata_xfer_busy - > 2 samples Maybe another good comment is needed here as well? ... > > > + dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val), > > > + (unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS))); > > Castings in printf() is a big red flag usually (it's rarely we need them). > > Why is it here? > > Compiler complains that the expression is "unsigned long". Probably because > of ADS1298_MASK_ID_CHANNELS being so. So, use the unsigned long specifier and drop casting. ... > > > + if (reset_gpio) { > > > + /* Minimum reset pulsewidth is 2 clock cycles */ > > > + udelay(ADS1298_CLOCKS_TO_USECS(2)); > > > + gpiod_set_value(reset_gpio, 0); > > I would rewrite it as > > > > /* Minimum reset pulsewidth is 2 clock cycles */ > > gpiod_set_value(reset_gpio, 1); > > udelay(ADS1298_CLOCKS_TO_USECS(2)); > > gpiod_set_value(reset_gpio, 0); > > > > to be sure we have a reset done correctly, and the comment will make more > > sense. > > If used, the reset must be asserted *before* the voltages and clocks are > activated. This would obfuscate that, and add a redundant call to set_value. Then perhaps you want reset framework to be used instead? Dunno, but this comment seems confusing in a way that you somewhere asserted it and it's not obvious where and here is the delay out of a blue. Perhaps you may extend the comment? > I did forget to use "cansleep" here, will add that.
On 06-02-2024 14:50, Andy Shevchenko wrote: > On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: >> On 06-02-2024 13:57, Andy Shevchenko wrote: >>> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: > ... > >>>> + factor = (rate >> ADS1298_SHIFT_DR_HR) / val; >>>> + if (factor >= 128) >>> I just realized that this comparison is probably better in a form >>> >>> if (factor >= ADS1298_MASK_CONFIG1_HR) >>> >>> as it points out why this is a special case in comparison to 'if (factor)' >>> below. What do you think? >> The "HR" bit sets the device to high-res mode (which apparently doubles the >> internal sample rate). >> >> But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the max >> oversampling factor. > Use BIT(..._DR_LP) and we are done here. Ok. > ... > >>>> + wasbusy = --priv->rdata_xfer_busy; >>> Why preincrement? How would it be different from postincrement? >> Maybe better write this as: >> >> --priv->rdata_xfer_busy; >> >> wasbusy = priv->rdata_xfer_busy; >> >> I want the value after decrementing it. > Yes, looks more obvious. Having done that, and looking at it again, it's better to just eliminate the local "wasbusy" altogether. More concise. > >>>> + if (wasbusy) { >>> To me more robust code would look like >>> >>> if (wasbusy < 1) >>> return; >>> ... >>> if (wasbusy > 1) >>> ... >> wasbusy could have been unsigned. >> >> This code will only ever execute with rdata_xfer_busy > 0 (or the SPI driver >> called our completion callback without us calling spi_async first) > You never know what may go wrong in the future :-) That said, I prefer robust > code against non-robust. Maybe: BUG_ON(!priv->rdata_xfer_busy) Adds more code, dunno what weighs heavier... Haven't seen other drivers do this though. I made rdata_xfer_busy unsigned as it should have been. > ... > >>>> + wasbusy = priv->rdata_xfer_busy++; >>> So, it starts from negative? >>> >>>> + /* When no SPI transfer in transit, start one now */ >>>> + if (!wasbusy) >>> To be compatible with above perhaps >>> >>> if (wasbusy < 1) >>> >>> also makes it more robust (all negative numbers will be considered the same. >>> >>>> + spi_async(priv->spi, &priv->rdata_msg); >> The "rdata_xfer_busy" starts at 0. >> >> Increments when a DRDY occurs. >> >> Decrements when SPI completion is reported. >> >> So the meaning of "rdata_xfer_busy" is: >> >> 0 = Waiting for DRDY interrupt >> >> 1 = SPI transfer in progress >> >> 2 = DRDY occured during SPI transfer, should start another on completion >> >>> 2 = Multiple DRDY during SPI transfer, overflow, we lost rdata_xfer_busy - >> 2 samples > > Maybe another good comment is needed here as well? I thought I had it covered with the comments... I'll add more. > > ... > >>>> + dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val), >>>> + (unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS))); >>> Castings in printf() is a big red flag usually (it's rarely we need them). >>> Why is it here? >> Compiler complains that the expression is "unsigned long". Probably because >> of ADS1298_MASK_ID_CHANNELS being so. > So, use the unsigned long specifier and drop casting. > > ... > >>>> + if (reset_gpio) { >>>> + /* Minimum reset pulsewidth is 2 clock cycles */ >>>> + udelay(ADS1298_CLOCKS_TO_USECS(2)); >>>> + gpiod_set_value(reset_gpio, 0); >>> I would rewrite it as >>> >>> /* Minimum reset pulsewidth is 2 clock cycles */ >>> gpiod_set_value(reset_gpio, 1); >>> udelay(ADS1298_CLOCKS_TO_USECS(2)); >>> gpiod_set_value(reset_gpio, 0); >>> >>> to be sure we have a reset done correctly, and the comment will make more >>> sense. >> If used, the reset must be asserted *before* the voltages and clocks are >> activated. This would obfuscate that, and add a redundant call to set_value. > Then perhaps you want reset framework to be used instead? > > Dunno, but this comment seems confusing in a way that you somewhere asserted it > and it's not obvious where and here is the delay out of a blue. Perhaps you may > extend the comment? Could use devm_reset_control_get_optional_shared() I guess, but that would change devicetree bindings as well... And it wouldn't change the order, as it'd still be asserted at the start of probe()
On 06-02-2024 15:25, Mike Looijmans wrote: > On 06-02-2024 14:50, Andy Shevchenko wrote: >> On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: >>> On 06-02-2024 13:57, Andy Shevchenko wrote: >>>> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: >> >> ... >> >>>>> + wasbusy = --priv->rdata_xfer_busy; >>>> Why preincrement? How would it be different from postincrement? >>> Maybe better write this as: >>> >>> --priv->rdata_xfer_busy; >>> >>> wasbusy = priv->rdata_xfer_busy; >>> >>> I want the value after decrementing it. >> Yes, looks more obvious. > > Having done that, and looking at it again, it's better to just > eliminate the local "wasbusy" altogether. More concise. This removes the decrement operator, so the method now looks like this: static void ads1298_rdata_release_busy_or_restart(struct ads1298_private *priv) { guard(spinlock_irqsave)(&priv->irq_busy_lock); if (priv->rdata_xfer_busy > 1) { /* * DRDY interrupt occurred before SPI completion. Start a new * SPI transaction now to retrieve the data that wasn't latched * into the ADS1298 chip's transfer buffer yet. */ spi_async(priv->spi, &priv->rdata_msg); /* * If more than one DRDY took place, there was an overrun. Since * the sample is already lost, reset the counter to 1 so that * we will wait for a DRDY interrupt after this SPI transaction. */ priv->rdata_xfer_busy = 1; } else { /* No pending data, wait for DRDY */ priv->rdata_xfer_busy = 0; } }
On Tue, Feb 06, 2024 at 03:25:30PM +0100, Mike Looijmans wrote: > On 06-02-2024 14:50, Andy Shevchenko wrote: > > On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: > > > On 06-02-2024 13:57, Andy Shevchenko wrote: > > > > On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: ... > > You never know what may go wrong in the future :-) That said, I prefer robust > > code against non-robust. > > Maybe: BUG_ON(!priv->rdata_xfer_busy) Definitely not. Linus T. will scream at us on this.
On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: > On 06-02-2024 15:25, Mike Looijmans wrote: > > On 06-02-2024 14:50, Andy Shevchenko wrote: > > > On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: > > > > On 06-02-2024 13:57, Andy Shevchenko wrote: > > > > > On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: ... > > > > > > + wasbusy = --priv->rdata_xfer_busy; > > > > > Why preincrement? How would it be different from postincrement? > > > > Maybe better write this as: > > > > > > > > --priv->rdata_xfer_busy; > > > > > > > > wasbusy = priv->rdata_xfer_busy; > > > > > > > > I want the value after decrementing it. > > > Yes, looks more obvious. > > > > Having done that, and looking at it again, it's better to just eliminate > > the local "wasbusy" altogether. More concise. > > > This removes the decrement operator, so the method now looks like this: > > > static void ads1298_rdata_release_busy_or_restart(struct ads1298_private > *priv) > { > guard(spinlock_irqsave)(&priv->irq_busy_lock); > > if (priv->rdata_xfer_busy > 1) { > /* > * DRDY interrupt occurred before SPI completion. Start a new > * SPI transaction now to retrieve the data that wasn't latched > * into the ADS1298 chip's transfer buffer yet. > */ > spi_async(priv->spi, &priv->rdata_msg); > /* > * If more than one DRDY took place, there was an overrun. Since > * the sample is already lost, reset the counter to 1 so that > * we will wait for a DRDY interrupt after this SPI transaction. > */ > priv->rdata_xfer_busy = 1; > } else { > /* No pending data, wait for DRDY */ > priv->rdata_xfer_busy = 0; > } > } Yep and it looks like you reinvented atomics :-) atomic_t rdata_xfer_busy; ... But it's up to you what to do with that. Maybe Jonathan can advice something different.
On Tue, Feb 06, 2024 at 07:58:17AM +0100, Mike Looijmans wrote: > Bindings for the TI ADS1298 medical ADC. This device is > typically used for ECG and similar measurements. Supports data > acquisition at configurable scale and sampling frequency. > > The device has so many options for connecting stuff, at this > point the bindings aren't nearly complete but partial bindings > are better than no bindings at all. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > > --- > > (no changes since v2) I think you forgot a tag: https://lore.kernel.org/all/20240202-lilly-dart-0968dbcc6b17@spud/ Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On 06-02-2024 16:09, Andy Shevchenko wrote: > On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: >> On 06-02-2024 15:25, Mike Looijmans wrote: >>> On 06-02-2024 14:50, Andy Shevchenko wrote: >>>> On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote: >>>>> On 06-02-2024 13:57, Andy Shevchenko wrote: >>>>>> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote: > ... > >>>>>>> + wasbusy = --priv->rdata_xfer_busy; >>>>>> Why preincrement? How would it be different from postincrement? >>>>> Maybe better write this as: >>>>> >>>>> --priv->rdata_xfer_busy; >>>>> >>>>> wasbusy = priv->rdata_xfer_busy; >>>>> >>>>> I want the value after decrementing it. >>>> Yes, looks more obvious. >>> Having done that, and looking at it again, it's better to just eliminate >>> the local "wasbusy" altogether. More concise. >> >> This removes the decrement operator, so the method now looks like this: >> >> >> static void ads1298_rdata_release_busy_or_restart(struct ads1298_private >> *priv) >> { >> guard(spinlock_irqsave)(&priv->irq_busy_lock); >> >> if (priv->rdata_xfer_busy > 1) { >> /* >> * DRDY interrupt occurred before SPI completion. Start a new >> * SPI transaction now to retrieve the data that wasn't latched >> * into the ADS1298 chip's transfer buffer yet. >> */ >> spi_async(priv->spi, &priv->rdata_msg); >> /* >> * If more than one DRDY took place, there was an overrun. Since >> * the sample is already lost, reset the counter to 1 so that >> * we will wait for a DRDY interrupt after this SPI transaction. >> */ >> priv->rdata_xfer_busy = 1; >> } else { >> /* No pending data, wait for DRDY */ >> priv->rdata_xfer_busy = 0; >> } >> } > Yep and it looks like you reinvented atomics :-) > > atomic_t rdata_xfer_busy; > ... > > But it's up to you what to do with that. > Maybe Jonathan can advice something different. > The spinlock also protects the call to spi_async().
On Tue, Feb 06, 2024 at 04:44:03PM +0100, Mike Looijmans wrote: > On 06-02-2024 16:09, Andy Shevchenko wrote: > > On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: ... > > But it's up to you what to do with that. > > Maybe Jonathan can advice something different. > > > The spinlock also protects the call to spi_async(). I don't get this. Locks usually protect the data and not the code. Can you elaborate?
On 06-02-2024 17:32, Andy Shevchenko wrote: > On Tue, Feb 06, 2024 at 04:44:03PM +0100, Mike Looijmans wrote: >> On 06-02-2024 16:09, Andy Shevchenko wrote: >>> On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: > ... > >>> But it's up to you what to do with that. >>> Maybe Jonathan can advice something different. >>> >> The spinlock also protects the call to spi_async(). > I don't get this. Locks usually protect the data and not the code. > Can you elaborate? > Either the DRDY or SPI completion handler will call spi_async(), the lock assures that it's only called by one. Usually the DRDY handler will call spi_async(). If the next DRDY arrives before the spi_async transfer finishes, the SPI completion handler must call spi_async() a.s.a.p. to also read the newly arrived sample. There's no way to ask the chip whether there's data to read, so all the driver can do is use the ISR to remember that DRDY did trigger. The lock protects that the "busy" counter matches the actual pending calls to spi_async, and also protects that only one handler will call spi_async (and update the counter). Maybe this picture helps: DRDY ---+-----+-----+-----+- SPI ------+------------+-+-- busy 00001100011111112211101
On Tue, 6 Feb 2024 18:38:29 +0100 Mike Looijmans <mike.looijmans@topic.nl> wrote: > On 06-02-2024 17:32, Andy Shevchenko wrote: > > On Tue, Feb 06, 2024 at 04:44:03PM +0100, Mike Looijmans wrote: > >> On 06-02-2024 16:09, Andy Shevchenko wrote: > >>> On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: > > ... > > > >>> But it's up to you what to do with that. > >>> Maybe Jonathan can advice something different. > >>> > >> The spinlock also protects the call to spi_async(). > > I don't get this. Locks usually protect the data and not the code. > > Can you elaborate? > > > Either the DRDY or SPI completion handler will call spi_async(), the > lock assures that it's only called by one. Arguably it's protecting the destination buffer of the spi_async() call. We don't really care if we issue two reads (it's a waste of time and we would store two sets of readings but meh), but we do care about being sure that don't issue a second read into a buffer that we are potentially simultaneously getting data back from. There are comments where the release is to describe when it can be safely unlocked. I'm not super keen on this whole structure but I don't really have a better idea. Who builds a device where you have no latched way of seeing if there is new data? (some) Hardware folk love to assume they have a RTOS only talking to their device and that no pulse signals will ever be missed. We get to educate them when ever the opportunity arises :) Jonathan > > Usually the DRDY handler will call spi_async(). If the next DRDY arrives > before the spi_async transfer finishes, the SPI completion handler must > call spi_async() a.s.a.p. to also read the newly arrived sample. There's > no way to ask the chip whether there's data to read, so all the driver > can do is use the ISR to remember that DRDY did trigger. > > The lock protects that the "busy" counter matches the actual pending > calls to spi_async, and also protects that only one handler will call > spi_async (and update the counter). > > Maybe this picture helps: > > DRDY ---+-----+-----+-----+- > > SPI ------+------------+-+-- > > busy 00001100011111112211101 > >
On 10-02-2024 17:27, Jonathan Cameron wrote: > On Tue, 6 Feb 2024 18:38:29 +0100 > Mike Looijmans <mike.looijmans@topic.nl> wrote: > >> On 06-02-2024 17:32, Andy Shevchenko wrote: >>> On Tue, Feb 06, 2024 at 04:44:03PM +0100, Mike Looijmans wrote: >>>> On 06-02-2024 16:09, Andy Shevchenko wrote: >>>>> On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: >>> ... >>> >>>>> But it's up to you what to do with that. >>>>> Maybe Jonathan can advice something different. >>>>> >>>> The spinlock also protects the call to spi_async(). >>> I don't get this. Locks usually protect the data and not the code. >>> Can you elaborate? >>> >> Either the DRDY or SPI completion handler will call spi_async(), the >> lock assures that it's only called by one. > > Arguably it's protecting the destination buffer of the spi_async() > call. We don't really care if we issue two reads (it's a waste > of time and we would store two sets of readings but meh), but we do > care about being sure that don't issue a second read into a buffer > that we are potentially simultaneously getting data back from. Indeed, that. > > There are comments where the release is to describe when it can > be safely unlocked. > > I'm not super keen on this whole structure but I don't really have a better > idea. Who builds a device where you have no latched way of seeing > if there is new data? (some) Hardware folk love to assume they have a RTOS only > talking to their device and that no pulse signals will ever be missed. > > We get to educate them when ever the opportunity arises :) Even on RTOS this chip was a pain - to get it to work reliably I had to set up a DMA controller to run the SPI transactions, which took some smart daisy-chaining (I recall having the DMA controller write to its own control registers to avoid involving the CPU). It's probably possible to trick audio hardware (I2S controller) into grabbing the data (my chip doesn't have that) without involving the CPU. As the code is now, I can grab data and display it with the IIO oscilloscope over network at 4kHz without losing samples on an A9 at 600Mhz.
On Wed, 14 Feb 2024 07:48:40 +0100 Mike Looijmans <mike.looijmans@topic.nl> wrote: > On 10-02-2024 17:27, Jonathan Cameron wrote: > > On Tue, 6 Feb 2024 18:38:29 +0100 > > Mike Looijmans <mike.looijmans@topic.nl> wrote: > > > >> On 06-02-2024 17:32, Andy Shevchenko wrote: > >>> On Tue, Feb 06, 2024 at 04:44:03PM +0100, Mike Looijmans wrote: > >>>> On 06-02-2024 16:09, Andy Shevchenko wrote: > >>>>> On Tue, Feb 06, 2024 at 03:47:45PM +0100, Mike Looijmans wrote: > >>> ... > >>> > >>>>> But it's up to you what to do with that. > >>>>> Maybe Jonathan can advice something different. > >>>>> > >>>> The spinlock also protects the call to spi_async(). > >>> I don't get this. Locks usually protect the data and not the code. > >>> Can you elaborate? > >>> > >> Either the DRDY or SPI completion handler will call spi_async(), the > >> lock assures that it's only called by one. > > > > Arguably it's protecting the destination buffer of the spi_async() > > call. We don't really care if we issue two reads (it's a waste > > of time and we would store two sets of readings but meh), but we do > > care about being sure that don't issue a second read into a buffer > > that we are potentially simultaneously getting data back from. > > Indeed, that. > > > > > There are comments where the release is to describe when it can > > be safely unlocked. > > > > I'm not super keen on this whole structure but I don't really have a better > > idea. Who builds a device where you have no latched way of seeing > > if there is new data? (some) Hardware folk love to assume they have a RTOS only > > talking to their device and that no pulse signals will ever be missed. > > > > We get to educate them when ever the opportunity arises :) > > Even on RTOS this chip was a pain - to get it to work reliably I had to set up > a DMA controller to run the SPI transactions, which took some smart > daisy-chaining (I recall having the DMA controller write to its own control > registers to avoid involving the CPU). Always fun when that sort of mess is needed! > > It's probably possible to trick audio hardware (I2S controller) into grabbing > the data (my chip doesn't have that) without involving the CPU. Yeah, sometimes it feels like these ADCs have been designed with that sort of bus in mind. > > As the code is now, I can grab data and display it with the IIO oscilloscope > over network at 4kHz without losing samples on an A9 at 600Mhz. Nice. >
diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml new file mode 100644 index 000000000000..bf5a43a81d59 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments' ads1298 medical ADC chips + +description: | + Datasheet at: https://www.ti.com/product/ADS1298 + Bindings for this chip aren't complete. + +maintainers: + - Mike Looijmans <mike.looijmans@topic.nl> + +properties: + compatible: + enum: + - ti,ads1298 + + reg: + maxItems: 1 + + spi-cpha: true + + reset-gpios: + maxItems: 1 + + avdd-supply: + description: + Analog power supply, voltage between AVDD and AVSS. When providing a + symmetric +/- 2.5V, the regulator should report 5V. + + vref-supply: + description: + Optional reference voltage. If omitted, internal reference is used, + which is 2.4V when analog supply is below 4.4V, 4V otherwise. + + clocks: + description: Optional 2.048 MHz external source clock on CLK pin + maxItems: 1 + + interrupts: + description: Interrupt on DRDY pin, triggers on falling edge + maxItems: 1 + + label: true + +required: + - compatible + - reg + - avdd-supply + - interrupts + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@1 { + reg = <1>; + compatible = "ti,ads1298"; + label = "ads1298-1-ecg"; + avdd-supply = <®_iso_5v_a>; + clocks = <&clk_ads1298>; + interrupt-parent = <&gpio0>; + interrupts = <78 IRQ_TYPE_EDGE_FALLING>; + spi-max-frequency = <20000000>; + spi-cpha; + }; + }; +...
Bindings for the TI ADS1298 medical ADC. This device is typically used for ECG and similar measurements. Supports data acquisition at configurable scale and sampling frequency. The device has so many options for connecting stuff, at this point the bindings aren't nearly complete but partial bindings are better than no bindings at all. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- (no changes since v2) Changes in v2: Remove "clk" name Add datasheet and "incomplete" note .../bindings/iio/adc/ti,ads1298.yaml | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml