2021-07-05 11:38:18

by Andreas Kemnade

[permalink] [raw]
Subject: [PATCH v2 0/2] mfd: rn5t618: Extend ADC support

Add iio map to make voltage_now related channels accessible to power
driver.

Changes in v2:
- use iio_map instead of devicetree to allow mapping which does not
block future extension by devicetree.

Andreas Kemnade (2):
iio: adc: rn5t618: Add iio map
power: supply: rn5t618: Add voltage_now property

drivers/iio/adc/rn5t618-adc.c | 23 ++++++++++++++++
drivers/power/supply/Kconfig | 2 ++
drivers/power/supply/rn5t618_power.c | 40 ++++++++++++++++++++++++++++
3 files changed, 65 insertions(+)

--
2.30.2


2021-07-05 11:38:44

by Andreas Kemnade

[permalink] [raw]
Subject: [PATCH v2 2/2] power: supply: rn5t618: Add voltage_now property

Read voltage_now via IIO and provide the property.

Signed-off-by: Andreas Kemnade <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
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;
+ 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;
--
2.30.2

2021-07-11 10:25:45

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] power: supply: rn5t618: Add voltage_now property

On Mon, 5 Jul 2021 13:36:37 +0200
Andreas Kemnade <[email protected]> wrote:

> Read voltage_now via IIO and provide the property.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> Reported-by: kernel test robot <[email protected]>
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 <[email protected]>

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;

2021-07-12 10:47:55

by Andreas Kemnade

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] power: supply: rn5t618: Add voltage_now property

On Sun, 11 Jul 2021 11:20:39 +0100
Jonathan Cameron <[email protected]> wrote:

> On Mon, 5 Jul 2021 13:36:37 +0200
> Andreas Kemnade <[email protected]> wrote:
>
> > Read voltage_now via IIO and provide the property.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> 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

2021-07-12 11:39:23

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] power: supply: rn5t618: Add voltage_now property

On Mon, 12 Jul 2021 09:11:30 +0200
Andreas Kemnade <[email protected]> wrote:

> On Sun, 11 Jul 2021 11:20:39 +0100
> Jonathan Cameron <[email protected]> wrote:
>
> > On Mon, 5 Jul 2021 13:36:37 +0200
> > Andreas Kemnade <[email protected]> wrote:
> >
> > > Read voltage_now via IIO and provide the property.
> > >
> > > Signed-off-by: Andreas Kemnade <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > 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 <[email protected]> # 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