mbox series

[v3,00/16] Add support for AXP192 PMIC

Message ID 20220618214009.2178567-1-aidanmacdonald.0x0@gmail.com
Headers show
Series Add support for AXP192 PMIC | expand

Message

Aidan MacDonald June 18, 2022, 9:39 p.m. UTC
Changes in v3:

* Update pinctrl driver to address Andy Shevchenko's review comments
  from v1, and fix a few other issues.
* Add gpio-ranges property and example snippet to gpio DT bindings.
* Update commit message of patch 01/16 to point out that all register
  addresses are obtained using sub_irq_reg().
* Document ccc_table in axp20x_battery. Also update commit message to
  note a small fix that is part of that patch.
* Drop axp20x_adc consolidation patch in favor of using separate adc_raw
  functions. It's a minor code size optimization that may not be worth
  the effort due to implementation complexity.
* Use the FIELD_GET macro in axp20x_adc to further clarify intent.
* Fix a typo in the regulator driver where an AXP20X regulator ID was
  mistakenly used instead of an AXP192 regulator ID. Also carry over
  an Acked-by: tag from v1. Hope that's okay.
* Accumulate Acked-by: tags from v1 on DT patches.
* Accumulate Acked-by: tags from v2.

Note that regmap maintainer Mark Brown has said the first two patches to
regmap-irq aren't suitable for inclusion into the kernel in their current
state. I'm including them for v3 so the series remains testable.

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 (16):
  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: 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   |  68 +++
 .../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                  | 359 +++++++++--
 drivers/mfd/axp20x-i2c.c                      |   2 +
 drivers/mfd/axp20x.c                          | 153 +++++
 drivers/pinctrl/Kconfig                       |  14 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-axp192.c              | 562 ++++++++++++++++++
 drivers/power/supply/axp20x_battery.c         | 143 ++++-
 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, 1529 insertions(+), 83 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
 create mode 100644 drivers/pinctrl/pinctrl-axp192.c

Comments

Andy Shevchenko June 19, 2022, 11:12 a.m. UTC | #1
On Sun, Jun 19, 2022 at 1:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 19 Jun 2022 00:43:07 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Saturday, June 18, 2022, Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> > wrote:
> >
> > > Changes in v3:
> > >
> > > * Update pinctrl driver to address Andy Shevchenko's review comments
> > >   from v1, and fix a few other
> >
> > I believe I gave more comments than just against pin control driver. Even
> > though, some comments are still not addressed in the series, including pin
> > control. Am I mistaken?
>
> Hi Andy,
>
> Maybe, it's a question of clarity/misunderstanding? You had some 'global' comments
> at the end of the pinctrl review. Perhaps not clear enough you meant
> they should apply to the rest of the patch series (and more generally to
> the driver being modified I think).

Yeah, I think that is.
I don't remember if we have somewhere a documentation on how to
respond to the review comments, in which the point of addressing
comment everywhere in the series, and not only in the place(s) where
it was given.
Jonathan Cameron June 19, 2022, 11:17 a.m. UTC | #2
On Sun, 19 Jun 2022 00:43:07 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Saturday, June 18, 2022, Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> wrote:
> 
> > Changes in v3:
> >
> > * Update pinctrl driver to address Andy Shevchenko's review comments
> >   from v1, and fix a few other  
> 
> 
> I believe I gave more comments than just against pin control driver. Even
> though, some comments are still not addressed in the series, including pin
> control. Am I mistaken?

Hi Andy,

Maybe, it's a question of clarity/misunderstanding? You had some 'global' comments
at the end of the pinctrl review. Perhaps not clear enough you meant
they should apply to the rest of the patch series (and more generally to
the driver being modified I think).

Just guessing!

Jonathan

> 
> 
> > * Add gpio-ranges property and example snippet to gpio DT bindings.
> > * Update commit message of patch 01/16 to point out that all register
> >   addresses are obtained using sub_irq_reg().
> > * Document ccc_table in axp20x_battery. Also update commit message to
> >   note a small fix that is part of that patch.
> > * Drop axp20x_adc consolidation patch in favor of using separate adc_raw
> >   functions. It's a minor code size optimization that may not be worth
> >   the effort due to implementation complexity.
> > * Use the FIELD_GET macro in axp20x_adc to further clarify intent.
> > * Fix a typo in the regulator driver where an AXP20X regulator ID was
> >   mistakenly used instead of an AXP192 regulator ID. Also carry over
> >   an Acked-by: tag from v1. Hope that's okay.
> > * Accumulate Acked-by: tags from v1 on DT patches.
> > * Accumulate Acked-by: tags from v2.
> >
> > Note that regmap maintainer Mark Brown has said the first two patches to
> > regmap-irq aren't suitable for inclusion into the kernel in their current
> > state. I'm including them for v3 so the series remains testable.
> >
> > 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 (16):
> >   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: 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   |  68 +++
> >  .../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                  | 359 +++++++++--
> >  drivers/mfd/axp20x-i2c.c                      |   2 +
> >  drivers/mfd/axp20x.c                          | 153 +++++
> >  drivers/pinctrl/Kconfig                       |  14 +
> >  drivers/pinctrl/Makefile                      |   1 +
> >  drivers/pinctrl/pinctrl-axp192.c              | 562 ++++++++++++++++++
> >  drivers/power/supply/axp20x_battery.c         | 143 ++++-
> >  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, 1529 insertions(+), 83 deletions(-)
> >  create mode 100644 Documentation/devicetree/
> > bindings/gpio/x-powers,axp192-gpio.yaml
> >  create mode 100644 drivers/pinctrl/pinctrl-axp192.c
> >
> > --
> > 2.35.1
> >
> >  
>
Aidan MacDonald June 19, 2022, 2:54 p.m. UTC | #3
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Sun, Jun 19, 2022 at 1:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>> On Sun, 19 Jun 2022 00:43:07 +0200
>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> > On Saturday, June 18, 2022, Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> > wrote:
>> >
>> > > Changes in v3:
>> > >
>> > > * Update pinctrl driver to address Andy Shevchenko's review comments
>> > >   from v1, and fix a few other
>> >
>> > I believe I gave more comments than just against pin control driver. Even
>> > though, some comments are still not addressed in the series, including pin
>> > control. Am I mistaken?
>>
>> Hi Andy,
>>
>> Maybe, it's a question of clarity/misunderstanding? You had some 'global' comments
>> at the end of the pinctrl review. Perhaps not clear enough you meant
>> they should apply to the rest of the patch series (and more generally to
>> the driver being modified I think).
>
> Yeah, I think that is.
> I don't remember if we have somewhere a documentation on how to
> respond to the review comments, in which the point of addressing
> comment everywhere in the series, and not only in the place(s) where
> it was given.

That's exactly it, I was only looking at the pinctrl patch since that's
the one you replied to, and didn't think to check the other patches for
similar cases even though that's obvious in retrospect. Sorry for any
confusion.