From: Jonathan Cameron <jic23@kernel.org> To: Olivier Moysan <olivier.moysan@st.com> Cc: <robh+dt@kernel.org>, <mark.rutland@arm.com>, <knaack.h@gmx.de>, <lars@metafoo.de>, <devicetree@vger.kernel.org>, <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <pmeerw@pmeerw.net>, <linux-stm32@st-md-mailman.stormreply.com>, <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 2/4] iio: adc: sd modulator: add scale support Date: Sat, 8 Feb 2020 16:03:27 +0000 [thread overview] Message-ID: <20200208160327.334da020@archlinux> (raw) In-Reply-To: <20200204101008.11411-3-olivier.moysan@st.com> On Tue, 4 Feb 2020 11:10:06 +0100 Olivier Moysan <olivier.moysan@st.com> wrote: > Add scale support to sigma delta modulator. > > Signed-off-by: Olivier Moysan <olivier.moysan@st.com> I note below that there are probably some complexities around whether vref is used as you have it here or not. A few other bits inline around a race condition introduced in probe / remove. Thanks, Jonathan > --- > drivers/iio/adc/sd_adc_modulator.c | 108 ++++++++++++++++++++++++++--- > 1 file changed, 100 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c > index 560d8c7d9d86..a83f35832050 100644 > --- a/drivers/iio/adc/sd_adc_modulator.c > +++ b/drivers/iio/adc/sd_adc_modulator.c > @@ -10,8 +10,7 @@ > #include <linux/iio/triggered_buffer.h> > #include <linux/module.h> > #include <linux/of_device.h> > - > -static const struct iio_info iio_sd_mod_iio_info; > +#include <linux/regulator/consumer.h> > > static const struct iio_chan_spec iio_sd_mod_ch = { > .type = IIO_VOLTAGE, > @@ -21,34 +20,126 @@ static const struct iio_chan_spec iio_sd_mod_ch = { > .realbits = 1, > .shift = 0, > }, > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), This relies on generic sigma delta modulators using an external vref. They might have an internal always on regulator... I would suggest we only support scale for devices with explicitly defined compatibles where we can know what regulators are used etc. For many devices, there will be a single powersupply called vdd-supply or similar in DT. It may also provide a reference voltage. > +}; > + > +static const struct iio_chan_spec iio_sd_mod_ch_ads = { > + .type = IIO_VOLTAGE, > + .indexed = 1, > + .scan_type = { > + .sign = 'u', > + .realbits = 1, > + .shift = 0, > + }, > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), > + .differential = 1, > +}; > + > +struct iio_sd_mod_priv { > + struct regulator *vref; > + int vref_mv; > +}; > + > +static int iio_sd_mod_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct iio_sd_mod_priv *priv = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + *val = priv->vref_mv; > + *val2 = chan->scan_type.realbits; > + return IIO_VAL_INT; > + } > + > + return -EINVAL; > +} > + > +static const struct iio_info iio_sd_mod_iio_info = { > + .read_raw = iio_sd_mod_read_raw, > }; > > static int iio_sd_mod_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct iio_sd_mod_priv *priv; > struct iio_dev *iio; > + int ret; > > - iio = devm_iio_device_alloc(dev, 0); > + iio = devm_iio_device_alloc(dev, sizeof(*priv)); > if (!iio) > return -ENOMEM; > > + iio->channels = (const struct iio_chan_spec *) > + of_device_get_match_data(&pdev->dev); > + > + priv = iio_priv(iio); > + > iio->dev.parent = dev; > iio->dev.of_node = dev->of_node; > iio->name = dev_name(dev); > iio->info = &iio_sd_mod_iio_info; > iio->modes = INDIO_BUFFER_HARDWARE; > - > iio->num_channels = 1; > - iio->channels = &iio_sd_mod_ch; > > platform_set_drvdata(pdev, iio); > > - return devm_iio_device_register(&pdev->dev, iio); > + priv->vref = devm_regulator_get_optional(dev, "vref"); > + if (IS_ERR(priv->vref)) { > + ret = PTR_ERR(priv->vref); > + if (ret != -ENODEV) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "vref get failed, %d\n", ret); > + return ret; > + } > + } > + > + if (!IS_ERR(priv->vref)) { > + ret = regulator_enable(priv->vref); > + if (ret < 0) { > + dev_err(dev, "vref enable failed %d\n", ret); > + return ret; > + } > + > + ret = regulator_get_voltage(priv->vref); > + if (ret < 0) { > + dev_err(dev, "vref get failed, %d\n", ret); > + goto err_regulator_disable; > + } > + > + priv->vref_mv = ret / 1000; > + dev_dbg(dev, "vref+=%dmV\n", priv->vref_mv); > + } > + > + ret = devm_iio_device_register(&pdev->dev, iio); This exposes the userspace and in kernel interfaces. Those are partly dependent on the regulator enable you have above. Using devm_ version fo this interface leaves you with a race in remove. The regulator is disabled before you have remove the interfaces that will only work if we assume it is still on. Hence, you should either use devm_add_action_or_reset magic to ensure we still do everything in the right order, or do it manually by using iio_device_register and iio_device_unregister. > + if (ret < 0) { > + dev_err(dev, "Failed to register sd-modulator, %d\n", ret); > + goto err_regulator_disable; > + } > + > + return 0; > + > +err_regulator_disable: > + regulator_disable(priv->vref); > + > + return ret; > +} > + > +static int iio_sd_mod_remove(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct iio_sd_mod_priv *priv = iio_priv(indio_dev); > + > + if (priv->vref) > + return regulator_disable(priv->vref); > + > + return 0; > } > > static const struct of_device_id sd_adc_of_match[] = { > - { .compatible = "sd-modulator" }, > - { .compatible = "ads1201" }, > + { .compatible = "sd-modulator", .data = &iio_sd_mod_ch }, > + { .compatible = "ads1201", .data = &iio_sd_mod_ch_ads }, > { } > }; > MODULE_DEVICE_TABLE(of, sd_adc_of_match); > @@ -59,6 +150,7 @@ static struct platform_driver iio_sd_mod_adc = { > .of_match_table = of_match_ptr(sd_adc_of_match), > }, > .probe = iio_sd_mod_probe, > + .remove = iio_sd_mod_remove, > }; > > module_platform_driver(iio_sd_mod_adc);
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <jic23@kernel.org> To: Olivier Moysan <olivier.moysan@st.com> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, lars@metafoo.de, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, pmeerw@pmeerw.net, knaack.h@gmx.de, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/4] iio: adc: sd modulator: add scale support Date: Sat, 8 Feb 2020 16:03:27 +0000 [thread overview] Message-ID: <20200208160327.334da020@archlinux> (raw) In-Reply-To: <20200204101008.11411-3-olivier.moysan@st.com> On Tue, 4 Feb 2020 11:10:06 +0100 Olivier Moysan <olivier.moysan@st.com> wrote: > Add scale support to sigma delta modulator. > > Signed-off-by: Olivier Moysan <olivier.moysan@st.com> I note below that there are probably some complexities around whether vref is used as you have it here or not. A few other bits inline around a race condition introduced in probe / remove. Thanks, Jonathan > --- > drivers/iio/adc/sd_adc_modulator.c | 108 ++++++++++++++++++++++++++--- > 1 file changed, 100 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c > index 560d8c7d9d86..a83f35832050 100644 > --- a/drivers/iio/adc/sd_adc_modulator.c > +++ b/drivers/iio/adc/sd_adc_modulator.c > @@ -10,8 +10,7 @@ > #include <linux/iio/triggered_buffer.h> > #include <linux/module.h> > #include <linux/of_device.h> > - > -static const struct iio_info iio_sd_mod_iio_info; > +#include <linux/regulator/consumer.h> > > static const struct iio_chan_spec iio_sd_mod_ch = { > .type = IIO_VOLTAGE, > @@ -21,34 +20,126 @@ static const struct iio_chan_spec iio_sd_mod_ch = { > .realbits = 1, > .shift = 0, > }, > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), This relies on generic sigma delta modulators using an external vref. They might have an internal always on regulator... I would suggest we only support scale for devices with explicitly defined compatibles where we can know what regulators are used etc. For many devices, there will be a single powersupply called vdd-supply or similar in DT. It may also provide a reference voltage. > +}; > + > +static const struct iio_chan_spec iio_sd_mod_ch_ads = { > + .type = IIO_VOLTAGE, > + .indexed = 1, > + .scan_type = { > + .sign = 'u', > + .realbits = 1, > + .shift = 0, > + }, > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), > + .differential = 1, > +}; > + > +struct iio_sd_mod_priv { > + struct regulator *vref; > + int vref_mv; > +}; > + > +static int iio_sd_mod_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct iio_sd_mod_priv *priv = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + *val = priv->vref_mv; > + *val2 = chan->scan_type.realbits; > + return IIO_VAL_INT; > + } > + > + return -EINVAL; > +} > + > +static const struct iio_info iio_sd_mod_iio_info = { > + .read_raw = iio_sd_mod_read_raw, > }; > > static int iio_sd_mod_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct iio_sd_mod_priv *priv; > struct iio_dev *iio; > + int ret; > > - iio = devm_iio_device_alloc(dev, 0); > + iio = devm_iio_device_alloc(dev, sizeof(*priv)); > if (!iio) > return -ENOMEM; > > + iio->channels = (const struct iio_chan_spec *) > + of_device_get_match_data(&pdev->dev); > + > + priv = iio_priv(iio); > + > iio->dev.parent = dev; > iio->dev.of_node = dev->of_node; > iio->name = dev_name(dev); > iio->info = &iio_sd_mod_iio_info; > iio->modes = INDIO_BUFFER_HARDWARE; > - > iio->num_channels = 1; > - iio->channels = &iio_sd_mod_ch; > > platform_set_drvdata(pdev, iio); > > - return devm_iio_device_register(&pdev->dev, iio); > + priv->vref = devm_regulator_get_optional(dev, "vref"); > + if (IS_ERR(priv->vref)) { > + ret = PTR_ERR(priv->vref); > + if (ret != -ENODEV) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "vref get failed, %d\n", ret); > + return ret; > + } > + } > + > + if (!IS_ERR(priv->vref)) { > + ret = regulator_enable(priv->vref); > + if (ret < 0) { > + dev_err(dev, "vref enable failed %d\n", ret); > + return ret; > + } > + > + ret = regulator_get_voltage(priv->vref); > + if (ret < 0) { > + dev_err(dev, "vref get failed, %d\n", ret); > + goto err_regulator_disable; > + } > + > + priv->vref_mv = ret / 1000; > + dev_dbg(dev, "vref+=%dmV\n", priv->vref_mv); > + } > + > + ret = devm_iio_device_register(&pdev->dev, iio); This exposes the userspace and in kernel interfaces. Those are partly dependent on the regulator enable you have above. Using devm_ version fo this interface leaves you with a race in remove. The regulator is disabled before you have remove the interfaces that will only work if we assume it is still on. Hence, you should either use devm_add_action_or_reset magic to ensure we still do everything in the right order, or do it manually by using iio_device_register and iio_device_unregister. > + if (ret < 0) { > + dev_err(dev, "Failed to register sd-modulator, %d\n", ret); > + goto err_regulator_disable; > + } > + > + return 0; > + > +err_regulator_disable: > + regulator_disable(priv->vref); > + > + return ret; > +} > + > +static int iio_sd_mod_remove(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct iio_sd_mod_priv *priv = iio_priv(indio_dev); > + > + if (priv->vref) > + return regulator_disable(priv->vref); > + > + return 0; > } > > static const struct of_device_id sd_adc_of_match[] = { > - { .compatible = "sd-modulator" }, > - { .compatible = "ads1201" }, > + { .compatible = "sd-modulator", .data = &iio_sd_mod_ch }, > + { .compatible = "ads1201", .data = &iio_sd_mod_ch_ads }, > { } > }; > MODULE_DEVICE_TABLE(of, sd_adc_of_match); > @@ -59,6 +150,7 @@ static struct platform_driver iio_sd_mod_adc = { > .of_match_table = of_match_ptr(sd_adc_of_match), > }, > .probe = iio_sd_mod_probe, > + .remove = iio_sd_mod_remove, > }; > > module_platform_driver(iio_sd_mod_adc); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-02-08 16:03 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-04 10:10 [PATCH 0/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan 2020-02-04 10:10 ` Olivier Moysan 2020-02-04 10:10 ` [PATCH 1/4] dt-bindings: iio: adc: sd modulator: add vref support Olivier Moysan 2020-02-04 10:10 ` Olivier Moysan 2020-02-06 19:08 ` Rob Herring 2020-02-06 19:08 ` Rob Herring 2020-02-08 16:04 ` Jonathan Cameron 2020-02-08 16:04 ` Jonathan Cameron 2020-02-11 15:24 ` Olivier MOYSAN 2020-02-11 15:24 ` Olivier MOYSAN 2020-02-04 10:10 ` [PATCH 2/4] iio: adc: sd modulator: add scale support Olivier Moysan 2020-02-04 10:10 ` Olivier Moysan 2020-02-08 16:03 ` Jonathan Cameron [this message] 2020-02-08 16:03 ` Jonathan Cameron 2020-02-11 15:22 ` Olivier MOYSAN 2020-02-11 15:22 ` Olivier MOYSAN 2020-02-04 10:10 ` [PATCH 3/4] iio: adc: stm32-dfsdm: use resolution define Olivier Moysan 2020-02-04 10:10 ` Olivier Moysan 2020-02-04 10:10 ` [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support Olivier Moysan 2020-02-04 10:10 ` Olivier Moysan 2020-02-08 16:18 ` Jonathan Cameron 2020-02-08 16:18 ` Jonathan Cameron 2020-02-11 15:19 ` Olivier MOYSAN 2020-02-11 15:19 ` Olivier MOYSAN 2020-02-14 13:11 ` Jonathan Cameron 2020-02-14 13:11 ` Jonathan Cameron 2020-02-14 14:49 ` Olivier MOYSAN 2020-02-14 14:49 ` Olivier MOYSAN 2020-02-14 15:10 ` Jonathan Cameron 2020-02-14 15:10 ` Jonathan Cameron [not found] ` <AM9PR10MB43558CEB8DAE7F373E9E7A5DF9D69@AM9PR10MB4355.EURPRD10.PROD.OUTLOOK.COM> 2021-09-10 15:56 ` Olivier MOYSAN 2021-09-10 15:56 ` Olivier MOYSAN 2021-09-12 17:26 ` Jonathan Cameron 2021-09-12 17:26 ` Jonathan Cameron 2021-09-14 15:43 ` Olivier MOYSAN 2021-09-14 15:43 ` Olivier MOYSAN 2021-09-19 18:14 ` Jonathan Cameron 2021-09-19 18:14 ` Jonathan Cameron 2021-09-24 13:14 ` Olivier MOYSAN 2021-09-24 13:14 ` Olivier MOYSAN 2021-09-26 14:56 ` Jonathan Cameron 2021-09-26 14:56 ` Jonathan Cameron 2021-09-29 16:44 ` Olivier MOYSAN 2021-09-29 16:44 ` Olivier MOYSAN 2021-09-30 15:21 ` Jonathan Cameron 2021-09-30 15:21 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200208160327.334da020@archlinux \ --to=jic23@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=knaack.h@gmx.de \ --cc=lars@metafoo.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mark.rutland@arm.com \ --cc=olivier.moysan@st.com \ --cc=pmeerw@pmeerw.net \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.