| Message ID | ce92ae93b1c2e36b20a9881b145c8c2c85acb1dd.1702746240.git.marcelo.schmitt1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD7091R-2/-4/-8 | expand |
On Sat, 16 Dec 2023 14:49:07 -0300 Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > The ADC needs a voltage reference to work correctly. > Enable AD7091R internal voltage reference if no external vref is supplied. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> This one sounds to me like it should have a fixes tag and be much earlier in the set to perhaps simplify backports. Jonathan > --- > drivers/iio/adc/ad7091r-base.c | 7 +++++++ > drivers/iio/adc/ad7091r-base.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > index aead72ef55b6..9d0b489966f5 100644 > --- a/drivers/iio/adc/ad7091r-base.c > +++ b/drivers/iio/adc/ad7091r-base.c > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, > if (IS_ERR(st->vref)) { > if (PTR_ERR(st->vref) == -EPROBE_DEFER) > return -EPROBE_DEFER; > + > st->vref = NULL; > + /* Enable internal vref */ > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, > + AD7091R_REG_CONF_INT_VREF, BIT(0)); > + if (ret) > + return dev_err_probe(st->dev, ret, > + "Error on enable internal reference\n"); > } else { > ret = regulator_enable(st->vref); > if (ret) > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h > index 81b8a4bbb929..9cfb362a00a4 100644 > --- a/drivers/iio/adc/ad7091r-base.h > +++ b/drivers/iio/adc/ad7091r-base.h > @@ -20,6 +20,7 @@ > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) > > /* AD7091R_REG_CONF */ > +#define AD7091R_REG_CONF_INT_VREF BIT(0) > #define AD7091R_REG_CONF_ALERT_EN BIT(4) > #define AD7091R_REG_CONF_AUTO BIT(8) > #define AD7091R_REG_CONF_CMD BIT(10)
On Sat, Dec 16, 2023 at 11:49 AM Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > The ADC needs a voltage reference to work correctly. > Enable AD7091R internal voltage reference if no external vref is supplied. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > drivers/iio/adc/ad7091r-base.c | 7 +++++++ > drivers/iio/adc/ad7091r-base.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > index aead72ef55b6..9d0b489966f5 100644 > --- a/drivers/iio/adc/ad7091r-base.c > +++ b/drivers/iio/adc/ad7091r-base.c > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, > if (IS_ERR(st->vref)) { > if (PTR_ERR(st->vref) == -EPROBE_DEFER) > return -EPROBE_DEFER; > + > st->vref = NULL; > + /* Enable internal vref */ > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, > + AD7091R_REG_CONF_INT_VREF, BIT(0)); Can we use regmap_set_bits() here to avoid the BIT(0)? The same comment applies to other patches in this series. > + if (ret) > + return dev_err_probe(st->dev, ret, > + "Error on enable internal reference\n"); > } else { > ret = regulator_enable(st->vref); > if (ret) > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h > index 81b8a4bbb929..9cfb362a00a4 100644 > --- a/drivers/iio/adc/ad7091r-base.h > +++ b/drivers/iio/adc/ad7091r-base.h > @@ -20,6 +20,7 @@ > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) > > /* AD7091R_REG_CONF */ > +#define AD7091R_REG_CONF_INT_VREF BIT(0) > #define AD7091R_REG_CONF_ALERT_EN BIT(4) > #define AD7091R_REG_CONF_AUTO BIT(8) > #define AD7091R_REG_CONF_CMD BIT(10) > -- > 2.42.0 >
On 12/17, Jonathan Cameron wrote: > On Sat, 16 Dec 2023 14:49:07 -0300 > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > The ADC needs a voltage reference to work correctly. > > Enable AD7091R internal voltage reference if no external vref is supplied. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > This one sounds to me like it should have a fixes tag and be > much earlier in the set to perhaps simplify backports. Could be. If we stick to the fact that the dt-binding does not require a voltage regulator then this can be seen as a fix. Though, if users can provide an external reference this patch makes no difference them. I am using the internal reference for testing so having this one makes a difference for me. > > Jonathan > > > --- > > drivers/iio/adc/ad7091r-base.c | 7 +++++++ > > drivers/iio/adc/ad7091r-base.h | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > > index aead72ef55b6..9d0b489966f5 100644 > > --- a/drivers/iio/adc/ad7091r-base.c > > +++ b/drivers/iio/adc/ad7091r-base.c > > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, > > if (IS_ERR(st->vref)) { > > if (PTR_ERR(st->vref) == -EPROBE_DEFER) > > return -EPROBE_DEFER; > > + > > st->vref = NULL; > > + /* Enable internal vref */ > > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, > > + AD7091R_REG_CONF_INT_VREF, BIT(0)); > > + if (ret) > > + return dev_err_probe(st->dev, ret, > > + "Error on enable internal reference\n"); > > } else { > > ret = regulator_enable(st->vref); > > if (ret) > > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h > > index 81b8a4bbb929..9cfb362a00a4 100644 > > --- a/drivers/iio/adc/ad7091r-base.h > > +++ b/drivers/iio/adc/ad7091r-base.h > > @@ -20,6 +20,7 @@ > > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) > > > > /* AD7091R_REG_CONF */ > > +#define AD7091R_REG_CONF_INT_VREF BIT(0) > > #define AD7091R_REG_CONF_ALERT_EN BIT(4) > > #define AD7091R_REG_CONF_AUTO BIT(8) > > #define AD7091R_REG_CONF_CMD BIT(10) > >
On 12/17, David Lechner wrote: > On Sat, Dec 16, 2023 at 11:49 AM Marcelo Schmitt > <marcelo.schmitt@analog.com> wrote: > > > > The ADC needs a voltage reference to work correctly. > > Enable AD7091R internal voltage reference if no external vref is supplied. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > drivers/iio/adc/ad7091r-base.c | 7 +++++++ > > drivers/iio/adc/ad7091r-base.h | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > > index aead72ef55b6..9d0b489966f5 100644 > > --- a/drivers/iio/adc/ad7091r-base.c > > +++ b/drivers/iio/adc/ad7091r-base.c > > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, > > if (IS_ERR(st->vref)) { > > if (PTR_ERR(st->vref) == -EPROBE_DEFER) > > return -EPROBE_DEFER; > > + > > st->vref = NULL; > > + /* Enable internal vref */ > > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, > > + AD7091R_REG_CONF_INT_VREF, BIT(0)); > > Can we use regmap_set_bits() here to avoid the BIT(0)? > > The same comment applies to other patches in this series. Looks good, will do. Thanks > > > + if (ret) > > + return dev_err_probe(st->dev, ret, > > + "Error on enable internal reference\n"); > > } else { > > ret = regulator_enable(st->vref); > > if (ret) > > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h > > index 81b8a4bbb929..9cfb362a00a4 100644 > > --- a/drivers/iio/adc/ad7091r-base.h > > +++ b/drivers/iio/adc/ad7091r-base.h > > @@ -20,6 +20,7 @@ > > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) > > > > /* AD7091R_REG_CONF */ > > +#define AD7091R_REG_CONF_INT_VREF BIT(0) > > #define AD7091R_REG_CONF_ALERT_EN BIT(4) > > #define AD7091R_REG_CONF_AUTO BIT(8) > > #define AD7091R_REG_CONF_CMD BIT(10) > > -- > > 2.42.0 > >
On Mon, 18 Dec 2023 14:35:27 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 12/17, Jonathan Cameron wrote: > > On Sat, 16 Dec 2023 14:49:07 -0300 > > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > > > > The ADC needs a voltage reference to work correctly. > > > Enable AD7091R internal voltage reference if no external vref is supplied. > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > This one sounds to me like it should have a fixes tag and be > > much earlier in the set to perhaps simplify backports. > > Could be. If we stick to the fact that the dt-binding does not require a voltage > regulator then this can be seen as a fix. > Though, if users can provide an external reference this patch makes no > difference them. > I am using the internal reference for testing so having this one makes a > difference for me. The binding has it as optional, though usually when not having an external reference leads to use of an internal one, we call it out in the description. Meh, can backport it as a fix if anyone asks for it. Jonathan > > > > > Jonathan > > > > > --- > > > drivers/iio/adc/ad7091r-base.c | 7 +++++++ > > > drivers/iio/adc/ad7091r-base.h | 1 + > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > > > index aead72ef55b6..9d0b489966f5 100644 > > > --- a/drivers/iio/adc/ad7091r-base.c > > > +++ b/drivers/iio/adc/ad7091r-base.c > > > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, > > > if (IS_ERR(st->vref)) { > > > if (PTR_ERR(st->vref) == -EPROBE_DEFER) > > > return -EPROBE_DEFER; > > > + > > > st->vref = NULL; > > > + /* Enable internal vref */ > > > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, > > > + AD7091R_REG_CONF_INT_VREF, BIT(0)); > > > + if (ret) > > > + return dev_err_probe(st->dev, ret, > > > + "Error on enable internal reference\n"); > > > } else { > > > ret = regulator_enable(st->vref); > > > if (ret) > > > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h > > > index 81b8a4bbb929..9cfb362a00a4 100644 > > > --- a/drivers/iio/adc/ad7091r-base.h > > > +++ b/drivers/iio/adc/ad7091r-base.h > > > @@ -20,6 +20,7 @@ > > > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) > > > > > > /* AD7091R_REG_CONF */ > > > +#define AD7091R_REG_CONF_INT_VREF BIT(0) > > > #define AD7091R_REG_CONF_ALERT_EN BIT(4) > > > #define AD7091R_REG_CONF_AUTO BIT(8) > > > #define AD7091R_REG_CONF_CMD BIT(10) > > > >
diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c index aead72ef55b6..9d0b489966f5 100644 --- a/drivers/iio/adc/ad7091r-base.c +++ b/drivers/iio/adc/ad7091r-base.c @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, if (IS_ERR(st->vref)) { if (PTR_ERR(st->vref) == -EPROBE_DEFER) return -EPROBE_DEFER; + st->vref = NULL; + /* Enable internal vref */ + ret = regmap_update_bits(st->map, AD7091R_REG_CONF, + AD7091R_REG_CONF_INT_VREF, BIT(0)); + if (ret) + return dev_err_probe(st->dev, ret, + "Error on enable internal reference\n"); } else { ret = regulator_enable(st->vref); if (ret) diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h index 81b8a4bbb929..9cfb362a00a4 100644 --- a/drivers/iio/adc/ad7091r-base.h +++ b/drivers/iio/adc/ad7091r-base.h @@ -20,6 +20,7 @@ #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) /* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0) #define AD7091R_REG_CONF_ALERT_EN BIT(4) #define AD7091R_REG_CONF_AUTO BIT(8) #define AD7091R_REG_CONF_CMD BIT(10)
The ADC needs a voltage reference to work correctly. Enable AD7091R internal voltage reference if no external vref is supplied. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)