Messages in this thread |  | | Date | Wed, 26 Nov 2025 15:41:58 -0500 | | Subject | Re: [PATCH 2/2] iio: adc: Add ti-ads1x18 driver | | From | "Kurt Borja" <> |
| |
On Sat Nov 22, 2025 at 10:56 AM -05, David Lechner wrote: > On 11/21/25 6:24 PM, Kurt Borja wrote: >> On Fri Nov 21, 2025 at 5:33 PM -05, David Lechner wrote: >>> On 11/21/25 11:16 AM, Kurt Borja wrote: >>> > > ... > >>> #define ADS1018_CFG_REG 0x0000 >> >> I didn't define these because ads1118 dumps all registers (2) in each >> transfer. > > Oh, right, there is basically only one register so we don't have to > address it. :-) > > >>> >>> It is a bit confusing to have this here rather than in the buffer >>> enable callback since that is also setting the config that triggers >>> the first conversion. >>> >>> Having the spi_bus_lock() and enable_irq() in the buffer enable >>> would make more sense to me too. >> >> This is the approach ad_sigma_delta takes. >> > > I did some work on that with that module recently. I would not say that > it is an ideal reference. IIRC, it still has some race condition with > enabling/disabling interrupts in some cases. So hopefully we can do better > here.
Oh - I meant haveing spi_bus_lock() and enable_irq() in buffer enable is the approach ad_sigma_delta takes.
I moved them to set_trigger_state() to be able to use other triggers. Furthermore if interrupts or drdy-gpios are not defined, we don't really have to take spi_bus_lock() and doing it this way ensures that.
For this reason I would like to keep this for v2 and we can discuss it further if you disagree.
> >>>> + >>>> +static int ads1x18_message_init(struct ads1x18 *ads1x18) >>>> +{ >>>> + struct spi_device *spi = ads1x18->spi; >>>> + >>>> + /* >>>> + * We need to keep CS asserted to catch "data-ready" interrupts. >>>> + * Otherwise the DOUT/DRDY line enters a Hi-Z state and it can't be >>>> + * driven by the ADC. >>>> + */ >>>> + ads1x18->xfer.cs_change = 1; >>> >>> I think this is going to be problematic for reading/writing the configuration >>> register and for direct reads of a single sample. My suggestion to make a >> >> Can you elaborate on why it would be problematic? > > This transfer is used for all SPI messages. So it means that CS will still > be high after every transfer, not just the ones during a buffered read where > it is actually needed. > > This would be a problem if there were any other devices on the SPI bus. > When the controller communicates with the other device, the ADC will > still be listening and responding because CS is still high.
Thanks for the heads up!
This was a misunderstading on my part, I thought the SPI core would de-assert CS if another device requested a transfer.
> > >>>> + >>>> +static int ads1x18_channels_init(struct ads1x18 *ads1x18, >>>> + const struct ads1x18_chip_info *info, >>>> + struct iio_chan_spec **cs) >>>> +{ >>>> + struct device *dev = &ads1x18->spi->dev; >>>> + struct iio_chan_spec *channels; >>>> + int ret, nchans, index = 0; >>>> + >>>> + nchans = device_get_named_child_node_count(dev, "channel"); >>>> + if (!nchans) >>>> + return dev_err_probe(dev, -ENODEV, >>>> + "No ADC channels described.\n"); >>>> + >>>> + channels = devm_kcalloc(dev, nchans + 2, sizeof(*channels), GFP_KERNEL); >>>> + if (!channels) >>>> + return -ENOMEM; >>>> + >>>> + device_for_each_named_child_node_scoped(dev, child, "channel") { >>>> + ret = ads1x18_fill_properties(ads1x18, child, &channels[index]); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + channels[index].scan_index = index; >>>> + ads1x18->bufidx_to_addr[index] = channels[index].address; >>>> + index++; >>>> + } >>> >>> There is a small enough number of channels that we shouldn't need any of this. >>> We can just make an array big enough for all channels in struct ads1x18. >> >> Ack. >> >> Do you think we should just let every channel be visible in sysfs or >> should we still control visibility with the channel@[0-7] node? > > Yes. It is normal to show all channels. The few exceptions, like multiplexed > chips where there can be 100s or 1000s of possible combinations of differential > channels possible. And sometimes for ADCs built into a SoC, we omit the channels > that aren't wired up to something. > > It makes it much easier to write userspace software though if every instance > of the ADC has exactly the same attributes, so I try to advocate for that.
Agreed.
> > >>>> + ads1x18->chip_info = info; >>>> + mutex_init(&ads1x18->msg_lock); >>>> + init_completion(&ads1x18->data_ready); >>>> + spi_set_drvdata(spi, ads1x18); >>> >>> There is no spi_get_drvdata(), so we don't need this. >> >> I do however use dev_get_drvdata() directly in PM ops. >> > > OK, so dev_set_drvdata() would make it symmetric. > > >>> >>> I think we could simplify this and avoid needing to use pm runtime (and use >>> even less power!). During probe, put the chip in power down mode. When doing >>> direct reads of a single value, put the chip in single-shot mode. When doing >>> starting a buffered read, put it in continuous mode and when the buffered read >>> is stopped, put it back in shutdown mode. >> >> These chips only have two modes single-shot (low-power) and continuous. >> Are you suggesting we shut it down using the vdd regulator? >> >> Either way, can't the system go to sleep while in buffer mode? If that's >> the case we should still need these handlers. >> > > I hope not. I would suspect that most IIO drivers are broken in this > regard. I've never attempted to try to implement suspend/resume in an > IIO driver yet because I didn't have an application that required it > and it would be very difficult to get right without very extensive > testing.
Then I'll drop it.
Thanks! I will submit v2 soon :)
-- ~ Kurt
|  |