Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and similar ADCs

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2024-06-26 at 09:42 -0500, David Lechner wrote:
> On 6/26/24 6:47 AM, Nuno Sá wrote:
> > Hi David,
> > 
> > minor stuff from me..
> > 
> > 
> > ...
> > 
> > > +
> > > +static int ad4695_write_chn_cfg(struct ad4695_state *st,
> > > +				struct ad4695_channel_config *cfg)
> > > +{
> > > +	u32 mask = 0, val = 0;
> > > +
> > > +	mask |= AD4695_REG_CONFIG_IN_MODE;
> > > +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_MODE, cfg->bipolar ? 1 :
> > > 0);
> > > +
> > 
> > nit: don't need to OR the first assignments and so initializing the
> > variables.
> 
> :+1:
> 
> > 
> > > +	mask |= AD4695_REG_CONFIG_IN_PAIR;
> > > +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_PAIR, cfg->pin_pairing);
> > > +
> > > +	mask |= AD4695_REG_CONFIG_IN_AINHIGHZ_EN;
> > > +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_AINHIGHZ_EN, cfg->highz_en
> > > ? 1
> > > : 0);
> > > +
> > > +	return regmap_update_bits(st->regmap, AD4695_REG_CONFIG_IN(cfg-
> > > > channel),
> > > +				  mask, val);
> > > +}
> > > +
> > > +/**
> > > + * ad4695_read_one_sample - Read a single sample using single-cycle mode
> > > + * @st: The AD4695 state
> > > + * @address: The address of the channel to read
> > > + *
> > > + * Upon return, the sample will be stored in the raw_data field of @st.
> > > + *
> > > + * Context: can sleep, must be called with iio_device_claim_direct held
> > > + * Return: 0 on success, a negative error code on failure
> > > + */
> > > +static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int
> > > address)
> > > +{
> > > +	struct spi_transfer xfer[2] = { };
> > > +	int ret;
> > > +
> > > +	ret = ad4695_set_single_cycle_mode(st, address);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Setting the first channel to the temperature channel isn't
> > > supported
> > > +	 * in single-cycle mode, so we have to do an extra xfer to read
> > > the
> > > +	 * temperature.
> > > +	 */
> > > +	if (address == AD4695_CMD_TEMP_CHAN) {
> > > +		/* We aren't reading, so we can make this a short xfer.
> > > */
> > > +		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
> > > +		xfer[0].bits_per_word = 8;
> > 
> > nit: isn't this the default?
> 
> yes (looks like leftover from testing when I was trying 16 instead of 8)
> 
> > 
> > > +		xfer[0].tx_buf = &st->cnv_cmd2;
> > > +		xfer[0].len = 1;
> > > +		xfer[0].cs_change = 1;
> > > +		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
> > > +		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> > > +
> > > +		/* Then read the result and exit conversion mode. */
> > > +		st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
> > > +		xfer[1].bits_per_word = 16;
> > > +		xfer[1].tx_buf = &st->cnv_cmd;
> > > +		xfer[1].rx_buf = &st->raw_data;
> > > +		xfer[1].len = 2;
> > > +
> > > +		return spi_sync_transfer(st->spi, xfer, 2);
> > > +	}
> > > +
> 
> ...
> 
> > > +
> > > +static int ad4695_parse_channel_cfg(struct iio_dev *indio_dev)
> > > +{
> > > +	struct device *dev = indio_dev->dev.parent;
> > > +	struct ad4695_state *st = iio_priv(indio_dev);
> > 
> > Why not passing in struct ad4695_state directly?
> 
> Probably because that is how it was done in the ADI tree driver
> I started with. Changing it to two parameters would be fine.
> 

I think you're fine in just passing ad4695_state. If I'm not missing nothing the
iio_dev is only to get the parent device and you have that in st->spi.

> > 
> > ...
> > 
> > > 
> > > +
> > > +	/* Needed for debugfs since it only access registers 1 byte at a
> > > time. */
> > > +	ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_C,
> > > +			      AD4695_REG_SPI_CONFIG_C_MB_STRICT);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > 
> > Question... do we gain something but not doing the above? Because debugfs is
> > optional and always doing it even when it's not present looks unnecessary. 
> 
> I haven't got to a place where we need to read or write a 2 byte register
> yet, so I'm not sure. My plan is to defer worrying about it until then
> and update this if necessary in a future patch when it actually makes a
> difference. But for now, this is harmless because we are only reading
> and writing single byte registers.
> 

ack

- Nuno Sá
>