RE: [PATCH v2 2/2] iio: adc: max14001: New driver

[Paller, Kim Seer]

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