[PATCH] iio:adc:twl6030: Enable measurements of VUSB, VBAT and others

[PATCH] iio:adc:twl6030: Enable measurements of VUSB, VBAT and others

[Andreas Kemnade]

Some inputs need to be wired up to produce proper measurements,
without this change only near zero values are reported.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/iio/adc/twl6030-gpadc.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
index f53e8558b560c..40438e5b49702 100644
--- a/drivers/iio/adc/twl6030-gpadc.c
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -57,6 +57,18 @@
 #define TWL6030_GPADCS				BIT(1)
 #define TWL6030_GPADCR				BIT(0)
 
+#define USB_VBUS_CTRL_SET			0x04
+#define USB_ID_CTRL_SET				0x06
+
+#define TWL6030_MISC1				0xE4
+#define VBUS_MEAS				0x01
+#define ID_MEAS					0x01
+
+#define VAC_MEAS                0x04
+#define VBAT_MEAS               0x02
+#define BB_MEAS                 0x01
+
+
 /**
  * struct twl6030_chnl_calib - channel calibration
  * @gain:		slope coefficient for ideal curve
@@ -927,6 +939,26 @@ static int twl6030_gpadc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = twl_i2c_write_u8(TWL_MODULE_USB, VBUS_MEAS, USB_VBUS_CTRL_SET);
+	if (ret < 0) {
+		dev_err(dev, "failed to wire up inputs\n");
+		return ret;
+	}
+
+	ret = twl_i2c_write_u8(TWL_MODULE_USB, ID_MEAS, USB_ID_CTRL_SET);
+	if (ret < 0) {
+		dev_err(dev, "failed to wire up inputs\n");
+		return ret;
+	}
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID0,
+				VBAT_MEAS | BB_MEAS | BB_MEAS,
+				TWL6030_MISC1);
+	if (ret < 0) {
+		dev_err(dev, "failed to wire up inputs\n");
+		return ret;
+	}
+
 	indio_dev->name = DRIVER_NAME;
 	indio_dev->info = &twl6030_gpadc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-- 
2.30.2

Re: [PATCH] iio:adc:twl6030: Enable measurements of VUSB, VBAT and others

[Jonathan Cameron]

On Thu,  1 Dec 2022 19:16:35 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> Some inputs need to be wired up to produce proper measurements,
> without this change only near zero values are reported.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

Sounds like a fix to me.  If so, Fixes tag?

Anything in here we should be turning off again if the driver is removed 
or toggling on suspend? If not, other than the space below this looks fine to me.

Jonathan

> ---
>  drivers/iio/adc/twl6030-gpadc.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
> index f53e8558b560c..40438e5b49702 100644
> --- a/drivers/iio/adc/twl6030-gpadc.c
> +++ b/drivers/iio/adc/twl6030-gpadc.c
> @@ -57,6 +57,18 @@
>  #define TWL6030_GPADCS				BIT(1)
>  #define TWL6030_GPADCR				BIT(0)
>  
> +#define USB_VBUS_CTRL_SET			0x04
> +#define USB_ID_CTRL_SET				0x06
> +
> +#define TWL6030_MISC1				0xE4
> +#define VBUS_MEAS				0x01
> +#define ID_MEAS					0x01
> +
> +#define VAC_MEAS                0x04
> +#define VBAT_MEAS               0x02
> +#define BB_MEAS                 0x01
> +
I'm always of the view one blank line is enough! I'll tidy this up whilst applying.
> +
>  /**
>   * struct twl6030_chnl_calib - channel calibration
>   * @gain:		slope coefficient for ideal curve
> @@ -927,6 +939,26 @@ static int twl6030_gpadc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = twl_i2c_write_u8(TWL_MODULE_USB, VBUS_MEAS, USB_VBUS_CTRL_SET);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to wire up inputs\n");
> +		return ret;
> +	}
> +
> +	ret = twl_i2c_write_u8(TWL_MODULE_USB, ID_MEAS, USB_ID_CTRL_SET);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to wire up inputs\n");
> +		return ret;
> +	}
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID0,
> +				VBAT_MEAS | BB_MEAS | BB_MEAS,
> +				TWL6030_MISC1);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to wire up inputs\n");
> +		return ret;
> +	}
> +
>  	indio_dev->name = DRIVER_NAME;
>  	indio_dev->info = &twl6030_gpadc_iio_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;

Re: [PATCH] iio:adc:twl6030: Enable measurements of VUSB, VBAT and others

[Andreas Kemnade]

On Sun, 4 Dec 2022 15:41:52 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu,  1 Dec 2022 19:16:35 +0100
> Andreas Kemnade <andreas@kemnade.info> wrote:
> 
> > Some inputs need to be wired up to produce proper measurements,
> > without this change only near zero values are reported.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>  
> 
> Sounds like a fix to me.  If so, Fixes tag?

seems to be there since the beginning, to it would be
Fixes: 1696f36482e70 ("iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver")

I think it was just not used with the charger (which is not in mainline yet),
so it was probably ignored.

> 
> Anything in here we should be turning off again if the driver is removed 
> or toggling on suspend? If not, other than the space below this looks fine to me.
> 
I would consider that as configuration, comparing with the nearest relative twl4030,
there a similar bit is set in the probe() without disabling it in the remove().

But I think we should set TWL6030_GPADCR in remove() as we do in suspend(),
but that is another fix.

Regards,
Andreas

Re: [PATCH] iio:adc:twl6030: Enable measurements of VUSB, VBAT and others

[Jonathan Cameron]

On Sun, 4 Dec 2022 21:27:51 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Sun, 4 Dec 2022 15:41:52 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Thu,  1 Dec 2022 19:16:35 +0100
> > Andreas Kemnade <andreas@kemnade.info> wrote:
> >   
> > > Some inputs need to be wired up to produce proper measurements,
> > > without this change only near zero values are reported.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>    
> > 
> > Sounds like a fix to me.  If so, Fixes tag?  
> 
> seems to be there since the beginning, to it would be
> Fixes: 1696f36482e70 ("iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver")
> 
> I think it was just not used with the charger (which is not in mainline yet),
> so it was probably ignored.
Makes sense.  I've applied it at marked it for stable on basis it wants the
fixes tag and would get picked up anyway + appears unlikely to have bad side effects.
> 
> > 
> > Anything in here we should be turning off again if the driver is removed 
> > or toggling on suspend? If not, other than the space below this looks fine to me.
> >   
> I would consider that as configuration, comparing with the nearest relative twl4030,
> there a similar bit is set in the probe() without disabling it in the remove().
> 
> But I think we should set TWL6030_GPADCR in remove() as we do in suspend(),
> but that is another fix.

There are always more fixes :)

Jonathan

> 
> Regards,
> Andreas