On Sat Aug 24, 2024 at 1:21 PM CEST, Jonathan Cameron wrote:
> On Thu, 22 Aug 2024 14:45:18 +0200
> Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
>
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> >
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> >
> > Signed-off-by: Esteban Blanc <eblanc@xxxxxxxxxxxx>
> Hi Esteban
>
> Some additional comments. David did a good review already so
> I've tried not to duplicate too much of that.
>
> The big one in here is don't use extended_name.
> It's effectively deprecated for new drivers plus
> it would have required you add a lot of ABI docs as every
> sysfs file would have a strange name.
>
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > new file mode 100644
> > index 000000000000..a981dce988e5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4030.c
>
> > +struct ad4030_state {
> > + struct spi_device *spi;
> > + struct regmap *regmap;
> > + const struct ad4030_chip_info *chip;
> > + struct gpio_desc *cnv_gpio;
> > + int vref_uv;
> > + int vio_uv;
> > + int offset_avail[3];
> > + u32 conversion_speed_hz;
> > + enum ad4030_out_mode mode;
> > +
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the transfer buffers
> > + * to live in their own cache lines.
> > + */
> > + u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> > + struct {
> > + union {
> > + u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> > + struct {
> > + s32 val;
> > + u32 common;
> > + } __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
>
> David pointed out this doesn't need to be packed.
> Given you have a union here, add __beXX as needed to avoid casts below.
They also pointed out that I should reduce the size for the common field.
I was planing to use an u32 bitfield here, 8 bits for common and 24 bits for
padding. As far as I understood, the C standard is quite flexible on the
size used for bitfield, so I should probably keep the __packed, right?
> > +};
> > +
> > +#define AD4030_CHAN_CMO(_idx) { \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = _idx * 2 + 2, \
> > + .scan_index = _idx * 2 + 1, \
> > + .extend_name = "Channel" #_idx " common byte part", \
>
> We more or less never use extend name any more because it makes writing
> userspace code much harder. Use the label callback to assign a label instead.
>
> If we were still using this, it would need to be a lot simpler than that
> and no spaces etc as it ends up int he sysfs file names.
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .storagebits = 32, \
> > + .realbits = 8, \
> > + .endianness = IIO_BE, \
> > + }, \
> > +}
> > +
> > +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) { \
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE), \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> > + BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> > + BIT(IIO_CHAN_INFO_RAW), \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> > + BIT(IIO_CHAN_INFO_CALIBSCALE), \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = _idx * 2, \
> > + .channel2 = _idx * 2 + 1, \
> > + .scan_index = _idx * 2, \
> > + .extend_name = "Channel" #_idx " differential part", \
>
> As above, no to this for same reason.
> This will generate a crazy ABI so I'm a bit surprised that didn't show
> up in your testing. Would have needed a lot of docs even if we did
> still do things this way.
I'm using ADI IIO oscilloscope to check the signals so I didn't get
impacted by the sysfs change. Anyway I will use labels.
> > + .differential = true, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .storagebits = _storage, \
> > + .realbits = _real, \
> > + .shift = _shift, \
> > + .endianness = IIO_BE, \
> > + }, \
> > +}
> > +
> > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > + void *val, size_t val_size)
> > +{
> > + struct ad4030_state *st = context;
> > +
> > + struct spi_transfer xfer = {
> > + .tx_buf = st->tx_data,
> > + .rx_buf = st->rx_data.raw,
> > + .len = reg_size + val_size,
> > + };
> > + int ret;
> > +
> > + memcpy(st->tx_data, reg, reg_size);
> > +
> > + /*
> > + * This should use spi_write_the_read but when doing so, CS never get
> > + * deasserted.
>
> I'm confused. As a single transfer it won't be deasserted in the transfer
> whereas spi_write_then_read() will. So is this comment backwards or
> is it referring to something else?
So, with a single transfer (what is done now), the transfer is working
as expected: CS goes low, the data is transferred, CS goes high again.
With spi_write_then_read(), CS goes low, data is transferred but CS never
goes high again. After some time I get a timeout error in the kernel logs.
> > +static int ad4030_conversion(struct ad4030_state *st,
> > + const struct iio_chan_spec *chan)
> > +{
> > + unsigned int bytes_to_read;
> > + unsigned char byte_index;
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Number of bytes for one differential channel */
> > + bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
> > + /* Add one byte if we are using a differential + common byte mode */
> > + bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> > + st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
> > + /* Mulitiply by the number of hardware channels */
> > + bytes_to_read *= st->chip->num_channels;
> > +
> > + gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > + ndelay(AD4030_TCNVH_NS);
> > + gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > + ndelay(st->chip->tcyc);
> > +
> > + ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > + return 0;
> > +
> > + byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > + for (i = 0; i < st->chip->num_channels; i++)
> > + st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> break line after =.
>
> When it doesn't significantly hurt readability we still try to keep to 80
> chars for IIO drivers. People have big screens but a lot of kernel devs
> love to have lots of windows across them - or have bad eyesight due to
> years of code review!
I keep forgeting that checkpatch now defaults at 100 chars...
> > +static int ad4030_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long info)
> > +{
> > + struct ad4030_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + return ad4030_single_conversion(indio_dev, chan, val);
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = (st->vref_uv * 2) / MILLI;
> > + *val2 = st->chip->precision_bits;
> > + return IIO_VAL_FRACTIONAL_LOG2;
>
> No reason you can't read this whilst buffered capture in progress.
> Maybe it's not worth the effort of special casing though.
>
> It is the one thing people do read whilst doing buffered capture
> though because they didn't cache it before starting the buffer
> and it's needed for data interpretation unlike all the other controls.
>
> Maybe just do a
> if (info == IIO_CHAN_INFO_SCALE) {
> }
> block at top of function?
Good catch. I will check for IIO_CHAN_INFO_SCALE before the whole block
> > +static int ad4030_reset(struct ad4030_state *st)
> > +{
> > + struct device *dev = &st->spi->dev;
> > + struct gpio_desc *reset;
> > + int ret;
> > +
> > + /* Use GPIO if available ... */
> > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(reset))
> > + return dev_err_probe(dev, PTR_ERR(reset),
> > + "Failed to get reset GPIO\n");
> > +
> > + if (reset) {
> > + ndelay(50);
> > + gpiod_set_value_cansleep(reset, 0);
> > + } else {
> > + /* ... falback to software reset otherwise */
> > + ret = ad4030_enter_config_mode(st);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> > + AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /* Wait for reset to complete before communicating to it */
>
> I'd rather see a reference for the value than a generic comment
> like this. Also pull the actual value down here. Not particularly
> useful to have a define for what is a real time unless you are going
> to have some combined docs for a bunch of timings (i.e a datasheet
> table reference)
I will put the real value in fsleep call directly. When you say "I'd
rather see a reference for the value", you ment a reference to the place
the value is defined in the datasheet, right?
> > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > +{
> > + unsigned int grade;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > + if (ret)
> > + return ret;
> > +
> > + grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > + if (grade != st->chip->grade)
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "Unknown grade(0x%x) for %s\n", grade,
> > + st->chip->name);
>
> Is this similar to a missmatch on a whoami value?
Yes. It also saved me multiple hours of debuging when the eval board
was not connected porperly and the SPI link was just not working.
> I.e. should we print a message and carry on in the interests of providing
> some degree of support for newer devices on older kernel?
> (fallback compatibles in DT)
Ok, let's go with a warning then.
> > +static const struct spi_device_id ad4030_id_table[] = {
> > + { "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> > + {}
>
> I'm going to assume you have a bunch of other parts you plan to
> support soon. Otherwise we normally don't add the chip specific
> support until it is needed. It tends to complicate initial driver
> review a little and experience says that sometimes no other devices
> are ever added.
I'm sending the other devices in the same series (patch 4 and 5).
For the sake of reducing noise in the later patches, I've put it in
the initial driver. If you feel like I should wait and do it in the
following patch (patch 4), I can do that.
Thanks for your time,
--
Esteban Blanc
BayLibre
