| Message ID | 20210818111139.330636-12-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Bring software triggers support to MAX1027-like ADCs | expand |
> -----Original Message----- > From: Miquel Raynal <miquel.raynal@bootlin.com> > Sent: Wednesday, August 18, 2021 1:12 PM > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > <miquel.raynal@bootlin.com> > Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler > from the read logic > > [External] > > Create a max1027_read_scan() helper which will make clearer the > future IRQ > handler updates (no functional change). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index 83526f3d7d3a..afc3ce69f7ea 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -427,19 +427,18 @@ static int > max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > return 0; > } > > -static irqreturn_t max1027_trigger_handler(int irq, void *private) > +static int max1027_read_scan(struct iio_dev *indio_dev) > { > - struct iio_poll_func *pf = private; > - struct iio_dev *indio_dev = pf->indio_dev; > struct max1027_state *st = iio_priv(indio_dev); > unsigned int scanned_chans = fls(*indio_dev- > >active_scan_mask); > u16 *buf = st->buffer; > unsigned int bit; > - > - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > private); > + int ret; > > /* fill buffer with all channel */ > - spi_read(st->spi, st->buffer, scanned_chans * 2); > + ret = spi_read(st->spi, st->buffer, scanned_chans * 2); > + if (ret < 0) > + return ret; > > /* Only keep the channels selected by the user */ > for_each_set_bit(bit, indio_dev->active_scan_mask, > @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int > irq, void *private) > > iio_push_to_buffers(indio_dev, st->buffer); > > + return 0; > +} > + > +static irqreturn_t max1027_trigger_handler(int irq, void *private) > +{ > + struct iio_poll_func *pf = private; > + struct iio_dev *indio_dev = pf->indio_dev; > + int ret; > + > + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > private); > + This should be more consistent... use 'dev_err()'. I would also argue to use the spi dev as the driver state structure holds a pointer to it... - Nuno Sá > + ret = max1027_read_scan(indio_dev); > + if (ret) > + dev_err(&indio_dev->dev, > + "Cannot read scanned values (%d)\n", ret); > + > iio_trigger_notify_done(indio_dev->trig); > > return IRQ_HANDLED; > -- > 2.27.0
Hi Nuno, "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Fri, 20 Aug 2021 07:23:33 +0000: > > -----Original Message----- > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > Sent: Wednesday, August 18, 2021 1:12 PM > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > > <miquel.raynal@bootlin.com> > > Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler > > from the read logic > > > > [External] > > > > Create a max1027_read_scan() helper which will make clearer the > > future IRQ > > handler updates (no functional change). > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------ > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > index 83526f3d7d3a..afc3ce69f7ea 100644 > > --- a/drivers/iio/adc/max1027.c > > +++ b/drivers/iio/adc/max1027.c > > @@ -427,19 +427,18 @@ static int > > max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > > return 0; > > } > > > > -static irqreturn_t max1027_trigger_handler(int irq, void *private) > > +static int max1027_read_scan(struct iio_dev *indio_dev) > > { > > - struct iio_poll_func *pf = private; > > - struct iio_dev *indio_dev = pf->indio_dev; > > struct max1027_state *st = iio_priv(indio_dev); > > unsigned int scanned_chans = fls(*indio_dev- > > >active_scan_mask); > > u16 *buf = st->buffer; > > unsigned int bit; > > - > > - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > private); > > + int ret; > > > > /* fill buffer with all channel */ > > - spi_read(st->spi, st->buffer, scanned_chans * 2); > > + ret = spi_read(st->spi, st->buffer, scanned_chans * 2); > > + if (ret < 0) > > + return ret; > > > > /* Only keep the channels selected by the user */ > > for_each_set_bit(bit, indio_dev->active_scan_mask, > > @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int > > irq, void *private) > > > > iio_push_to_buffers(indio_dev, st->buffer); > > > > + return 0; > > +} > > + > > +static irqreturn_t max1027_trigger_handler(int irq, void *private) > > +{ > > + struct iio_poll_func *pf = private; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + int ret; > > + > > + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > private); > > + > > This should be more consistent... use 'dev_err()'. I would also > argue to use the spi dev as the driver state structure holds a > pointer to it... I honestly don't see the point of these debug messages (there is another useless pr_debug in probe). I kept it here as I am just moving code around without any changes, but if you don't like it (me neither) I'll add a simple patch dropping them. Thanks, Miquèl
On Thu, 2 Sep 2021 10:55:19 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Nuno, > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Fri, 20 Aug 2021 07:23:33 > +0000: > > > > -----Original Message----- > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > Sent: Wednesday, August 18, 2021 1:12 PM > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > <lars@metafoo.de> > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > > > <miquel.raynal@bootlin.com> > > > Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler > > > from the read logic > > > > > > [External] > > > > > > Create a max1027_read_scan() helper which will make clearer the > > > future IRQ > > > handler updates (no functional change). > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------ > > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > > index 83526f3d7d3a..afc3ce69f7ea 100644 > > > --- a/drivers/iio/adc/max1027.c > > > +++ b/drivers/iio/adc/max1027.c > > > @@ -427,19 +427,18 @@ static int > > > max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > > > return 0; > > > } > > > > > > -static irqreturn_t max1027_trigger_handler(int irq, void *private) > > > +static int max1027_read_scan(struct iio_dev *indio_dev) > > > { > > > - struct iio_poll_func *pf = private; > > > - struct iio_dev *indio_dev = pf->indio_dev; > > > struct max1027_state *st = iio_priv(indio_dev); > > > unsigned int scanned_chans = fls(*indio_dev- > > > >active_scan_mask); > > > u16 *buf = st->buffer; > > > unsigned int bit; > > > - > > > - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > > private); > > > + int ret; > > > > > > /* fill buffer with all channel */ > > > - spi_read(st->spi, st->buffer, scanned_chans * 2); > > > + ret = spi_read(st->spi, st->buffer, scanned_chans * 2); > > > + if (ret < 0) > > > + return ret; > > > > > > /* Only keep the channels selected by the user */ > > > for_each_set_bit(bit, indio_dev->active_scan_mask, > > > @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int > > > irq, void *private) > > > > > > iio_push_to_buffers(indio_dev, st->buffer); > > > > > > + return 0; > > > +} > > > + > > > +static irqreturn_t max1027_trigger_handler(int irq, void *private) > > > +{ > > > + struct iio_poll_func *pf = private; > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > + int ret; > > > + > > > + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > > private); > > > + > > > > This should be more consistent... use 'dev_err()'. I would also > > argue to use the spi dev as the driver state structure holds a > > pointer to it... > > I honestly don't see the point of these debug messages (there is > another useless pr_debug in probe). I kept it here as I am just moving > code around without any changes, but if you don't like it (me neither) > I'll add a simple patch dropping them. Go for it :) > > Thanks, > Miquèl
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 83526f3d7d3a..afc3ce69f7ea 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -427,19 +427,18 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) return 0; } -static irqreturn_t max1027_trigger_handler(int irq, void *private) +static int max1027_read_scan(struct iio_dev *indio_dev) { - struct iio_poll_func *pf = private; - struct iio_dev *indio_dev = pf->indio_dev; struct max1027_state *st = iio_priv(indio_dev); unsigned int scanned_chans = fls(*indio_dev->active_scan_mask); u16 *buf = st->buffer; unsigned int bit; - - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); + int ret; /* fill buffer with all channel */ - spi_read(st->spi, st->buffer, scanned_chans * 2); + ret = spi_read(st->spi, st->buffer, scanned_chans * 2); + if (ret < 0) + return ret; /* Only keep the channels selected by the user */ for_each_set_bit(bit, indio_dev->active_scan_mask, @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) iio_push_to_buffers(indio_dev, st->buffer); + return 0; +} + +static irqreturn_t max1027_trigger_handler(int irq, void *private) +{ + struct iio_poll_func *pf = private; + struct iio_dev *indio_dev = pf->indio_dev; + int ret; + + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); + + ret = max1027_read_scan(indio_dev); + if (ret) + dev_err(&indio_dev->dev, + "Cannot read scanned values (%d)\n", ret); + iio_trigger_notify_done(indio_dev->trig); return IRQ_HANDLED;
Create a max1027_read_scan() helper which will make clearer the future IRQ handler updates (no functional change). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)