linux-kernel - RE: [PATCH v4 3/3] 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] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SN6PR03MB432025ED9C0CB4CDD1CE243FF3932@SN6PR03MB4320.namprd03.prod.outlook.com>
Date: Tue, 3 Sep 2024 14:26:41 +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>,
        "Tanislav, Cosmin" <Cosmin.Tanislav@...log.com>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...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>,
        Ivan Mikhaylov <fr0st61te@...il.com>,
        Mike Looijmans
	<mike.looijmans@...ic.nl>,
        Marcus Folkesson <marcus.folkesson@...il.com>,
        Liam Beguin <liambeguin@...il.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>
Subject: RE: [PATCH v4 3/3] drivers: iio: adc: add support for ad777x family


Hi all,

I implemented most of the feedback from previous emails and I will be ready to send a new patch soon. One minor question inline.

Thank you,
Ramona
-----Original Message-----
From: Jonathan Cameron <jic23@...nel.org> 
Sent: Saturday, July 27, 2024 6:41 PM
To: Nechita, Ramona <Ramona.Nechita@...log.com>
Cc: linux-iio@...r.kernel.org; Lars-Peter Clausen <lars@...afoo.de>; Tanislav, Cosmin <Cosmin.Tanislav@...log.com>; Hennerich, Michael <Michael.Hennerich@...log.com>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...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>; Ivan Mikhaylov <fr0st61te@...il.com>; Mike Looijmans <mike.looijmans@...ic.nl>; Marcus Folkesson <marcus.folkesson@...il.com>; Liam Beguin <liambeguin@...il.com>; linux-kernel@...r.kernel.org; devicetree@...r.kernel.org
Subject: Re: [PATCH v4 3/3] drivers: iio: adc: add support for ad777x family

[>> Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of 
>> sending out data both on DOUT lines interface,as on the SDO line.
>> The driver currently implements only theSDO data streaming mode. SPI 
>> communication is used alternatively foraccessingregisters and 
>> streaming data. Register access are protected by crc8.
>> 
>> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@...log.com>
>Hi Ramona,
>
>Various comments inline.  Key one though is make sure you read your own code before posting as there is a bunch of left over stuff from updates still in the code.
>
>Jonathan
>
>> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c new 
>> file mode 100644 index 000000000000..7a83977fd00c
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7779.c
>
>> +
>.....
>> +static irqreturn_t ad7779_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ad7779_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +	int bit;
>> +	int k = 0;
>> +	/* Each channel shifts out HEADER + 24 bits of data
>
>Wrong comment syntax, and also early than necessary line wrap.
>
>> +	 * therefore 8 * u32 for the data and 64 bits for
>> +	 * the timestamp
>> +	 */
>> +	u32 tmp[10]; 
>> +
>> +	struct spi_transfer sd_readback_tr[] = {
>> +		{
>> +			.rx_buf = st->spidata_rx,
>> +			.tx_buf = st->spidata_tx,
>> +			.len = 32,
>
>Why 32?  Good to add some maths or a comment on the sizing.
>
>> +		}
>> +	};
>> +
>> +	if (!iio_buffer_enabled(indio_dev)){
>> +		iio_trigger_notify_done(indio_dev->trig);
>> +		return IRQ_HANDLED;
>
>use a goto.
>
>> +	}
>> +
>> +	st->spidata_tx[0] = AD7779_SPI_READ_CMD;
>> +	ret = spi_sync_transfer(st->spi, sd_readback_tr,
>> +				ARRAY_SIZE(sd_readback_tr));
>> +	if (ret) {
>> +		dev_err(&st->spi->dev,
>> +			"spi transfer error in irq handler");
>goto.
>
>> +		iio_trigger_notify_done(indio_dev->trig);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1)
>> +		tmp[k++] = st->spidata_rx[bit];
>
>Update this to use Nuno's new macros for iterating over the scan mask.

Does this refer to iio_for_each_active_channel ? I checked and noticed that the patch containing this macro is not upstream yet, should I wait for it to be merged before sending out a new patch?

>
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp);
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>......
>
>Don't need __maybe_unused as DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr() ensure
>these functions are visible to the compiler, but that it them removes them
>if they aren't in use.
>
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ad7779_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1,
>> +				    AD7779_MOD_POWERMODE_MSK,
>> +				    FIELD_PREP(AD7779_MOD_POWERMODE_MSK,
>> +					       AD7779_HIGH_POWER));
>> +	if (ret)
>> +		return ret;
>> +
>> +	st->power_mode = AD7779_HIGH_POWER;
>> +
>> +	return 0;
>> +}