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

[andy . shevchenko]

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?

...

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

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