From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 55D40ECAAD1 for ; Thu, 1 Sep 2022 11:46:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232629AbiIALp6 (ORCPT ); Thu, 1 Sep 2022 07:45:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231668AbiIALpy (ORCPT ); Thu, 1 Sep 2022 07:45:54 -0400 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E8583E740; Thu, 1 Sep 2022 04:45:53 -0700 (PDT) Received: from fraeml744-chm.china.huawei.com (unknown [172.18.147.206]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4MJK3T3xmfz67Zm0; Thu, 1 Sep 2022 19:45:09 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (7.191.163.240) by fraeml744-chm.china.huawei.com (10.206.15.225) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 1 Sep 2022 13:45:50 +0200 Received: from localhost (10.202.226.42) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 1 Sep 2022 12:45:50 +0100 Date: Thu, 1 Sep 2022 12:45:49 +0100 From: Jonathan Cameron To: Oleksij Rempel CC: Jonathan Cameron , Lars-Peter Clausen , Rob Herring , Krzysztof Kozlowski , , , , , Andy Shevchenko Subject: Re: [PATCH v3 2/3] iio: adc: tsc2046: add vref support Message-ID: <20220901124549.00002287@huawei.com> In-Reply-To: <20220901041146.3652287-2-o.rempel@pengutronix.de> References: <20220901041146.3652287-1-o.rempel@pengutronix.de> <20220901041146.3652287-2-o.rempel@pengutronix.de> X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.29; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.226.42] X-ClientProxiedBy: lhrpeml100001.china.huawei.com (7.191.160.183) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 1 Sep 2022 06:11:45 +0200 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 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 > #include > #include > +#include > #include > > #include > @@ -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); >