| Message ID | 20210818111139.330636-16-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 15/16] iio: adc: max1027: Support software triggers > > [External] > > Now that max1027_trigger_handler() has been freed from handling > hardware > triggers EOC situations, we can use it for what it has been designed in > the first place: trigger software originated conversions. In other > words, when userspace initiates a conversion with a sysfs trigger or a > hrtimer trigger, we must do all configuration steps, ie: > 1- Configuring the trigger > 2- Configuring the channels to scan > 3- Starting the conversion (actually done automatically by step 2 in > this case) > 4- Waiting for the conversion to end > 5- Retrieving the data from the ADC > 6- Push the data to the IIO core and notify it > > Add the missing steps to this helper and drop the trigger verification > hook otherwise software triggers would simply not be accepted at all. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index 8c5995ae59f2..bb437e43adaf 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct > iio_dev *indio_dev, > return spi_write(st->spi, val, 1); > } > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > - struct iio_trigger *trig) > -{ > - struct max1027_state *st = iio_priv(indio_dev); > - > - if (st->trig != trig) > - return -EINVAL; > - > - return 0; > -} > - > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, > bool state) > { > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int > irq, void *private) > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > private); > > + ret = max1027_configure_trigger(indio_dev); > + if (ret) > + goto out; > + > + ret = max1027_configure_chans_to_scan(indio_dev); > + if (ret) > + goto out; > + > + ret = max1027_wait_eoc(indio_dev); > + if (ret) > + goto out; > + > ret = max1027_read_scan(indio_dev); There's something that I'm not getting... How are we checking that we have software triggers? This API is called only if the device allocates it's own trigger which will happen if there's a spi IRQ. I'm probably missing something as this series is fairly big but the way I would do it is (in the probe): - always call 'devm_iio_triggered_buffer_setup()' function and properly use buffer ops [1] (for example, you can use 'validate_scan_mask()' to setup the channels to read); - only allocate a trigger if an IRQ is present in which case, we assume HW triggering is supported. Does this makes sense? - Nuno Sá
On Wed, 18 Aug 2021 13:11:38 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Now that max1027_trigger_handler() has been freed from handling hardware > triggers EOC situations, we can use it for what it has been designed in > the first place: trigger software originated conversions. As mentioned earlier, this is not how I'd normally expect this sort of case to be handled. I'd be expecting the cnvst trigger to still be calling this function and the function to do the relevant check to ensure it knows the data is already available in that case. > In other > words, when userspace initiates a conversion with a sysfs trigger or a > hrtimer trigger, we must do all configuration steps, ie: > 1- Configuring the trigger > 2- Configuring the channels to scan > 3- Starting the conversion (actually done automatically by step 2 in > this case) > 4- Waiting for the conversion to end > 5- Retrieving the data from the ADC > 6- Push the data to the IIO core and notify it > > Add the missing steps to this helper and drop the trigger verification > hook otherwise software triggers would simply not be accepted at all. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index 8c5995ae59f2..bb437e43adaf 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, > return spi_write(st->spi, val, 1); > } > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > - struct iio_trigger *trig) > -{ > - struct max1027_state *st = iio_priv(indio_dev); > - > - if (st->trig != trig) > - return -EINVAL; > - > - return 0; > -} > - > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > { > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); > > + ret = max1027_configure_trigger(indio_dev); I'd not expect to see this ever time. The configuration shouldn't change from one call of this function to the next. > + if (ret) > + goto out; > + > + ret = max1027_configure_chans_to_scan(indio_dev); This should also not change unless it is also responsible for the 'go' signal. If that's true then it is badly named. > + if (ret) > + goto out; > + > + ret = max1027_wait_eoc(indio_dev); > + if (ret) > + goto out; > + > ret = max1027_read_scan(indio_dev); > + > +out: > if (ret) > dev_err(&indio_dev->dev, > "Cannot read scanned values (%d)\n", ret); > @@ -529,7 +532,6 @@ static const struct iio_trigger_ops max1027_trigger_ops = { > > static const struct iio_info max1027_info = { > .read_raw = &max1027_read_raw, > - .validate_trigger = &max1027_validate_trigger, > .debugfs_reg_access = &max1027_debugfs_reg_access, > }; >
Hi Nuno, "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Fri, 20 Aug 2021 07:58:25 +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 15/16] iio: adc: max1027: Support software triggers > > > > [External] > > > > Now that max1027_trigger_handler() has been freed from handling > > hardware > > triggers EOC situations, we can use it for what it has been designed in > > the first place: trigger software originated conversions. In other > > words, when userspace initiates a conversion with a sysfs trigger or a > > hrtimer trigger, we must do all configuration steps, ie: > > 1- Configuring the trigger > > 2- Configuring the channels to scan > > 3- Starting the conversion (actually done automatically by step 2 in > > this case) > > 4- Waiting for the conversion to end > > 5- Retrieving the data from the ADC > > 6- Push the data to the IIO core and notify it > > > > Add the missing steps to this helper and drop the trigger verification > > hook otherwise software triggers would simply not be accepted at all. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > index 8c5995ae59f2..bb437e43adaf 100644 > > --- a/drivers/iio/adc/max1027.c > > +++ b/drivers/iio/adc/max1027.c > > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct > > iio_dev *indio_dev, > > return spi_write(st->spi, val, 1); > > } > > > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > > - struct iio_trigger *trig) > > -{ > > - struct max1027_state *st = iio_priv(indio_dev); > > - > > - if (st->trig != trig) > > - return -EINVAL; > > - > > - return 0; > > -} > > - > > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, > > bool state) > > { > > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int > > irq, void *private) > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > private); > > > > + ret = max1027_configure_trigger(indio_dev); > > + if (ret) > > + goto out; > > + > > + ret = max1027_configure_chans_to_scan(indio_dev); > > + if (ret) > > + goto out; > > + > > + ret = max1027_wait_eoc(indio_dev); > > + if (ret) > > + goto out; > > + > > ret = max1027_read_scan(indio_dev); > > There's something that I'm not getting... How are we checking that > we have software triggers? This API is called only if the device > allocates it's own trigger which will happen if there's a spi IRQ. > > I'm probably missing something as this series is fairly big but the way > I would do it is (in the probe): > > - always call 'devm_iio_triggered_buffer_setup()' function and properly use > buffer ops [1] (for example, you can use 'validate_scan_mask()' to setup the > channels to read); > - only allocate a trigger if an IRQ is present in which case, we assume HW > triggering is supported. I think these are the exact steps that are enforced in the next patch, I can squash them if you wish but I think it makes sense to have it in two steps. Thanks, Miquèl
Hi Jonathan, Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 11:50:46 +0100: > On Wed, 18 Aug 2021 13:11:38 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Now that max1027_trigger_handler() has been freed from handling hardware > > triggers EOC situations, we can use it for what it has been designed in > > the first place: trigger software originated conversions. > > As mentioned earlier, this is not how I'd normally expect this sort of > case to be handled. I'd be expecting the cnvst trigger to still be calling > this function and the function to do the relevant check to ensure it > knows the data is already available in that case. I tried to follow your advice and Nuno's regarding this, I hope the new version will match your expectations (new version coming soon). However if my changes do not match, I will probably need more guidance to understand in deep what you suggest. > > In other > > words, when userspace initiates a conversion with a sysfs trigger or a > > hrtimer trigger, we must do all configuration steps, ie: > > 1- Configuring the trigger > > 2- Configuring the channels to scan > > 3- Starting the conversion (actually done automatically by step 2 in > > this case) > > 4- Waiting for the conversion to end > > 5- Retrieving the data from the ADC > > 6- Push the data to the IIO core and notify it > > > > Add the missing steps to this helper and drop the trigger verification > > hook otherwise software triggers would simply not be accepted at all. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > index 8c5995ae59f2..bb437e43adaf 100644 > > --- a/drivers/iio/adc/max1027.c > > +++ b/drivers/iio/adc/max1027.c > > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, > > return spi_write(st->spi, val, 1); > > } > > > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > > - struct iio_trigger *trig) > > -{ > > - struct max1027_state *st = iio_priv(indio_dev); > > - > > - if (st->trig != trig) > > - return -EINVAL; > > - > > - return 0; > > -} > > - > > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > > { > > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); > > > > + ret = max1027_configure_trigger(indio_dev); > > I'd not expect to see this ever time. The configuration shouldn't change > from one call of this function to the next. True, this is not needed. > > + if (ret) > > + goto out; > > + > > + ret = max1027_configure_chans_to_scan(indio_dev); > > This should also not change unless it is also responsible for the 'go' signal. > If that's true then it is badly named. It's responsible for the go signal, I renamed it "configure_and_start". However, just for my own understanding, when would I be supposed to configure the channels requested by the user otherwise? Thanks, Miquèl
> -----Original Message----- > From: Miquel Raynal <miquel.raynal@bootlin.com> > Sent: Thursday, September 2, 2021 2:25 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; Thomas Petazzoni > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 15/16] iio: adc: max1027: Support software > triggers > > [External] > > Hi Nuno, > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Fri, 20 Aug 2021 07:58:25 > +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 15/16] iio: adc: max1027: Support software triggers > > > > > > [External] > > > > > > Now that max1027_trigger_handler() has been freed from > handling > > > hardware > > > triggers EOC situations, we can use it for what it has been designed > in > > > the first place: trigger software originated conversions. In other > > > words, when userspace initiates a conversion with a sysfs trigger or > a > > > hrtimer trigger, we must do all configuration steps, ie: > > > 1- Configuring the trigger > > > 2- Configuring the channels to scan > > > 3- Starting the conversion (actually done automatically by step 2 in > > > this case) > > > 4- Waiting for the conversion to end > > > 5- Retrieving the data from the ADC > > > 6- Push the data to the IIO core and notify it > > > > > > Add the missing steps to this helper and drop the trigger > verification > > > hook otherwise software triggers would simply not be accepted at > all. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > > index 8c5995ae59f2..bb437e43adaf 100644 > > > --- a/drivers/iio/adc/max1027.c > > > +++ b/drivers/iio/adc/max1027.c > > > @@ -413,17 +413,6 @@ static int > max1027_debugfs_reg_access(struct > > > iio_dev *indio_dev, > > > return spi_write(st->spi, val, 1); > > > } > > > > > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > > > - struct iio_trigger *trig) > > > -{ > > > - struct max1027_state *st = iio_priv(indio_dev); > > > - > > > - if (st->trig != trig) > > > - return -EINVAL; > > > - > > > - return 0; > > > -} > > > - > > > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, > > > bool state) > > > { > > > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > > > @@ -512,7 +501,21 @@ static irqreturn_t > max1027_trigger_handler(int > > > irq, void *private) > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > > private); > > > > > > + ret = max1027_configure_trigger(indio_dev); > > > + if (ret) > > > + goto out; > > > + > > > + ret = max1027_configure_chans_to_scan(indio_dev); > > > + if (ret) > > > + goto out; > > > + > > > + ret = max1027_wait_eoc(indio_dev); > > > + if (ret) > > > + goto out; > > > + > > > ret = max1027_read_scan(indio_dev); > > > > There's something that I'm not getting... How are we checking that > > we have software triggers? This API is called only if the device > > allocates it's own trigger which will happen if there's a spi IRQ. > > > > I'm probably missing something as this series is fairly big but the way > > I would do it is (in the probe): > > > > - always call 'devm_iio_triggered_buffer_setup()' function and > properly use > > buffer ops [1] (for example, you can use 'validate_scan_mask()' to > setup the > > channels to read); > > - only allocate a trigger if an IRQ is present in which case, we assume > HW > > triggering is supported. > > I think these are the exact steps that are enforced in the next patch, > I can squash them if you wish but I think it makes sense to have it in > two steps. > Yeah, my bad here... - Nuno Sá
On Thu, 2 Sep 2021 17:21:00 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Jonathan, > > Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 11:50:46 > +0100: > > > On Wed, 18 Aug 2021 13:11:38 +0200 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Now that max1027_trigger_handler() has been freed from handling hardware > > > triggers EOC situations, we can use it for what it has been designed in > > > the first place: trigger software originated conversions. > > > > As mentioned earlier, this is not how I'd normally expect this sort of > > case to be handled. I'd be expecting the cnvst trigger to still be calling > > this function and the function to do the relevant check to ensure it > > knows the data is already available in that case. > > I tried to follow your advice and Nuno's regarding this, I hope the new > version will match your expectations (new version coming soon). > However if my changes do not match, I will probably need more guidance > to understand in deep what you suggest. > > > > In other > > > words, when userspace initiates a conversion with a sysfs trigger or a > > > hrtimer trigger, we must do all configuration steps, ie: > > > 1- Configuring the trigger > > > 2- Configuring the channels to scan > > > 3- Starting the conversion (actually done automatically by step 2 in > > > this case) > > > 4- Waiting for the conversion to end > > > 5- Retrieving the data from the ADC > > > 6- Push the data to the IIO core and notify it > > > > > > Add the missing steps to this helper and drop the trigger verification > > > hook otherwise software triggers would simply not be accepted at all. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ > > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > > index 8c5995ae59f2..bb437e43adaf 100644 > > > --- a/drivers/iio/adc/max1027.c > > > +++ b/drivers/iio/adc/max1027.c > > > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, > > > return spi_write(st->spi, val, 1); > > > } > > > > > > -static int max1027_validate_trigger(struct iio_dev *indio_dev, > > > - struct iio_trigger *trig) > > > -{ > > > - struct max1027_state *st = iio_priv(indio_dev); > > > - > > > - if (st->trig != trig) > > > - return -EINVAL; > > > - > > > - return 0; > > > -} > > > - > > > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) > > > { > > > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > > > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); > > > > > > + ret = max1027_configure_trigger(indio_dev); > > > > I'd not expect to see this ever time. The configuration shouldn't change > > from one call of this function to the next. > > True, this is not needed. > > > > + if (ret) > > > + goto out; > > > + > > > + ret = max1027_configure_chans_to_scan(indio_dev); > > > > This should also not change unless it is also responsible for the 'go' signal. > > If that's true then it is badly named. > > It's responsible for the go signal, I renamed it "configure_and_start". > > However, just for my own understanding, when would I be supposed to > configure the channels requested by the user otherwise? For buffered operation update_scan_mask callback. There is a quirk where that's not called on the buffer disable path because it's not always obvious what to 'reset to'. J > > Thanks, > Miquèl
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 8c5995ae59f2..bb437e43adaf 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, return spi_write(st->spi, val, 1); } -static int max1027_validate_trigger(struct iio_dev *indio_dev, - struct iio_trigger *trig) -{ - struct max1027_state *st = iio_priv(indio_dev); - - if (st->trig != trig) - return -EINVAL; - - return 0; -} - static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state) { struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); + ret = max1027_configure_trigger(indio_dev); + if (ret) + goto out; + + ret = max1027_configure_chans_to_scan(indio_dev); + if (ret) + goto out; + + ret = max1027_wait_eoc(indio_dev); + if (ret) + goto out; + ret = max1027_read_scan(indio_dev); + +out: if (ret) dev_err(&indio_dev->dev, "Cannot read scanned values (%d)\n", ret); @@ -529,7 +532,6 @@ static const struct iio_trigger_ops max1027_trigger_ops = { static const struct iio_info max1027_info = { .read_raw = &max1027_read_raw, - .validate_trigger = &max1027_validate_trigger, .debugfs_reg_access = &max1027_debugfs_reg_access, };
Now that max1027_trigger_handler() has been freed from handling hardware triggers EOC situations, we can use it for what it has been designed in the first place: trigger software originated conversions. In other words, when userspace initiates a conversion with a sysfs trigger or a hrtimer trigger, we must do all configuration steps, ie: 1- Configuring the trigger 2- Configuring the channels to scan 3- Starting the conversion (actually done automatically by step 2 in this case) 4- Waiting for the conversion to end 5- Retrieving the data from the ADC 6- Push the data to the IIO core and notify it Add the missing steps to this helper and drop the trigger verification hook otherwise software triggers would simply not be accepted at all. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/iio/adc/max1027.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)