diff mbox series

[v2,2/2] power: supply: rn5t618: Add voltage_now property

Message ID 20210705113637.28908-3-andreas@kemnade.info
State Superseded
Headers show
Series mfd: rn5t618: Extend ADC support | expand

Commit Message

Andreas Kemnade July 5, 2021, 11:36 a.m. UTC
Read voltage_now via IIO and provide the property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reported-by: kernel test robot <lkp@intel.com>
---
Changes in v2:
- different error handling needed for iio_map usage
- fix dependencies in Kconfig

 drivers/power/supply/Kconfig         |  2 ++
 drivers/power/supply/rn5t618_power.c | 40 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Jonathan Cameron July 11, 2021, 10:20 a.m. UTC | #1
On Mon,  5 Jul 2021 13:36:37 +0200
Andreas Kemnade <andreas@kemnade.info> wrote:

> Read voltage_now via IIO and provide the property.

> 

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

> Reported-by: kernel test robot <lkp@intel.com>

Huh?  Seems unlikely it pointed out that this patch was necessary in general.
If highlighting a particular fix in an earlier version, then state what it was
in the commit message. Note for fixes that get rolled into patches, it's
often just mentioned in the change log and we skip the tag because it can
cause confusion.

One other comment inline but it's up to you whether you care or not!

These could go via the IIO tree or power. I don't mind which, but unless
someone shouts, I'm assuming power.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


Jonathan


> ---

> Changes in v2:

> - different error handling needed for iio_map usage

> - fix dependencies in Kconfig

> 

>  drivers/power/supply/Kconfig         |  2 ++

>  drivers/power/supply/rn5t618_power.c | 40 ++++++++++++++++++++++++++++

>  2 files changed, 42 insertions(+)

> 

> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig

> index e696364126f1..b2910d950929 100644

> --- a/drivers/power/supply/Kconfig

> +++ b/drivers/power/supply/Kconfig

> @@ -790,6 +790,8 @@ config CHARGER_WILCO

>  config RN5T618_POWER

>  	tristate "RN5T618 charger/fuel gauge support"

>  	depends on MFD_RN5T618

> +	depends on RN5T618_ADC

> +	depends on IIO

>  	help

>  	  Say Y here to have support for RN5T618 PMIC family fuel gauge and charger.

>  	  This driver can also be built as a module. If so, the module will be

> diff --git a/drivers/power/supply/rn5t618_power.c b/drivers/power/supply/rn5t618_power.c

> index 819061918b2a..bca3fd86c14d 100644

> --- a/drivers/power/supply/rn5t618_power.c

> +++ b/drivers/power/supply/rn5t618_power.c

> @@ -9,10 +9,12 @@

>  #include <linux/device.h>

>  #include <linux/bitops.h>

>  #include <linux/errno.h>

> +#include <linux/iio/consumer.h>

>  #include <linux/init.h>

>  #include <linux/interrupt.h>

>  #include <linux/module.h>

>  #include <linux/mfd/rn5t618.h>

> +#include <linux/of_device.h>

>  #include <linux/platform_device.h>

>  #include <linux/power_supply.h>

>  #include <linux/regmap.h>

> @@ -64,6 +66,8 @@ struct rn5t618_power_info {

>  	struct power_supply *battery;

>  	struct power_supply *usb;

>  	struct power_supply *adp;

> +	struct iio_channel *channel_vusb;

> +	struct iio_channel *channel_vadp;

>  	int irq;

>  };

>  

> @@ -77,6 +81,7 @@ static enum power_supply_usb_type rn5t618_usb_types[] = {

>  static enum power_supply_property rn5t618_usb_props[] = {

>  	/* input current limit is not very accurate */

>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,

> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,

>  	POWER_SUPPLY_PROP_STATUS,

>  	POWER_SUPPLY_PROP_USB_TYPE,

>  	POWER_SUPPLY_PROP_ONLINE,

> @@ -85,6 +90,7 @@ static enum power_supply_property rn5t618_usb_props[] = {

>  static enum power_supply_property rn5t618_adp_props[] = {

>  	/* input current limit is not very accurate */

>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,

> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,

>  	POWER_SUPPLY_PROP_STATUS,

>  	POWER_SUPPLY_PROP_ONLINE,

>  };

> @@ -464,6 +470,16 @@ static int rn5t618_adp_get_property(struct power_supply *psy,

>  

>  		val->intval = FROM_CUR_REG(regval);

>  		break;

> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:

> +		if (!info->channel_vadp)

> +			return -ENODATA;

> +

> +		ret = iio_read_channel_processed(info->channel_vadp, &val->intval);

> +		if (ret < 0)

> +			return ret;

> +

> +		val->intval *= 1000;

> +		break;

>  	default:

>  		return -EINVAL;

>  	}

> @@ -589,6 +605,16 @@ static int rn5t618_usb_get_property(struct power_supply *psy,

>  			val->intval = FROM_CUR_REG(regval);

>  		}

>  		break;

> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:

> +		if (!info->channel_vusb)

> +			return -ENODATA;

> +

> +		ret = iio_read_channel_processed(info->channel_vusb, &val->intval);

> +		if (ret < 0)

> +			return ret;

> +

> +		val->intval *= 1000;


It's a recent addition, but we now have an iio_read_channel_processed_scale()
function that can retain a little more precision because, in a fractional scale
case like with the adc here, it will multiply by 1000 before doing the division.

May make a negligable difference though depending on noise level of the ADC etc.


> +		break;

>  	default:

>  		return -EINVAL;

>  	}

> @@ -711,6 +737,20 @@ static int rn5t618_power_probe(struct platform_device *pdev)

>  

>  	platform_set_drvdata(pdev, info);

>  

> +	info->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");

> +	if (IS_ERR(info->channel_vusb)) {

> +		if (PTR_ERR(info->channel_vusb) == -ENODEV)

> +			return -EPROBE_DEFER;

> +		return PTR_ERR(info->channel_vusb);

> +	}

> +

> +	info->channel_vadp = devm_iio_channel_get(&pdev->dev, "vadp");

> +	if (IS_ERR(info->channel_vadp)) {

> +		if (PTR_ERR(info->channel_vadp) == -ENODEV)

> +			return -EPROBE_DEFER;

> +		return PTR_ERR(info->channel_vadp);

> +	}

> +

>  	ret = regmap_read(info->rn5t618->regmap, RN5T618_CONTROL, &v);

>  	if (ret)

>  		return ret;
Andreas Kemnade July 12, 2021, 7:11 a.m. UTC | #2
On Sun, 11 Jul 2021 11:20:39 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon,  5 Jul 2021 13:36:37 +0200

> Andreas Kemnade <andreas@kemnade.info> wrote:

> 

> > Read voltage_now via IIO and provide the property.

> > 

> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

> > Reported-by: kernel test robot <lkp@intel.com>  

> Huh?  Seems unlikely it pointed out that this patch was necessary in general.

> If highlighting a particular fix in an earlier version, then state what it was

> in the commit message. Note for fixes that get rolled into patches, it's

> often just mentioned in the change log and we skip the tag because it can

> cause confusion.

> 

The robot found a problem in v1 (missing depends on IIO). It is fixed
now. The message from the bot tells to add a tag. It seems not to make
sense. But probably the bot is also running on public branches (which
will not be rebase) and uses the same message where it actually makes
sense.

I will send a v3 with that tag removed and the other comment addressed.

Regards,
Andreas
Jonathan Cameron July 12, 2021, 10:15 a.m. UTC | #3
On Mon, 12 Jul 2021 09:11:30 +0200
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Sun, 11 Jul 2021 11:20:39 +0100

> Jonathan Cameron <jic23@kernel.org> wrote:

> 

> > On Mon,  5 Jul 2021 13:36:37 +0200

> > Andreas Kemnade <andreas@kemnade.info> wrote:

> >   

> > > Read voltage_now via IIO and provide the property.

> > > 

> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

> > > Reported-by: kernel test robot <lkp@intel.com>    

> > Huh?  Seems unlikely it pointed out that this patch was necessary in general.

> > If highlighting a particular fix in an earlier version, then state what it was

> > in the commit message. Note for fixes that get rolled into patches, it's

> > often just mentioned in the change log and we skip the tag because it can

> > cause confusion.

> >   

> The robot found a problem in v1 (missing depends on IIO). It is fixed

> now. The message from the bot tells to add a tag. It seems not to make

> sense. But probably the bot is also running on public branches (which

> will not be rebase) and uses the same message where it actually makes

> sense.


Yup. It might be helpful if they modified that message to suggest
commented format if the fix is rolled into an existing patch.  I've seen
things like.

Reported-by: kernel test robot <lkp@intel.com> # Fix something interesting.

Which makes it clear what is going on.

Jonathan
> 

> I will send a v3 with that tag removed and the other comment addressed.

> 

> Regards,

> Andreas
diff mbox series

Patch

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index e696364126f1..b2910d950929 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -790,6 +790,8 @@  config CHARGER_WILCO
 config RN5T618_POWER
 	tristate "RN5T618 charger/fuel gauge support"
 	depends on MFD_RN5T618
+	depends on RN5T618_ADC
+	depends on IIO
 	help
 	  Say Y here to have support for RN5T618 PMIC family fuel gauge and charger.
 	  This driver can also be built as a module. If so, the module will be
diff --git a/drivers/power/supply/rn5t618_power.c b/drivers/power/supply/rn5t618_power.c
index 819061918b2a..bca3fd86c14d 100644
--- a/drivers/power/supply/rn5t618_power.c
+++ b/drivers/power/supply/rn5t618_power.c
@@ -9,10 +9,12 @@ 
 #include <linux/device.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
+#include <linux/iio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/mfd/rn5t618.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
@@ -64,6 +66,8 @@  struct rn5t618_power_info {
 	struct power_supply *battery;
 	struct power_supply *usb;
 	struct power_supply *adp;
+	struct iio_channel *channel_vusb;
+	struct iio_channel *channel_vadp;
 	int irq;
 };
 
@@ -77,6 +81,7 @@  static enum power_supply_usb_type rn5t618_usb_types[] = {
 static enum power_supply_property rn5t618_usb_props[] = {
 	/* input current limit is not very accurate */
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_USB_TYPE,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -85,6 +90,7 @@  static enum power_supply_property rn5t618_usb_props[] = {
 static enum power_supply_property rn5t618_adp_props[] = {
 	/* input current limit is not very accurate */
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
 };
@@ -464,6 +470,16 @@  static int rn5t618_adp_get_property(struct power_supply *psy,
 
 		val->intval = FROM_CUR_REG(regval);
 		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		if (!info->channel_vadp)
+			return -ENODATA;
+
+		ret = iio_read_channel_processed(info->channel_vadp, &val->intval);
+		if (ret < 0)
+			return ret;
+
+		val->intval *= 1000;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -589,6 +605,16 @@  static int rn5t618_usb_get_property(struct power_supply *psy,
 			val->intval = FROM_CUR_REG(regval);
 		}
 		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		if (!info->channel_vusb)
+			return -ENODATA;
+
+		ret = iio_read_channel_processed(info->channel_vusb, &val->intval);
+		if (ret < 0)
+			return ret;
+
+		val->intval *= 1000;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -711,6 +737,20 @@  static int rn5t618_power_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
+	info->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");
+	if (IS_ERR(info->channel_vusb)) {
+		if (PTR_ERR(info->channel_vusb) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(info->channel_vusb);
+	}
+
+	info->channel_vadp = devm_iio_channel_get(&pdev->dev, "vadp");
+	if (IS_ERR(info->channel_vadp)) {
+		if (PTR_ERR(info->channel_vadp) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(info->channel_vadp);
+	}
+
 	ret = regmap_read(info->rn5t618->regmap, RN5T618_CONTROL, &v);
 	if (ret)
 		return ret;