[PATCH v2 00/17] Add support for AXP192 PMIC

[PATCH v2 00/17] Add support for AXP192 PMIC

[Aidan MacDonald]

Thanks to all reviewers for the feedback on v1. I believe I've made all
the suggested changes, and I've also added support for the battery power
supply in this version.

Changes in v2:

* Do a little cleanup of axp20x_adc suggested by Jonathan Cameron
* Consolidate ADC read functions in axp20x_adc
* Drop the axp192's read_label callback in axp20x_adc
* Clean up the axp192-gpio dt bindings
* Rewrite a problematic bit of code in axp20x_usb_power reported
  by kernel test robot
* Support AXP192 in axp20x_battery
* Split up regmap-irq changes to two separate patches

Cover letter from v1:

Hi all,

This patch series adds support for the X-Powers AXP192 PMIC to the
AXP20x driver framework.

The first patch is a small change to regmap-irq to support the AXP192's
unusual IRQ register layout. It isn't possible to include all of the
IRQ registers in one regmap-irq chip without this.

The rest of the changes are pretty straightforward, I think the only
notable parts are the axp20x_adc driver where there seems to be some
opportunities for code reuse (the axp192 is nearly a duplicate of the
axp20x) and the addition of a new pinctrl driver for the axp192, since
the axp20x pinctrl driver was not very easy to adapt.

Aidan MacDonald (17):
  regmap-irq: Use sub_irq_reg() to calculate unmask register address
  regmap-irq: Add get_irq_reg to support unusual register layouts
  dt-bindings: mfd: add bindings for AXP192 MFD device
  dt-bindings: iio: adc: axp209: Add AXP192 compatible
  dt-bindings: power: supply: axp20x: Add AXP192 compatible
  dt-bindings: gpio: Add AXP192 GPIO bindings
  dt-bindings: power: axp20x-battery: Add AXP192 compatible
  mfd: axp20x: Add support for AXP192
  regulator: axp20x: Add support for AXP192
  iio: adc: axp20x_adc: Minor code cleanups
  iio: adc: axp20x_adc: Consolidate ADC raw read functions
  iio: adc: axp20x_adc: Add support for AXP192
  power: supply: axp20x_usb_power: Add support for AXP192
  pinctrl: Add AXP192 pin control driver
  power: axp20x_battery: Add constant charge current table
  power: axp20x_battery: Support battery status without fuel gauge
  power: axp20x_battery: Add support for AXP192

 .../bindings/gpio/x-powers,axp192-gpio.yaml   |  57 ++
 .../bindings/iio/adc/x-powers,axp209-adc.yaml |  18 +
 .../bindings/mfd/x-powers,axp152.yaml         |   1 +
 .../x-powers,axp20x-battery-power-supply.yaml |   1 +
 .../x-powers,axp20x-usb-power-supply.yaml     |   1 +
 drivers/base/regmap/regmap-irq.c              |  19 +-
 drivers/iio/adc/axp20x_adc.c                  | 389 ++++++++++--
 drivers/mfd/axp20x-i2c.c                      |   2 +
 drivers/mfd/axp20x.c                          | 153 +++++
 drivers/pinctrl/Kconfig                       |  14 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-axp192.c              | 589 ++++++++++++++++++
 drivers/power/supply/axp20x_battery.c         | 136 +++-
 drivers/power/supply/axp20x_usb_power.c       |  80 ++-
 drivers/regulator/axp20x-regulator.c          | 101 ++-
 include/linux/mfd/axp20x.h                    |  84 +++
 include/linux/regmap.h                        |   5 +
 17 files changed, 1540 insertions(+), 111 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
 create mode 100644 drivers/pinctrl/pinctrl-axp192.c

-- 
2.35.1

[PATCH v2 01/17] regmap-irq: Use sub_irq_reg() to calculate unmask register address

[Aidan MacDonald]

Call sub_irq_reg() instead of calculating the offset of the register
to avoid relying on the fact that sub_irq_reg() is a linear function.
This prepares for allowing drivers to override the default sub_irq_reg
implementation if the default behavior is unsuitable.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 400c7412a7dc..4a2e1f6aa94d 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -97,7 +97,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	struct regmap *map = d->map;
 	int i, j, ret;
 	u32 reg;
-	u32 unmask_offset;
 	u32 val;
 
 	if (d->chip->runtime_pm) {
@@ -141,11 +140,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 				dev_err(d->map->dev,
 					"Failed to sync unmasks in %x\n",
 					reg);
-			unmask_offset = d->chip->unmask_base -
-							d->chip->mask_base;
+
 			/* clear mask with unmask_base register */
+			reg = sub_irq_reg(d, d->chip->unmask_base, i);
 			ret = regmap_irq_update_bits(d,
-					reg + unmask_offset,
+					reg,
 					d->mask_buf_def[i],
 					d->mask_buf[i]);
 		} else {
@@ -632,7 +631,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	int ret = -ENOMEM;
 	int num_type_reg;
 	u32 reg;
-	u32 unmask_offset;
 
 	if (chip->num_regs <= 0)
 		return -EINVAL;
@@ -773,10 +771,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 			ret = regmap_irq_update_bits(d, reg,
 					 d->mask_buf[i], ~d->mask_buf[i]);
 		else if (d->chip->unmask_base) {
-			unmask_offset = d->chip->unmask_base -
-					d->chip->mask_base;
+			reg = sub_irq_reg(d, d->chip->unmask_base, i);
 			ret = regmap_irq_update_bits(d,
-					reg + unmask_offset,
+					reg,
 					d->mask_buf[i],
 					d->mask_buf[i]);
 		} else
-- 
2.35.1

Re: [PATCH v2 01/17] regmap-irq: Use sub_irq_reg() to calculate unmask register address

[Guru Das Srinagesh]

On Tue, Jun 07, 2022 at 04:53:08PM +0100, Aidan MacDonald wrote:
> Call sub_irq_reg() instead of calculating the offset of the register
> to avoid relying on the fact that sub_irq_reg() is a linear function.

Seems like unmask_reg is the only register whose address is not calculated
using sub_irq_reg(). Switching to using sub_irq_reg() will bring it in line
with the other calculations.

Could you please incorporate this info in your commit message as well? This
should be the rationale for this change; that it allows for the get_irq_reg()
patch should be secondary.

The change seems okay to me, but I'd ideally like someone to pick this up and
test it out just to make sure it doesn't break existing behaviour for them.

Thank you.

Guru Das.

Re: [PATCH v2 01/17] regmap-irq: Use sub_irq_reg() to calculate unmask register address

[Aidan MacDonald]

Guru Das Srinagesh <quic_gurus@quicinc.com> writes:

> On Tue, Jun 07, 2022 at 04:53:08PM +0100, Aidan MacDonald wrote:
>> Call sub_irq_reg() instead of calculating the offset of the register
>> to avoid relying on the fact that sub_irq_reg() is a linear function.
>
> Seems like unmask_reg is the only register whose address is not calculated
> using sub_irq_reg(). Switching to using sub_irq_reg() will bring it in line
> with the other calculations.
>
> Could you please incorporate this info in your commit message as well? This
> should be the rationale for this change; that it allows for the get_irq_reg()
> patch should be secondary.

I'll note that in v3, thanks.

>
> The change seems okay to me, but I'd ideally like someone to pick this up and
> test it out just to make sure it doesn't break existing behaviour for them.
>
> Thank you.
>
> Guru Das.

[PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Aidan MacDonald]

Add a new callback, get_irq_reg, for regmap IRQ chips, to support devices
with unusual register layouts. This is required in the rare cases where
the offset of an IRQ register is not constant with respect to the base
register. This is probably best illustrated with an example (taken from
the AXP192 PMIC):

            mask    status
    IRQ0    0x40    0x44
    IRQ1    0x41    0x45
    IRQ2    0x42    0x46
    IRQ3    0x43    0x47
    IRQ4    0x4a    0x4d

If we set mask_base = 0x40 and status_base = 0x44, the offsets of each
register relative to the base are:

            mask    status
    IRQ0    0       0
    IRQ1    1       1
    IRQ2    2       2
    IRQ3    3       3
    IRQ4    10      9

The existing mapping mechanisms can't include IRQ4 in the same irqchip
as IRQ0-3 because the offset of IRQ4's register depends on which type
of register we're asking for, ie. which base register is used.

The get_irq_reg callback allows drivers to specify an arbitrary mapping
of (base register, register index) pairs to register addresses, instead
of the default linear mapping "base_register + register_index". This
allows unusual layouts, like the one above, to be handled using a single
regmap IRQ chip.

The drawback is that when get_irq_reg is used, it's impossible to use
bulk reads for status registers even if some of them are contiguous,
because the mapping is opaque to regmap-irq. This should be acceptable
for the case of a few infrequently-polled status registers.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 6 ++++--
 include/linux/regmap.h           | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 4a2e1f6aa94d..e50437b18284 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -55,7 +55,9 @@ static int sub_irq_reg(struct regmap_irq_chip_data *data,
 	unsigned int offset;
 	int reg = 0;
 
-	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
+	if (chip->get_irq_reg) {
+		reg = chip->get_irq_reg(base_reg, i);
+	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
 		/* Assume linear mapping */
 		reg = base_reg + (i * map->reg_stride * data->irq_reg_stride);
 	} else {
@@ -479,7 +481,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 
 		}
 	} else if (!map->use_single_read && map->reg_stride == 1 &&
-		   data->irq_reg_stride == 1) {
+		   data->irq_reg_stride == 1 && !chip->get_irq_reg) {
 
 		u8 *buf8 = data->status_reg_buf;
 		u16 *buf16 = data->status_reg_buf;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 8952fa3d0d59..4828021ab9e8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1495,6 +1495,10 @@ struct regmap_irq_sub_irq_map {
  *		     after handling the interrupts in regmap_irq_handler().
  * @set_type_virt:   Driver specific callback to extend regmap_irq_set_type()
  *		     and configure virt regs.
+ * @get_irq_reg:     Callback to map a register index in range [0, num_regs[
+ *		     to a register, relative to a specific base register. This
+ *		     is mainly useful for devices where the register offsets
+ *		     change depending on the base register.
  * @irq_drv_data:    Driver specific IRQ data which is passed as parameter when
  *		     driver specific pre/post interrupt handler is called.
  *
@@ -1545,6 +1549,7 @@ struct regmap_irq_chip {
 	int (*handle_post_irq)(void *irq_drv_data);
 	int (*set_type_virt)(unsigned int **buf, unsigned int type,
 			     unsigned long hwirq, int reg);
+	int (*get_irq_reg)(unsigned int base_reg, int i);
 	void *irq_drv_data;
 };
 
-- 
2.35.1

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:

> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> +	if (chip->get_irq_reg) {
> +		reg = chip->get_irq_reg(base_reg, i);
> +	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {

It seems like it would be cleaner and clearer to refactor things so that
we always have a get_irq_reg() with standard chips getting given a
default implementation which implements the current behaviour.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Aidan MacDonald]

Mark Brown <broonie@kernel.org> writes:

> On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:
>
>> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>> +	if (chip->get_irq_reg) {
>> +		reg = chip->get_irq_reg(base_reg, i);
>> +	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>
> It seems like it would be cleaner and clearer to refactor things so that
> we always have a get_irq_reg() with standard chips getting given a
> default implementation which implements the current behaviour.

I don't think that is a good way to clean things up. I only intended
get_irq_reg() to be a quick hack to solve a problem; in my opinion it
would be a poor abstraction to base the API around.

What I'd suggest is something that will simplify regmap-irq. Instead of
defining the base registers, etc. in the chip, introduce a new struct
to describe a register group:

    struct regmap_irq_reg_group {
        unsigned int status_base;
        unsigned int mask_base;
        ...

        unsigned int irq_reg_stride;

        int num_regs;
    };

The idea is that the registers in a group are linearly mapped using the
formula "base + (i * irq_reg_stride)". Then it's possible to allow for
multiple register groups in regmap_irq_chip:

    struct regmap_irq_chip {
        const struct regmap_irq_reg_group *groups;
        unsigned int num_groups;

        unsigned int main_status_base;
        unsigned int num_main_status_bits;
        int num_main_regs;

        ...
    };

It should be straightforward to fit existing chips into this model.

- "Normal" chips which do not use sub_reg_offsets or not_fixed_stride
  will have a single register group describing their register layout.

- Chips which use not_fixed_stride=1 (eg. qcom-pm8008.c) will define
  multiple register groups instead of using sub_reg_offsets, so they
  will look more like a normal chip.

- Chips that use a main status + sub-block IRQ layout will define
  one register group for each sub-block and continue to describe the
  location of the main status registers inside of regmap_irq_chip.
  A group will only get polled if the corresponding main status bit
  is set -- n'th group is polled if n'th bit is set.

I think this scheme is easier to understand than having three or four
different hacks to deal with minor deviations from the simple cases.
It's also more flexible because groups do not need to be homogenous,
unlike the way sub_reg_offsets works.

For the AXP192, I'd just need to add two register groups, one for IRQ0-3
and another with IRQ4 off by itself. So this would remove the need for
get_irq_reg() entirely.

On the other hand, basing the public API around get_irq_reg() doesn't
appear to simplify any existing use case.

What do you think? If you're happy with the idea I don't mind doing the
refactoring in a separate patch series.

I had hoped to avoid a big refactor just to add one chip, though.

Best regards,
Aidan

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Fri, Jun 10, 2022 at 04:40:20PM +0100, Aidan MacDonald wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:

> >> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> >> +	if (chip->get_irq_reg) {
> >> +		reg = chip->get_irq_reg(base_reg, i);
> >> +	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {

> > It seems like it would be cleaner and clearer to refactor things so that
> > we always have a get_irq_reg() with standard chips getting given a
> > default implementation which implements the current behaviour.

> I don't think that is a good way to clean things up. I only intended
> get_irq_reg() to be a quick hack to solve a problem; in my opinion it
> would be a poor abstraction to base the API around.

I'm not sure why you are proposing this change if you are so convinced
it's a bad idea.  If you want to propose a different rework go ahead,
but adding the new operation without any form of factoring out is an
issue.  At first glance your suggestion looked plausible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Aidan MacDonald]

Mark Brown <broonie@kernel.org> writes:

> On Fri, Jun 10, 2022 at 04:40:20PM +0100, Aidan MacDonald wrote:
>> Mark Brown <broonie@kernel.org> writes:
>> > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:
>
>> >> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>> >> +	if (chip->get_irq_reg) {
>> >> +		reg = chip->get_irq_reg(base_reg, i);
>> >> +	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>
>> > It seems like it would be cleaner and clearer to refactor things so that
>> > we always have a get_irq_reg() with standard chips getting given a
>> > default implementation which implements the current behaviour.
>
>> I don't think that is a good way to clean things up. I only intended
>> get_irq_reg() to be a quick hack to solve a problem; in my opinion it
>> would be a poor abstraction to base the API around.
>
> I'm not sure why you are proposing this change if you are so convinced
> it's a bad idea.  If you want to propose a different rework go ahead,
> but adding the new operation without any form of factoring out is an
> issue.  At first glance your suggestion looked plausible.

This patch isn't a refactor and I don't think it's a bad idea when
viewed as minimal solution to a problem, which was my intention.
I just think it wouldn't be a good abstraction to refactor around.
Thanks for your input anyhow.

Just as a heads up, I'll be resending these regmap-irq patches in v3
so the series stays self-contained while I work on refactoring. Feel
free to ignore them if you don't want to take them.

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Matti Vaittinen]

Hi dee Ho peeps!

Sorry for the late reply.

pe 10. kesäk. 2022 klo 18.43 Aidan MacDonald
(aidanmacdonald.0x0@gmail.com) kirjoitti:
>
> Mark Brown <broonie@kernel.org> writes:
>
> > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:
> >
> >> -    if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> >> +    if (chip->get_irq_reg) {
> >> +            reg = chip->get_irq_reg(base_reg, i);
> >> +    } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> >
> > It seems like it would be cleaner and clearer to refactor things so that
> > we always have a get_irq_reg() with standard chips getting given a
> > default implementation which implements the current behaviour.
>
> I don't think that is a good way to clean things up. I only intended
> get_irq_reg() to be a quick hack to solve a problem; in my opinion it
> would be a poor abstraction to base the API around.
>
> What I'd suggest is something that will simplify regmap-irq. Instead of
> defining the base registers, etc. in the chip, introduce a new struct
> to describe a register group:
>
>     struct regmap_irq_reg_group {
>         unsigned int status_base;
>         unsigned int mask_base;
>         ...
>
>         unsigned int irq_reg_stride;
>
>         int num_regs;
>     };
>
> The idea is that the registers in a group are linearly mapped using the
> formula "base + (i * irq_reg_stride)". Then it's possible to allow for
> multiple register groups in regmap_irq_chip:
>
>     struct regmap_irq_chip {
>         const struct regmap_irq_reg_group *groups;
>         unsigned int num_groups;
>
>         unsigned int main_status_base;
>         unsigned int num_main_status_bits;
>         int num_main_regs;
>
>         ...
>     };
>
> It should be straightforward to fit existing chips into this model.
>
> - Chips that use a main status + sub-block IRQ layout will define
>   one register group for each sub-block and continue to describe the
>   location of the main status registers inside of regmap_irq_chip.
>   A group will only get polled if the corresponding main status bit
>   is set -- n'th group is polled if n'th bit is set.

Does this work for devices where a single main status bit can flag
IRQs in more than one sub-registers?

Best Regards
 -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

[Aidan MacDonald]

Matti Vaittinen <mazziesaccount@gmail.com> writes:

> Hi dee Ho peeps!
>
> Sorry for the late reply.
>
> pe 10. kesäk. 2022 klo 18.43 Aidan MacDonald
> (aidanmacdonald.0x0@gmail.com) kirjoitti:
>>
>> Mark Brown <broonie@kernel.org> writes:
>>
>> > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:
>> >
>> >> -    if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>> >> +    if (chip->get_irq_reg) {
>> >> +            reg = chip->get_irq_reg(base_reg, i);
>> >> +    } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>> >
>> > It seems like it would be cleaner and clearer to refactor things so that
>> > we always have a get_irq_reg() with standard chips getting given a
>> > default implementation which implements the current behaviour.
>>
>> I don't think that is a good way to clean things up. I only intended
>> get_irq_reg() to be a quick hack to solve a problem; in my opinion it
>> would be a poor abstraction to base the API around.
>>
>> What I'd suggest is something that will simplify regmap-irq. Instead of
>> defining the base registers, etc. in the chip, introduce a new struct
>> to describe a register group:
>>
>>     struct regmap_irq_reg_group {
>>         unsigned int status_base;
>>         unsigned int mask_base;
>>         ...
>>
>>         unsigned int irq_reg_stride;
>>
>>         int num_regs;
>>     };
>>
>> The idea is that the registers in a group are linearly mapped using the
>> formula "base + (i * irq_reg_stride)". Then it's possible to allow for
>> multiple register groups in regmap_irq_chip:
>>
>>     struct regmap_irq_chip {
>>         const struct regmap_irq_reg_group *groups;
>>         unsigned int num_groups;
>>
>>         unsigned int main_status_base;
>>         unsigned int num_main_status_bits;
>>         int num_main_regs;
>>
>>         ...
>>     };
>>
>> It should be straightforward to fit existing chips into this model.
>>
>> - Chips that use a main status + sub-block IRQ layout will define
>>   one register group for each sub-block and continue to describe the
>>   location of the main status registers inside of regmap_irq_chip.
>>   A group will only get polled if the corresponding main status bit
>>   is set -- n'th group is polled if n'th bit is set.
>
> Does this work for devices where a single main status bit can flag
> IRQs in more than one sub-registers?
>
> Best Regards
>  -- Matti

No, I realized once I got into the refactor that what I outlined here
wouldn't fit that use case well, which is what rohm-bd71828 needs.

There are some other complications with this approach, like how to
go between IRQs and register groups efficiently, and it's generally
a rather heavyweight solution. It might be useful for handling very
hierarchical chips, but I couldn't justify the added complexity when
most chips don't need it -- after all most chips behind slow busses
will have a small number of interrupts and a fairly flat structure.

In the end I went with Mark's suggestion to factor things out around
->get_irq_reg(). At first I thought there might be too many "gotchas"
that'd limit its usefulness, but in the end it proved to be a better
option and a lot easier to implement.

[PATCH v2 03/17] dt-bindings: mfd: add bindings for AXP192 MFD device

[Aidan MacDonald]

The AXP192 is another X-Powers PMIC similar to the existing ones.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
index 3a53bae611bc..33c9b1b3cc04 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -84,6 +84,7 @@ properties:
     oneOf:
       - enum:
           - x-powers,axp152
+          - x-powers,axp192
           - x-powers,axp202
           - x-powers,axp209
           - x-powers,axp221
-- 
2.35.1

Re: [PATCH v2 03/17] dt-bindings: mfd: add bindings for AXP192 MFD device

[Krzysztof Kozlowski]

On 07/06/2022 17:53, Aidan MacDonald wrote:
> The AXP192 is another X-Powers PMIC similar to the existing ones.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +

You got here ack, didn't you? Why sending without it?

https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/process/submitting-patches.rst#L536


Best regards,
Krzysztof

Re: [PATCH v2 03/17] dt-bindings: mfd: add bindings for AXP192 MFD device

[Aidan MacDonald]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 07/06/2022 17:53, Aidan MacDonald wrote:
>> The AXP192 is another X-Powers PMIC similar to the existing ones.
>> 
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +
>
> You got here ack, didn't you? Why sending without it?
>
> https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/process/submitting-patches.rst#L536
>
>
> Best regards,
> Krzysztof

I'm sorry, I'm pretty new to kernel development and I forgot. Will make
sure to add them in future. Thanks for pointing out my mistake.

[PATCH v2 04/17] dt-bindings: iio: adc: axp209: Add AXP192 compatible

[Aidan MacDonald]

The AXP192 is identical to the AXP20x, except for two additional
GPIO ADC channels.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 .../bindings/iio/adc/x-powers,axp209-adc.yaml  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml b/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
index d6d3d8590171..1a68e650ac7d 100644
--- a/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/x-powers,axp209-adc.yaml
@@ -14,6 +14,23 @@ description: |
   Device is a child of an axp209 multifunction device
   ADC channels and their indexes per variant:
 
+  AXP192
+  ------
+   0 | acin_v
+   1 | acin_i
+   2 | vbus_v
+   3 | vbus_i
+   4 | pmic_temp
+   5 | gpio0_v
+   6 | gpio1_v
+   7 | gpio2_v
+   8 | gpio3_v
+   9 | ipsout_v
+  10 | batt_v
+  11 | batt_chrg_i
+  12 | batt_dischrg_i
+  13 | ts_v
+
   AXP209
   ------
    0 | acin_v
@@ -50,6 +67,7 @@ description: |
 properties:
   compatible:
     oneOf:
+      - const: x-powers,axp192-adc
       - const: x-powers,axp209-adc
       - const: x-powers,axp221-adc
       - const: x-powers,axp813-adc
-- 
2.35.1

Re: [PATCH v2 04/17] dt-bindings: iio: adc: axp209: Add AXP192 compatible

[Krzysztof Kozlowski]

On 07/06/2022 17:53, Aidan MacDonald wrote:
> The AXP192 is identical to the AXP20x, except for two additional
> GPIO ADC channels.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Same problem...


Best regards,
Krzysztof

[PATCH v2 05/17] dt-bindings: power: supply: axp20x: Add AXP192 compatible

[Aidan MacDonald]

The AXP192's USB power supply is similar to the AXP202 but it has
different USB current limits.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 .../bindings/power/supply/x-powers,axp20x-usb-power-supply.yaml  | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-usb-power-supply.yaml b/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-usb-power-supply.yaml
index 0c371b55c9e1..e800b3b97f0d 100644
--- a/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-usb-power-supply.yaml
+++ b/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-usb-power-supply.yaml
@@ -22,6 +22,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - x-powers,axp192-usb-power-supply
           - x-powers,axp202-usb-power-supply
           - x-powers,axp221-usb-power-supply
           - x-powers,axp223-usb-power-supply
-- 
2.35.1

Re: [PATCH v2 05/17] dt-bindings: power: supply: axp20x: Add AXP192 compatible

[Krzysztof Kozlowski]

On 07/06/2022 17:53, Aidan MacDonald wrote:
> The AXP192's USB power supply is similar to the AXP202 but it has
> different USB current limits.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Please send a v3 with tags properly accumulated.


Best regards,
Krzysztof

[PATCH v2 06/17] dt-bindings: gpio: Add AXP192 GPIO bindings

[Aidan MacDonald]

The AXP192 PMIC is different enough from the PMICs supported by
the AXP20x GPIO driver to warrant a separate driver. The AXP192
driver also supports interrupts and pinconf settings.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 .../bindings/gpio/x-powers,axp192-gpio.yaml   | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml b/Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
new file mode 100644
index 000000000000..a5ba894383b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/gpio/x-powers,axp192-gpio.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: X-Powers AXP192 GPIO Device Tree Bindings
+
+maintainers:
+  - Chen-Yu Tsai <wens@csie.org>
+
+properties:
+  "#gpio-cells":
+    const: 2
+    description: >
+      The first cell is the pin number and the second is the GPIO flags.
+
+  compatible:
+    const: x-powers,axp192-gpio
+
+  gpio-controller: true
+
+patternProperties:
+  "-pins$":
+    $ref: /schemas/pinctrl/pinmux-node.yaml#
+
+    properties:
+      pins:
+        items:
+          enum:
+            - GPIO0
+            - GPIO1
+            - GPIO2
+            - GPIO3
+            - GPIO4
+            - N_RSTO
+
+      function:
+        enum:
+          - output
+          - input
+          - ldo
+          - pwm
+          - adc
+          - low_output
+          - floating
+          - ext_chg_ctl
+          - ldo_status
+
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+...
-- 
2.35.1

Re: [PATCH v2 06/17] dt-bindings: gpio: Add AXP192 GPIO bindings

[Rob Herring]

On Tue, 07 Jun 2022 16:53:13 +0100, Aidan MacDonald wrote:
> The AXP192 PMIC is different enough from the PMICs supported by
> the AXP20x GPIO driver to warrant a separate driver. The AXP192
> driver also supports interrupts and pinconf settings.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  .../bindings/gpio/x-powers,axp192-gpio.yaml   | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 07/17] dt-bindings: power: axp20x-battery: Add AXP192 compatible

[Aidan MacDonald]

The AXP192's battery charger is similar to the others supported by
the axp20x_battery driver.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 .../power/supply/x-powers,axp20x-battery-power-supply.yaml       | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml b/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml
index d055428ae39f..b7347683a07e 100644
--- a/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml
+++ b/Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml
@@ -20,6 +20,7 @@ allOf:
 properties:
   compatible:
     oneOf:
+      - const: x-powers,axp192-battery-power-supply
       - const: x-powers,axp202-battery-power-supply
       - const: x-powers,axp209-battery-power-supply
       - const: x-powers,axp221-battery-power-supply
-- 
2.35.1

Re: [PATCH v2 07/17] dt-bindings: power: axp20x-battery: Add AXP192 compatible

[Rob Herring]

On Tue, 07 Jun 2022 16:53:14 +0100, Aidan MacDonald wrote:
> The AXP192's battery charger is similar to the others supported by
> the axp20x_battery driver.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  .../power/supply/x-powers,axp20x-battery-power-supply.yaml       | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 08/17] mfd: axp20x: Add support for AXP192

[Aidan MacDonald]

The AXP192 PMIC is similar to the AXP202/AXP209, but with different
regulators, additional GPIOs, and a different IRQ register layout.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/mfd/axp20x-i2c.c   |   2 +
 drivers/mfd/axp20x.c       | 153 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h |  84 ++++++++++++++++++++
 3 files changed, 239 insertions(+)

diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
index 00ab48018d8d..9ada58fad77f 100644
--- a/drivers/mfd/axp20x-i2c.c
+++ b/drivers/mfd/axp20x-i2c.c
@@ -62,6 +62,7 @@ static int axp20x_i2c_remove(struct i2c_client *i2c)
 #ifdef CONFIG_OF
 static const struct of_device_id axp20x_i2c_of_match[] = {
 	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
+	{ .compatible = "x-powers,axp192", .data = (void *)AXP192_ID },
 	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
 	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
 	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
@@ -75,6 +76,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
 
 static const struct i2c_device_id axp20x_i2c_id[] = {
 	{ "axp152", 0 },
+	{ "axp192", 0 },
 	{ "axp202", 0 },
 	{ "axp209", 0 },
 	{ "axp221", 0 },
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 8161a5dc68e8..1011e668589a 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -34,6 +34,7 @@
 
 static const char * const axp20x_model_names[] = {
 	"AXP152",
+	"AXP192",
 	"AXP202",
 	"AXP209",
 	"AXP221",
@@ -92,6 +93,35 @@ static const struct regmap_access_table axp20x_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(axp20x_volatile_ranges),
 };
 
+static const struct regmap_range axp192_writeable_ranges[] = {
+	regmap_reg_range(AXP192_DATACACHE(0), AXP192_DATACACHE(5)),
+	regmap_reg_range(AXP192_PWR_OUT_CTRL, AXP192_IRQ5_STATE),
+	regmap_reg_range(AXP20X_DCDC_MODE, AXP192_N_RSTO_CTRL),
+	regmap_reg_range(AXP20X_CC_CTRL, AXP20X_CC_CTRL),
+};
+
+static const struct regmap_range axp192_volatile_ranges[] = {
+	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP192_USB_OTG_STATUS),
+	regmap_reg_range(AXP192_IRQ1_STATE, AXP192_IRQ4_STATE),
+	regmap_reg_range(AXP192_IRQ5_STATE, AXP192_IRQ5_STATE),
+	regmap_reg_range(AXP20X_ACIN_V_ADC_H, AXP20X_IPSOUT_V_HIGH_L),
+	regmap_reg_range(AXP20X_TIMER_CTRL, AXP20X_TIMER_CTRL),
+	regmap_reg_range(AXP192_GPIO2_0_STATE, AXP192_GPIO2_0_STATE),
+	regmap_reg_range(AXP192_GPIO4_3_STATE, AXP192_GPIO4_3_STATE),
+	regmap_reg_range(AXP192_N_RSTO_CTRL, AXP192_N_RSTO_CTRL),
+	regmap_reg_range(AXP20X_CHRG_CC_31_24, AXP20X_CC_CTRL),
+};
+
+static const struct regmap_access_table axp192_writeable_table = {
+	.yes_ranges	= axp192_writeable_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(axp192_writeable_ranges),
+};
+
+static const struct regmap_access_table axp192_volatile_table = {
+	.yes_ranges	= axp192_volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(axp192_volatile_ranges),
+};
+
 /* AXP22x ranges are shared with the AXP809, as they cover the same range */
 static const struct regmap_range axp22x_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
@@ -173,6 +203,25 @@ static const struct resource axp152_pek_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
 };
 
+static const struct resource axp192_gpio_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_GPIO0_INPUT, "GPIO0"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_GPIO1_INPUT, "GPIO1"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_GPIO2_INPUT, "GPIO2"),
+};
+
+static const struct resource axp192_ac_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
+};
+
+static const struct resource axp192_usb_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_VBUS_VALID, "VBUS_VALID"),
+	DEFINE_RES_IRQ_NAMED(AXP192_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
+};
+
 static const struct resource axp20x_ac_power_supply_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
 	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
@@ -245,6 +294,15 @@ static const struct regmap_config axp152_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config axp192_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.wr_table	= &axp192_writeable_table,
+	.volatile_table	= &axp192_volatile_table,
+	.max_register	= AXP20X_CC_CTRL,
+	.cache_type	= REGCACHE_RBTREE,
+};
+
 static const struct regmap_config axp20x_regmap_config = {
 	.reg_bits	= 8,
 	.val_bits	= 8,
@@ -304,6 +362,55 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
 };
 
+static const struct regmap_irq axp192_regmap_irqs[] = {
+	INIT_REGMAP_IRQ(AXP192, ACIN_OVER_V,		0, 7),
+	INIT_REGMAP_IRQ(AXP192, ACIN_PLUGIN,		0, 6),
+	INIT_REGMAP_IRQ(AXP192, ACIN_REMOVAL,		0, 5),
+	INIT_REGMAP_IRQ(AXP192, VBUS_OVER_V,		0, 4),
+	INIT_REGMAP_IRQ(AXP192, VBUS_PLUGIN,		0, 3),
+	INIT_REGMAP_IRQ(AXP192, VBUS_REMOVAL,		0, 2),
+	INIT_REGMAP_IRQ(AXP192, VBUS_V_LOW,		0, 1),
+	INIT_REGMAP_IRQ(AXP192, BATT_PLUGIN,		1, 7),
+	INIT_REGMAP_IRQ(AXP192, BATT_REMOVAL,	        1, 6),
+	INIT_REGMAP_IRQ(AXP192, BATT_ENT_ACT_MODE,	1, 5),
+	INIT_REGMAP_IRQ(AXP192, BATT_EXIT_ACT_MODE,	1, 4),
+	INIT_REGMAP_IRQ(AXP192, CHARG,		        1, 3),
+	INIT_REGMAP_IRQ(AXP192, CHARG_DONE,		1, 2),
+	INIT_REGMAP_IRQ(AXP192, BATT_TEMP_HIGH,	        1, 1),
+	INIT_REGMAP_IRQ(AXP192, BATT_TEMP_LOW,	        1, 0),
+	INIT_REGMAP_IRQ(AXP192, DIE_TEMP_HIGH,	        2, 7),
+	INIT_REGMAP_IRQ(AXP192, CHARG_I_LOW,		2, 6),
+	INIT_REGMAP_IRQ(AXP192, DCDC1_V_LONG,	        2, 5),
+	INIT_REGMAP_IRQ(AXP192, DCDC2_V_LONG,	        2, 4),
+	INIT_REGMAP_IRQ(AXP192, DCDC3_V_LONG,	        2, 3),
+	INIT_REGMAP_IRQ(AXP192, PEK_SHORT,		2, 1),
+	INIT_REGMAP_IRQ(AXP192, PEK_LONG,		2, 0),
+	INIT_REGMAP_IRQ(AXP192, N_OE_PWR_ON,		3, 7),
+	INIT_REGMAP_IRQ(AXP192, N_OE_PWR_OFF,	        3, 6),
+	INIT_REGMAP_IRQ(AXP192, VBUS_VALID,		3, 5),
+	INIT_REGMAP_IRQ(AXP192, VBUS_NOT_VALID,	        3, 4),
+	INIT_REGMAP_IRQ(AXP192, VBUS_SESS_VALID,	3, 3),
+	INIT_REGMAP_IRQ(AXP192, VBUS_SESS_END,	        3, 2),
+	INIT_REGMAP_IRQ(AXP192, LOW_PWR_LVL,	        3, 0),
+	INIT_REGMAP_IRQ(AXP192, TIMER,			4, 7),
+	INIT_REGMAP_IRQ(AXP192, GPIO2_INPUT,		4, 2),
+	INIT_REGMAP_IRQ(AXP192, GPIO1_INPUT,		4, 1),
+	INIT_REGMAP_IRQ(AXP192, GPIO0_INPUT,		4, 0),
+};
+
+static int axp192_get_irq_reg(unsigned int base_reg, int i)
+{
+	/* linear mapping for IRQ1 to IRQ4 */
+	if (i < 4)
+		return base_reg + i;
+
+	/* handle IRQ5 separately */
+	if (base_reg == AXP192_IRQ1_EN)
+		return AXP192_IRQ5_EN;
+	else
+		return AXP192_IRQ5_STATE;
+}
+
 static const struct regmap_irq axp20x_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
 	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
@@ -514,6 +621,19 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
 	.num_regs		= 3,
 };
 
+static const struct regmap_irq_chip axp192_regmap_irq_chip = {
+	.name			= "axp192_irq_chip",
+	.status_base		= AXP192_IRQ1_STATE,
+	.ack_base		= AXP192_IRQ1_STATE,
+	.mask_base		= AXP192_IRQ1_EN,
+	.mask_invert		= true,
+	.init_ack_masked	= true,
+	.irqs			= axp192_regmap_irqs,
+	.num_irqs		= ARRAY_SIZE(axp192_regmap_irqs),
+	.num_regs		= 5,
+	.get_irq_reg		= axp192_get_irq_reg,
+};
+
 static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
 	.name			= "axp20x_irq_chip",
 	.status_base		= AXP20X_IRQ1_STATE,
@@ -588,6 +708,33 @@ static const struct regmap_irq_chip axp809_regmap_irq_chip = {
 	.num_regs		= 5,
 };
 
+static const struct mfd_cell axp192_cells[] = {
+	{
+		.name		= "axp192-gpio",
+		.of_compatible	= "x-powers,axp192-gpio",
+		.num_resources	= ARRAY_SIZE(axp192_gpio_resources),
+		.resources	= axp192_gpio_resources,
+	}, {
+		.name		= "axp20x-regulator",
+	}, {
+		.name		= "axp192-adc",
+		.of_compatible	= "x-powers,axp192-adc",
+	}, {
+		.name		= "axp20x-battery-power-supply",
+		.of_compatible	= "x-powers,axp192-battery-power-supply",
+	}, {
+		.name		= "axp20x-ac-power-supply",
+		.of_compatible	= "x-powers,axp202-ac-power-supply",
+		.num_resources	= ARRAY_SIZE(axp192_ac_power_supply_resources),
+		.resources	= axp192_ac_power_supply_resources,
+	}, {
+		.name		= "axp20x-usb-power-supply",
+		.of_compatible	= "x-powers,axp192-usb-power-supply",
+		.num_resources	= ARRAY_SIZE(axp192_usb_power_supply_resources),
+		.resources	= axp192_usb_power_supply_resources,
+	}
+};
+
 static const struct mfd_cell axp20x_cells[] = {
 	{
 		.name		= "axp20x-gpio",
@@ -865,6 +1012,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_cfg = &axp152_regmap_config;
 		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
 		break;
+	case AXP192_ID:
+		axp20x->nr_cells = ARRAY_SIZE(axp192_cells);
+		axp20x->cells = axp192_cells;
+		axp20x->regmap_cfg = &axp192_regmap_config;
+		axp20x->regmap_irq_chip = &axp192_regmap_irq_chip;
+		break;
 	case AXP202_ID:
 	case AXP209_ID:
 		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 9ab0e2fca7ea..18546e124919 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -12,6 +12,7 @@
 
 enum axp20x_variants {
 	AXP152_ID = 0,
+	AXP192_ID,
 	AXP202_ID,
 	AXP209_ID,
 	AXP221_ID,
@@ -24,6 +25,7 @@ enum axp20x_variants {
 	NR_AXP20X_VARIANTS,
 };
 
+#define AXP192_DATACACHE(m)		(0x06 + (m))
 #define AXP20X_DATACACHE(m)		(0x04 + (m))
 
 /* Power supply */
@@ -45,6 +47,13 @@ enum axp20x_variants {
 #define AXP152_DCDC_FREQ		0x37
 #define AXP152_DCDC_MODE		0x80
 
+#define AXP192_USB_OTG_STATUS		0x04
+#define AXP192_PWR_OUT_CTRL		0x12
+#define AXP192_DCDC2_V_OUT		0x23
+#define AXP192_DCDC1_V_OUT		0x26
+#define AXP192_DCDC3_V_OUT		0x27
+#define AXP192_LDO2_3_V_OUT		0x28
+
 #define AXP20X_PWR_INPUT_STATUS		0x00
 #define AXP20X_PWR_OP_MODE		0x01
 #define AXP20X_USB_OTG_STATUS		0x02
@@ -139,6 +148,17 @@ enum axp20x_variants {
 #define AXP152_IRQ2_STATE		0x49
 #define AXP152_IRQ3_STATE		0x4a
 
+#define AXP192_IRQ1_EN			0x40
+#define AXP192_IRQ2_EN			0x41
+#define AXP192_IRQ3_EN			0x42
+#define AXP192_IRQ4_EN			0x43
+#define AXP192_IRQ1_STATE		0x44
+#define AXP192_IRQ2_STATE		0x45
+#define AXP192_IRQ3_STATE		0x46
+#define AXP192_IRQ4_STATE		0x47
+#define AXP192_IRQ5_EN			0x4a
+#define AXP192_IRQ5_STATE		0x4d
+
 #define AXP20X_IRQ1_EN			0x40
 #define AXP20X_IRQ2_EN			0x41
 #define AXP20X_IRQ3_EN			0x42
@@ -153,6 +173,11 @@ enum axp20x_variants {
 #define AXP20X_IRQ6_STATE		0x4d
 
 /* ADC */
+#define AXP192_GPIO2_V_ADC_H		0x68
+#define AXP192_GPIO2_V_ADC_L		0x69
+#define AXP192_GPIO3_V_ADC_H		0x6a
+#define AXP192_GPIO3_V_ADC_L		0x6b
+
 #define AXP20X_ACIN_V_ADC_H		0x56
 #define AXP20X_ACIN_V_ADC_L		0x57
 #define AXP20X_ACIN_I_ADC_H		0x58
@@ -182,6 +207,8 @@ enum axp20x_variants {
 #define AXP20X_IPSOUT_V_HIGH_L		0x7f
 
 /* Power supply */
+#define AXP192_GPIO30_IN_RANGE		0x85
+
 #define AXP20X_DCDC_MODE		0x80
 #define AXP20X_ADC_EN1			0x82
 #define AXP20X_ADC_EN2			0x83
@@ -210,6 +237,16 @@ enum axp20x_variants {
 #define AXP152_PWM1_FREQ_Y		0x9c
 #define AXP152_PWM1_DUTY_CYCLE		0x9d
 
+#define AXP192_GPIO0_CTRL		0x90
+#define AXP192_LDO_IO0_V_OUT		0x91
+#define AXP192_GPIO1_CTRL		0x92
+#define AXP192_GPIO2_CTRL		0x93
+#define AXP192_GPIO2_0_STATE		0x94
+#define AXP192_GPIO4_3_CTRL		0x95
+#define AXP192_GPIO4_3_STATE		0x96
+#define AXP192_GPIO2_0_PULL		0x97
+#define AXP192_N_RSTO_CTRL		0x9e
+
 #define AXP20X_GPIO0_CTRL		0x90
 #define AXP20X_LDO5_V_OUT		0x91
 #define AXP20X_GPIO1_CTRL		0x92
@@ -287,6 +324,17 @@ enum axp20x_variants {
 #define AXP288_FG_TUNE5             0xed
 
 /* Regulators IDs */
+enum {
+	AXP192_DCDC1 = 0,
+	AXP192_DCDC2,
+	AXP192_DCDC3,
+	AXP192_LDO1,
+	AXP192_LDO2,
+	AXP192_LDO3,
+	AXP192_LDO_IO0,
+	AXP192_REG_ID_MAX,
+};
+
 enum {
 	AXP20X_LDO1 = 0,
 	AXP20X_LDO2,
@@ -440,6 +488,42 @@ enum {
 	AXP152_IRQ_GPIO0_INPUT,
 };
 
+enum axp192_irqs {
+	AXP192_IRQ_ACIN_OVER_V = 1,
+	AXP192_IRQ_ACIN_PLUGIN,
+	AXP192_IRQ_ACIN_REMOVAL,
+	AXP192_IRQ_VBUS_OVER_V,
+	AXP192_IRQ_VBUS_PLUGIN,
+	AXP192_IRQ_VBUS_REMOVAL,
+	AXP192_IRQ_VBUS_V_LOW,
+	AXP192_IRQ_BATT_PLUGIN,
+	AXP192_IRQ_BATT_REMOVAL,
+	AXP192_IRQ_BATT_ENT_ACT_MODE,
+	AXP192_IRQ_BATT_EXIT_ACT_MODE,
+	AXP192_IRQ_CHARG,
+	AXP192_IRQ_CHARG_DONE,
+	AXP192_IRQ_BATT_TEMP_HIGH,
+	AXP192_IRQ_BATT_TEMP_LOW,
+	AXP192_IRQ_DIE_TEMP_HIGH,
+	AXP192_IRQ_CHARG_I_LOW,
+	AXP192_IRQ_DCDC1_V_LONG,
+	AXP192_IRQ_DCDC2_V_LONG,
+	AXP192_IRQ_DCDC3_V_LONG,
+	AXP192_IRQ_PEK_SHORT = 22,
+	AXP192_IRQ_PEK_LONG,
+	AXP192_IRQ_N_OE_PWR_ON,
+	AXP192_IRQ_N_OE_PWR_OFF,
+	AXP192_IRQ_VBUS_VALID,
+	AXP192_IRQ_VBUS_NOT_VALID,
+	AXP192_IRQ_VBUS_SESS_VALID,
+	AXP192_IRQ_VBUS_SESS_END,
+	AXP192_IRQ_LOW_PWR_LVL = 31,
+	AXP192_IRQ_TIMER,
+	AXP192_IRQ_GPIO2_INPUT = 37,
+	AXP192_IRQ_GPIO1_INPUT,
+	AXP192_IRQ_GPIO0_INPUT,
+};
+
 enum {
 	AXP20X_IRQ_ACIN_OVER_V = 1,
 	AXP20X_IRQ_ACIN_PLUGIN,
-- 
2.35.1

[PATCH v2 09/17] regulator: axp20x: Add support for AXP192

[Aidan MacDonald]

Add support for the AXP192 PMIC.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/regulator/axp20x-regulator.c | 101 ++++++++++++++++++++++++---
 1 file changed, 92 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d260c442b788..1edf2bbf1c16 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -27,6 +27,29 @@
 #include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
 
+#define AXP192_GPIO0_FUNC_MASK		GENMASK(2, 0)
+
+#define AXP192_IO_ENABLED		0x02
+#define AXP192_IO_DISABLED		0x06
+
+#define AXP192_WORKMODE_DCDC1_MASK	BIT_MASK(3)
+#define AXP192_WORKMODE_DCDC2_MASK	BIT_MASK(2)
+#define AXP192_WORKMODE_DCDC3_MASK	BIT_MASK(1)
+
+#define AXP192_DCDC1_V_OUT_MASK		GENMASK(6, 0)
+#define AXP192_DCDC2_V_OUT_MASK		GENMASK(5, 0)
+#define AXP192_DCDC3_V_OUT_MASK		GENMASK(6, 0)
+#define AXP192_LDO2_V_OUT_MASK		GENMASK(7, 4)
+#define AXP192_LDO3_V_OUT_MASK		GENMASK(3, 0)
+#define AXP192_LDO_IO0_V_OUT_MASK	GENMASK(7, 4)
+
+#define AXP192_PWR_OUT_EXTEN_MASK	BIT_MASK(6)
+#define AXP192_PWR_OUT_DCDC2_MASK	BIT_MASK(4)
+#define AXP192_PWR_OUT_LDO3_MASK	BIT_MASK(3)
+#define AXP192_PWR_OUT_LDO2_MASK	BIT_MASK(2)
+#define AXP192_PWR_OUT_DCDC3_MASK	BIT_MASK(1)
+#define AXP192_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
+
 #define AXP20X_GPIO0_FUNC_MASK		GENMASK(3, 0)
 #define AXP20X_GPIO1_FUNC_MASK		GENMASK(3, 0)
 
@@ -375,25 +398,32 @@ static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
 
 	switch (axp20x->variant) {
 	case AXP209_ID:
-		if (id == AXP20X_DCDC2) {
+		if (id == AXP20X_LDO3) {
 			slew_rates = axp209_dcdc2_ldo3_slew_rates;
 			rate_count = ARRAY_SIZE(axp209_dcdc2_ldo3_slew_rates);
 			reg = AXP20X_DCDC2_LDO3_V_RAMP;
-			mask = AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_RATE_MASK |
-			       AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN_MASK;
+			mask = AXP20X_DCDC2_LDO3_V_RAMP_LDO3_RATE_MASK |
+			       AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN_MASK;
 			enable = (ramp > 0) ?
-				 AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN : 0;
+				 AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN : 0;
 			break;
 		}
 
-		if (id == AXP20X_LDO3) {
+		fallthrough;
+
+	case AXP192_ID:
+		/*
+		 * AXP192 and AXP209 share the same DCDC2 ramp configuration
+		 */
+		if ((axp20x->variant == AXP209_ID && id == AXP20X_DCDC2) ||
+		    (axp20x->variant == AXP192_ID && id == AXP20X_DCDC2)) {
 			slew_rates = axp209_dcdc2_ldo3_slew_rates;
 			rate_count = ARRAY_SIZE(axp209_dcdc2_ldo3_slew_rates);
 			reg = AXP20X_DCDC2_LDO3_V_RAMP;
-			mask = AXP20X_DCDC2_LDO3_V_RAMP_LDO3_RATE_MASK |
-			       AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN_MASK;
+			mask = AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_RATE_MASK |
+			       AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN_MASK;
 			enable = (ramp > 0) ?
-				 AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN : 0;
+				 AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN : 0;
 			break;
 		}
 
@@ -401,6 +431,7 @@ static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
 			break;
 
 		fallthrough;
+
 	default:
 		/* Not supported for this regulator */
 		return -ENOTSUPP;
@@ -415,7 +446,8 @@ static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
 			if (ramp > slew_rates[i])
 				break;
 
-			if (id == AXP20X_DCDC2)
+			if ((axp20x->variant == AXP209_ID && id == AXP20X_DCDC2) ||
+			    (axp20x->variant == AXP192_ID && id == AXP192_DCDC2))
 				cfg = AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_RATE(i);
 			else
 				cfg = AXP20X_DCDC2_LDO3_V_RAMP_LDO3_RATE(i);
@@ -511,6 +543,29 @@ static const struct regulator_ops axp20x_ops_sw = {
 	.is_enabled		= regulator_is_enabled_regmap,
 };
 
+static const struct regulator_desc axp192_regulators[] = {
+	AXP_DESC(AXP192, DCDC1, "dcdc1", "vin1", 700, 3500, 25,
+		 AXP192_DCDC1_V_OUT, AXP192_DCDC1_V_OUT_MASK,
+		 AXP192_PWR_OUT_CTRL, AXP192_PWR_OUT_DCDC1_MASK),
+	AXP_DESC(AXP192, DCDC2, "dcdc2", "vin2", 700, 2275, 25,
+		 AXP192_DCDC2_V_OUT, AXP192_DCDC2_V_OUT_MASK,
+		 AXP192_PWR_OUT_CTRL, AXP192_PWR_OUT_DCDC2_MASK),
+	AXP_DESC(AXP192, DCDC3, "dcdc3", "vin3", 700, 3500, 25,
+		 AXP192_DCDC3_V_OUT, AXP192_DCDC3_V_OUT_MASK,
+		 AXP192_PWR_OUT_CTRL, AXP192_PWR_OUT_DCDC3_MASK),
+	AXP_DESC_FIXED(AXP192, LDO1, "ldo1", "acin", 1250),
+	AXP_DESC(AXP192, LDO2, "ldo2", "ldoin", 1800, 3300, 100,
+		 AXP192_LDO2_3_V_OUT, AXP192_LDO2_V_OUT_MASK,
+		 AXP192_PWR_OUT_CTRL, AXP192_PWR_OUT_LDO2_MASK),
+	AXP_DESC(AXP192, LDO3, "ldo3", "ldoin", 1800, 3300, 100,
+		 AXP192_LDO2_3_V_OUT, AXP192_LDO3_V_OUT_MASK,
+		 AXP192_PWR_OUT_CTRL, AXP192_PWR_OUT_LDO3_MASK),
+	AXP_DESC_IO(AXP192, LDO_IO0, "ldo_io0", "ips", 700, 3300, 100,
+		    AXP192_LDO_IO0_V_OUT, AXP192_LDO_IO0_V_OUT_MASK,
+		    AXP192_GPIO0_CTRL, AXP192_GPIO0_FUNC_MASK,
+		    AXP192_IO_ENABLED, AXP192_IO_DISABLED),
+};
+
 static const struct linear_range axp20x_ldo4_ranges[] = {
 	REGULATOR_LINEAR_RANGE(1250000,
 			       AXP20X_LDO4_V_OUT_1250mV_START,
@@ -1008,6 +1063,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 	u32 min, max, def, step;
 
 	switch (axp20x->variant) {
+	case AXP192_ID:
+		min = 900;
+		max = 2025;
+		def = 1500;
+		step = 75;
+		break;
 	case AXP202_ID:
 	case AXP209_ID:
 		min = 750;
@@ -1100,6 +1161,24 @@ static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 work
 	unsigned int mask;
 
 	switch (axp20x->variant) {
+	case AXP192_ID:
+		switch (id) {
+		case AXP192_DCDC1:
+			mask = AXP192_WORKMODE_DCDC1_MASK;
+			break;
+		case AXP192_DCDC2:
+			mask = AXP192_WORKMODE_DCDC2_MASK;
+			break;
+		case AXP192_DCDC3:
+			mask = AXP192_WORKMODE_DCDC3_MASK;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		workmode <<= ffs(mask) - 1;
+		break;
+
 	case AXP202_ID:
 	case AXP209_ID:
 		if ((id != AXP20X_DCDC2) && (id != AXP20X_DCDC3))
@@ -1220,6 +1299,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	bool drivevbus = false;
 
 	switch (axp20x->variant) {
+	case AXP192_ID:
+		regulators = axp192_regulators;
+		nregulators = AXP192_REG_ID_MAX;
+		break;
 	case AXP202_ID:
 	case AXP209_ID:
 		regulators = axp20x_regulators;
-- 
2.35.1

[PATCH v2 10/17] iio: adc: axp20x_adc: Minor code cleanups

[Aidan MacDonald]

The code may be clearer if parameters are not re-purposed to hold
temporary results like register values, so introduce local variables
as necessary to avoid that. Also, use the common FIELD_PREP macro
instead of a hand-rolled version.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 53bf7d4899d2..9d5b1de24908 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -15,6 +15,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/thermal.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
@@ -22,20 +23,20 @@
 #include <linux/mfd/axp20x.h>
 
 #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
-
 #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
+
 #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
 
 #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
 #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
-#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
-#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
 
 #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
-#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
-#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
 #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
+
 #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
+
+#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
+#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
 #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
 #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
 #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
@@ -234,7 +235,7 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	int size = 12;
+	int ret, size;
 
 	/*
 	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
@@ -246,10 +247,11 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	else
 		size = 12;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -257,11 +259,13 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	int ret;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -269,11 +273,13 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	int ret;
 
-	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (*val < 0)
-		return *val;
+	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
+	if (ret < 0)
+		return ret;
 
+	*val = ret;
 	return IIO_VAL_INT;
 }
 
@@ -443,27 +449,27 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
 				     int *val)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	unsigned int regval;
 	int ret;
 
-	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
+	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
 	if (ret < 0)
 		return ret;
 
 	switch (channel) {
 	case AXP20X_GPIO0_V:
-		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
+		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;
 		break;
 
 	case AXP20X_GPIO1_V:
-		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
+		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	*val = *val ? 700000 : 0;
-
+	*val = regval ? 700000 : 0;
 	return IIO_VAL_INT;
 }
 
@@ -548,7 +554,7 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	unsigned int reg, regval;
+	unsigned int regmask, regval;
 
 	/*
 	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
@@ -560,25 +566,24 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
 	if (val != 0 && val != 700000)
 		return -EINVAL;
 
-	val = val ? 1 : 0;
+	regval = val ? 1 : 0;
 
 	switch (chan->channel) {
 	case AXP20X_GPIO0_V:
-		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
-		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
+		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
+		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
 		break;
 
 	case AXP20X_GPIO1_V:
-		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
-		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
+		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
+		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
-				  regval);
+	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
 }
 
 static const struct iio_info axp20x_adc_iio_info = {
-- 
2.35.1

Re: [PATCH v2 10/17] iio: adc: axp20x_adc: Minor code cleanups

[Jonathan Cameron]

On Tue,  7 Jun 2022 16:53:17 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> The code may be clearer if parameters are not re-purposed to hold
> temporary results like register values, so introduce local variables
> as necessary to avoid that. Also, use the common FIELD_PREP macro
> instead of a hand-rolled version.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Hi Aidan,

Looks good.  One trivial further suggestion inline.

Also, am I fine picking up the IIO patches, or does the whole
set need to go in via mfd?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 53bf7d4899d2..9d5b1de24908 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -15,6 +15,7 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/thermal.h>
> +#include <linux/bitfield.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/driver.h>
> @@ -22,20 +23,20 @@
>  #include <linux/mfd/axp20x.h>
>  
>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
> -
>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
> +
>  #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
>  
>  #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
>  #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
> -#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
> -#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>  
>  #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
> -#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
> -#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>  #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
> +
>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
> +
> +#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
> +#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>  #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
>  #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
>  #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
> @@ -234,7 +235,7 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int size = 12;
> +	int ret, size;
>  
>  	/*
>  	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> @@ -246,10 +247,11 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	else
>  		size = 12;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -257,11 +259,13 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -269,11 +273,13 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret;
>  
> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (*val < 0)
> -		return *val;
> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> +	if (ret < 0)
> +		return ret;
>  
> +	*val = ret;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -443,27 +449,27 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>  				     int *val)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	unsigned int regval;
>  	int ret;
>  
> -	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
> +	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
>  	if (ret < 0)
>  		return ret;
>  
>  	switch (channel) {
>  	case AXP20X_GPIO0_V:
> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;

Maybe use FIELD_GET() here to be clear you are extracting that
field (even though we don't care about the shift).

Hopefully the compiler will be clever enough to remove the shift
anyway and using FIELD_GET() would act as slightly more 'documentation
in code'.



>  		break;
>  
>  	case AXP20X_GPIO1_V:
> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	*val = *val ? 700000 : 0;
> -
> +	*val = regval ? 700000 : 0;
>  	return IIO_VAL_INT;
>  }
>  
> @@ -548,7 +554,7 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	unsigned int reg, regval;
> +	unsigned int regmask, regval;
>  
>  	/*
>  	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
> @@ -560,25 +566,24 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>  	if (val != 0 && val != 700000)
>  		return -EINVAL;
>  
> -	val = val ? 1 : 0;
> +	regval = val ? 1 : 0;
>  
>  	switch (chan->channel) {
>  	case AXP20X_GPIO0_V:
> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
>  		break;
>  
>  	case AXP20X_GPIO1_V:
> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
> -				  regval);
> +	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
>  }
>  
>  static const struct iio_info axp20x_adc_iio_info = {

Re: [PATCH v2 10/17] iio: adc: axp20x_adc: Minor code cleanups

[Aidan MacDonald]

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Tue,  7 Jun 2022 16:53:17 +0100
> Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>
>> The code may be clearer if parameters are not re-purposed to hold
>> temporary results like register values, so introduce local variables
>> as necessary to avoid that. Also, use the common FIELD_PREP macro
>> instead of a hand-rolled version.
>> 
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>
> Hi Aidan,
>
> Looks good.  One trivial further suggestion inline.
>
> Also, am I fine picking up the IIO patches, or does the whole
> set need to go in via mfd?
>
> Thanks,
>
> Jonathan
>

I think it has to go through mfd because of the GPIO2-3 ADC registers
which are added in the mfd patch. Cleanups are okay to pick up though.

>> ---
>>  drivers/iio/adc/axp20x_adc.c | 61 +++++++++++++++++++-----------------
>>  1 file changed, 33 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 53bf7d4899d2..9d5b1de24908 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/property.h>
>>  #include <linux/regmap.h>
>>  #include <linux/thermal.h>
>> +#include <linux/bitfield.h>
>>  
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/driver.h>
>> @@ -22,20 +23,20 @@
>>  #include <linux/mfd/axp20x.h>
>>  
>>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
>> -
>>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
>> +
>>  #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
>>  
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
>> -#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
>> -#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>>  
>>  #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
>> -#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
>> -#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
>> +
>>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
>> +
>> +#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
>> +#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
>>  #define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
>>  #define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
>> @@ -234,7 +235,7 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	int size = 12;
>> +	int ret, size;
>>  
>>  	/*
>>  	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
>> @@ -246,10 +247,11 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  	else
>>  		size = 12;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, size);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -257,11 +259,13 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	int ret;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -269,11 +273,13 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>>  			  struct iio_chan_spec const *chan, int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	int ret;
>>  
>> -	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (*val < 0)
>> -		return *val;
>> +	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> +	*val = ret;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -443,27 +449,27 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>>  				     int *val)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	unsigned int regval;
>>  	int ret;
>>  
>> -	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, val);
>> +	ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &regval);
>>  	if (ret < 0)
>>  		return ret;
>>  
>>  	switch (channel) {
>>  	case AXP20X_GPIO0_V:
>> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO0;
>> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO0;
>
> Maybe use FIELD_GET() here to be clear you are extracting that
> field (even though we don't care about the shift).
>
> Hopefully the compiler will be clever enough to remove the shift
> anyway and using FIELD_GET() would act as slightly more 'documentation
> in code'.
>

You're probably right; I erred on the side of premature optimization.
I'll go back to FIELD_GET in the v3 since I've got to resend anyway.

>>  		break;
>>  
>>  	case AXP20X_GPIO1_V:
>> -		*val &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>> +		regval &= AXP20X_GPIO10_IN_RANGE_GPIO1;
>>  		break;
>>  
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  
>> -	*val = *val ? 700000 : 0;
>> -
>> +	*val = regval ? 700000 : 0;
>>  	return IIO_VAL_INT;
>>  }
>>  
>> @@ -548,7 +554,7 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  			    long mask)
>>  {
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	unsigned int reg, regval;
>> +	unsigned int regmask, regval;
>>  
>>  	/*
>>  	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
>> @@ -560,25 +566,24 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  	if (val != 0 && val != 700000)
>>  		return -EINVAL;
>>  
>> -	val = val ? 1 : 0;
>> +	regval = val ? 1 : 0;
>>  
>>  	switch (chan->channel) {
>>  	case AXP20X_GPIO0_V:
>> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO0;
>> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(val);
>> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO0;
>> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO0, regval);
>>  		break;
>>  
>>  	case AXP20X_GPIO1_V:
>> -		reg = AXP20X_GPIO10_IN_RANGE_GPIO1;
>> -		regval = AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(val);
>> +		regmask = AXP20X_GPIO10_IN_RANGE_GPIO1;
>> +		regval = FIELD_PREP(AXP20X_GPIO10_IN_RANGE_GPIO1, regval);
>>  		break;
>>  
>>  	default:
>>  		return -EINVAL;
>>  	}
>>  
>> -	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, reg,
>> -				  regval);
>> +	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
>>  }
>>  
>>  static const struct iio_info axp20x_adc_iio_info = {

[PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Aidan MacDonald]

Add an axp20x_id variant field to the axp_data struct and use it
to consolidate the adc_raw functions, reducing code duplication.
Variant IDs are chosen to match the OF compatible strings.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
 1 file changed, 34 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 9d5b1de24908..0260433782d8 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -71,6 +71,18 @@ struct axp20x_adc_iio {
 	const struct axp_data	*data;
 };
 
+struct axp_data {
+	const struct iio_info		*iio_info;
+	int				num_channels;
+	struct iio_chan_spec const	*channels;
+	unsigned long			adc_en1_mask;
+	unsigned long			adc_en2_mask;
+	int				(*adc_rate)(struct axp20x_adc_iio *info,
+						    int rate);
+	struct iio_map			*maps;
+	enum axp20x_variants		axp20x_id;
+};
+
 enum axp20x_adc_channel_v {
 	AXP20X_ACIN_V = 0,
 	AXP20X_VBUS_V,
@@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	struct axp20x_adc_iio *info = iio_priv(indio_dev);
 	int ret, size;
 
-	/*
-	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
-	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
-	 * bits.
-	 */
-	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
-		size = 13;
-	else
+	switch (info->data->axp20x_id) {
+	case AXP202_ID:
+	case AXP209_ID:
+		/*
+		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
+		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
+		 * bits.
+		 */
+		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
+			size = 13;
+		else
+			size = 12;
+		break;
+
+	default:
 		size = 12;
+		break;
+	}
 
 	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
 	if (ret < 0)
@@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
-static int axp22x_adc_raw(struct iio_dev *indio_dev,
-			  struct iio_chan_spec const *chan, int *val)
-{
-	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	int ret;
-
-	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (ret < 0)
-		return ret;
-
-	*val = ret;
-	return IIO_VAL_INT;
-}
-
-static int axp813_adc_raw(struct iio_dev *indio_dev,
-			  struct iio_chan_spec const *chan, int *val)
-{
-	struct axp20x_adc_iio *info = iio_priv(indio_dev);
-	int ret;
-
-	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
-	if (ret < 0)
-		return ret;
-
-	*val = ret;
-	return IIO_VAL_INT;
-}
-
 static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
 {
 	switch (channel) {
@@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
 		return axp22x_adc_scale(chan, val, val2);
 
 	case IIO_CHAN_INFO_RAW:
-		return axp22x_adc_raw(indio_dev, chan, val);
+		return axp20x_adc_raw(indio_dev, chan, val);
 
 	default:
 		return -EINVAL;
@@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
 		return axp813_adc_scale(chan, val, val2);
 
 	case IIO_CHAN_INFO_RAW:
-		return axp813_adc_raw(indio_dev, chan, val);
+		return axp20x_adc_raw(indio_dev, chan, val);
 
 	default:
 		return -EINVAL;
@@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
 				 AXP813_ADC_RATE_HZ(rate));
 }
 
-struct axp_data {
-	const struct iio_info		*iio_info;
-	int				num_channels;
-	struct iio_chan_spec const	*channels;
-	unsigned long			adc_en1_mask;
-	int				(*adc_rate)(struct axp20x_adc_iio *info,
-						    int rate);
-	bool				adc_en2;
-	struct iio_map			*maps;
-};
-
 static const struct axp_data axp20x_data = {
 	.iio_info = &axp20x_adc_iio_info,
 	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
@@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
 	.adc_rate = axp20x_adc_rate,
 	.adc_en2 = true,
 	.maps = axp20x_maps,
+	.axp20x_id = AXP209_ID,
 };
 
 static const struct axp_data axp22x_data = {
@@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
 	.adc_rate = axp22x_adc_rate,
 	.adc_en2 = false,
 	.maps = axp22x_maps,
+	.axp20x_id = AXP221_ID,
 };
 
 static const struct axp_data axp813_data = {
@@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
 	.adc_rate = axp813_adc_rate,
 	.adc_en2 = false,
 	.maps = axp22x_maps,
+	.axp20x_id = AXP813_ID,
 };
 
 static const struct of_device_id axp20x_adc_of_match[] = {
-- 
2.35.1

Re: [PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Jonathan Cameron]

On Tue,  7 Jun 2022 16:53:18 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> Add an axp20x_id variant field to the axp_data struct and use it
> to consolidate the adc_raw functions, reducing code duplication.
> Variant IDs are chosen to match the OF compatible strings.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Hi Aidan,

I'm not a big fan of using variant IDs, rather than a description
of what is actually different between devices.  Long term, variant
IDs tend to scale (as we add more supported devices) much worse
than a flag describing the actual difference.

Here I would have a field in struct axp_data called something like
discharge_curr_res and set it to 12 or 13 as appropriate.

> ---
>  drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 9d5b1de24908..0260433782d8 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -71,6 +71,18 @@ struct axp20x_adc_iio {
>  	const struct axp_data	*data;
>  };
>  
> +struct axp_data {
> +	const struct iio_info		*iio_info;
> +	int				num_channels;
> +	struct iio_chan_spec const	*channels;
> +	unsigned long			adc_en1_mask;
> +	unsigned long			adc_en2_mask;
> +	int				(*adc_rate)(struct axp20x_adc_iio *info,
> +						    int rate);
> +	struct iio_map			*maps;
> +	enum axp20x_variants		axp20x_id;
> +};
> +
>  enum axp20x_adc_channel_v {
>  	AXP20X_ACIN_V = 0,
>  	AXP20X_VBUS_V,
> @@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>  	int ret, size;
>  
> -	/*
> -	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> -	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> -	 * bits.
> -	 */
> -	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
> -		size = 13;
> -	else
> +	switch (info->data->axp20x_id) {
> +	case AXP202_ID:
> +	case AXP209_ID:
> +		/*
> +		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> +		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> +		 * bits.
> +		 */
> +		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)

This line is getting a bit long, break it after the &&

> +			size = 13;
> +		else
> +			size = 12;
> +		break;
> +
> +	default:
>  		size = 12;
> +		break;
> +	}
>  
>  	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>  	if (ret < 0)
> @@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> -static int axp22x_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
> -static int axp813_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
>  		return axp22x_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp22x_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>  		return axp813_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp813_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
>  				 AXP813_ADC_RATE_HZ(rate));
>  }
>  
> -struct axp_data {
> -	const struct iio_info		*iio_info;
> -	int				num_channels;
> -	struct iio_chan_spec const	*channels;
> -	unsigned long			adc_en1_mask;
> -	int				(*adc_rate)(struct axp20x_adc_iio *info,
> -						    int rate);
> -	bool				adc_en2;
> -	struct iio_map			*maps;
> -};
> -
>  static const struct axp_data axp20x_data = {
>  	.iio_info = &axp20x_adc_iio_info,
>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
> @@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
>  	.adc_rate = axp20x_adc_rate,
>  	.adc_en2 = true,
>  	.maps = axp20x_maps,
> +	.axp20x_id = AXP209_ID,
>  };
>  
>  static const struct axp_data axp22x_data = {
> @@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
>  	.adc_rate = axp22x_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP221_ID,
>  };
>  
>  static const struct axp_data axp813_data = {
> @@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
>  	.adc_rate = axp813_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP813_ID,
>  };
>  
>  static const struct of_device_id axp20x_adc_of_match[] = {

Re: [PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Aidan MacDonald]

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Tue,  7 Jun 2022 16:53:18 +0100
> Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>
>> Add an axp20x_id variant field to the axp_data struct and use it
>> to consolidate the adc_raw functions, reducing code duplication.
>> Variant IDs are chosen to match the OF compatible strings.
>> 
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>
> Hi Aidan,
>
> I'm not a big fan of using variant IDs, rather than a description
> of what is actually different between devices.  Long term, variant
> IDs tend to scale (as we add more supported devices) much worse
> than a flag describing the actual difference.
>
> Here I would have a field in struct axp_data called something like
> discharge_curr_res and set it to 12 or 13 as appropriate.
>

I agree with your point in general, but here it seems impossible to get
away from variant IDs because the channel numbering depends on it and we
use the channel number to decide what number of bits to use. The code
I'm replacing is just disguising the variant IDs by giving every variant
its own set of functions.

To me it seemed clearer to describe the channel properties and then use
one read_raw function for all variants, but when I did that it turned
out not to make any difference in size for x86. Probably because tables
encode a lot of redundant information compared to switches. It also
relied on a hack to associate extra info with an iio_chan_spec so it
wasn't much of an improvement, in the end.

So it's a question of using a variant ID explicitly or having separate
functions for each device. Combining the functions with an explicit ID
saves 752 bytes on x86, once the axp192 is added, and I don't think it
is any harder to understand than the separate functions. And it's still
possible to use a separate function when needed.

Nonetheless, if you'd prefer to stick with separate functions I'm fine
with that.

Regards,
Aidan

>> ---
>>  drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
>>  1 file changed, 34 insertions(+), 49 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 9d5b1de24908..0260433782d8 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -71,6 +71,18 @@ struct axp20x_adc_iio {
>>  	const struct axp_data	*data;
>>  };
>>  
>> +struct axp_data {
>> +	const struct iio_info		*iio_info;
>> +	int				num_channels;
>> +	struct iio_chan_spec const	*channels;
>> +	unsigned long			adc_en1_mask;
>> +	unsigned long			adc_en2_mask;
>> +	int				(*adc_rate)(struct axp20x_adc_iio *info,
>> +						    int rate);
>> +	struct iio_map			*maps;
>> +	enum axp20x_variants		axp20x_id;
>> +};
>> +
>>  enum axp20x_adc_channel_v {
>>  	AXP20X_ACIN_V = 0,
>>  	AXP20X_VBUS_V,
>> @@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>>  	int ret, size;
>>  
>> -	/*
>> -	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
>> -	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
>> -	 * bits.
>> -	 */
>> -	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
>> -		size = 13;
>> -	else
>> +	switch (info->data->axp20x_id) {
>> +	case AXP202_ID:
>> +	case AXP209_ID:
>> +		/*
>> +		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
>> +		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
>> +		 * bits.
>> +		 */
>> +		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
>
> This line is getting a bit long, break it after the &&
>
>> +			size = 13;
>> +		else
>> +			size = 12;
>> +		break;
>> +
>> +	default:
>>  		size = 12;
>> +		break;
>> +	}
>>  
>>  	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>>  	if (ret < 0)
>> @@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>  	return IIO_VAL_INT;
>>  }
>>  
>> -static int axp22x_adc_raw(struct iio_dev *indio_dev,
>> -			  struct iio_chan_spec const *chan, int *val)
>> -{
>> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	int ret;
>> -
>> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	*val = ret;
>> -	return IIO_VAL_INT;
>> -}
>> -
>> -static int axp813_adc_raw(struct iio_dev *indio_dev,
>> -			  struct iio_chan_spec const *chan, int *val)
>> -{
>> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> -	int ret;
>> -
>> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	*val = ret;
>> -	return IIO_VAL_INT;
>> -}
>> -
>>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>>  {
>>  	switch (channel) {
>> @@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
>>  		return axp22x_adc_scale(chan, val, val2);
>>  
>>  	case IIO_CHAN_INFO_RAW:
>> -		return axp22x_adc_raw(indio_dev, chan, val);
>> +		return axp20x_adc_raw(indio_dev, chan, val);
>>  
>>  	default:
>>  		return -EINVAL;
>> @@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>>  		return axp813_adc_scale(chan, val, val2);
>>  
>>  	case IIO_CHAN_INFO_RAW:
>> -		return axp813_adc_raw(indio_dev, chan, val);
>> +		return axp20x_adc_raw(indio_dev, chan, val);
>>  
>>  	default:
>>  		return -EINVAL;
>> @@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
>>  				 AXP813_ADC_RATE_HZ(rate));
>>  }
>>  
>> -struct axp_data {
>> -	const struct iio_info		*iio_info;
>> -	int				num_channels;
>> -	struct iio_chan_spec const	*channels;
>> -	unsigned long			adc_en1_mask;
>> -	int				(*adc_rate)(struct axp20x_adc_iio *info,
>> -						    int rate);
>> -	bool				adc_en2;
>> -	struct iio_map			*maps;
>> -};
>> -
>>  static const struct axp_data axp20x_data = {
>>  	.iio_info = &axp20x_adc_iio_info,
>>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
>> @@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
>>  	.adc_rate = axp20x_adc_rate,
>>  	.adc_en2 = true,
>>  	.maps = axp20x_maps,
>> +	.axp20x_id = AXP209_ID,
>>  };
>>  
>>  static const struct axp_data axp22x_data = {
>> @@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
>>  	.adc_rate = axp22x_adc_rate,
>>  	.adc_en2 = false,
>>  	.maps = axp22x_maps,
>> +	.axp20x_id = AXP221_ID,
>>  };
>>  
>>  static const struct axp_data axp813_data = {
>> @@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
>>  	.adc_rate = axp813_adc_rate,
>>  	.adc_en2 = false,
>>  	.maps = axp22x_maps,
>> +	.axp20x_id = AXP813_ID,
>>  };
>>  
>>  static const struct of_device_id axp20x_adc_of_match[] = {

Re: [PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Jonathan Cameron]

On Thu, 09 Jun 2022 00:13:47 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> 
> > On Tue,  7 Jun 2022 16:53:18 +0100
> > Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
> >  
> >> Add an axp20x_id variant field to the axp_data struct and use it
> >> to consolidate the adc_raw functions, reducing code duplication.
> >> Variant IDs are chosen to match the OF compatible strings.
> >> 
> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>  
> >
> > Hi Aidan,
> >
> > I'm not a big fan of using variant IDs, rather than a description
> > of what is actually different between devices.  Long term, variant
> > IDs tend to scale (as we add more supported devices) much worse
> > than a flag describing the actual difference.
> >
> > Here I would have a field in struct axp_data called something like
> > discharge_curr_res and set it to 12 or 13 as appropriate.
> >  
> 
> I agree with your point in general, but here it seems impossible to get
> away from variant IDs because the channel numbering depends on it and we
> use the channel number to decide what number of bits to use.

Ah. I'd missed that detail.  Perhaps add a comment to remind us that's
the case in future.

> The code
> I'm replacing is just disguising the variant IDs by giving every variant
> its own set of functions.
> 
> To me it seemed clearer to describe the channel properties and then use
> one read_raw function for all variants, but when I did that it turned
> out not to make any difference in size for x86. Probably because tables
> encode a lot of redundant information compared to switches. It also
> relied on a hack to associate extra info with an iio_chan_spec so it
> wasn't much of an improvement, in the end.
> 
> So it's a question of using a variant ID explicitly or having separate
> functions for each device. Combining the functions with an explicit ID
> saves 752 bytes on x86, once the axp192 is added, and I don't think it
> is any harder to understand than the separate functions. And it's still
> possible to use a separate function when needed.
> 
> Nonetheless, if you'd prefer to stick with separate functions I'm fine
> with that.

This may well be a case of doing what you have here for now, but revisit
in future if we end up with more cases of this turning up in the function.
It may well become too complex and need the separate functions again.

Jonathan

> 
> Regards,
> Aidan
> 
> >> ---
> >>  drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
> >>  1 file changed, 34 insertions(+), 49 deletions(-)
> >> 
> >> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> >> index 9d5b1de24908..0260433782d8 100644
> >> --- a/drivers/iio/adc/axp20x_adc.c
> >> +++ b/drivers/iio/adc/axp20x_adc.c
> >> @@ -71,6 +71,18 @@ struct axp20x_adc_iio {
> >>  	const struct axp_data	*data;
> >>  };
> >>  
> >> +struct axp_data {
> >> +	const struct iio_info		*iio_info;
> >> +	int				num_channels;
> >> +	struct iio_chan_spec const	*channels;
> >> +	unsigned long			adc_en1_mask;
> >> +	unsigned long			adc_en2_mask;
> >> +	int				(*adc_rate)(struct axp20x_adc_iio *info,
> >> +						    int rate);
> >> +	struct iio_map			*maps;
> >> +	enum axp20x_variants		axp20x_id;
> >> +};
> >> +
> >>  enum axp20x_adc_channel_v {
> >>  	AXP20X_ACIN_V = 0,
> >>  	AXP20X_VBUS_V,
> >> @@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
> >>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> >>  	int ret, size;
> >>  
> >> -	/*
> >> -	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> >> -	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> >> -	 * bits.
> >> -	 */
> >> -	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
> >> -		size = 13;
> >> -	else
> >> +	switch (info->data->axp20x_id) {
> >> +	case AXP202_ID:
> >> +	case AXP209_ID:
> >> +		/*
> >> +		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> >> +		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> >> +		 * bits.
> >> +		 */
> >> +		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)  
> >
> > This line is getting a bit long, break it after the &&
> >  
> >> +			size = 13;
> >> +		else
> >> +			size = 12;
> >> +		break;
> >> +
> >> +	default:
> >>  		size = 12;
> >> +		break;
> >> +	}
> >>  
> >>  	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
> >>  	if (ret < 0)
> >> @@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
> >>  	return IIO_VAL_INT;
> >>  }
> >>  
> >> -static int axp22x_adc_raw(struct iio_dev *indio_dev,
> >> -			  struct iio_chan_spec const *chan, int *val)
> >> -{
> >> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> >> -	int ret;
> >> -
> >> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	*val = ret;
> >> -	return IIO_VAL_INT;
> >> -}
> >> -
> >> -static int axp813_adc_raw(struct iio_dev *indio_dev,
> >> -			  struct iio_chan_spec const *chan, int *val)
> >> -{
> >> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> >> -	int ret;
> >> -
> >> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	*val = ret;
> >> -	return IIO_VAL_INT;
> >> -}
> >> -
> >>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
> >>  {
> >>  	switch (channel) {
> >> @@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
> >>  		return axp22x_adc_scale(chan, val, val2);
> >>  
> >>  	case IIO_CHAN_INFO_RAW:
> >> -		return axp22x_adc_raw(indio_dev, chan, val);
> >> +		return axp20x_adc_raw(indio_dev, chan, val);
> >>  
> >>  	default:
> >>  		return -EINVAL;
> >> @@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
> >>  		return axp813_adc_scale(chan, val, val2);
> >>  
> >>  	case IIO_CHAN_INFO_RAW:
> >> -		return axp813_adc_raw(indio_dev, chan, val);
> >> +		return axp20x_adc_raw(indio_dev, chan, val);
> >>  
> >>  	default:
> >>  		return -EINVAL;
> >> @@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
> >>  				 AXP813_ADC_RATE_HZ(rate));
> >>  }
> >>  
> >> -struct axp_data {
> >> -	const struct iio_info		*iio_info;
> >> -	int				num_channels;
> >> -	struct iio_chan_spec const	*channels;
> >> -	unsigned long			adc_en1_mask;
> >> -	int				(*adc_rate)(struct axp20x_adc_iio *info,
> >> -						    int rate);
> >> -	bool				adc_en2;
> >> -	struct iio_map			*maps;
> >> -};
> >> -
> >>  static const struct axp_data axp20x_data = {
> >>  	.iio_info = &axp20x_adc_iio_info,
> >>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
> >> @@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
> >>  	.adc_rate = axp20x_adc_rate,
> >>  	.adc_en2 = true,
> >>  	.maps = axp20x_maps,
> >> +	.axp20x_id = AXP209_ID,
> >>  };
> >>  
> >>  static const struct axp_data axp22x_data = {
> >> @@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
> >>  	.adc_rate = axp22x_adc_rate,
> >>  	.adc_en2 = false,
> >>  	.maps = axp22x_maps,
> >> +	.axp20x_id = AXP221_ID,
> >>  };
> >>  
> >>  static const struct axp_data axp813_data = {
> >> @@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
> >>  	.adc_rate = axp813_adc_rate,
> >>  	.adc_en2 = false,
> >>  	.maps = axp22x_maps,
> >> +	.axp20x_id = AXP813_ID,
> >>  };
> >>  
> >>  static const struct of_device_id axp20x_adc_of_match[] = {  
> 

Re: [PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Jonathan Cameron]

On Tue,  7 Jun 2022 16:53:18 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> Add an axp20x_id variant field to the axp_data struct and use it
> to consolidate the adc_raw functions, reducing code duplication.
> Variant IDs are chosen to match the OF compatible strings.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Also, try building after this patch....

> ---
>  drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 9d5b1de24908..0260433782d8 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -71,6 +71,18 @@ struct axp20x_adc_iio {
>  	const struct axp_data	*data;
>  };
>  
> +struct axp_data {
> +	const struct iio_info		*iio_info;
> +	int				num_channels;
> +	struct iio_chan_spec const	*channels;
> +	unsigned long			adc_en1_mask;
> +	unsigned long			adc_en2_mask;

This new field should be in the next patch.

> +	int				(*adc_rate)(struct axp20x_adc_iio *info,
> +						    int rate);
> +	struct iio_map			*maps;
> +	enum axp20x_variants		axp20x_id;
> +};
> +
>  enum axp20x_adc_channel_v {
>  	AXP20X_ACIN_V = 0,
>  	AXP20X_VBUS_V,
> @@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>  	int ret, size;
>  
> -	/*
> -	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> -	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> -	 * bits.
> -	 */
> -	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
> -		size = 13;
> -	else
> +	switch (info->data->axp20x_id) {
> +	case AXP202_ID:
> +	case AXP209_ID:
> +		/*
> +		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> +		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> +		 * bits.
> +		 */
> +		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
> +			size = 13;
> +		else
> +			size = 12;
> +		break;
> +
> +	default:
>  		size = 12;
> +		break;
> +	}
>  
>  	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>  	if (ret < 0)
> @@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> -static int axp22x_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
> -static int axp813_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
>  		return axp22x_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp22x_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>  		return axp813_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp813_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
>  				 AXP813_ADC_RATE_HZ(rate));
>  }
>  
> -struct axp_data {
> -	const struct iio_info		*iio_info;
> -	int				num_channels;
> -	struct iio_chan_spec const	*channels;
> -	unsigned long			adc_en1_mask;
> -	int				(*adc_rate)(struct axp20x_adc_iio *info,
> -						    int rate);
> -	bool				adc_en2;
> -	struct iio_map			*maps;
> -};
> -
>  static const struct axp_data axp20x_data = {
>  	.iio_info = &axp20x_adc_iio_info,
>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
> @@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
>  	.adc_rate = axp20x_adc_rate,
>  	.adc_en2 = true,
>  	.maps = axp20x_maps,
> +	.axp20x_id = AXP209_ID,
>  };
>  
>  static const struct axp_data axp22x_data = {
> @@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
>  	.adc_rate = axp22x_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP221_ID,
>  };
>  
>  static const struct axp_data axp813_data = {
> @@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
>  	.adc_rate = axp813_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP813_ID,
>  };
>  
>  static const struct of_device_id axp20x_adc_of_match[] = {

[PATCH v2 12/17] iio: adc: axp20x_adc: Add support for AXP192

[Aidan MacDonald]

The AXP192 is identical to the AXP20x, except for the addition of
two more GPIO ADC channels.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/iio/adc/axp20x_adc.c | 285 ++++++++++++++++++++++++++++++++++-
 1 file changed, 277 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 0260433782d8..fcd37d39884d 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -22,11 +22,19 @@
 #include <linux/iio/machine.h>
 #include <linux/mfd/axp20x.h>
 
+#define AXP192_ADC_EN1_MASK			GENMASK(7, 0)
+#define AXP192_ADC_EN2_MASK			(GENMASK(3, 0) | BIT(7))
+
 #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
 #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
 
 #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
 
+#define AXP192_GPIO30_IN_RANGE_GPIO0		BIT(0)
+#define AXP192_GPIO30_IN_RANGE_GPIO1		BIT(1)
+#define AXP192_GPIO30_IN_RANGE_GPIO2		BIT(2)
+#define AXP192_GPIO30_IN_RANGE_GPIO3		BIT(3)
+
 #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
 #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
 
@@ -83,6 +91,25 @@ struct axp_data {
 	enum axp20x_variants		axp20x_id;
 };
 
+enum axp192_adc_channel_v {
+	AXP192_ACIN_V = 0,
+	AXP192_VBUS_V,
+	AXP192_TS_IN,
+	AXP192_GPIO0_V,
+	AXP192_GPIO1_V,
+	AXP192_GPIO2_V,
+	AXP192_GPIO3_V,
+	AXP192_IPSOUT_V,
+	AXP192_BATT_V,
+};
+
+enum axp192_adc_channel_i {
+	AXP192_ACIN_I = 0,
+	AXP192_VBUS_I,
+	AXP192_BATT_CHRG_I,
+	AXP192_BATT_DISCHRG_I,
+};
+
 enum axp20x_adc_channel_v {
 	AXP20X_ACIN_V = 0,
 	AXP20X_VBUS_V,
@@ -170,6 +197,43 @@ static struct iio_map axp22x_maps[] = {
  * The only exception is for the battery. batt_v will be in_voltage6_raw and
  * charge current in_current6_raw and discharge current will be in_current7_raw.
  */
+static const struct iio_chan_spec axp192_adc_channels[] = {
+	AXP20X_ADC_CHANNEL(AXP192_ACIN_V, "acin_v", IIO_VOLTAGE,
+			   AXP20X_ACIN_V_ADC_H),
+	AXP20X_ADC_CHANNEL(AXP192_ACIN_I, "acin_i", IIO_CURRENT,
+			   AXP20X_ACIN_I_ADC_H),
+	AXP20X_ADC_CHANNEL(AXP192_VBUS_V, "vbus_v", IIO_VOLTAGE,
+			   AXP20X_VBUS_V_ADC_H),
+	AXP20X_ADC_CHANNEL(AXP192_VBUS_I, "vbus_i", IIO_CURRENT,
+			   AXP20X_VBUS_I_ADC_H),
+	{
+		.type = IIO_TEMP,
+		.address = AXP20X_TEMP_ADC_H,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.datasheet_name = "pmic_temp",
+	},
+	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
+				  AXP20X_GPIO0_V_ADC_H),
+	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO1_V, "gpio1_v", IIO_VOLTAGE,
+				  AXP20X_GPIO1_V_ADC_H),
+	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO2_V, "gpio2_v", IIO_VOLTAGE,
+				  AXP192_GPIO2_V_ADC_H),
+	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO3_V, "gpio3_v", IIO_VOLTAGE,
+				  AXP192_GPIO3_V_ADC_H),
+	AXP20X_ADC_CHANNEL(AXP192_IPSOUT_V, "ipsout_v", IIO_VOLTAGE,
+			   AXP20X_IPSOUT_V_HIGH_H),
+	AXP20X_ADC_CHANNEL(AXP192_BATT_V, "batt_v", IIO_VOLTAGE,
+			   AXP20X_BATT_V_H),
+	AXP20X_ADC_CHANNEL(AXP192_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
+			   AXP20X_BATT_CHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP192_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
+			   AXP20X_BATT_DISCHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP192_TS_IN, "ts_v", IIO_VOLTAGE,
+			   AXP20X_TS_IN_H),
+};
+
 static const struct iio_chan_spec axp20x_adc_channels[] = {
 	AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE,
 			   AXP20X_ACIN_V_ADC_H),
@@ -250,6 +314,15 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	int ret, size;
 
 	switch (info->data->axp20x_id) {
+	case AXP192_ID:
+		/* Battery current ADCs on the AXP192 are 13 bits. */
+		if (chan->type == IIO_CURRENT &&
+		    (chan->channel == AXP20X_BATT_CHRG_I || chan->channel == AXP20X_BATT_DISCHRG_I))
+			size = 13;
+		else
+			size = 12;
+		break;
+
 	case AXP202_ID:
 	case AXP209_ID:
 		/*
@@ -276,6 +349,44 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
+static int axp192_adc_scale_voltage(int channel, int *val, int *val2)
+{
+	switch (channel) {
+	case AXP192_ACIN_V:
+	case AXP192_VBUS_V:
+		*val = 1;
+		*val2 = 700000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP192_GPIO0_V:
+	case AXP192_GPIO1_V:
+	case AXP192_GPIO2_V:
+	case AXP192_GPIO3_V:
+		*val = 0;
+		*val2 = 500000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP192_BATT_V:
+		*val = 1;
+		*val2 = 100000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP192_IPSOUT_V:
+		*val = 1;
+		*val2 = 400000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP192_TS_IN:
+		/* 0.8 mV per LSB */
+		*val = 0;
+		*val2 = 800000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
 {
 	switch (channel) {
@@ -379,6 +490,29 @@ static int axp20x_adc_scale_current(int channel, int *val, int *val2)
 	}
 }
 
+static int axp192_adc_scale(struct iio_chan_spec const *chan, int *val,
+			    int *val2)
+{
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		return axp192_adc_scale_voltage(chan->channel, val, val2);
+
+	case IIO_CURRENT:
+		/*
+		 * AXP192 current channels are identical to the AXP20x,
+		 * therefore we can re-use the scaling function.
+		 */
+		return axp20x_adc_scale_current(chan->channel, val, val2);
+
+	case IIO_TEMP:
+		*val = 100;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_adc_scale(struct iio_chan_spec const *chan, int *val,
 			    int *val2)
 {
@@ -438,6 +572,42 @@ static int axp813_adc_scale(struct iio_chan_spec const *chan, int *val,
 	}
 }
 
+static int axp192_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
+				     int *val)
+{
+	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(info->regmap, AXP192_GPIO30_IN_RANGE, &regval);
+	if (ret < 0)
+		return ret;
+
+	switch (channel) {
+	case AXP192_GPIO0_V:
+		regval &= AXP192_GPIO30_IN_RANGE_GPIO0;
+		break;
+
+	case AXP192_GPIO1_V:
+		regval &= AXP192_GPIO30_IN_RANGE_GPIO1;
+		break;
+
+	case AXP192_GPIO2_V:
+		regval &= AXP192_GPIO30_IN_RANGE_GPIO2;
+		break;
+
+	case AXP192_GPIO3_V:
+		regval &= AXP192_GPIO30_IN_RANGE_GPIO3;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	*val = regval ? 700000 : 0;
+	return IIO_VAL_INT;
+}
+
 static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
 				     int *val)
 {
@@ -466,6 +636,22 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
 	return IIO_VAL_INT;
 }
 
+static int axp192_adc_offset(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val)
+{
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		return axp192_adc_offset_voltage(indio_dev, chan->channel, val);
+
+	case IIO_TEMP:
+		*val = -1447;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_adc_offset(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan, int *val)
 {
@@ -482,6 +668,25 @@ static int axp20x_adc_offset(struct iio_dev *indio_dev,
 	}
 }
 
+static int axp192_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		return axp192_adc_offset(indio_dev, chan, val);
+
+	case IIO_CHAN_INFO_SCALE:
+		return axp192_adc_scale(chan, val, val2);
+
+	case IIO_CHAN_INFO_RAW:
+		return axp20x_adc_raw(indio_dev, chan, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
@@ -542,6 +747,53 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int axp192_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+	unsigned int regmask, regval;
+
+	/*
+	 * The AXP192 PMIC allows the user to choose between 0V and 0.7V offsets
+	 * for (independently) GPIO0-3 when in ADC mode.
+	 */
+	if (mask != IIO_CHAN_INFO_OFFSET)
+		return -EINVAL;
+
+	if (val != 0 && val != 700000)
+		return -EINVAL;
+
+	regval = val ? 1 : 0;
+
+	switch (chan->channel) {
+	case AXP192_GPIO0_V:
+		regmask = AXP192_GPIO30_IN_RANGE_GPIO0;
+		regval = FIELD_PREP(AXP192_GPIO30_IN_RANGE_GPIO0, regval);
+		break;
+
+	case AXP192_GPIO1_V:
+		regmask = AXP192_GPIO30_IN_RANGE_GPIO1;
+		regval = FIELD_PREP(AXP192_GPIO30_IN_RANGE_GPIO1, regval);
+		break;
+
+	case AXP192_GPIO2_V:
+		regmask = AXP192_GPIO30_IN_RANGE_GPIO2;
+		regval = FIELD_PREP(AXP192_GPIO30_IN_RANGE_GPIO2, regval);
+		break;
+
+	case AXP192_GPIO3_V:
+		regmask = AXP192_GPIO30_IN_RANGE_GPIO3;
+		regval = FIELD_PREP(AXP192_GPIO30_IN_RANGE_GPIO3, regval);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(info->regmap, AXP192_GPIO30_IN_RANGE, regmask, regval);
+}
+
 static int axp20x_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int val, int val2,
 			    long mask)
@@ -579,6 +831,11 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
 	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE, regmask, regval);
 }
 
+static const struct iio_info axp192_adc_iio_info = {
+	.read_raw = axp192_read_raw,
+	.write_raw = axp192_write_raw,
+};
+
 static const struct iio_info axp20x_adc_iio_info = {
 	.read_raw = axp20x_read_raw,
 	.write_raw = axp20x_write_raw,
@@ -613,13 +870,24 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
 				 AXP813_ADC_RATE_HZ(rate));
 }
 
+static const struct axp_data axp192_data = {
+	.iio_info = &axp192_adc_iio_info,
+	.num_channels = ARRAY_SIZE(axp192_adc_channels),
+	.channels = axp192_adc_channels,
+	.adc_en1_mask = AXP192_ADC_EN1_MASK,
+	.adc_en2_mask = AXP192_ADC_EN2_MASK,
+	.adc_rate = axp20x_adc_rate,
+	.maps = axp20x_maps,
+	.axp20x_id = AXP192_ID,
+};
+
 static const struct axp_data axp20x_data = {
 	.iio_info = &axp20x_adc_iio_info,
 	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
 	.channels = axp20x_adc_channels,
 	.adc_en1_mask = AXP20X_ADC_EN1_MASK,
+	.adc_en2_mask = AXP20X_ADC_EN2_MASK,
 	.adc_rate = axp20x_adc_rate,
-	.adc_en2 = true,
 	.maps = axp20x_maps,
 	.axp20x_id = AXP209_ID,
 };
@@ -630,7 +898,6 @@ static const struct axp_data axp22x_data = {
 	.channels = axp22x_adc_channels,
 	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
 	.adc_rate = axp22x_adc_rate,
-	.adc_en2 = false,
 	.maps = axp22x_maps,
 	.axp20x_id = AXP221_ID,
 };
@@ -641,12 +908,12 @@ static const struct axp_data axp813_data = {
 	.channels = axp813_adc_channels,
 	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
 	.adc_rate = axp813_adc_rate,
-	.adc_en2 = false,
 	.maps = axp22x_maps,
 	.axp20x_id = AXP813_ID,
 };
 
 static const struct of_device_id axp20x_adc_of_match[] = {
+	{ .compatible = "x-powers,axp192-adc", .data = (void *)&axp192_data, },
 	{ .compatible = "x-powers,axp209-adc", .data = (void *)&axp20x_data, },
 	{ .compatible = "x-powers,axp221-adc", .data = (void *)&axp22x_data, },
 	{ .compatible = "x-powers,axp813-adc", .data = (void *)&axp813_data, },
@@ -655,6 +922,7 @@ static const struct of_device_id axp20x_adc_of_match[] = {
 MODULE_DEVICE_TABLE(of, axp20x_adc_of_match);
 
 static const struct platform_device_id axp20x_adc_id_match[] = {
+	{ .name = "axp192-adc", .driver_data = (kernel_ulong_t)&axp192_data, },
 	{ .name = "axp20x-adc", .driver_data = (kernel_ulong_t)&axp20x_data, },
 	{ .name = "axp22x-adc", .driver_data = (kernel_ulong_t)&axp22x_data, },
 	{ .name = "axp813-adc", .driver_data = (kernel_ulong_t)&axp813_data, },
@@ -700,10 +968,11 @@ static int axp20x_probe(struct platform_device *pdev)
 	/* Enable the ADCs on IP */
 	regmap_write(info->regmap, AXP20X_ADC_EN1, info->data->adc_en1_mask);
 
-	if (info->data->adc_en2)
-		/* Enable GPIO0/1 and internal temperature ADCs */
+	if (info->data->adc_en2_mask)
+		/* Enable GPIO and internal temperature ADCs */
 		regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
-				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
+				   info->data->adc_en2_mask,
+				   info->data->adc_en2_mask);
 
 	/* Configure ADCs rate */
 	info->data->adc_rate(info, 100);
@@ -728,7 +997,7 @@ static int axp20x_probe(struct platform_device *pdev)
 fail_map:
 	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
 
-	if (info->data->adc_en2)
+	if (info->data->adc_en2_mask)
 		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
 
 	return ret;
@@ -744,7 +1013,7 @@ static int axp20x_remove(struct platform_device *pdev)
 
 	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
 
-	if (info->data->adc_en2)
+	if (info->data->adc_en2_mask)
 		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
 
 	return 0;
-- 
2.35.1

Re: [PATCH v2 12/17] iio: adc: axp20x_adc: Add support for AXP192

[Jonathan Cameron]

On Tue,  7 Jun 2022 16:53:19 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> The AXP192 is identical to the AXP20x, except for the addition of
> two more GPIO ADC channels.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Hi Aidan,

A few things follow through from review of previous 2 patches.
Otherwise looks good to me.

Thanks,

Jonathan


>  	AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE,
>  			   AXP20X_ACIN_V_ADC_H),
> @@ -250,6 +314,15 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	int ret, size;
>  
>  	switch (info->data->axp20x_id) {
> +	case AXP192_ID:
> +		/* Battery current ADCs on the AXP192 are 13 bits. */
> +		if (chan->type == IIO_CURRENT &&
> +		    (chan->channel == AXP20X_BATT_CHRG_I || chan->channel == AXP20X_BATT_DISCHRG_I))

Ah, you'll be needing two _res variables as suggested in earlier patch.

> +			size = 13;
> +		else
> +			size = 12;
> +		break;
> +
>  	case AXP202_ID:
>  	case AXP209_ID:
>  		/*
> @@ -276,6 +349,44 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }

...

> +static int axp192_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
> +				     int *val)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, AXP192_GPIO30_IN_RANGE, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (channel) {
> +	case AXP192_GPIO0_V:
> +		regval &= AXP192_GPIO30_IN_RANGE_GPIO0;

As per earlier patch, FIELD_GET() would act as 'documentation'
of what is going on here, even though it'll make no real difference.

> +		break;
> +
> +	case AXP192_GPIO1_V:
> +		regval &= AXP192_GPIO30_IN_RANGE_GPIO1;
> +		break;
> +
> +	case AXP192_GPIO2_V:
> +		regval &= AXP192_GPIO30_IN_RANGE_GPIO2;
> +		break;
> +
> +	case AXP192_GPIO3_V:
> +		regval &= AXP192_GPIO30_IN_RANGE_GPIO3;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*val = regval ? 700000 : 0;
> +	return IIO_VAL_INT;
> +}
> +

[PATCH v2 13/17] power: supply: axp20x_usb_power: Add support for AXP192

[Aidan MacDonald]

The AXP192 is mostly the same as the AXP202 but has a different
current limit.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/power/supply/axp20x_usb_power.c | 80 +++++++++++++++++++++----
 1 file changed, 69 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index a1e6d1d44808..03145374ae72 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -48,6 +48,9 @@
 #define AXP813_VBUS_CLIMIT_2000mA	2
 #define AXP813_VBUS_CLIMIT_2500mA	3
 
+#define AXP192_VBUS_CLIMIT_EN		BIT(1)
+#define AXP192_VBUS_CLIMIT_100mA	BIT(0)
+
 #define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
 #define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
 
@@ -121,6 +124,24 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
 		mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
 }
 
+static int axp192_get_current_max(struct axp20x_usb_power *power, int *val)
+{
+	unsigned int v;
+	int ret = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
+
+	if (ret)
+		return ret;
+
+	if (!(v & AXP192_VBUS_CLIMIT_EN))
+		*val = -1;
+	else if (v & AXP192_VBUS_CLIMIT_100mA)
+		*val = 100000;
+	else
+		*val = 500000;
+
+	return 0;
+}
+
 static int axp20x_get_current_max(struct axp20x_usb_power *power, int *val)
 {
 	unsigned int v;
@@ -179,7 +200,7 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 	enum power_supply_property psp, union power_supply_propval *val)
 {
 	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
-	unsigned int input, v;
+	unsigned int input, v, reg;
 	int ret;
 
 	switch (psp) {
@@ -215,6 +236,8 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CURRENT_MAX:
 		if (power->axp20x_id == AXP813_ID)
 			return axp813_get_current_max(power, &val->intval);
+		else if (power->axp20x_id == AXP192_ID)
+			return axp192_get_current_max(power, &val->intval);
 		return axp20x_get_current_max(power, &val->intval);
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (IS_ENABLED(CONFIG_AXP20X_ADC)) {
@@ -256,16 +279,20 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 
 		val->intval = POWER_SUPPLY_HEALTH_GOOD;
 
-		if (power->axp20x_id == AXP202_ID) {
-			ret = regmap_read(power->regmap,
-					  AXP20X_USB_OTG_STATUS, &v);
-			if (ret)
-				return ret;
+		if (power->axp20x_id == AXP192_ID)
+			reg = AXP192_USB_OTG_STATUS;
+		else if (power->axp20x_id == AXP202_ID)
+			reg = AXP20X_USB_OTG_STATUS;
+		else
+			/* Other chips do not have an OTG status register */
+			break;
 
-			if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
-				val->intval =
-					POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
-		}
+		ret = regmap_read(power->regmap, reg, &v);
+		if (ret)
+			return ret;
+
+		if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
+			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
@@ -316,6 +343,24 @@ static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
 	return -EINVAL;
 }
 
+static int axp192_usb_power_set_current_max(struct axp20x_usb_power *power,
+					    int intval)
+{
+	int val = AXP192_VBUS_CLIMIT_EN;
+	const int mask = AXP192_VBUS_CLIMIT_EN | AXP192_VBUS_CLIMIT_100mA;
+
+	switch (intval) {
+	case 100000:
+		val |= AXP192_VBUS_CLIMIT_100mA;
+		fallthrough;
+	case 500000:
+		return regmap_update_bits(power->regmap,
+					  AXP20X_VBUS_IPSOUT_MGMT, mask, val);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp813_usb_power_set_current_max(struct axp20x_usb_power *power,
 					    int intval)
 {
@@ -383,6 +428,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
 		if (power->axp20x_id == AXP813_ID)
 			return axp813_usb_power_set_current_max(power,
 								val->intval);
+		else if (power->axp20x_id == AXP192_ID)
+			return axp192_usb_power_set_current_max(power,
+								val->intval);
 		return axp20x_usb_power_set_current_max(power, val->intval);
 
 	default:
@@ -468,6 +516,13 @@ struct axp_data {
 	enum axp20x_variants		axp20x_id;
 };
 
+static const struct axp_data axp192_data = {
+	.power_desc	= &axp20x_usb_power_desc,
+	.irq_names	= axp20x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
+	.axp20x_id	= AXP192_ID,
+};
+
 static const struct axp_data axp202_data = {
 	.power_desc	= &axp20x_usb_power_desc,
 	.irq_names	= axp20x_irq_names,
@@ -600,7 +655,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (power->axp20x_id == AXP202_ID) {
+	if (power->axp20x_id == AXP192_ID || power->axp20x_id == AXP202_ID) {
 		/* Enable vbus valid checking */
 		ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
 					 AXP20X_VBUS_MON_VBUS_VALID,
@@ -659,6 +714,9 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 
 static const struct of_device_id axp20x_usb_power_match[] = {
 	{
+		.compatible = "x-powers,axp192-usb-power-supply",
+		.data = &axp192_data,
+	}, {
 		.compatible = "x-powers,axp202-usb-power-supply",
 		.data = &axp202_data,
 	}, {
-- 
2.35.1

Re: [PATCH v2 13/17] power: supply: axp20x_usb_power: Add support for AXP192

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5828 bytes --]

Hi,

On Tue, Jun 07, 2022 at 04:53:20PM +0100, Aidan MacDonald wrote:
> The AXP192 is mostly the same as the AXP202 but has a different
> current limit.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/power/supply/axp20x_usb_power.c | 80 +++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index a1e6d1d44808..03145374ae72 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -48,6 +48,9 @@
>  #define AXP813_VBUS_CLIMIT_2000mA	2
>  #define AXP813_VBUS_CLIMIT_2500mA	3
>  
> +#define AXP192_VBUS_CLIMIT_EN		BIT(1)
> +#define AXP192_VBUS_CLIMIT_100mA	BIT(0)
> +
>  #define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
>  #define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
>  
> @@ -121,6 +124,24 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
>  		mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
>  }
>  
> +static int axp192_get_current_max(struct axp20x_usb_power *power, int *val)
> +{
> +	unsigned int v;
> +	int ret = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (!(v & AXP192_VBUS_CLIMIT_EN))
> +		*val = -1;
> +	else if (v & AXP192_VBUS_CLIMIT_100mA)
> +		*val = 100000;
> +	else
> +		*val = 500000;
> +
> +	return 0;
> +}
> +
>  static int axp20x_get_current_max(struct axp20x_usb_power *power, int *val)
>  {
>  	unsigned int v;
> @@ -179,7 +200,7 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>  	enum power_supply_property psp, union power_supply_propval *val)
>  {
>  	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> -	unsigned int input, v;
> +	unsigned int input, v, reg;
>  	int ret;
>  
>  	switch (psp) {
> @@ -215,6 +236,8 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CURRENT_MAX:
>  		if (power->axp20x_id == AXP813_ID)
>  			return axp813_get_current_max(power, &val->intval);
> +		else if (power->axp20x_id == AXP192_ID)
> +			return axp192_get_current_max(power, &val->intval);
>  		return axp20x_get_current_max(power, &val->intval);
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  		if (IS_ENABLED(CONFIG_AXP20X_ADC)) {
> @@ -256,16 +279,20 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>  
>  		val->intval = POWER_SUPPLY_HEALTH_GOOD;
>  
> -		if (power->axp20x_id == AXP202_ID) {
> -			ret = regmap_read(power->regmap,
> -					  AXP20X_USB_OTG_STATUS, &v);
> -			if (ret)
> -				return ret;
> +		if (power->axp20x_id == AXP192_ID)
> +			reg = AXP192_USB_OTG_STATUS;
> +		else if (power->axp20x_id == AXP202_ID)
> +			reg = AXP20X_USB_OTG_STATUS;
> +		else
> +			/* Other chips do not have an OTG status register */
> +			break;
>  
> -			if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
> -				val->intval =
> -					POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> -		}
> +		ret = regmap_read(power->regmap, reg, &v);
> +		if (ret)
> +			return ret;
> +
> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>  		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> @@ -316,6 +343,24 @@ static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
>  	return -EINVAL;
>  }
>  
> +static int axp192_usb_power_set_current_max(struct axp20x_usb_power *power,
> +					    int intval)
> +{
> +	int val = AXP192_VBUS_CLIMIT_EN;
> +	const int mask = AXP192_VBUS_CLIMIT_EN | AXP192_VBUS_CLIMIT_100mA;
> +
> +	switch (intval) {
> +	case 100000:
> +		val |= AXP192_VBUS_CLIMIT_100mA;
> +		fallthrough;
> +	case 500000:
> +		return regmap_update_bits(power->regmap,
> +					  AXP20X_VBUS_IPSOUT_MGMT, mask, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp813_usb_power_set_current_max(struct axp20x_usb_power *power,
>  					    int intval)
>  {
> @@ -383,6 +428,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>  		if (power->axp20x_id == AXP813_ID)
>  			return axp813_usb_power_set_current_max(power,
>  								val->intval);
> +		else if (power->axp20x_id == AXP192_ID)
> +			return axp192_usb_power_set_current_max(power,
> +								val->intval);
>  		return axp20x_usb_power_set_current_max(power, val->intval);
>  
>  	default:
> @@ -468,6 +516,13 @@ struct axp_data {
>  	enum axp20x_variants		axp20x_id;
>  };
>  
> +static const struct axp_data axp192_data = {
> +	.power_desc	= &axp20x_usb_power_desc,
> +	.irq_names	= axp20x_irq_names,
> +	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
> +	.axp20x_id	= AXP192_ID,
> +};
> +
>  static const struct axp_data axp202_data = {
>  	.power_desc	= &axp20x_usb_power_desc,
>  	.irq_names	= axp20x_irq_names,
> @@ -600,7 +655,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (power->axp20x_id == AXP202_ID) {
> +	if (power->axp20x_id == AXP192_ID || power->axp20x_id == AXP202_ID) {
>  		/* Enable vbus valid checking */
>  		ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>  					 AXP20X_VBUS_MON_VBUS_VALID,
> @@ -659,6 +714,9 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>  
>  static const struct of_device_id axp20x_usb_power_match[] = {
>  	{
> +		.compatible = "x-powers,axp192-usb-power-supply",
> +		.data = &axp192_data,
> +	}, {
>  		.compatible = "x-powers,axp202-usb-power-supply",
>  		.data = &axp202_data,
>  	}, {
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 14/17] pinctrl: Add AXP192 pin control driver

[Aidan MacDonald]

The AXP192 PMIC's GPIO registers are much different from the GPIO
registers of the AXP20x and AXP813 PMICs supported by the existing
pinctrl-axp209 driver. It makes more sense to add a new driver for
the AXP192, rather than add support in the existing axp20x driver.

The pinctrl-axp192 driver is considerably more flexible in terms of
register layout and should be able to support other X-Powers PMICs.
Interrupts and pull down resistor configuration are supported too.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/pinctrl/Kconfig          |  14 +
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-axp192.c | 589 +++++++++++++++++++++++++++++++
 3 files changed, 604 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-axp192.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index f52960d2dfbe..a71e35de333d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -113,6 +113,20 @@ config PINCTRL_AT91PIO4
 	  Say Y here to enable the at91 pinctrl/gpio driver for Atmel PIO4
 	  controller available on sama5d2 SoC.
 
+config PINCTRL_AXP192
+	tristate "X-Powers AXP192 PMIC pinctrl and GPIO Support"
+	depends on MFD_AXP20X
+	depends on OF
+	select PINMUX
+	select GENERIC_PINCONF
+	select GPIOLIB
+	help
+	  AXP PMICs provides multiple GPIOs that can be muxed for different
+	  functions. This driver bundles a pinctrl driver to select the function
+	  muxing and a GPIO driver to handle the GPIO when the GPIO function is
+	  selected.
+	  Say Y to enable pinctrl and GPIO support for the AXP192 PMIC.
+
 config PINCTRL_AXP209
 	tristate "X-Powers AXP209 PMIC pinctrl and GPIO Support"
 	depends on MFD_AXP20X
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e76f5cdc64b0..9d2b6420c5dd 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PINCTRL_ARTPEC6)	+= pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_AT91)	+= pinctrl-at91.o
 obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
+obj-$(CONFIG_PINCTRL_AXP192)	+= pinctrl-axp192.o
 obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
 obj-$(CONFIG_PINCTRL_BM1880)	+= pinctrl-bm1880.o
 obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
diff --git a/drivers/pinctrl/pinctrl-axp192.c b/drivers/pinctrl/pinctrl-axp192.c
new file mode 100644
index 000000000000..0ff2d0b84978
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-axp192.c
@@ -0,0 +1,589 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AXP192 pinctrl and GPIO driver
+ *
+ * Copyright (C) 2022 Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+enum {
+	AXP192_FUNC_OUTPUT = 0,
+	AXP192_FUNC_INPUT,
+	AXP192_FUNC_LDO,
+	AXP192_FUNC_PWM,
+	AXP192_FUNC_ADC,
+	AXP192_FUNC_LOW_OUTPUT,
+	AXP192_FUNC_FLOATING,
+	AXP192_FUNC_EXT_CHG_CTL,
+	AXP192_FUNC_LDO_STATUS,
+	AXP192_FUNCS_NB,
+};
+
+struct axp192_pctl_function {
+	const char		*name;
+	/* Mux value written to the control register to select the function (-1 if unsupported) */
+	const u8		*muxvals;
+	const char * const	*groups;
+	unsigned int		ngroups;
+};
+
+struct axp192_pctl_reg_info {
+	u8 reg;
+	u8 mask;
+};
+
+struct axp192_pctl_desc {
+	unsigned int				npins;
+	const struct pinctrl_pin_desc		*pins;
+	/* Description of the function control register for each pin */
+	const struct axp192_pctl_reg_info	*ctrl_regs;
+	/* Description of the output signal register for each pin */
+	const struct axp192_pctl_reg_info	*out_regs;
+	/* Description of the input signal register for each pin */
+	const struct axp192_pctl_reg_info	*in_regs;
+	/* Description of the pull down resistor config register for each pin */
+	const struct axp192_pctl_reg_info	*pull_down_regs;
+
+	unsigned int				nfunctions;
+	const struct axp192_pctl_function	*functions;
+};
+
+static const struct pinctrl_pin_desc axp192_pins[] = {
+	PINCTRL_PIN(0, "GPIO0"),
+	PINCTRL_PIN(1, "GPIO1"),
+	PINCTRL_PIN(2, "GPIO2"),
+	PINCTRL_PIN(3, "GPIO3"),
+	PINCTRL_PIN(4, "GPIO4"),
+	PINCTRL_PIN(5, "N_RSTO"),
+};
+
+static const char * const axp192_io_groups[] = { "GPIO0", "GPIO1", "GPIO2",
+						 "GPIO3", "GPIO4", "N_RSTO" };
+static const char * const axp192_ldo_groups[] = { "GPIO0" };
+static const char * const axp192_pwm_groups[] = { "GPIO1", "GPIO2" };
+static const char * const axp192_adc_groups[] = { "GPIO0", "GPIO1", "GPIO2", "GPIO3" };
+static const char * const axp192_extended_io_groups[] = { "GPIO0", "GPIO1", "GPIO2" };
+static const char * const axp192_ext_chg_ctl_groups[] = { "GPIO3", "GPIO4" };
+static const char * const axp192_ldo_status_groups[] = { "N_RSTO" };
+
+static const u8 axp192_output_muxvals[]		= {  0,  0,  0,  1,  1,  2 };
+static const u8 axp192_input_muxvals[]		= {  1,  1,  1,  2,  2,  3 };
+static const u8 axp192_ldo_muxvals[]		= {  2, -1, -1, -1, -1, -1 };
+static const u8 axp192_pwm_muxvals[]		= { -1,  2,  2, -1, -1, -1 };
+static const u8 axp192_adc_muxvals[]		= {  4,  4,  4,  3, -1, -1 };
+static const u8 axp192_low_output_muxvals[]	= {  5,  5,  5, -1, -1, -1 };
+static const u8 axp192_floating_muxvals[]	= {  6,  6,  6, -1, -1, -1 };
+static const u8 axp192_ext_chg_ctl_muxvals[]	= { -1, -1, -1,  0,  0, -1 };
+static const u8 axp192_ldo_status_muxvals[]	= { -1, -1, -1, -1, -1,  0 };
+
+static const struct axp192_pctl_function axp192_functions[AXP192_FUNCS_NB] = {
+	[AXP192_FUNC_OUTPUT] = {
+		.name = "output",
+		.muxvals = axp192_output_muxvals,
+		.groups = axp192_io_groups,
+		.ngroups = ARRAY_SIZE(axp192_io_groups),
+	},
+	[AXP192_FUNC_INPUT] = {
+		.name = "input",
+		.muxvals = axp192_input_muxvals,
+		.groups = axp192_io_groups,
+		.ngroups = ARRAY_SIZE(axp192_io_groups),
+	},
+	[AXP192_FUNC_LDO] = {
+		.name = "ldo",
+		.muxvals = axp192_ldo_muxvals,
+		.groups = axp192_ldo_groups,
+		.ngroups = ARRAY_SIZE(axp192_ldo_groups),
+	},
+	[AXP192_FUNC_PWM] = {
+		.name = "pwm",
+		.muxvals = axp192_pwm_muxvals,
+		.groups = axp192_pwm_groups,
+		.ngroups = ARRAY_SIZE(axp192_pwm_groups),
+	},
+	[AXP192_FUNC_ADC] = {
+		.name = "adc",
+		.muxvals = axp192_adc_muxvals,
+		.groups = axp192_adc_groups,
+		.ngroups = ARRAY_SIZE(axp192_adc_groups),
+	},
+	[AXP192_FUNC_LOW_OUTPUT] = {
+		.name = "low_output",
+		.muxvals = axp192_low_output_muxvals,
+		.groups = axp192_extended_io_groups,
+		.ngroups = ARRAY_SIZE(axp192_extended_io_groups),
+	},
+	[AXP192_FUNC_FLOATING] = {
+		.name = "floating",
+		.muxvals = axp192_floating_muxvals,
+		.groups = axp192_extended_io_groups,
+		.ngroups = ARRAY_SIZE(axp192_extended_io_groups),
+	},
+	[AXP192_FUNC_EXT_CHG_CTL] = {
+		.name = "ext_chg_ctl",
+		.muxvals = axp192_ext_chg_ctl_muxvals,
+		.groups = axp192_ext_chg_ctl_groups,
+		.ngroups = ARRAY_SIZE(axp192_ext_chg_ctl_groups),
+	},
+	[AXP192_FUNC_LDO_STATUS] = {
+		.name = "ldo_status",
+		.muxvals = axp192_ldo_status_muxvals,
+		.groups = axp192_ldo_groups,
+		.ngroups = ARRAY_SIZE(axp192_ldo_status_groups),
+	},
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_ctrl_regs[] = {
+	{ .reg = AXP192_GPIO0_CTRL,   .mask = 0x07 },
+	{ .reg = AXP192_GPIO1_CTRL,   .mask = 0x07 },
+	{ .reg = AXP192_GPIO2_CTRL,   .mask = 0x07 },
+	{ .reg = AXP192_GPIO4_3_CTRL, .mask = 0x03 },
+	{ .reg = AXP192_GPIO4_3_CTRL, .mask = 0x0c },
+	{ .reg = AXP192_N_RSTO_CTRL,  .mask = 0xc0 },
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_in_regs[] = {
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(4) },
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(5) },
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(6) },
+	{ .reg = AXP192_GPIO4_3_STATE, .mask = BIT(4) },
+	{ .reg = AXP192_GPIO4_3_STATE, .mask = BIT(5) },
+	{ .reg = AXP192_N_RSTO_CTRL,   .mask = BIT(4) },
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_out_regs[] = {
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(0) },
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(1) },
+	{ .reg = AXP192_GPIO2_0_STATE, .mask = BIT(2) },
+	{ .reg = AXP192_GPIO4_3_STATE, .mask = BIT(0) },
+	{ .reg = AXP192_GPIO4_3_STATE, .mask = BIT(1) },
+	{ .reg = AXP192_N_RSTO_CTRL,   .mask = BIT(5) },
+};
+
+static const struct axp192_pctl_reg_info axp192_pull_down_regs[] = {
+	{ .reg = AXP192_GPIO2_0_PULL, .mask = BIT(0) },
+	{ .reg = AXP192_GPIO2_0_PULL, .mask = BIT(1) },
+	{ .reg = AXP192_GPIO2_0_PULL, .mask = BIT(2) },
+	{ .reg = 0, .mask = 0 /* unsupported */ },
+	{ .reg = 0, .mask = 0 /* unsupported */ },
+	{ .reg = 0, .mask = 0 /* unsupported */ },
+};
+
+static const struct axp192_pctl_desc axp192_data = {
+	.npins = ARRAY_SIZE(axp192_pins),
+	.pins = axp192_pins,
+	.ctrl_regs = axp192_pin_ctrl_regs,
+	.out_regs = axp192_pin_out_regs,
+	.in_regs = axp192_pin_in_regs,
+	.pull_down_regs = axp192_pull_down_regs,
+
+	.nfunctions = ARRAY_SIZE(axp192_functions),
+	.functions = axp192_functions,
+};
+
+
+struct axp192_pctl {
+	struct gpio_chip		chip;
+	struct regmap			*regmap;
+	struct pinctrl_dev		*pctl_dev;
+	struct device			*dev;
+	const struct axp192_pctl_desc	*desc;
+	int				*irqs;
+};
+
+static int axp192_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct axp192_pctl *pctl = gpiochip_get_data(chip);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->in_regs[offset];
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & reginfo->mask);
+}
+
+static int axp192_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct axp192_pctl *pctl = gpiochip_get_data(chip);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->ctrl_regs[offset];
+	const u8 *input_muxvals = pctl->desc->functions[AXP192_FUNC_INPUT].muxvals;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+	if (ret)
+		return ret;
+
+	if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
+		return GPIO_LINE_DIRECTION_IN;
+	else
+		return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void axp192_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct axp192_pctl *pctl = gpiochip_get_data(chip);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->out_regs[offset];
+
+	regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask, value ? reginfo->mask : 0);
+}
+
+static int axp192_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	return pinctrl_gpio_direction_input(chip->base + offset);
+}
+
+static int axp192_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	chip->set(chip, offset, value);
+	return 0;
+}
+
+static int axp192_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct axp192_pctl *pctl = gpiochip_get_data(chip);
+
+	return pctl->irqs[offset];
+}
+
+static int axp192_pinconf_get_pull_down(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->pull_down_regs[pin];
+	unsigned int val;
+	int ret;
+
+	if (!reginfo->mask)
+		return -EOPNOTSUPP;
+
+	ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & reginfo->mask);
+}
+
+static int axp192_pinconf_set_pull_down(struct pinctrl_dev *pctldev, unsigned int pin, int value)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->pull_down_regs[pin];
+
+	if (!reginfo->mask)
+		return -EOPNOTSUPP;
+
+	return regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask,
+				  value ? reginfo->mask : 0);
+}
+
+static int axp192_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, unsigned long *config)
+{
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned int arg = 1;
+	int ret;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		ret = axp192_pinconf_get_pull_down(pctldev, pin);
+		if (ret < 0)
+			return ret;
+		else if (ret != 0)
+			return -EINVAL;
+		break;
+
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		ret = axp192_pinconf_get_pull_down(pctldev, pin);
+		if (ret < 0)
+			return ret;
+		else if (ret == 0)
+			return -EINVAL;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+	return 0;
+}
+
+static int axp192_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			      unsigned long *configs, unsigned int num_configs)
+{
+	int ret;
+	unsigned int cfg;
+
+	for (cfg = 0; cfg < num_configs; ++cfg) {
+		switch (pinconf_to_config_param(configs[cfg])) {
+		case PIN_CONFIG_BIAS_DISABLE:
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			continue;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	for (cfg = 0; cfg < num_configs; ++cfg) {
+		switch (pinconf_to_config_param(configs[cfg])) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			ret = axp192_pinconf_set_pull_down(pctldev, pin, 0);
+			if (ret)
+				return ret;
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			ret = axp192_pinconf_set_pull_down(pctldev, pin, 1);
+			if (ret)
+				return ret;
+			break;
+
+		default:
+			/* unreachable */
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops axp192_conf_ops = {
+	.is_generic = true,
+	.pin_config_get = axp192_pinconf_get,
+	.pin_config_set = axp192_pinconf_set,
+	.pin_config_group_get = axp192_pinconf_get,
+	.pin_config_group_set = axp192_pinconf_set,
+};
+
+static int axp192_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset, u8 config)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct axp192_pctl_reg_info *reginfo = &pctl->desc->ctrl_regs[offset];
+	unsigned int regval = config << (ffs(reginfo->mask) - 1);
+
+	return regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask, regval);
+}
+
+static int axp192_pmx_func_cnt(struct pinctrl_dev *pctldev)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->desc->nfunctions;
+}
+
+static const char *axp192_pmx_func_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->desc->functions[selector].name;
+}
+
+static int axp192_pmx_func_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+				  const char * const **groups, unsigned int *num_groups)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pctl->desc->functions[selector].groups;
+	*num_groups = pctl->desc->functions[selector].ngroups;
+
+	return 0;
+}
+
+static int axp192_pmx_set_mux(struct pinctrl_dev *pctldev,
+			      unsigned int function, unsigned int group)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const u8 *muxvals = pctl->desc->functions[function].muxvals;
+
+	if (muxvals[group] == (u8)-1)
+		return -EINVAL;
+
+	/*
+	 * Switching to LDO or PWM function will enable LDO/PWM output, so it's
+	 * better to ignore these requests and let the regulator or PWM drivers
+	 * handle muxing to avoid interfering with them.
+	 */
+	if (function == AXP192_FUNC_LDO || function == AXP192_FUNC_PWM)
+		return 0;
+
+	return axp192_pmx_set(pctldev, group, muxvals[group]);
+}
+
+static int axp192_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					 struct pinctrl_gpio_range *range,
+					 unsigned int offset, bool input)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	const u8 *muxvals = input ? pctl->desc->functions[AXP192_FUNC_INPUT].muxvals
+				  : pctl->desc->functions[AXP192_FUNC_OUTPUT].muxvals;
+
+	return axp192_pmx_set(pctldev, offset, muxvals[offset]);
+}
+
+static const struct pinmux_ops axp192_pmx_ops = {
+	.get_functions_count	= axp192_pmx_func_cnt,
+	.get_function_name	= axp192_pmx_func_name,
+	.get_function_groups	= axp192_pmx_func_groups,
+	.set_mux		= axp192_pmx_set_mux,
+	.gpio_set_direction	= axp192_pmx_gpio_set_direction,
+	.strict			= true,
+};
+
+static int axp192_groups_cnt(struct pinctrl_dev *pctldev)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->desc->npins;
+}
+
+static const char *axp192_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->desc->pins[selector].name;
+}
+
+static int axp192_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
+			     const unsigned int **pins, unsigned int *num_pins)
+{
+	struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = &pctl->desc->pins[selector].number;
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops axp192_pctrl_ops = {
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
+	.dt_free_map		= pinconf_generic_dt_free_map,
+	.get_groups_count	= axp192_groups_cnt,
+	.get_group_name		= axp192_group_name,
+	.get_group_pins		= axp192_group_pins,
+};
+
+static int axp192_pctl_probe(struct platform_device *pdev)
+{
+	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct axp192_pctl *pctl;
+	struct pinctrl_desc *pctrl_desc;
+	int ret, i;
+
+	if (!of_device_is_available(pdev->dev.of_node))
+		return -ENODEV;
+
+	if (!axp20x) {
+		dev_err(&pdev->dev, "Parent drvdata not set\n");
+		return -EINVAL;
+	}
+
+	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+	if (!pctl)
+		return -ENOMEM;
+
+	pctl->desc = of_device_get_match_data(&pdev->dev);
+	pctl->regmap = axp20x->regmap;
+	pctl->dev = &pdev->dev;
+
+	pctl->chip.base			= -1;
+	pctl->chip.can_sleep		= true;
+	pctl->chip.request		= gpiochip_generic_request;
+	pctl->chip.free			= gpiochip_generic_free;
+	pctl->chip.parent		= &pdev->dev;
+	pctl->chip.label		= dev_name(&pdev->dev);
+	pctl->chip.owner		= THIS_MODULE;
+	pctl->chip.get			= axp192_gpio_get;
+	pctl->chip.get_direction	= axp192_gpio_get_direction;
+	pctl->chip.set			= axp192_gpio_set;
+	pctl->chip.direction_input	= axp192_gpio_direction_input;
+	pctl->chip.direction_output	= axp192_gpio_direction_output;
+	pctl->chip.to_irq		= axp192_gpio_to_irq;
+	pctl->chip.ngpio		= pctl->desc->npins;
+
+	pctl->irqs = devm_kcalloc(&pdev->dev, pctl->desc->npins, sizeof(int), GFP_KERNEL);
+	if (!pctl->irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < pctl->desc->npins; ++i) {
+		ret = platform_get_irq_byname_optional(pdev, pctl->desc->pins[i].name);
+		if (ret > 0)
+			pctl->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, ret);
+	}
+
+	platform_set_drvdata(pdev, pctl);
+
+	pctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
+	if (!pctrl_desc)
+		return -ENOMEM;
+
+	pctrl_desc->name = dev_name(&pdev->dev);
+	pctrl_desc->owner = THIS_MODULE;
+	pctrl_desc->pins = pctl->desc->pins;
+	pctrl_desc->npins = pctl->desc->npins;
+	pctrl_desc->pctlops = &axp192_pctrl_ops;
+	pctrl_desc->pmxops = &axp192_pmx_ops;
+	pctrl_desc->confops = &axp192_conf_ops;
+
+	pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
+	if (IS_ERR(pctl->pctl_dev)) {
+		dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
+		return PTR_ERR(pctl->pctl_dev);
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &pctl->chip, pctl);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register GPIO chip\n");
+		return ret;
+	}
+
+	ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
+				     pctl->desc->pins->number,
+				     pctl->desc->pins->number,
+				     pctl->desc->npins);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add pin range\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "AXP192 pinctrl and GPIO driver loaded\n");
+
+	return 0;
+}
+
+static const struct of_device_id axp192_pctl_match[] = {
+	{ .compatible = "x-powers,axp192-gpio", .data = &axp192_data, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axp192_pctl_match);
+
+static struct platform_driver axp192_pctl_driver = {
+	.probe		= axp192_pctl_probe,
+	.driver = {
+		.name		= "axp192-gpio",
+		.of_match_table	= axp192_pctl_match,
+	},
+};
+
+module_platform_driver(axp192_pctl_driver);
+
+MODULE_AUTHOR("Aidan MacDonald <aidanmacdonald.0x0@gmail.com>");
+MODULE_DESCRIPTION("AXP192 PMIC pinctrl and GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v2 15/17] power: axp20x_battery: Add constant charge current table

[Aidan MacDonald]

Add a table-based lookup method for constant charge current,
which is necessary when the setting cannot be represented as
a linear range.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/power/supply/axp20x_battery.c | 53 +++++++++++++++++++++------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index 9106077c0dbb..87fb958f2224 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -61,6 +61,7 @@ struct axp20x_batt_ps;
 struct axp_data {
 	int	ccc_scale;
 	int	ccc_offset;
+	const int *ccc_table;
 	bool	has_fg_valid;
 	int	(*get_max_voltage)(struct axp20x_batt_ps *batt, int *val);
 	int	(*set_max_voltage)(struct axp20x_batt_ps *batt, int val);
@@ -176,7 +177,10 @@ static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp,
 
 	*val &= AXP20X_CHRG_CTRL1_TGT_CURR;
 
-	*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
+	if (axp->data->ccc_table)
+		*val = axp->data->ccc_table[*val];
+	else
+		*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
 
 	return 0;
 }
@@ -389,16 +393,36 @@ static int axp20x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
 				  AXP20X_CHRG_CTRL1_TGT_VOLT, val);
 }
 
+static int axp20x_get_constant_charge_current_sel(struct axp20x_batt_ps *axp_batt,
+						  int charge_current)
+{
+	int i;
+
+	if (axp_batt->data->ccc_table) {
+		for (i = AXP20X_CHRG_CTRL1_TGT_CURR; i >= 0; --i) {
+			if (axp_batt->data->ccc_table[i] <= charge_current)
+				return i;
+		}
+
+		return -EINVAL;
+	}
+
+	i = (charge_current - axp_batt->data->ccc_offset) / axp_batt->data->ccc_scale;
+
+	if (i > AXP20X_CHRG_CTRL1_TGT_CURR || i < 0)
+		return -EINVAL;
+
+	return i;
+}
+
 static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
 					      int charge_current)
 {
 	if (charge_current > axp_batt->max_ccc)
 		return -EINVAL;
 
-	charge_current = (charge_current - axp_batt->data->ccc_offset) /
-		axp_batt->data->ccc_scale;
-
-	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
+	charge_current = axp20x_get_constant_charge_current_sel(axp_batt, charge_current);
+	if (charge_current < 0)
 		return -EINVAL;
 
 	return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
@@ -410,14 +434,14 @@ static int axp20x_set_max_constant_charge_current(struct axp20x_batt_ps *axp,
 {
 	bool lower_max = false;
 
-	charge_current = (charge_current - axp->data->ccc_offset) /
-		axp->data->ccc_scale;
-
-	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
+	charge_current = axp20x_get_constant_charge_current_sel(axp, charge_current);
+	if (charge_current < 0)
 		return -EINVAL;
 
-	charge_current = charge_current * axp->data->ccc_scale +
-		axp->data->ccc_offset;
+	if (axp->data->ccc_table)
+		charge_current = axp->data->ccc_table[charge_current];
+	else
+		charge_current = charge_current * axp->data->ccc_scale + axp->data->ccc_offset;
 
 	if (charge_current > axp->max_ccc)
 		dev_warn(axp->dev,
@@ -629,7 +653,12 @@ static int axp20x_power_probe(struct platform_device *pdev)
 								   ccc)) {
 			dev_err(&pdev->dev,
 				"couldn't set constant charge current from DT: fallback to minimum value\n");
-			ccc = 300000;
+
+			if (axp20x_batt->data->ccc_table)
+				ccc = axp20x_batt->data->ccc_table[0];
+			else
+				ccc = axp20x_batt->data->ccc_offset;
+
 			axp20x_batt->max_ccc = ccc;
 			axp20x_set_constant_charge_current(axp20x_batt, ccc);
 		}
-- 
2.35.1

Re: [PATCH v2 15/17] power: axp20x_battery: Add constant charge current table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4108 bytes --]

Hi,

On Tue, Jun 07, 2022 at 04:53:22PM +0100, Aidan MacDonald wrote:
> Add a table-based lookup method for constant charge current,
> which is necessary when the setting cannot be represented as
> a linear range.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  drivers/power/supply/axp20x_battery.c | 53 +++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> index 9106077c0dbb..87fb958f2224 100644
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -61,6 +61,7 @@ struct axp20x_batt_ps;
>  struct axp_data {
>  	int	ccc_scale;
>  	int	ccc_offset;
> +	const int *ccc_table;

Please document the struct; especially the fact that ccc_table must
have a size of AXP20X_CHRG_CTRL1_TGT_CURR + 1.

-- Sebastian

>  	bool	has_fg_valid;
>  	int	(*get_max_voltage)(struct axp20x_batt_ps *batt, int *val);
>  	int	(*set_max_voltage)(struct axp20x_batt_ps *batt, int val);
> @@ -176,7 +177,10 @@ static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp,
>  
>  	*val &= AXP20X_CHRG_CTRL1_TGT_CURR;
>  
> -	*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
> +	if (axp->data->ccc_table)
> +		*val = axp->data->ccc_table[*val];
> +	else
> +		*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
>  
>  	return 0;
>  }
> @@ -389,16 +393,36 @@ static int axp20x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
>  				  AXP20X_CHRG_CTRL1_TGT_VOLT, val);
>  }
>  
> +static int axp20x_get_constant_charge_current_sel(struct axp20x_batt_ps *axp_batt,
> +						  int charge_current)
> +{
> +	int i;
> +
> +	if (axp_batt->data->ccc_table) {
> +		for (i = AXP20X_CHRG_CTRL1_TGT_CURR; i >= 0; --i) {
> +			if (axp_batt->data->ccc_table[i] <= charge_current)
> +				return i;
> +		}
> +
> +		return -EINVAL;
> +	}
> +
> +	i = (charge_current - axp_batt->data->ccc_offset) / axp_batt->data->ccc_scale;
> +
> +	if (i > AXP20X_CHRG_CTRL1_TGT_CURR || i < 0)
> +		return -EINVAL;
> +
> +	return i;
> +}
> +
>  static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
>  					      int charge_current)
>  {
>  	if (charge_current > axp_batt->max_ccc)
>  		return -EINVAL;
>  
> -	charge_current = (charge_current - axp_batt->data->ccc_offset) /
> -		axp_batt->data->ccc_scale;
> -
> -	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
> +	charge_current = axp20x_get_constant_charge_current_sel(axp_batt, charge_current);
> +	if (charge_current < 0)
>  		return -EINVAL;
>  
>  	return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
> @@ -410,14 +434,14 @@ static int axp20x_set_max_constant_charge_current(struct axp20x_batt_ps *axp,
>  {
>  	bool lower_max = false;
>  
> -	charge_current = (charge_current - axp->data->ccc_offset) /
> -		axp->data->ccc_scale;
> -
> -	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
> +	charge_current = axp20x_get_constant_charge_current_sel(axp, charge_current);
> +	if (charge_current < 0)
>  		return -EINVAL;
>  
> -	charge_current = charge_current * axp->data->ccc_scale +
> -		axp->data->ccc_offset;
> +	if (axp->data->ccc_table)
> +		charge_current = axp->data->ccc_table[charge_current];
> +	else
> +		charge_current = charge_current * axp->data->ccc_scale + axp->data->ccc_offset;
>  
>  	if (charge_current > axp->max_ccc)
>  		dev_warn(axp->dev,
> @@ -629,7 +653,12 @@ static int axp20x_power_probe(struct platform_device *pdev)
>  								   ccc)) {
>  			dev_err(&pdev->dev,
>  				"couldn't set constant charge current from DT: fallback to minimum value\n");
> -			ccc = 300000;
> +
> +			if (axp20x_batt->data->ccc_table)
> +				ccc = axp20x_batt->data->ccc_table[0];
> +			else
> +				ccc = axp20x_batt->data->ccc_offset;
> +
>  			axp20x_batt->max_ccc = ccc;
>  			axp20x_set_constant_charge_current(axp20x_batt, ccc);
>  		}
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 15/17] power: axp20x_battery: Add constant charge current table

[Aidan MacDonald]

Sebastian Reichel <sebastian.reichel@collabora.com> writes:

> Hi,
>
> On Tue, Jun 07, 2022 at 04:53:22PM +0100, Aidan MacDonald wrote:
>> Add a table-based lookup method for constant charge current,
>> which is necessary when the setting cannot be represented as
>> a linear range.
>> 
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  drivers/power/supply/axp20x_battery.c | 53 +++++++++++++++++++++------
>>  1 file changed, 41 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
>> index 9106077c0dbb..87fb958f2224 100644
>> --- a/drivers/power/supply/axp20x_battery.c
>> +++ b/drivers/power/supply/axp20x_battery.c
>> @@ -61,6 +61,7 @@ struct axp20x_batt_ps;
>>  struct axp_data {
>>  	int	ccc_scale;
>>  	int	ccc_offset;
>> +	const int *ccc_table;
>
> Please document the struct; especially the fact that ccc_table must
> have a size of AXP20X_CHRG_CTRL1_TGT_CURR + 1.
>
> -- Sebastian
>

Thanks, I'll make sure to do that in v3.

Regards,
Aidan

>>  	bool	has_fg_valid;
>>  	int	(*get_max_voltage)(struct axp20x_batt_ps *batt, int *val);
>>  	int	(*set_max_voltage)(struct axp20x_batt_ps *batt, int val);
>> @@ -176,7 +177,10 @@ static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp,
>>  
>>  	*val &= AXP20X_CHRG_CTRL1_TGT_CURR;
>>  
>> -	*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
>> +	if (axp->data->ccc_table)
>> +		*val = axp->data->ccc_table[*val];
>> +	else
>> +		*val = *val * axp->data->ccc_scale + axp->data->ccc_offset;
>>  
>>  	return 0;
>>  }
>> @@ -389,16 +393,36 @@ static int axp20x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
>>  				  AXP20X_CHRG_CTRL1_TGT_VOLT, val);
>>  }
>>  
>> +static int axp20x_get_constant_charge_current_sel(struct axp20x_batt_ps *axp_batt,
>> +						  int charge_current)
>> +{
>> +	int i;
>> +
>> +	if (axp_batt->data->ccc_table) {
>> +		for (i = AXP20X_CHRG_CTRL1_TGT_CURR; i >= 0; --i) {
>> +			if (axp_batt->data->ccc_table[i] <= charge_current)
>> +				return i;
>> +		}
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	i = (charge_current - axp_batt->data->ccc_offset) / axp_batt->data->ccc_scale;
>> +
>> +	if (i > AXP20X_CHRG_CTRL1_TGT_CURR || i < 0)
>> +		return -EINVAL;
>> +
>> +	return i;
>> +}
>> +
>>  static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
>>  					      int charge_current)
>>  {
>>  	if (charge_current > axp_batt->max_ccc)
>>  		return -EINVAL;
>>  
>> -	charge_current = (charge_current - axp_batt->data->ccc_offset) /
>> -		axp_batt->data->ccc_scale;
>> -
>> -	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
>> +	charge_current = axp20x_get_constant_charge_current_sel(axp_batt, charge_current);
>> +	if (charge_current < 0)
>>  		return -EINVAL;
>>  
>>  	return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
>> @@ -410,14 +434,14 @@ static int axp20x_set_max_constant_charge_current(struct axp20x_batt_ps *axp,
>>  {
>>  	bool lower_max = false;
>>  
>> -	charge_current = (charge_current - axp->data->ccc_offset) /
>> -		axp->data->ccc_scale;
>> -
>> -	if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
>> +	charge_current = axp20x_get_constant_charge_current_sel(axp, charge_current);
>> +	if (charge_current < 0)
>>  		return -EINVAL;
>>  
>> -	charge_current = charge_current * axp->data->ccc_scale +
>> -		axp->data->ccc_offset;
>> +	if (axp->data->ccc_table)
>> +		charge_current = axp->data->ccc_table[charge_current];
>> +	else
>> +		charge_current = charge_current * axp->data->ccc_scale + axp->data->ccc_offset;
>>  
>>  	if (charge_current > axp->max_ccc)
>>  		dev_warn(axp->dev,
>> @@ -629,7 +653,12 @@ static int axp20x_power_probe(struct platform_device *pdev)
>>  								   ccc)) {
>>  			dev_err(&pdev->dev,
>>  				"couldn't set constant charge current from DT: fallback to minimum value\n");
>> -			ccc = 300000;
>> +
>> +			if (axp20x_batt->data->ccc_table)
>> +				ccc = axp20x_batt->data->ccc_table[0];
>> +			else
>> +				ccc = axp20x_batt->data->ccc_offset;
>> +
>>  			axp20x_batt->max_ccc = ccc;
>>  			axp20x_set_constant_charge_current(axp20x_batt, ccc);
>>  		}
>> -- 
>> 2.35.1
>> 

[PATCH v2 16/17] power: axp20x_battery: Support battery status without fuel gauge

[Aidan MacDonald]

Add a "has_fg" flag to indicate that the chip has a fuel gauge.
Report battery full status on chips with no fuel gauge using the
battery voltage.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/power/supply/axp20x_battery.c | 34 ++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index 87fb958f2224..e9547e2d7c48 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -62,6 +62,7 @@ struct axp_data {
 	int	ccc_scale;
 	int	ccc_offset;
 	const int *ccc_table;
+	bool	has_fg;
 	bool	has_fg_valid;
 	int	(*get_max_voltage)(struct axp20x_batt_ps *batt, int *val);
 	int	(*set_max_voltage)(struct axp20x_batt_ps *batt, int val);
@@ -190,7 +191,7 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
 				   union power_supply_propval *val)
 {
 	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
-	int ret = 0, reg, val1;
+	int ret = 0, reg, val1, val2;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
@@ -224,6 +225,34 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
 			return 0;
 		}
 
+		/*
+		 * If the chip does not have a fuel gauge, we check battery full status
+		 * using voltage instead.
+		 */
+		if (!axp20x_batt->data->has_fg) {
+			ret = axp20x_batt->data->get_max_voltage(axp20x_batt, &val1);
+			if (ret)
+				return ret;
+
+			ret = iio_read_channel_processed(axp20x_batt->batt_v, &val2);
+			if (ret)
+				return ret;
+
+			/* IIO subsystem reports voltage in mV but we need uV */
+			val2 *= 1000;
+
+			/*
+			 * According to the AXP192 datasheet, charging will restart if
+			 * the battery voltage drops below V_rch = V_tgt - 0.1 V, so we
+			 * report the battery is full if its voltage is at least V_rch.
+			 */
+			if (val2 >= val1 - 100000)
+				val->intval = POWER_SUPPLY_STATUS_FULL;
+			else
+				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+			break;
+		}
+
 		ret = regmap_read(axp20x_batt->regmap, AXP20X_FG_RES, &val1);
 		if (ret)
 			return ret;
@@ -546,6 +575,7 @@ static const struct power_supply_desc axp20x_batt_ps_desc = {
 static const struct axp_data axp209_data = {
 	.ccc_scale = 100000,
 	.ccc_offset = 300000,
+	.has_fg = true,
 	.get_max_voltage = axp20x_battery_get_max_voltage,
 	.set_max_voltage = axp20x_battery_set_max_voltage,
 };
@@ -553,6 +583,7 @@ static const struct axp_data axp209_data = {
 static const struct axp_data axp221_data = {
 	.ccc_scale = 150000,
 	.ccc_offset = 300000,
+	.has_fg = true,
 	.has_fg_valid = true,
 	.get_max_voltage = axp22x_battery_get_max_voltage,
 	.set_max_voltage = axp22x_battery_set_max_voltage,
@@ -561,6 +592,7 @@ static const struct axp_data axp221_data = {
 static const struct axp_data axp813_data = {
 	.ccc_scale = 200000,
 	.ccc_offset = 200000,
+	.has_fg = true,
 	.has_fg_valid = true,
 	.get_max_voltage = axp813_battery_get_max_voltage,
 	.set_max_voltage = axp20x_battery_set_max_voltage,
-- 
2.35.1

Re: [PATCH v2 16/17] power: axp20x_battery: Support battery status without fuel gauge

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

Hi,

On Tue, Jun 07, 2022 at 04:53:23PM +0100, Aidan MacDonald wrote:
> Add a "has_fg" flag to indicate that the chip has a fuel gauge.
> Report battery full status on chips with no fuel gauge using the
> battery voltage.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/power/supply/axp20x_battery.c | 34 ++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> index 87fb958f2224..e9547e2d7c48 100644
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -62,6 +62,7 @@ struct axp_data {
>  	int	ccc_scale;
>  	int	ccc_offset;
>  	const int *ccc_table;
> +	bool	has_fg;
>  	bool	has_fg_valid;
>  	int	(*get_max_voltage)(struct axp20x_batt_ps *batt, int *val);
>  	int	(*set_max_voltage)(struct axp20x_batt_ps *batt, int val);
> @@ -190,7 +191,7 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>  				   union power_supply_propval *val)
>  {
>  	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
> -	int ret = 0, reg, val1;
> +	int ret = 0, reg, val1, val2;
>  
>  	switch (psp) {
>  	case POWER_SUPPLY_PROP_PRESENT:
> @@ -224,6 +225,34 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>  			return 0;
>  		}
>  
> +		/*
> +		 * If the chip does not have a fuel gauge, we check battery full status
> +		 * using voltage instead.
> +		 */
> +		if (!axp20x_batt->data->has_fg) {
> +			ret = axp20x_batt->data->get_max_voltage(axp20x_batt, &val1);
> +			if (ret)
> +				return ret;
> +
> +			ret = iio_read_channel_processed(axp20x_batt->batt_v, &val2);
> +			if (ret)
> +				return ret;
> +
> +			/* IIO subsystem reports voltage in mV but we need uV */
> +			val2 *= 1000;
> +
> +			/*
> +			 * According to the AXP192 datasheet, charging will restart if
> +			 * the battery voltage drops below V_rch = V_tgt - 0.1 V, so we
> +			 * report the battery is full if its voltage is at least V_rch.
> +			 */
> +			if (val2 >= val1 - 100000)
> +				val->intval = POWER_SUPPLY_STATUS_FULL;
> +			else
> +				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +			break;
> +		}
> +
>  		ret = regmap_read(axp20x_batt->regmap, AXP20X_FG_RES, &val1);
>  		if (ret)
>  			return ret;
> @@ -546,6 +575,7 @@ static const struct power_supply_desc axp20x_batt_ps_desc = {
>  static const struct axp_data axp209_data = {
>  	.ccc_scale = 100000,
>  	.ccc_offset = 300000,
> +	.has_fg = true,
>  	.get_max_voltage = axp20x_battery_get_max_voltage,
>  	.set_max_voltage = axp20x_battery_set_max_voltage,
>  };
> @@ -553,6 +583,7 @@ static const struct axp_data axp209_data = {
>  static const struct axp_data axp221_data = {
>  	.ccc_scale = 150000,
>  	.ccc_offset = 300000,
> +	.has_fg = true,
>  	.has_fg_valid = true,
>  	.get_max_voltage = axp22x_battery_get_max_voltage,
>  	.set_max_voltage = axp22x_battery_set_max_voltage,
> @@ -561,6 +592,7 @@ static const struct axp_data axp221_data = {
>  static const struct axp_data axp813_data = {
>  	.ccc_scale = 200000,
>  	.ccc_offset = 200000,
> +	.has_fg = true,
>  	.has_fg_valid = true,
>  	.get_max_voltage = axp813_battery_get_max_voltage,
>  	.set_max_voltage = axp20x_battery_set_max_voltage,
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 17/17] power: axp20x_battery: Add support for AXP192

[Aidan MacDonald]

The AXP192 has a battery charger similar to other X-Powers PMICs,
but unlike the other supported devices, it does not have a fuel
gauge and can't report battery capacity directly.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/power/supply/axp20x_battery.c | 49 +++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index e9547e2d7c48..3fa2faa6f0f8 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -538,6 +538,19 @@ static int axp20x_battery_set_prop(struct power_supply *psy,
 	}
 }
 
+static enum power_supply_property axp192_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+};
+
 static enum power_supply_property axp20x_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -562,6 +575,16 @@ static int axp20x_battery_prop_writeable(struct power_supply *psy,
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
 }
 
+static const struct power_supply_desc axp192_batt_ps_desc = {
+	.name = "axp192-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = axp192_battery_props,
+	.num_properties = ARRAY_SIZE(axp192_battery_props),
+	.property_is_writeable = axp20x_battery_prop_writeable,
+	.get_property = axp20x_battery_get_prop,
+	.set_property = axp20x_battery_set_prop,
+};
+
 static const struct power_supply_desc axp20x_batt_ps_desc = {
 	.name = "axp20x-battery",
 	.type = POWER_SUPPLY_TYPE_BATTERY,
@@ -572,6 +595,19 @@ static const struct power_supply_desc axp20x_batt_ps_desc = {
 	.set_property = axp20x_battery_set_prop,
 };
 
+static const int axp192_ccc_table[AXP20X_CHRG_CTRL1_TGT_CURR+1] = {
+	100000,  190000,  280000,  360000,
+	450000,  550000,  630000,  700000,
+	780000,  880000,  960000,  1000000,
+	1080000, 1160000, 1240000, 1320000,
+};
+
+static const struct axp_data axp192_data = {
+	.ccc_table = axp192_ccc_table,
+	.get_max_voltage = axp20x_battery_get_max_voltage,
+	.set_max_voltage = axp20x_battery_set_max_voltage,
+};
+
 static const struct axp_data axp209_data = {
 	.ccc_scale = 100000,
 	.ccc_offset = 300000,
@@ -600,6 +636,9 @@ static const struct axp_data axp813_data = {
 
 static const struct of_device_id axp20x_battery_ps_id[] = {
 	{
+		.compatible = "x-powers,axp192-battery-power-supply",
+		.data = (void *)&axp192_data,
+	}, {
 		.compatible = "x-powers,axp209-battery-power-supply",
 		.data = (void *)&axp209_data,
 	}, {
@@ -617,6 +656,7 @@ static int axp20x_power_probe(struct platform_device *pdev)
 	struct axp20x_batt_ps *axp20x_batt;
 	struct power_supply_config psy_cfg = {};
 	struct power_supply_battery_info *info;
+	const struct power_supply_desc *ps_desc;
 	struct device *dev = &pdev->dev;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -660,9 +700,12 @@ static int axp20x_power_probe(struct platform_device *pdev)
 
 	axp20x_batt->data = (struct axp_data *)of_device_get_match_data(dev);
 
-	axp20x_batt->batt = devm_power_supply_register(&pdev->dev,
-						       &axp20x_batt_ps_desc,
-						       &psy_cfg);
+	if (!axp20x_batt->data->has_fg)
+		ps_desc = &axp192_batt_ps_desc;
+	else
+		ps_desc = &axp20x_batt_ps_desc;
+
+	axp20x_batt->batt = devm_power_supply_register(&pdev->dev, ps_desc, &psy_cfg);
 	if (IS_ERR(axp20x_batt->batt)) {
 		dev_err(&pdev->dev, "failed to register power supply: %ld\n",
 			PTR_ERR(axp20x_batt->batt));
-- 
2.35.1

Re: [PATCH v2 17/17] power: axp20x_battery: Add support for AXP192

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4253 bytes --]

Hi,

On Tue, Jun 07, 2022 at 04:53:24PM +0100, Aidan MacDonald wrote:
> The AXP192 has a battery charger similar to other X-Powers PMICs,
> but unlike the other supported devices, it does not have a fuel
> gauge and can't report battery capacity directly.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

>  drivers/power/supply/axp20x_battery.c | 49 +++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> index e9547e2d7c48..3fa2faa6f0f8 100644
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -538,6 +538,19 @@ static int axp20x_battery_set_prop(struct power_supply *psy,
>  	}
>  }
>  
> +static enum power_supply_property axp192_battery_props[] = {
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +};
> +
>  static enum power_supply_property axp20x_battery_props[] = {
>  	POWER_SUPPLY_PROP_PRESENT,
>  	POWER_SUPPLY_PROP_ONLINE,
> @@ -562,6 +575,16 @@ static int axp20x_battery_prop_writeable(struct power_supply *psy,
>  	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
>  }
>  
> +static const struct power_supply_desc axp192_batt_ps_desc = {
> +	.name = "axp192-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.properties = axp192_battery_props,
> +	.num_properties = ARRAY_SIZE(axp192_battery_props),
> +	.property_is_writeable = axp20x_battery_prop_writeable,
> +	.get_property = axp20x_battery_get_prop,
> +	.set_property = axp20x_battery_set_prop,
> +};
> +
>  static const struct power_supply_desc axp20x_batt_ps_desc = {
>  	.name = "axp20x-battery",
>  	.type = POWER_SUPPLY_TYPE_BATTERY,
> @@ -572,6 +595,19 @@ static const struct power_supply_desc axp20x_batt_ps_desc = {
>  	.set_property = axp20x_battery_set_prop,
>  };
>  
> +static const int axp192_ccc_table[AXP20X_CHRG_CTRL1_TGT_CURR+1] = {
> +	100000,  190000,  280000,  360000,
> +	450000,  550000,  630000,  700000,
> +	780000,  880000,  960000,  1000000,
> +	1080000, 1160000, 1240000, 1320000,
> +};
> +
> +static const struct axp_data axp192_data = {
> +	.ccc_table = axp192_ccc_table,
> +	.get_max_voltage = axp20x_battery_get_max_voltage,
> +	.set_max_voltage = axp20x_battery_set_max_voltage,
> +};
> +
>  static const struct axp_data axp209_data = {
>  	.ccc_scale = 100000,
>  	.ccc_offset = 300000,
> @@ -600,6 +636,9 @@ static const struct axp_data axp813_data = {
>  
>  static const struct of_device_id axp20x_battery_ps_id[] = {
>  	{
> +		.compatible = "x-powers,axp192-battery-power-supply",
> +		.data = (void *)&axp192_data,
> +	}, {
>  		.compatible = "x-powers,axp209-battery-power-supply",
>  		.data = (void *)&axp209_data,
>  	}, {
> @@ -617,6 +656,7 @@ static int axp20x_power_probe(struct platform_device *pdev)
>  	struct axp20x_batt_ps *axp20x_batt;
>  	struct power_supply_config psy_cfg = {};
>  	struct power_supply_battery_info *info;
> +	const struct power_supply_desc *ps_desc;
>  	struct device *dev = &pdev->dev;
>  
>  	if (!of_device_is_available(pdev->dev.of_node))
> @@ -660,9 +700,12 @@ static int axp20x_power_probe(struct platform_device *pdev)
>  
>  	axp20x_batt->data = (struct axp_data *)of_device_get_match_data(dev);
>  
> -	axp20x_batt->batt = devm_power_supply_register(&pdev->dev,
> -						       &axp20x_batt_ps_desc,
> -						       &psy_cfg);
> +	if (!axp20x_batt->data->has_fg)
> +		ps_desc = &axp192_batt_ps_desc;
> +	else
> +		ps_desc = &axp20x_batt_ps_desc;
> +
> +	axp20x_batt->batt = devm_power_supply_register(&pdev->dev, ps_desc, &psy_cfg);
>  	if (IS_ERR(axp20x_batt->batt)) {
>  		dev_err(&pdev->dev, "failed to register power supply: %ld\n",
>  			PTR_ERR(axp20x_batt->batt));
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]