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á
>
