* [PATCH v3 1/3] dt-bindings: iio: adc: ti,tsc2046: add vref-supply property @ 2022-09-01 4:11 Oleksij Rempel 2022-09-01 4:11 ` [PATCH v3 2/3] iio: adc: tsc2046: add vref support Oleksij Rempel 2022-09-01 4:11 ` [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning Oleksij Rempel 0 siblings, 2 replies; 8+ messages in thread From: Oleksij Rempel @ 2022-09-01 4:11 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski Cc: Oleksij Rempel, Krzysztof Kozlowski, kernel, linux-kernel, linux-iio, devicetree, Andy Shevchenko Add property for the voltage reference supply. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- changes v2 - add Acked-by: Krzysztof Kozlowski --- Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml b/Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml index 601d69971d84a..7faf12b1598b9 100644 --- a/Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml +++ b/Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml @@ -25,6 +25,9 @@ properties: spi-max-frequency: true + vref-supply: + description: Optional supply of the reference voltage + "#io-channel-cells": const: 1 -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/3] iio: adc: tsc2046: add vref support 2022-09-01 4:11 [PATCH v3 1/3] dt-bindings: iio: adc: ti,tsc2046: add vref-supply property Oleksij Rempel @ 2022-09-01 4:11 ` Oleksij Rempel 2022-09-01 8:24 ` Marco Felsch 2022-09-01 11:45 ` Jonathan Cameron 2022-09-01 4:11 ` [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning Oleksij Rempel 1 sibling, 2 replies; 8+ messages in thread From: Oleksij Rempel @ 2022-09-01 4:11 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski Cc: Oleksij Rempel, kernel, linux-kernel, linux-iio, devicetree, Andy Shevchenko If VREF pin is attached, we should use external VREF source instead of the internal. Otherwise we will get wrong measurements on some of channel types. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/iio/adc/ti-tsc2046.c | 64 +++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c index 0d9436a69cbfb..bbc8b4137b0b1 100644 --- a/drivers/iio/adc/ti-tsc2046.c +++ b/drivers/iio/adc/ti-tsc2046.c @@ -8,6 +8,7 @@ #include <linux/bitfield.h> #include <linux/delay.h> #include <linux/module.h> +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h> #include <asm/unaligned.h> @@ -175,6 +176,9 @@ struct tsc2046_adc_priv { u32 time_per_bit_ns; struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN]; + bool use_internal_vref; + unsigned int vref_mv; + struct regulator *vref_reg; }; #define TI_TSC2046_V_CHAN(index, bits, name) \ @@ -252,7 +256,9 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, case TI_TSC2046_ADDR_AUX: case TI_TSC2046_ADDR_VBAT: case TI_TSC2046_ADDR_TEMP0: - pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON; + pd |= TI_TSC2046_SER; + if (priv->use_internal_vref) + pd |= TI_TSC2046_PD1_VREF_ON; } return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; @@ -468,7 +474,7 @@ static int tsc2046_adc_read_raw(struct iio_dev *indio_dev, * So, it is better to use external voltage-divider driver * instead, which is calculating complete chain. */ - *val = TI_TSC2046_INT_VREF; + *val = priv->vref_mv; *val2 = chan->scan_type.realbits; return IIO_VAL_FRACTIONAL_LOG2; } @@ -781,22 +787,42 @@ static int tsc2046_adc_probe(struct spi_device *spi) indio_dev->num_channels = dcfg->num_channels; indio_dev->info = &tsc2046_adc_info; + priv->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); + if (!IS_ERR(priv->vref_reg)) { + ret = regulator_enable(priv->vref_reg); + if (ret) + return ret; + + ret = regulator_get_voltage(priv->vref_reg); + if (ret < 0) + goto err_regulator_disable; + + priv->vref_mv = ret / 1000; + priv->use_internal_vref = false; + } else { + /* Use internal reference */ + priv->vref_mv = TI_TSC2046_INT_VREF; + priv->use_internal_vref = true; + } + tsc2046_adc_parse_fwnode(priv); ret = tsc2046_adc_setup_spi_msg(priv); if (ret) - return ret; + goto err_regulator_disable; mutex_init(&priv->slock); ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, IRQF_NO_AUTOEN, indio_dev->name, indio_dev); if (ret) - return ret; + goto err_regulator_disable; trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); - if (!trig) - return -ENOMEM; + if (!trig) { + ret = -ENOMEM; + goto err_regulator_disable; + } priv->trig = trig; iio_trigger_set_drvdata(trig, indio_dev); @@ -811,20 +837,39 @@ static int tsc2046_adc_probe(struct spi_device *spi) ret = devm_iio_trigger_register(dev, trig); if (ret) { dev_err(dev, "failed to register trigger\n"); - return ret; + goto err_regulator_disable; } ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, &tsc2046_adc_trigger_handler, NULL); if (ret) { dev_err(dev, "Failed to setup triggered buffer\n"); - return ret; + goto err_regulator_disable; } /* set default trigger */ indio_dev->trig = iio_trigger_get(priv->trig); - return devm_iio_device_register(dev, indio_dev); + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + goto err_regulator_disable; + + return 0; + +err_regulator_disable: + if (!IS_ERR(priv->vref_reg)) + regulator_disable(priv->vref_reg); + + return ret; +} + +static void tsc2046_adc_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct tsc2046_adc_priv *priv = iio_priv(indio_dev); + + if (!IS_ERR(priv->vref_reg)) + regulator_disable(priv->vref_reg); } static const struct of_device_id ads7950_of_table[] = { @@ -839,6 +884,7 @@ static struct spi_driver tsc2046_adc_driver = { .of_match_table = ads7950_of_table, }, .probe = tsc2046_adc_probe, + .remove = tsc2046_adc_remove, }; module_spi_driver(tsc2046_adc_driver); -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: tsc2046: add vref support 2022-09-01 4:11 ` [PATCH v3 2/3] iio: adc: tsc2046: add vref support Oleksij Rempel @ 2022-09-01 8:24 ` Marco Felsch 2022-09-01 11:35 ` Oleksij Rempel 2022-09-01 11:45 ` Jonathan Cameron 1 sibling, 1 reply; 8+ messages in thread From: Marco Felsch @ 2022-09-01 8:24 UTC (permalink / raw) To: Oleksij Rempel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, devicetree, linux-iio, linux-kernel, Andy Shevchenko, kernel Hi Oleksij, sorry for jumping in, please see below. On 22-09-01, Oleksij Rempel wrote: > If VREF pin is attached, we should use external VREF source instead of > the internal. Otherwise we will get wrong measurements on some of channel > types. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/iio/adc/ti-tsc2046.c | 64 +++++++++++++++++++++++++++++++----- > 1 file changed, 55 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > index 0d9436a69cbfb..bbc8b4137b0b1 100644 > --- a/drivers/iio/adc/ti-tsc2046.c > +++ b/drivers/iio/adc/ti-tsc2046.c > @@ -8,6 +8,7 @@ > #include <linux/bitfield.h> > #include <linux/delay.h> > #include <linux/module.h> > +#include <linux/regulator/consumer.h> > #include <linux/spi/spi.h> > > #include <asm/unaligned.h> > @@ -175,6 +176,9 @@ struct tsc2046_adc_priv { > u32 time_per_bit_ns; > > struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN]; > + bool use_internal_vref; > + unsigned int vref_mv; We wouldn't need those members if we just enable/disable the regulator on demand. This would also be more power efficient. > + struct regulator *vref_reg; > }; > > #define TI_TSC2046_V_CHAN(index, bits, name) \ > @@ -252,7 +256,9 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, > case TI_TSC2046_ADDR_AUX: > case TI_TSC2046_ADDR_VBAT: > case TI_TSC2046_ADDR_TEMP0: > - pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON; > + pd |= TI_TSC2046_SER; > + if (priv->use_internal_vref) > + pd |= TI_TSC2046_PD1_VREF_ON; Then this line would become: + if (!priv->vref_reg) + pd |= TI_TSC2046_PD1_VREF_ON; > } > > return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; > @@ -468,7 +474,7 @@ static int tsc2046_adc_read_raw(struct iio_dev *indio_dev, This function needs to enable the vref_reg before the switch-case and disable it afterwards. > * So, it is better to use external voltage-divider driver > * instead, which is calculating complete chain. > */ > - *val = TI_TSC2046_INT_VREF; > + *val = priv->vref_mv; > *val2 = chan->scan_type.realbits; > return IIO_VAL_FRACTIONAL_LOG2; > } > @@ -781,22 +787,42 @@ static int tsc2046_adc_probe(struct spi_device *spi) > indio_dev->num_channels = dcfg->num_channels; > indio_dev->info = &tsc2046_adc_info; > > + priv->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); This line would be enough and we could drop > + if (!IS_ERR(priv->vref_reg)) { > + ret = regulator_enable(priv->vref_reg); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(priv->vref_reg); > + if (ret < 0) > + goto err_regulator_disable; > + > + priv->vref_mv = ret / 1000; > + priv->use_internal_vref = false; > + } else { > + /* Use internal reference */ > + priv->vref_mv = TI_TSC2046_INT_VREF; > + priv->use_internal_vref = true; > + } > + this part. > tsc2046_adc_parse_fwnode(priv); > > ret = tsc2046_adc_setup_spi_msg(priv); > if (ret) > - return ret; > + goto err_regulator_disable; > > mutex_init(&priv->slock); > > ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, > IRQF_NO_AUTOEN, indio_dev->name, indio_dev); > if (ret) > - return ret; > + goto err_regulator_disable; > > trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); > - if (!trig) > - return -ENOMEM; > + if (!trig) { > + ret = -ENOMEM; > + goto err_regulator_disable; > + } > > priv->trig = trig; > iio_trigger_set_drvdata(trig, indio_dev); > @@ -811,20 +837,39 @@ static int tsc2046_adc_probe(struct spi_device *spi) > ret = devm_iio_trigger_register(dev, trig); > if (ret) { > dev_err(dev, "failed to register trigger\n"); > - return ret; > + goto err_regulator_disable; > } > > ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > &tsc2046_adc_trigger_handler, NULL); > if (ret) { > dev_err(dev, "Failed to setup triggered buffer\n"); > - return ret; > + goto err_regulator_disable; > } > > /* set default trigger */ > indio_dev->trig = iio_trigger_get(priv->trig); > > - return devm_iio_device_register(dev, indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + goto err_regulator_disable; > + > + return 0; > + > +err_regulator_disable: > + if (!IS_ERR(priv->vref_reg)) > + regulator_disable(priv->vref_reg); > + > + return ret; As well as the whole new error handling and > +} > + > +static void tsc2046_adc_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct tsc2046_adc_priv *priv = iio_priv(indio_dev); > + > + if (!IS_ERR(priv->vref_reg)) > + regulator_disable(priv->vref_reg); > } the remove callback. Regards, Marco > static const struct of_device_id ads7950_of_table[] = { > @@ -839,6 +884,7 @@ static struct spi_driver tsc2046_adc_driver = { > .of_match_table = ads7950_of_table, > }, > .probe = tsc2046_adc_probe, > + .remove = tsc2046_adc_remove, > }; > module_spi_driver(tsc2046_adc_driver); > > -- > 2.30.2 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: tsc2046: add vref support 2022-09-01 8:24 ` Marco Felsch @ 2022-09-01 11:35 ` Oleksij Rempel 0 siblings, 0 replies; 8+ messages in thread From: Oleksij Rempel @ 2022-09-01 11:35 UTC (permalink / raw) To: Marco Felsch Cc: devicetree, Lars-Peter Clausen, kernel, linux-iio, linux-kernel, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Jonathan Cameron Hi Marco, On Thu, Sep 01, 2022 at 10:24:25AM +0200, Marco Felsch wrote: > Hi Oleksij, > > sorry for jumping in, please see below. > > On 22-09-01, Oleksij Rempel wrote: > > If VREF pin is attached, we should use external VREF source instead of > > the internal. Otherwise we will get wrong measurements on some of channel > > types. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/iio/adc/ti-tsc2046.c | 64 +++++++++++++++++++++++++++++++----- > > 1 file changed, 55 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > > index 0d9436a69cbfb..bbc8b4137b0b1 100644 > > --- a/drivers/iio/adc/ti-tsc2046.c > > +++ b/drivers/iio/adc/ti-tsc2046.c > > @@ -8,6 +8,7 @@ > > #include <linux/bitfield.h> > > #include <linux/delay.h> > > #include <linux/module.h> > > +#include <linux/regulator/consumer.h> > > #include <linux/spi/spi.h> > > > > #include <asm/unaligned.h> > > @@ -175,6 +176,9 @@ struct tsc2046_adc_priv { > > u32 time_per_bit_ns; > > > > struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN]; > > + bool use_internal_vref; > > + unsigned int vref_mv; > > We wouldn't need those members if we just enable/disable the regulator > on demand. This would also be more power efficient. Not really. - it is reference voltage. There is no load. - usually no one care to add extra switch to disable reference voltage on a controller which should be enabled all the time any way. - we add more CPU cycles on every access to enable/disable things, which are usually never have dedicated regulator. > > + struct regulator *vref_reg; > > }; > > > > #define TI_TSC2046_V_CHAN(index, bits, name) \ > > @@ -252,7 +256,9 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, > > case TI_TSC2046_ADDR_AUX: > > case TI_TSC2046_ADDR_VBAT: > > case TI_TSC2046_ADDR_TEMP0: > > - pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON; > > + pd |= TI_TSC2046_SER; > > + if (priv->use_internal_vref) > > + pd |= TI_TSC2046_PD1_VREF_ON; > > Then this line would become: ack. I agree here. > + if (!priv->vref_reg) > + pd |= TI_TSC2046_PD1_VREF_ON; > > > > } > > > > return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; > > @@ -468,7 +474,7 @@ static int tsc2046_adc_read_raw(struct iio_dev *indio_dev, > > This function needs to enable the vref_reg before the switch-case and > disable it afterwards. it will not enable power supply of this controller. There is no need to do this. > > * So, it is better to use external voltage-divider driver > > * instead, which is calculating complete chain. > > */ > > - *val = TI_TSC2046_INT_VREF; > > + *val = priv->vref_mv; > > *val2 = chan->scan_type.realbits; > > return IIO_VAL_FRACTIONAL_LOG2; > > } > > @@ -781,22 +787,42 @@ static int tsc2046_adc_probe(struct spi_device *spi) > > indio_dev->num_channels = dcfg->num_channels; > > indio_dev->info = &tsc2046_adc_info; > > > > + priv->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); > > This line would be enough and we could drop > > > + if (!IS_ERR(priv->vref_reg)) { > > + ret = regulator_enable(priv->vref_reg); > > + if (ret) > > + return ret; > > + > > + ret = regulator_get_voltage(priv->vref_reg); > > + if (ret < 0) > > + goto err_regulator_disable; > > + > > + priv->vref_mv = ret / 1000; > > + priv->use_internal_vref = false; > > + } else { > > + /* Use internal reference */ > > + priv->vref_mv = TI_TSC2046_INT_VREF; > > + priv->use_internal_vref = true; > > + } > > + > > this part. In this case we will not get any power saving, but instead we will need to run enable/disable + regulator_get_voltage + extra calculation without any advantage. > > > tsc2046_adc_parse_fwnode(priv); > > > > ret = tsc2046_adc_setup_spi_msg(priv); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > mutex_init(&priv->slock); > > > > ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, > > IRQF_NO_AUTOEN, indio_dev->name, indio_dev); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); > > - if (!trig) > > - return -ENOMEM; > > + if (!trig) { > > + ret = -ENOMEM; > > + goto err_regulator_disable; > > + } > > > > priv->trig = trig; > > iio_trigger_set_drvdata(trig, indio_dev); > > @@ -811,20 +837,39 @@ static int tsc2046_adc_probe(struct spi_device *spi) > > ret = devm_iio_trigger_register(dev, trig); > > if (ret) { > > dev_err(dev, "failed to register trigger\n"); > > - return ret; > > + goto err_regulator_disable; > > } > > > > ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > > &tsc2046_adc_trigger_handler, NULL); > > if (ret) { > > dev_err(dev, "Failed to setup triggered buffer\n"); > > - return ret; > > + goto err_regulator_disable; > > } > > > > /* set default trigger */ > > indio_dev->trig = iio_trigger_get(priv->trig); > > > > - return devm_iio_device_register(dev, indio_dev); > > + ret = devm_iio_device_register(dev, indio_dev); > > + if (ret) > > + goto err_regulator_disable; > > + > > + return 0; > > + > > +err_regulator_disable: > > + if (!IS_ERR(priv->vref_reg)) > > + regulator_disable(priv->vref_reg); > > + > > + return ret; > > As well as the whole new error handling and > > > +} > > + > > +static void tsc2046_adc_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct tsc2046_adc_priv *priv = iio_priv(indio_dev); > > + > > + if (!IS_ERR(priv->vref_reg)) > > + regulator_disable(priv->vref_reg); > > } > > the remove callback. no. -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: tsc2046: add vref support 2022-09-01 4:11 ` [PATCH v3 2/3] iio: adc: tsc2046: add vref support Oleksij Rempel 2022-09-01 8:24 ` Marco Felsch @ 2022-09-01 11:45 ` Jonathan Cameron 2022-09-01 11:53 ` Oleksij Rempel 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2022-09-01 11:45 UTC (permalink / raw) To: Oleksij Rempel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, kernel, linux-kernel, linux-iio, devicetree, Andy Shevchenko On Thu, 1 Sep 2022 06:11:45 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > If VREF pin is attached, we should use external VREF source instead of > the internal. Otherwise we will get wrong measurements on some of channel > types. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Hi Oleksij, I'm not sure why I didn't review this patch in v1... Anyhow, some comments below. Jonathan > --- > drivers/iio/adc/ti-tsc2046.c | 64 +++++++++++++++++++++++++++++++----- > 1 file changed, 55 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > index 0d9436a69cbfb..bbc8b4137b0b1 100644 > --- a/drivers/iio/adc/ti-tsc2046.c > +++ b/drivers/iio/adc/ti-tsc2046.c > @@ -8,6 +8,7 @@ > #include <linux/bitfield.h> > #include <linux/delay.h> > #include <linux/module.h> > +#include <linux/regulator/consumer.h> > #include <linux/spi/spi.h> > > #include <asm/unaligned.h> > @@ -175,6 +176,9 @@ struct tsc2046_adc_priv { > u32 time_per_bit_ns; > > struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN]; > + bool use_internal_vref; > + unsigned int vref_mv; > + struct regulator *vref_reg; > }; > > #define TI_TSC2046_V_CHAN(index, bits, name) \ > @@ -252,7 +256,9 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, > case TI_TSC2046_ADDR_AUX: > case TI_TSC2046_ADDR_VBAT: > case TI_TSC2046_ADDR_TEMP0: > - pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON; > + pd |= TI_TSC2046_SER; > + if (priv->use_internal_vref) > + pd |= TI_TSC2046_PD1_VREF_ON; > } > > return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; > @@ -468,7 +474,7 @@ static int tsc2046_adc_read_raw(struct iio_dev *indio_dev, > * So, it is better to use external voltage-divider driver > * instead, which is calculating complete chain. > */ > - *val = TI_TSC2046_INT_VREF; > + *val = priv->vref_mv; > *val2 = chan->scan_type.realbits; > return IIO_VAL_FRACTIONAL_LOG2; > } > @@ -781,22 +787,42 @@ static int tsc2046_adc_probe(struct spi_device *spi) > indio_dev->num_channels = dcfg->num_channels; > indio_dev->info = &tsc2046_adc_info; > > + priv->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); > + if (!IS_ERR(priv->vref_reg)) { > + ret = regulator_enable(priv->vref_reg); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(priv->vref_reg); > + if (ret < 0) > + goto err_regulator_disable; Whilst regulators voltages of references rarely change at runtime they are allowed to, so it is logically better to query the voltage at the point of use. Requests for scale should be rare (unless there is a consumer that keeps querying this?) so the slightly overhead there shouldn't matter. > + > + priv->vref_mv = ret / 1000; > + priv->use_internal_vref = false; > + } else { > + /* Use internal reference */ > + priv->vref_mv = TI_TSC2046_INT_VREF; > + priv->use_internal_vref = true; > + } > + > tsc2046_adc_parse_fwnode(priv); > > ret = tsc2046_adc_setup_spi_msg(priv); > if (ret) > - return ret; > + goto err_regulator_disable; > > mutex_init(&priv->slock); > > ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, > IRQF_NO_AUTOEN, indio_dev->name, indio_dev); > if (ret) > - return ret; > + goto err_regulator_disable; > > trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); > - if (!trig) > - return -ENOMEM; > + if (!trig) { > + ret = -ENOMEM; > + goto err_regulator_disable; > + } > > priv->trig = trig; > iio_trigger_set_drvdata(trig, indio_dev); > @@ -811,20 +837,39 @@ static int tsc2046_adc_probe(struct spi_device *spi) > ret = devm_iio_trigger_register(dev, trig); > if (ret) { > dev_err(dev, "failed to register trigger\n"); > - return ret; > + goto err_regulator_disable; Please don't mix devm and non devm calls. It makes it much harder to reason about the correctness of ordering. Use devm_add_action_or_reset() to register a callback to disable the vref regulator. Alternative is back out the devm_ based registration of everything after the regulator enable. > } > > ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > &tsc2046_adc_trigger_handler, NULL); > if (ret) { > dev_err(dev, "Failed to setup triggered buffer\n"); > - return ret; > + goto err_regulator_disable; > } > > /* set default trigger */ > indio_dev->trig = iio_trigger_get(priv->trig); > > - return devm_iio_device_register(dev, indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + goto err_regulator_disable; > + > + return 0; > + > +err_regulator_disable: > + if (!IS_ERR(priv->vref_reg)) > + regulator_disable(priv->vref_reg); > + > + return ret; > +} > + > +static void tsc2046_adc_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct tsc2046_adc_priv *priv = iio_priv(indio_dev); > + > + if (!IS_ERR(priv->vref_reg)) > + regulator_disable(priv->vref_reg); > } > > static const struct of_device_id ads7950_of_table[] = { > @@ -839,6 +884,7 @@ static struct spi_driver tsc2046_adc_driver = { > .of_match_table = ads7950_of_table, > }, > .probe = tsc2046_adc_probe, > + .remove = tsc2046_adc_remove, > }; > module_spi_driver(tsc2046_adc_driver); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] iio: adc: tsc2046: add vref support 2022-09-01 11:45 ` Jonathan Cameron @ 2022-09-01 11:53 ` Oleksij Rempel 0 siblings, 0 replies; 8+ messages in thread From: Oleksij Rempel @ 2022-09-01 11:53 UTC (permalink / raw) To: Jonathan Cameron Cc: devicetree, Lars-Peter Clausen, Krzysztof Kozlowski, linux-iio, linux-kernel, Andy Shevchenko, Rob Herring, kernel, Jonathan Cameron On Thu, Sep 01, 2022 at 12:45:49PM +0100, Jonathan Cameron wrote: > On Thu, 1 Sep 2022 06:11:45 +0200 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > If VREF pin is attached, we should use external VREF source instead of > > the internal. Otherwise we will get wrong measurements on some of channel > > types. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > Hi Oleksij, > > I'm not sure why I didn't review this patch in v1... > > Anyhow, some comments below. > > Jonathan > > > --- > > drivers/iio/adc/ti-tsc2046.c | 64 +++++++++++++++++++++++++++++++----- > > 1 file changed, 55 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > > index 0d9436a69cbfb..bbc8b4137b0b1 100644 > > --- a/drivers/iio/adc/ti-tsc2046.c > > +++ b/drivers/iio/adc/ti-tsc2046.c > > @@ -8,6 +8,7 @@ > > #include <linux/bitfield.h> > > #include <linux/delay.h> > > #include <linux/module.h> > > +#include <linux/regulator/consumer.h> > > #include <linux/spi/spi.h> > > > > #include <asm/unaligned.h> > > @@ -175,6 +176,9 @@ struct tsc2046_adc_priv { > > u32 time_per_bit_ns; > > > > struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN]; > > + bool use_internal_vref; > > + unsigned int vref_mv; > > + struct regulator *vref_reg; > > }; > > > > #define TI_TSC2046_V_CHAN(index, bits, name) \ > > @@ -252,7 +256,9 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, > > case TI_TSC2046_ADDR_AUX: > > case TI_TSC2046_ADDR_VBAT: > > case TI_TSC2046_ADDR_TEMP0: > > - pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON; > > + pd |= TI_TSC2046_SER; > > + if (priv->use_internal_vref) > > + pd |= TI_TSC2046_PD1_VREF_ON; > > } > > > > return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; > > @@ -468,7 +474,7 @@ static int tsc2046_adc_read_raw(struct iio_dev *indio_dev, > > * So, it is better to use external voltage-divider driver > > * instead, which is calculating complete chain. > > */ > > - *val = TI_TSC2046_INT_VREF; > > + *val = priv->vref_mv; > > *val2 = chan->scan_type.realbits; > > return IIO_VAL_FRACTIONAL_LOG2; > > } > > @@ -781,22 +787,42 @@ static int tsc2046_adc_probe(struct spi_device *spi) > > indio_dev->num_channels = dcfg->num_channels; > > indio_dev->info = &tsc2046_adc_info; > > > > + priv->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); > > + if (!IS_ERR(priv->vref_reg)) { > > + ret = regulator_enable(priv->vref_reg); > > + if (ret) > > + return ret; > > + > > + ret = regulator_get_voltage(priv->vref_reg); > > + if (ret < 0) > > + goto err_regulator_disable; > > Whilst regulators voltages of references rarely change at runtime > they are allowed to, so it is logically better to query the > voltage at the point of use. Requests for scale should be > rare (unless there is a consumer that keeps querying this?) > so the slightly overhead there > shouldn't matter. The voltage scale is related to actual adc measurements. If it variates, we will have different scale of challenges. > > + > > + priv->vref_mv = ret / 1000; > > + priv->use_internal_vref = false; > > + } else { > > + /* Use internal reference */ > > + priv->vref_mv = TI_TSC2046_INT_VREF; > > + priv->use_internal_vref = true; > > + } > > + > > tsc2046_adc_parse_fwnode(priv); > > > > ret = tsc2046_adc_setup_spi_msg(priv); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > mutex_init(&priv->slock); > > > > ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, > > IRQF_NO_AUTOEN, indio_dev->name, indio_dev); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); > > - if (!trig) > > - return -ENOMEM; > > + if (!trig) { > > + ret = -ENOMEM; > > + goto err_regulator_disable; > > + } > > > > priv->trig = trig; > > iio_trigger_set_drvdata(trig, indio_dev); > > @@ -811,20 +837,39 @@ static int tsc2046_adc_probe(struct spi_device *spi) > > ret = devm_iio_trigger_register(dev, trig); > > if (ret) { > > dev_err(dev, "failed to register trigger\n"); > > - return ret; > > + goto err_regulator_disable; > > Please don't mix devm and non devm calls. It makes it much harder to reason about > the correctness of ordering. > Use devm_add_action_or_reset() to register a callback to disable the > vref regulator. > > Alternative is back out the devm_ based registration of everything after > the regulator enable. ok. Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning 2022-09-01 4:11 [PATCH v3 1/3] dt-bindings: iio: adc: ti,tsc2046: add vref-supply property Oleksij Rempel 2022-09-01 4:11 ` [PATCH v3 2/3] iio: adc: tsc2046: add vref support Oleksij Rempel @ 2022-09-01 4:11 ` Oleksij Rempel 2022-09-01 14:20 ` Andy Shevchenko 1 sibling, 1 reply; 8+ messages in thread From: Oleksij Rempel @ 2022-09-01 4:11 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski Cc: Oleksij Rempel, kernel, linux-kernel, linux-iio, devicetree, Andy Shevchenko Add spi_device_id to silent following kernel runtime warning: "SPI driver tsc2046 has no spi_device_id for ti,tsc2046e-adc". Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- changes v3: - add missing point - remove unneeded blank line - assignment id at the definition line changes v2: - attach actual driver_data - use spi_get_device_id fallback --- drivers/iio/adc/ti-tsc2046.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c index bbc8b4137b0b1..d5d799972fefd 100644 --- a/drivers/iio/adc/ti-tsc2046.c +++ b/drivers/iio/adc/ti-tsc2046.c @@ -762,6 +762,11 @@ static int tsc2046_adc_probe(struct spi_device *spi) } dcfg = device_get_match_data(dev); + if (!dcfg) { + const struct spi_device_id *id = spi_get_device_id(spi); + + dcfg = (const struct tsc2046_adc_dcfg *)id->driver_data; + } if (!dcfg) return -EINVAL; @@ -878,11 +883,18 @@ static const struct of_device_id ads7950_of_table[] = { }; MODULE_DEVICE_TABLE(of, ads7950_of_table); +static const struct spi_device_id tsc2046_adc_spi_ids[] = { + { "tsc2046e-adc", (unsigned long)&tsc2046_adc_dcfg_tsc2046e }, + { } +}; +MODULE_DEVICE_TABLE(spi, tsc2046_adc_spi_ids); + static struct spi_driver tsc2046_adc_driver = { .driver = { .name = "tsc2046", .of_match_table = ads7950_of_table, }, + .id_table = tsc2046_adc_spi_ids, .probe = tsc2046_adc_probe, .remove = tsc2046_adc_remove, }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning 2022-09-01 4:11 ` [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning Oleksij Rempel @ 2022-09-01 14:20 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2022-09-01 14:20 UTC (permalink / raw) To: Oleksij Rempel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Sascha Hauer, Linux Kernel Mailing List, linux-iio, devicetree On Thu, Sep 1, 2022 at 7:12 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > Add spi_device_id to silent following kernel runtime warning: > "SPI driver tsc2046 has no spi_device_id for ti,tsc2046e-adc". Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > changes v3: > - add missing point > - remove unneeded blank line > - assignment id at the definition line > changes v2: > - attach actual driver_data > - use spi_get_device_id fallback > --- > drivers/iio/adc/ti-tsc2046.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > index bbc8b4137b0b1..d5d799972fefd 100644 > --- a/drivers/iio/adc/ti-tsc2046.c > +++ b/drivers/iio/adc/ti-tsc2046.c > @@ -762,6 +762,11 @@ static int tsc2046_adc_probe(struct spi_device *spi) > } > > dcfg = device_get_match_data(dev); > + if (!dcfg) { > + const struct spi_device_id *id = spi_get_device_id(spi); > + > + dcfg = (const struct tsc2046_adc_dcfg *)id->driver_data; > + } > if (!dcfg) > return -EINVAL; > > @@ -878,11 +883,18 @@ static const struct of_device_id ads7950_of_table[] = { > }; > MODULE_DEVICE_TABLE(of, ads7950_of_table); > > +static const struct spi_device_id tsc2046_adc_spi_ids[] = { > + { "tsc2046e-adc", (unsigned long)&tsc2046_adc_dcfg_tsc2046e }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, tsc2046_adc_spi_ids); > + > static struct spi_driver tsc2046_adc_driver = { > .driver = { > .name = "tsc2046", > .of_match_table = ads7950_of_table, > }, > + .id_table = tsc2046_adc_spi_ids, > .probe = tsc2046_adc_probe, > .remove = tsc2046_adc_remove, > }; > -- > 2.30.2 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-01 14:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-01 4:11 [PATCH v3 1/3] dt-bindings: iio: adc: ti,tsc2046: add vref-supply property Oleksij Rempel 2022-09-01 4:11 ` [PATCH v3 2/3] iio: adc: tsc2046: add vref support Oleksij Rempel 2022-09-01 8:24 ` Marco Felsch 2022-09-01 11:35 ` Oleksij Rempel 2022-09-01 11:45 ` Jonathan Cameron 2022-09-01 11:53 ` Oleksij Rempel 2022-09-01 4:11 ` [PATCH v3 3/3] iio: adc: tsc2046: silent spi_device_id warning Oleksij Rempel 2022-09-01 14:20 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).