Hello,
Thank you for the feedback! I implemented most of what was mentioned, except for a few things aspects which were not exactly clear, therefore I ask some questions inline.
Best Regards,
Ramona
>
>> ---
>> drivers/iio/adc/Kconfig | 11 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ad7779.c | 951
>> +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 963 insertions(+)
>> create mode 100644 drivers/iio/adc/ad7779.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
>> 0d9282fa67f5..3e42cbc365d7 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -206,6 +206,17 @@ config AD7768_1
>> To compile this driver as a module, choose M here: the module will be
>> called ad7768-1.
>>
>> +config AD7779
>> + tristate "Analog Devices AD7779 ADC driver"
>> + depends on SPI
>> + select IIO_BUFFER
>> + help
>> + Say yes here to build support for Analog Devices AD7779 SPI
>In help text list all supported parts so that people can grep for them.
>
>> + analog to digital converter (ADC)
>It's not just an SPI converter. Seems to have a 4 wide serial interface for directly clocking out the data as >well. Might be worth mentioning that even if the driver doesn't yet support it.
>
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called ad7779.
>> +
>
>> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c new
>> file mode 100644 index 000000000000..089e352e2d40
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7779.c
>> @@ -0,0 +1,951 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AD777X ADC
>> + *
>> + * Copyright 2023 Analog Devices Inc.
>
>Probably worth updating given how much this is changing!
>
---------
>
>> +static int ad777x_set_calibscale(struct ad777x_state *st, int
>> +channel, int val) {
>> + int ret;
>> + u8 msb, mid, lsb;
>> + unsigned int gain;
>> + unsigned long long tmp;
>> +
>> + tmp = val * 5592405LL;
>> + gain = DIV_ROUND_CLOSEST_ULL(tmp, MEGA);
>> + msb = FIELD_GET(AD777X_UPPER, gain);
>> + mid = FIELD_GET(AD777X_MID, gain);
>> + lsb = FIELD_GET(AD777X_LOWER, gain);
>> + ret = ad777x_spi_write(st,
>> + AD777X_REG_CH_GAIN_UPPER_BYTE(channel),
>> + msb);
>> + if (ret)
>> + return ret;
>> + ret = ad777x_spi_write(st,
>> + AD777X_REG_CH_GAIN_MID_BYTE(channel),
>> + mid);
>> + if (ret)
>> + return ret;
>> + return ad777x_spi_write(st,
>> + AD777X_REG_CH_GAIN_LOWER_BYTE(channel),
>> + lsb);
>I assume these regisers are next to each other. If so I think Andy suggested creating your own bulk read />write. That would be a good cleanup.
There is no mention anywhere in the chip datasheet of autoincrement for spi read/writes. Therefore, implementing bulk would require constructing
Arrays of ADDR+DATA+CRC combinations, which I think would complicate the code more than simplify it.
-----
>> + ret = ad777x_spi_write(st,
>> + AD777X_REG_CH_OFFSET_MID_BYTE(channel),
>> + mid);
>> + if (ret)
>> + return ret;
>> + return ad777x_spi_write(st,
>> + AD777X_REG_CH_OFFSET_LOWER_BYTE(channel),
>> + lsb);
>> +}
>> +
>> +static int ad777x_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val,
>> + int *val2,
>> + long mask)
>> +{
>> + struct ad777x_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + ret = iio_device_claim_direct_mode(indio_dev);
>
>Use the scoped version to simplify this quite a bit.
Apologies, I am not sure what is the scoped version, could you please provide more details?
---
>> +static int __maybe_unused ad777x_suspend(struct device *dev) {
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct ad777x_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + ret = ad777x_spi_write_mask(st, AD777X_REG_GENERAL_USER_CONFIG_1,
>> + AD777X_MOD_POWERMODE_MSK,
>> + FIELD_PREP(AD777X_MOD_POWERMODE_MSK,
>> + AD777X_LOW_POWER));
>> + if (ret)
>> + return ret;
>> +
>> + st->power_mode = AD777X_LOW_POWER;
>
>This is never queried - so don't store this info until you add code that needs to know the current state and >for some reason can't just read it from the register.
Wouldn't it be more efficient to have the value stored (it is queried for setting sampling frequency), instead of performing a read each time?
