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