| Message ID | 20220625103853.2470346-4-marcus.folkesson@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,01/10] iio: adc: mcp3911: correct "microchip,device-addr" property | expand |
On Sat, 25 Jun 2022 12:38:47 +0200 Marcus Folkesson <marcus.folkesson@gmail.com> wrote: > Make it possible to read values upon interrupts. > Configure Data Ready Signal Output Pin to either HiZ or push-pull and > use it as interrupt source. > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> Hi Marcus, A few minor things inline. Jonathan > --- > > Notes: > v2: > - Removed blank lines (Andy Shevchenko) > - Removed dr_hiz variable (Andy Shevchenko) > > drivers/iio/adc/mcp3911.c | 65 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c > index 2a4bf374f140..f4ee0c27c2ab 100644 > --- a/drivers/iio/adc/mcp3911.c > +++ b/drivers/iio/adc/mcp3911.c > @@ -26,6 +26,7 @@ > #define MCP3911_REG_GAIN 0x09 > > #define MCP3911_REG_STATUSCOM 0x0a > +#define MCP3911_STATUSCOM_DRHIZ BIT(12) > #define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4) > #define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3) > #define MCP3911_STATUSCOM_EN_OFFCAL BIT(2) > @@ -58,6 +59,7 @@ struct mcp3911 { > struct regulator *vref; > struct clk *clki; > u32 dev_addr; > + struct iio_trigger *trig; > struct { > u32 channels[2]; > s64 ts __aligned(8); > @@ -252,6 +254,17 @@ static const struct iio_info mcp3911_info = { > .write_raw = mcp3911_write_raw, > }; > > +static irqreturn_t mcp3911_interrupt(int irq, void *dev_id) > +{ > + struct iio_dev *indio_dev = dev_id; > + struct mcp3911 *adc = iio_priv(indio_dev); > + > + if (iio_buffer_enabled(indio_dev)) Hmm. So I think this protection is only relevant for races around disabling of the trigger. Those shouldn't matter as just look like the actual write to disable the trigger was a bit later relative to the asynchronous capture of data. So I think you can drop it. If it is here for some other reason please ad a comment. With that dropped you can use iio_trigger_generic_data_rdy_poll() for your interrupt handler (as it's identical to the rest of this code). > + iio_trigger_poll(adc->trig); > + > + return IRQ_HANDLED; > +}; > + > static int mcp3911_config(struct mcp3911 *adc) > { > struct device *dev = &adc->spi->dev; > @@ -298,6 +311,23 @@ static int mcp3911_config(struct mcp3911 *adc) > return mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2); > } > > +static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable) > +{ > + struct mcp3911 *adc = iio_trigger_get_drvdata(trig); > + > + if (enable) > + enable_irq(adc->spi->irq); > + else > + disable_irq(adc->spi->irq); > + > + return 0; > +} > + > +static const struct iio_trigger_ops mcp3911_trigger_ops = { > + .validate_device = iio_trigger_validate_own_device, > + .set_trigger_state = mcp3911_set_trigger_state, > +}; > + > static int mcp3911_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi) > if (ret) > goto clk_disable; > > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz")) > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > + 0, 2); > + else > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > + MCP3911_STATUSCOM_DRHIZ, 2); > + if (ret < 0) > + goto clk_disable; > + > indio_dev->name = spi_get_device_id(spi)->name; > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; > indio_dev->info = &mcp3911_info; > @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi) > > mutex_init(&adc->lock); > > + if (spi->irq > 0) { > + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!adc->trig) > + goto clk_disable; You definitely want to use devm managed cleanup for these. There is a patch set that adds these as standard functions, but I haven't yet seen it being picked up for this cycle (reviews have been slow coming). https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/ In meantime role your own with devm_add_action_or_reset() > + > + adc->trig->ops = &mcp3911_trigger_ops; > + iio_trigger_set_drvdata(adc->trig, adc); > + ret = devm_iio_trigger_register(&spi->dev, adc->trig); > + if (ret) > + goto clk_disable; > + > + /* > + * The device generates interrupts as long as it is powered up. > + * Some platforms might not allow the option to power it down so > + * don't enable the interrupt to avoid extra load on the system > + */ Gah. Always annoying when devices don't support masking. Your handling is indeed the best we can do. > + ret = devm_request_irq(&spi->dev, spi->irq, > + &mcp3911_interrupt, > + IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN | IRQF_ONESHOT, Don't set trigger_falling in the driver, rely on the firmware bindings to do that for you as there may well be inverters in the path on some boards that aren't visible to Linux. We used to always do it in the driver and unfortunately are stuck with it where already present to avoid breaking boards that previously worked. We don't want to introduce more cases of this pattern though! Jonathan > + indio_dev->name, indio_dev); > + if (ret) > + goto clk_disable; > + } > + > ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > NULL, > mcp3911_trigger_handler, NULL);
... > > static int mcp3911_probe(struct spi_device *spi) > > { > > struct iio_dev *indio_dev; > > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi) > > if (ret) > > goto clk_disable; > > > > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz")) > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > > + 0, 2); > > + else > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > > + MCP3911_STATUSCOM_DRHIZ, 2); > > + if (ret < 0) > > + goto clk_disable; > > + > > indio_dev->name = spi_get_device_id(spi)->name; > > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; > > indio_dev->info = &mcp3911_info; > > @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi) > > > > mutex_init(&adc->lock); > > > > + if (spi->irq > 0) { > > + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", > > + indio_dev->name, > > + iio_device_id(indio_dev)); > > + if (!adc->trig) > > + goto clk_disable; > You definitely want to use devm managed cleanup for these. > > There is a patch set that adds these as standard functions, but I haven't > yet seen it being picked up for this cycle (reviews have been slow coming). > > https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/ Ah, elsewhere in my unread email was a thread that says it's in clk-next so will be in the next merge window. I haven't confirmed, but looks like Stephen put up an immutable branch so I could pull it into the IIO togreg branch if you want to transition directly to that new code. @Stephen, would you be fine with me pulling your clk-devm-enable branch into IIO (with the fix which got posted earlier in the week presumably also going on that branch when you push out?) Thanks, Jonathan > > In meantime role your own with devm_add_action_or_reset()
Quoting Jonathan Cameron (2022-06-25 05:06:37) > > > @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi) > > > > > > mutex_init(&adc->lock); > > > > > > + if (spi->irq > 0) { > > > + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", > > > + indio_dev->name, > > > + iio_device_id(indio_dev)); > > > + if (!adc->trig) > > > + goto clk_disable; > > You definitely want to use devm managed cleanup for these. > > > > There is a patch set that adds these as standard functions, but I haven't > > yet seen it being picked up for this cycle (reviews have been slow coming). > > > > https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/ > > Ah, elsewhere in my unread email was a thread that says it's in clk-next so > will be in the next merge window. I haven't confirmed, but looks like Stephen > put up an immutable branch so I could pull it into the IIO togreg branch if you > want to transition directly to that new code. @Stephen, would you be fine > with me pulling your clk-devm-enable branch into IIO (with the fix which > got posted earlier in the week presumably also going on that branch when > you push out?) Yes that's fine. Thanks for the heads up.
On Sat, Jun 25, 2022 at 01:06:37PM +0100, Jonathan Cameron wrote: > > ... > > > > static int mcp3911_probe(struct spi_device *spi) > > > { > > > struct iio_dev *indio_dev; > > > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi) > > > if (ret) > > > goto clk_disable; > > > > > > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz")) > > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > > > + 0, 2); > > > + else > > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, > > > + MCP3911_STATUSCOM_DRHIZ, 2); > > > + if (ret < 0) > > > + goto clk_disable; > > > + > > > indio_dev->name = spi_get_device_id(spi)->name; > > > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; > > > indio_dev->info = &mcp3911_info; > > > @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi) > > > > > > mutex_init(&adc->lock); > > > > > > + if (spi->irq > 0) { > > > + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", > > > + indio_dev->name, > > > + iio_device_id(indio_dev)); > > > + if (!adc->trig) > > > + goto clk_disable; > > You definitely want to use devm managed cleanup for these. > > > > There is a patch set that adds these as standard functions, but I haven't > > yet seen it being picked up for this cycle (reviews have been slow coming). > > > > https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/ > > Ah, elsewhere in my unread email was a thread that says it's in clk-next so > will be in the next merge window. I haven't confirmed, but looks like Stephen > put up an immutable branch so I could pull it into the IIO togreg branch if you > want to transition directly to that new code. @Stephen, would you be fine > with me pulling your clk-devm-enable branch into IIO (with the fix which > got posted earlier in the week presumably also going on that branch when > you push out?) Please do, thanks > > Thanks, > > Jonathan > > > > > > > In meantime role your own with devm_add_action_or_reset() Best regards, Marcus Folkesson
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c index 2a4bf374f140..f4ee0c27c2ab 100644 --- a/drivers/iio/adc/mcp3911.c +++ b/drivers/iio/adc/mcp3911.c @@ -26,6 +26,7 @@ #define MCP3911_REG_GAIN 0x09 #define MCP3911_REG_STATUSCOM 0x0a +#define MCP3911_STATUSCOM_DRHIZ BIT(12) #define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4) #define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3) #define MCP3911_STATUSCOM_EN_OFFCAL BIT(2) @@ -58,6 +59,7 @@ struct mcp3911 { struct regulator *vref; struct clk *clki; u32 dev_addr; + struct iio_trigger *trig; struct { u32 channels[2]; s64 ts __aligned(8); @@ -252,6 +254,17 @@ static const struct iio_info mcp3911_info = { .write_raw = mcp3911_write_raw, }; +static irqreturn_t mcp3911_interrupt(int irq, void *dev_id) +{ + struct iio_dev *indio_dev = dev_id; + struct mcp3911 *adc = iio_priv(indio_dev); + + if (iio_buffer_enabled(indio_dev)) + iio_trigger_poll(adc->trig); + + return IRQ_HANDLED; +}; + static int mcp3911_config(struct mcp3911 *adc) { struct device *dev = &adc->spi->dev; @@ -298,6 +311,23 @@ static int mcp3911_config(struct mcp3911 *adc) return mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2); } +static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable) +{ + struct mcp3911 *adc = iio_trigger_get_drvdata(trig); + + if (enable) + enable_irq(adc->spi->irq); + else + disable_irq(adc->spi->irq); + + return 0; +} + +static const struct iio_trigger_ops mcp3911_trigger_ops = { + .validate_device = iio_trigger_validate_own_device, + .set_trigger_state = mcp3911_set_trigger_state, +}; + static int mcp3911_probe(struct spi_device *spi) { struct iio_dev *indio_dev; @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi) if (ret) goto clk_disable; + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz")) + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, + 0, 2); + else + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ, + MCP3911_STATUSCOM_DRHIZ, 2); + if (ret < 0) + goto clk_disable; + indio_dev->name = spi_get_device_id(spi)->name; indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; indio_dev->info = &mcp3911_info; @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi) mutex_init(&adc->lock); + if (spi->irq > 0) { + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!adc->trig) + goto clk_disable; + + adc->trig->ops = &mcp3911_trigger_ops; + iio_trigger_set_drvdata(adc->trig, adc); + ret = devm_iio_trigger_register(&spi->dev, adc->trig); + if (ret) + goto clk_disable; + + /* + * The device generates interrupts as long as it is powered up. + * Some platforms might not allow the option to power it down so + * don't enable the interrupt to avoid extra load on the system + */ + ret = devm_request_irq(&spi->dev, spi->irq, + &mcp3911_interrupt, + IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN | IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (ret) + goto clk_disable; + } + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL, mcp3911_trigger_handler, NULL);
Make it possible to read values upon interrupts. Configure Data Ready Signal Output Pin to either HiZ or push-pull and use it as interrupt source. Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> --- Notes: v2: - Removed blank lines (Andy Shevchenko) - Removed dr_hiz variable (Andy Shevchenko) drivers/iio/adc/mcp3911.c | 65 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)