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

[David Lechner]

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.

> 
> ...
> 
>>
>> +
>> +	/* 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.

> 
> - Nuno Sá
>