On Tue, 27 Aug 2024 18:45:49 +0200
Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
> 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?
Don't use a bitfield - they are a pain as the C standard is very vague
on the data arrangement. Just use big enough storage and #define x GENMASK()
etc to extract the dta.
>
> > > +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.
That's odd. spi_write_then_read() should not behave differently.
Perhaps a quirk of your SPI controller?
That one is worth digging into as in both cases we should have some
transactions and after that the chip select should behave the same.
> > > +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?
Exactly.
>
> > > +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.
Oops. I didn't ready on ;) Absolutely fine to have this here.
Jonathan
>
> Thanks for your time,
>
