linux-kernel - RE: [PATCH v2] drivers: iio: adc: add support for ad777x family

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: 
 <SN6PR03MB432075B9AADF1082F87EBD62F3CF2@SN6PR03MB4320.namprd03.prod.outlook.com>
Date: Wed, 19 Jun 2024 12:36:36 +0000
From: "Nechita, Ramona" <Ramona.Nechita@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        Lars-Peter
 Clausen <lars@...afoo.de>,
        "Hennerich, Michael"
	<Michael.Hennerich@...log.com>,
        Liam Girdwood <lgirdwood@...il.com>, Mark
 Brown <broonie@...nel.org>,
        Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>,
        "Sa, Nuno" <Nuno.Sa@...log.com>,
        "Schmitt, Marcelo" <Marcelo.Schmitt@...log.com>,
        Marius Cristea
	<marius.cristea@...rochip.com>,
        Maksim Kiselev <bigunclemax@...il.com>,
        Marcus Folkesson <marcus.folkesson@...il.com>,
        "Sahin, Okan"
	<Okan.Sahin@...log.com>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Liam
 Beguin <liambeguin@...il.com>,
        Ivan Mikhaylov <fr0st61te@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] drivers: iio: adc: add support for ad777x family

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?