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