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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 
Sent: Saturday, July 27, 2024 6:41 PM
To: Nechita, Ramona <Ramona.Nechita@xxxxxxxxxx>
Cc: linux-iio@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx>; Tanislav, Cosmin <Cosmin.Tanislav@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Schmitt, Marcelo <Marcelo.Schmitt@xxxxxxxxxx>; Marius Cristea <marius.cristea@xxxxxxxxxxxxx>; Ivan Mikhaylov <fr0st61te@xxxxxxxxx>; Mike Looijmans <mike.looijmans@xxxxxxxx>; Marcus Folkesson <marcus.folkesson@xxxxxxxxx>; Liam Beguin <liambeguin@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
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@xxxxxxxxxx>
>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;
>> +}





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux