On 6/21/25 11:08 AM, Jonathan Cameron wrote:
> On Mon, 16 Jun 2025 09:54:52 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
>> On 6/14/25 5:08 AM, Jonathan Cameron wrote:
>>> On Tue, 10 Jun 2025 09:34:37 +0200
>>> Jorge Marques <jorge.marques@xxxxxxxxxx> wrote:
>>>
>>>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
>>>> approximation register (SAR) analog-to-digital converter (ADC) that
>>>> enables low-power, high-density data acquisition solutions without
>>>> sacrificing precision. This ADC offers a unique balance of performance
>>>> and power efficiency, plus innovative features for seamlessly switching
>>>> between high-resolution and low-power modes tailored to the immediate
>>>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
>>>> battery-powered, compact data acquisition and edge sensing applications.
>>>>
>>
>> ...
>>
>>>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *chan)
>>>> +{
>>>> + struct ad4052_state *st = iio_priv(indio_dev);
>>>> + const struct iio_scan_type *scan_type;
>>>> + struct spi_transfer *xfer = &st->xfer;
>>>> +
>>>> + scan_type = iio_get_current_scan_type(indio_dev, chan);
>>>> + if (IS_ERR(scan_type))
>>>> + return PTR_ERR(scan_type);
>>>> +
>>>> + xfer->rx_buf = st->raw;
>>>> + xfer->bits_per_word = scan_type->realbits;
>>>> + xfer->len = scan_type->realbits == 24 ? 4 : 2;
>>>
>>> This is a little odd. I'm not sure what happens with len not dividing
>>> into a whole number of bits per word chunks.
>>> Maybe a comment?
>>
>> Even better, there is now spi_bpw_to_bytes() for this.
>>
>>>
>>>> + xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>>
>>
>> ...
>>
>>>
>>>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
>>>> +{
>>>> + struct spi_device *spi = st->spi;
>>>> + struct spi_transfer t_cnv = {};
>>>> + int ret;
>>>> +
>>>> + reinit_completion(&st->completion);
>>>> +
>>>> + if (st->cnv_gp) {
>>>> + gpiod_set_value_cansleep(st->cnv_gp, 1);
>>>> + gpiod_set_value_cansleep(st->cnv_gp, 0);
>>>> + } else {
>>>> + ret = spi_sync_transfer(spi, &t_cnv, 1);
>>>
>>> Add a comment for this. I can't immediately spot documentation on what
>>> a content free transfer actually does. I assume pulses the chip select?
>>> is that true for all SPI controllers?
>>
>> Should be. Setting .delay in the xfer would also make it more
>> clear that this is doing.
>>
>>>
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> + /*
>>>> + * Single sample read should be used only for oversampling and
>>>> + * sampling frequency pairs that take less than 1 sec.
>>>> + */
>>>> + if (st->gp1_irq) {
>>>> + ret = wait_for_completion_timeout(&st->completion,
>>>> + msecs_to_jiffies(1000));
>>>> + if (!ret)
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + ret = spi_sync_transfer(spi, &st->xfer, 1);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (st->xfer.len == 2)
>>>> + *val = sign_extend32(*(u16 *)(st->raw), 15);
>>>> + else
>>>> + *val = sign_extend32(*(u32 *)(st->raw), 23);
>>>> +
>>>> + return ret;
>>>> +}
>>>
>>
>> ...
>>
>>>> +
>>>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>>>> + unsigned int writeval, unsigned int *readval)
>>>> +{
>>>> + struct ad4052_state *st = iio_priv(indio_dev);
>>>> + int ret;
>>>> +
>>>> + if (!iio_device_claim_direct(indio_dev))
>>>
>>> For these guards in the debugfs callback, please add a comment on why they
>>> are needed. We've had a lot of questions about these recently and I'd
>>> like it to be clear to people when they should cut and paste these and when
>>> not.
>>
>> The reason I started doing this is that running the iio_info command attemps
>> to read register 0x00 via the debug attribute of every single iio device. So
>> if you run iio_info during a buffered read, and 0x00 is a valid register, it
>> would break things without this check.
>>
>> Ideally, general purpose commands wouldn't be poking debug registers, but
>> that isn't the case. But I suppose we could "fix" iio_info instead.
>>
>
> Please do fix iio_info. It absolutely should not be poking the debug interfaces
> except on specific debug calls. The user has to know they may be shooting themselves
> in the foot.
>
> I'm not sure why a read of that register would break buffered capture though.
> Is it a volatile register or is there a sequencing problem with multiple
> accesses in this driver? If it is multiple accesses then that should be
> prevented via a local lock, not whether we are in buffer mode or not.
IIRC, this was particularly a problem on chips that have a separate data
capture mode and reading a register exits data capture mode.
>
> So I'm fine with this defense where it is necessary for all register
> accesses, but I would like to see comments on why it is necessary.
>
> Jonathan
>
>>>
>>>> + return -EBUSY;
>>>> +
>>>> + if (readval)
>>>> + ret = regmap_read(st->regmap, reg, readval);
>>>> + else
>>>> + ret = regmap_write(st->regmap, reg, writeval);
>>>> + iio_device_release_direct(indio_dev);
>>>> + return ret;
>>>> +}
>>>
>
