RE: [PATCH] drivers: iio: adc: add support for ad777x family

[Nechita, Ramona]

Hello, 

Thank you for the review. I implemented most of your comments, excepting some things that were slightly unclear to me or I had a different explanation I left a couple of comments below, and I will send a new patch in a separate email.

>On Wed, May 22, 2024 at 02:59:53PM +0300, ranechita wrote:
>> Added support for ad7770,ad7771,ad7779 ADCs. The data is streamed only 
>> on the spi-mode, without using the data lines.
>
>> ---
>
>Please, explain here, in the comment area, why any existing driver can not be reused (extended) for these ADCs.
>
>...
>
>> +#include <linux/gpio.h>
>
>This header must not be in the new code.
>
>...
>
>> +#define AD777X_SINC3_MAXFREQ			16000
>> +#define AD777X_SINC5_MAXFREQ			128000
>
>HZ_PER_KHZ ? You will need units.h.
>
>...
>
>> +#define DEC3					1000
>> +#define DEC6					1000000
>
>NIH some of units.h constants. Use them.
>
>...
>
>
>> +	/*
>> +	 * DMA (thus cache coherency maintenance) requires the
>> +	 * transfer buffers to live in their own cache lines.
>> +	 */
>> +	u8			reg_rx_buf[3] ____cacheline_aligned;
>> +	u8			reg_tx_buf[3];
>
>> +	u8			spidata_rx[32];
>> +	u8			spidata_tx[32];
>
>These will not be cache aligned. Is it a problem?

No, it should be fine without the alignment.

> ------
>
>> +{
>> +	int ret;
>> +	u8 regval, data;
>> +
>> +	ret = ad777x_spi_read(st, reg, &data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regval = data;
>> +	regval &= ~mask;
>> +	regval |= val;
>> +
>> +	if (regval != data) {
>> +		ret = ad777x_spi_write(st, reg, regval);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>
>This all can be written as
>
>	if (regval != data)
>		return ad777x_spi_write(st, reg, regval);
>
>	return 0;
>
>...or...
>
>	if (regval == data)
>		return 0;
>
>	return ad777x_spi_write(st, reg, regval);
>
>(I prefer the latter as it shows better the flow)
>
>> +}
>
>No mutex no nothing for RMW op like this?
>
>Btw, can't you use regmap for IO?

Unfortunately, I don't think regmap could be used, because of the crc and the fact that data is shifted out on the SPI SDO line in the interrupt. I consider perhaps adding regmap to the mix might complicate things a bit. 

>
>...
>
>> +	if (st->filter_enabled == AD777X_SINC3 &&
>> +	    sampling_freq > AD777X_SINC3_MAXFREQ) {
>> +		return -EINVAL;
>> +	} else if (st->filter_enabled == AD777X_SINC5 &&
>
>Redundant 'else'
>
>> +		   sampling_freq > AD777X_SINC5_MAXFREQ) {
>
>Broken indentation.
>
>> +		return -EINVAL;
>> +	}
>
>Unneeded {}.
>
>...
>
>> +	ret = ad777x_spi_write(st, AD777X_REG_SRC_N_LSB, lsb);
>> +	if (ret)
>> +		return ret;
>> +	ret = ad777x_spi_write(st, AD777X_REG_SRC_N_MSB, msb);
>> +	if (ret)
>> +		return ret;
>
>Can you use 16-bit writes?
>Same Q to all similar LSB/MSB write groups.

I cannot do 16-bit writes due to how the spi functions on the chip and because the registers for MSB/LSB are at different addresses.

>--------
>> +static void ad777x_clk_disable(void *data) {
>> +	clk_disable_unprepare(data);
>> +}
>
>See below.
>
>...
>
>> +	st->mclk = devm_clk_get(&spi->dev, "mclk");
>> +	if (IS_ERR(st->mclk))
>> +		return PTR_ERR(st->mclk);
>> +
>> +	ret = clk_prepare_enable(st->mclk);
>> +	if (ret < 0)
>> +		return ret;
>
>> +	ret = devm_add_action_or_reset(&spi->dev,
>> +				       ad777x_clk_disable,
>> +				       st->mclk);
>> +	if (ret)
>> +		return ret;
>
>So, what's wrong with the _enabled() API?

Sorry, I am not sure what you mean here by _enabled() API, is there a different mechanism that can be used for this type of operations?

Best Regards,
Ramona