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
