> -----Original Message-----
> From: andy.shevchenko@xxxxxxxxx <andy.shevchenko@xxxxxxxxx>
> Sent: Tuesday, June 6, 2023 8:49 AM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> Cc: jic23@xxxxxxxxxx; lars@xxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx;
> broonie@xxxxxxxxxx; lgirdwood@xxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
>
> [External]
>
> Mon, Jun 05, 2023 at 09:07:55PM +0800, Kim Seer Paller kirjoitti:
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
>
> First question, why don't you use regmap SPI?
I found it difficult to implement the regmap interface due to the timing diagram
requirements. The chip select needs to be changed between transfers, which, as
far as I know, does not work with regmap. Additionally, the regmap_config
parameters, specifically reg_bits and val_bits, cannot be set to specific values.
This limitation makes it challenging to accommodate specific configurations
such as using 5 bits for register address and 10 bits for data.
>
> ...
>
> > +static int max14001_read(void *context, unsigned int reg_addr,
> > + unsigned int *data)
>
> Strange indentation.
>
> > +{
> > + struct max14001_state *st = context;
> > + u16 tx = 0;
>
> Redundant assignment.
>
> > + int ret;
> > +
> > + struct spi_transfer xfers[] = {
> > + {
> > + .tx_buf = &st->spi_tx_buffer,
> > + .len = 2,
> > + .cs_change = 1,
> > + }, {
> > + .rx_buf = &st->spi_rx_buffer,
> > + .len = 2,
> > + },
> > + };
> > +
> > + tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
>
> > + st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
>
> This is strange. Why this monsteur construct with bitrev16() is used?
>
> According to the datasheet, the bit stream is LE16, where 10 LSB is data,
> 5 MSB is address and bit 11 is R/W.
I discovered that following the instructions in the application note made
the implementation much easier.
https://www.analog.com/media/en/technical-documentation/user-guides/guide-to-programming-the-max14001max14002-isolated-adcs--maxim-integrated.pdf
To ensure compatibility with the device, I changed the format to big
endian and reversed the data before sending it to the device.
This is why I used the "bitrev" function. I'm happy to report that
this approach worked perfectly.
>
> > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > + if (ret)
> > + return ret;
>
> > + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) &
> > +MAX14001_DATA_MASK;
>
> Ditto.
>
> > + return 0;
> > +}
>
> > +static int max14001_write(void *context, unsigned int reg_addr,
> > + unsigned int data)
> > +{
> > + struct max14001_state *st = context;
> > + u16 tx = 0;
>
> Redundant assignment.
>
> > + tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > + tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> > + tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> > +
> > + st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > + return spi_write(st->spi, &st->spi_tx_buffer, 2);
>
> sizeof() ?
>
> > +}
>
> ...
>
> > + return dev_err_probe(&spi->dev, PTR_ERR(vref),
> > + "Failed to get vref regulator");
>
> With
>
> struct device *dev = &spi->dev;
>
> this and other calls in this function can be made neater.
>
> --
> With Best Regards,
> Andy Shevchenko
>
