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;
>> +}
