Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-02-02 Thread Lee Jones
Linus,

On Fri, 02 Feb 2024, Karel Balej wrote:

> Lee Jones, 2024-02-02T12:45:50+00:00:
> > On Thu, 01 Feb 2024, Karel Balej wrote:
> >
> > > Lee Jones, 2024-01-31T11:03:11+00:00:
> > > > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > > > + /* GPIO1: DVC, GPIO0: input */
> > > > > > > + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),

Do we have a precedent for drivers setting up their own pins like this?

> > > > > > Shouldn't you set these up using Pintrl?
> > > > > 
> > > > > You mean to add a new MFD cell for the pins and write the respective
> > > > > driver? The downstream implementation has no such thing so I'm not 
> > > > > sure
> > > > > if I would be able to do that from scratch.
> > > >
> > > > This is not a Pinctrl driver.
> > > >
> > > > Isn't there a generic API you can use?
> > > 
> > > I'm sorry, I don't think I understand what you mean.
> >
> > Perhaps I misunderstand the code.  It looks like this regmap patch hack
> > is configuring pins and a bunch of other things.  Would that be a
> > correct assessment?
> 
> Yes, that sounds correct.
> 
> > If so, where do we draw the line here?  Do we accept a 1000 line driver
> > which configures a large SoC with a bunch of bespoke register writes?
> 
> I understand, I just don't know what you mean by "a generic API". I'm
> also not clear on whether what you have in mind is simply adding a
> dedicated driver for the pins as a new subdevice of this MFD.

-- 
Lee Jones [李琼斯]



Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-02-02 Thread Karel Balej
Lee Jones, 2024-02-02T12:45:50+00:00:
> On Thu, 01 Feb 2024, Karel Balej wrote:
>
> > Lee Jones, 2024-01-31T11:03:11+00:00:
> > > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > > +   /* GPIO1: DVC, GPIO0: input */
> > > > > > +   REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > > > >
> > > > > Shouldn't you set these up using Pintrl?
> > > > 
> > > > You mean to add a new MFD cell for the pins and write the respective
> > > > driver? The downstream implementation has no such thing so I'm not sure
> > > > if I would be able to do that from scratch.
> > >
> > > This is not a Pinctrl driver.
> > >
> > > Isn't there a generic API you can use?
> > 
> > I'm sorry, I don't think I understand what you mean.
>
> Perhaps I misunderstand the code.  It looks like this regmap patch hack
> is configuring pins and a bunch of other things.  Would that be a
> correct assessment?

Yes, that sounds correct.

> If so, where do we draw the line here?  Do we accept a 1000 line driver
> which configures a large SoC with a bunch of bespoke register writes?

I understand, I just don't know what you mean by "a generic API". I'm
also not clear on whether what you have in mind is simply adding a
dedicated driver for the pins as a new subdevice of this MFD.

Thanks,
K. B.



Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-02-02 Thread Lee Jones
On Thu, 01 Feb 2024, Karel Balej wrote:

> Lee Jones, 2024-01-31T11:03:11+00:00:
> > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > + /* GPIO1: DVC, GPIO0: input */
> > > > > + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > > >
> > > > Shouldn't you set these up using Pintrl?
> > > 
> > > You mean to add a new MFD cell for the pins and write the respective
> > > driver? The downstream implementation has no such thing so I'm not sure
> > > if I would be able to do that from scratch.
> >
> > This is not a Pinctrl driver.
> >
> > Isn't there a generic API you can use?
> 
> I'm sorry, I don't think I understand what you mean.

Perhaps I misunderstand the code.  It looks like this regmap patch hack
is configuring pins and a bunch of other things.  Would that be a
correct assessment?

If so, where do we draw the line here?  Do we accept a 1000 line driver
which configures a large SoC with a bunch of bespoke register writes?

-- 
Lee Jones [李琼斯]



Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-02-01 Thread Karel Balej
Lee Jones, 2024-01-31T11:03:11+00:00:
> On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > +   /* GPIO1: DVC, GPIO0: input */
> > > > +   REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > >
> > > Shouldn't you set these up using Pintrl?
> > 
> > You mean to add a new MFD cell for the pins and write the respective
> > driver? The downstream implementation has no such thing so I'm not sure
> > if I would be able to do that from scratch.
>
> This is not a Pinctrl driver.
>
> Isn't there a generic API you can use?

I'm sorry, I don't think I understand what you mean.

Thank you,
K. B.



Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-01-31 Thread Lee Jones
On Sun, 28 Jan 2024, Karel Balej wrote:
> > > + /* GPIO1: DVC, GPIO0: input */
> > > + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> >
> > Shouldn't you set these up using Pintrl?
> 
> You mean to add a new MFD cell for the pins and write the respective
> driver? The downstream implementation has no such thing so I'm not sure
> if I would be able to do that from scratch.

This is not a Pinctrl driver.

Isn't there a generic API you can use?

> > > + /* GPIO2: input */
> > > + REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> > > + /* DVC2, DVC1 */
> >
> > Please unify all of the comments.
> >
> > They all use a different structure.
> >
> > > + REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> > > + /* GPIO5V_1:input, GPIO5V_2: input */
> > > + REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> > > + /* output 32 kHz from XO */
> > > + REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> > > + /* OSC_FREERUN = 1, to lock FLL */
> > > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> > > + /* XO_LJ = 1, enable low jitter for 32 kHz */
> > > + REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> > > + /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 
> > > 5.6V */
> > > + REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> > > + /* set the duty cycle of charger DC/DC to max */
> > > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
> >
> > These all looks like they should be handled in their respective drivers?
> >
> > "patch"ing these in seems like a hack.
> 
> To be honest, I don't really know why these are required and what effect
> they have -- the comments above taken from the downstream version are
> the only thing I have to go by. I might try removing them to see if
> there is any noticable change and whether they could be added only later
> with the respective drivers.

If you don't know that they are or what they do and you haven't tested
them, they should not be submitted upstream.

> > > +static struct mfd_cell pm88x_devs[] = {
> > > + {
> > > + .name = "88pm88x-onkey",
> > > + .num_resources = ARRAY_SIZE(onkey_resources),
> > > + .resources = onkey_resources,
> > > + .id = -1,
> > > + },
> > > +};
> >
> > It's not an MFD if it only supports a single device.
> 
> As I have noted above with respect to the IRQ enum and also in the
> commit message, I have so far only added the parts which there is
> already use for. I intend to add the other parts along with the
> respective subdevice drivers, please see my regulator series [1] for an
> example.
> 
> I thought this approach would make for shorter and simpler patches and
> also would allow me to make more informed decisions as I familiarize
> myself with the downstream subdevice drivers more closely one by one.

One device doesn't warrant an MFD.  Please add more devices.

-- 
Lee Jones [李琼斯]



Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-01-28 Thread Karel Balej
Lee,

thank you for your feedback.

On Thu Jan 25, 2024 at 1:26 PM CET, Lee Jones wrote:

[...]

> > +#define PM88X_REG_INT_STATUS1  0x05
> > +
> > +#define PM88X_REG_INT_ENA_10x0a
> > +#define PM88X_INT_ENA1_ONKEY   BIT(0)
> > +
> > +enum pm88x_irq_number {
> > +   PM88X_IRQ_ONKEY,
> > +
> > +   PM88X_MAX_IRQ
> > +};
>
> An enum for a single IRQ?

There will be a lot more IRQs but I have only added this one so far as
it is the only one used by this series -- is that OK?

> > +static struct reg_sequence pm886_presets[] = {
> > +   /* disable watchdog */
> > +   REG_SEQ0(PM88X_REG_WDOG, 0x01),
>
> Easier to read if you place spaces between them.
>
> > +   /* GPIO1: DVC, GPIO0: input */
> > +   REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
>
> Shouldn't you set these up using Pintrl?

You mean to add a new MFD cell for the pins and write the respective
driver? The downstream implementation has no such thing so I'm not sure
if I would be able to do that from scratch.

> > +   /* GPIO2: input */
> > +   REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> > +   /* DVC2, DVC1 */
>
> Please unify all of the comments.
>
> They all use a different structure.
>
> > +   REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> > +   /* GPIO5V_1:input, GPIO5V_2: input */
> > +   REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> > +   /* output 32 kHz from XO */
> > +   REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> > +   /* OSC_FREERUN = 1, to lock FLL */
> > +   REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> > +   /* XO_LJ = 1, enable low jitter for 32 kHz */
> > +   REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> > +   /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 
> > 5.6V */
> > +   REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> > +   /* set the duty cycle of charger DC/DC to max */
> > +   REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
>
> These all looks like they should be handled in their respective drivers?
>
> "patch"ing these in seems like a hack.

To be honest, I don't really know why these are required and what effect
they have -- the comments above taken from the downstream version are
the only thing I have to go by. I might try removing them to see if
there is any noticable change and whether they could be added only later
with the respective drivers.

>
> > +};
>
> Why this instead of 

What are you refering to here please?

> > +static struct resource onkey_resources[] = {
> > +   DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> > +};
> > +
> > +static struct mfd_cell pm88x_devs[] = {
> > +   {
> > +   .name = "88pm88x-onkey",
> > +   .num_resources = ARRAY_SIZE(onkey_resources),
> > +   .resources = onkey_resources,
> > +   .id = -1,
> > +   },
> > +};
>
> It's not an MFD if it only supports a single device.

As I have noted above with respect to the IRQ enum and also in the
commit message, I have so far only added the parts which there is
already use for. I intend to add the other parts along with the
respective subdevice drivers, please see my regulator series [1] for an
example.

I thought this approach would make for shorter and simpler patches and
also would allow me to make more informed decisions as I familiarize
myself with the downstream subdevice drivers more closely one by one.

> > +   i2c_set_clientdata(client, chip);
> > +
> > +   device_init_wakeup(>dev, 1);
> > +
> > +   chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, 
> > _i2c_regmap);
> > +   if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {
>
> Just define different regmaps if you really need them.

You mean not to use an array of regmaps but add new struct members
instead? One for each regmap?

> > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > new file mode 100644
> > index ..a34c57447827
> > --- /dev/null
> > +++ b/include/linux/mfd/88pm88x.h

[...]

> > +#define PM88X_REG_ID   0x00
> > +
> > +#define PM88X_REG_STATUS1  0x01
> > +#define PM88X_ONKEY_STS1   BIT(0)
> > +
> > +#define PM88X_REG_MISC_CONFIG1 0x14
> > +#define PM88X_SW_PDOWN BIT(5)
> > +
> > +#define PM88X_REG_MISC_CONFIG2 0x15
> > +#define PM88X_INT_INV  BIT(0)
> > +#define PM88X_INT_CLEARBIT(1)
> > +#define PM88X_INT_RC   0x00
> > +#define PM88X_INT_WC   BIT(1)
> > +#define PM88X_INT_MASK_MODEBIT(2)
> > +
> > +#define PM88X_REG_WDOG 0x1d
> > +
> > +#define PM88X_REG_LOWPOWER20x21
> > +#define PM88X_REG_LOWPOWER40x23
> > +
> > +#define PM88X_REG_GPIO_CTRL1   0x30
>
> These don't really need to be spaced out, do they?

I have spaced them out already as I expect to add some related
definitions to each of these in the future and thought it would then
perhaps be more easily readable like this.

[1] 

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

2024-01-25 Thread Lee Jones
On Sun, 17 Dec 2023, Karel Balej wrote:

> From: Karel Balej 
> 
> Marvell 88PM880 and 8PM886 are two similar PMICs with mostly matching
> register mapping. They provide various functions such as onkey, battery,
> charger and regulators.
> 
> Add support for 88PM886 found for instance in the samsung,coreprimevelte
> smartphone with which this was tested. Support for 88PM880 is not
> implemented here but should be straightforward to add.
> 
> Implement only the most basic support omitting the currently unused
> registers and I2C subclients which should thus be added with the
> respective subdevices. However, add support for the onkey already.
> 
> Signed-off-by: Karel Balej 
> ---
>  drivers/mfd/88pm88x.c   | 199 
>  drivers/mfd/Kconfig |  11 ++
>  drivers/mfd/Makefile|   1 +
>  include/linux/mfd/88pm88x.h |  60 +++
>  4 files changed, 271 insertions(+)
>  create mode 100644 drivers/mfd/88pm88x.c
>  create mode 100644 include/linux/mfd/88pm88x.h
> 
> diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> new file mode 100644
> index ..5db6c65b667d
> --- /dev/null
> +++ b/drivers/mfd/88pm88x.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Alphabetical

> +#include 
> +
> +/* interrupt status registers */

Use correct grammar in comments, including capital letters.

 - Applies throughout

The comment is not required - we can see what they are from the
nomenclature.

> +#define PM88X_REG_INT_STATUS10x05
> +
> +#define PM88X_REG_INT_ENA_1  0x0a
> +#define PM88X_INT_ENA1_ONKEY BIT(0)
> +
> +enum pm88x_irq_number {
> + PM88X_IRQ_ONKEY,
> +
> + PM88X_MAX_IRQ
> +};

An enum for a single IRQ?

> +static struct regmap_irq pm88x_regmap_irqs[] = {
> + REGMAP_IRQ_REG(PM88X_IRQ_ONKEY, 0, PM88X_INT_ENA1_ONKEY),
> +};
> +
> +static struct regmap_irq_chip pm88x_regmap_irq_chip = {
> + .name = "88pm88x",
> + .irqs = pm88x_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(pm88x_regmap_irqs),
> + .num_regs = 4,
> + .status_base = PM88X_REG_INT_STATUS1,
> + .ack_base = PM88X_REG_INT_STATUS1,
> + .unmask_base = PM88X_REG_INT_ENA_1,
> +};
> +
> +static struct reg_sequence pm886_presets[] = {
> + /* disable watchdog */
> + REG_SEQ0(PM88X_REG_WDOG, 0x01),

Easier to read if you place spaces between them.

> + /* GPIO1: DVC, GPIO0: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),

Shouldn't you set these up using Pintrl?

> + /* GPIO2: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> + /* DVC2, DVC1 */

Please unify all of the comments.

They all use a different structure.

> + REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> + /* GPIO5V_1:input, GPIO5V_2: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> + /* output 32 kHz from XO */
> + REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> + /* OSC_FREERUN = 1, to lock FLL */
> + REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> + /* XO_LJ = 1, enable low jitter for 32 kHz */
> + REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> + /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 
> 5.6V */
> + REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> + /* set the duty cycle of charger DC/DC to max */
> + REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),

These all looks like they should be handled in their respective drivers?

"patch"ing these in seems like a hack.

> +};

Why this instead of 
> +static struct resource onkey_resources[] = {
> + DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> +};
> +
> +static struct mfd_cell pm88x_devs[] = {
> + {
> + .name = "88pm88x-onkey",
> + .num_resources = ARRAY_SIZE(onkey_resources),
> + .resources = onkey_resources,
> + .id = -1,
> + },
> +};

It's not an MFD if it only supports a single device.

> +static struct pm88x_data pm886_a1_data = {
> + .whoami = PM886_A1_WHOAMI,
> + .presets = pm886_presets,
> + .num_presets = ARRAY_SIZE(pm886_presets),
> +};

Just pass the device ID through DT's .data, then match on that instead
of passing pointer to random data structures.

> +static const struct regmap_config pm88x_i2c_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0xfe,

Define this please.

> +};
> +
> +static int pm88x_power_off_handler(struct sys_off_data *data)

'data' is a terrible variable name.  Please change throughout.

> +{
> + struct pm88x_chip *chip = data->cb_data;
> + int ret;
> +
> + ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], 
> PM88X_REG_MISC_CONFIG1,
> + PM88X_SW_PDOWN, PM88X_SW_PDOWN);
> + if (ret) {
> + dev_err(>client->dev, "Failed to power off the device: 
> %d\n", ret);
> + return NOTIFY_BAD;
> + }
> + return NOTIFY_DONE;
> +}

[RFC PATCH 2/5] mfd: add 88pm88x driver

2023-12-17 Thread Karel Balej
From: Karel Balej 

Marvell 88PM880 and 8PM886 are two similar PMICs with mostly matching
register mapping. They provide various functions such as onkey, battery,
charger and regulators.

Add support for 88PM886 found for instance in the samsung,coreprimevelte
smartphone with which this was tested. Support for 88PM880 is not
implemented here but should be straightforward to add.

Implement only the most basic support omitting the currently unused
registers and I2C subclients which should thus be added with the
respective subdevices. However, add support for the onkey already.

Signed-off-by: Karel Balej 
---
 drivers/mfd/88pm88x.c   | 199 
 drivers/mfd/Kconfig |  11 ++
 drivers/mfd/Makefile|   1 +
 include/linux/mfd/88pm88x.h |  60 +++
 4 files changed, 271 insertions(+)
 create mode 100644 drivers/mfd/88pm88x.c
 create mode 100644 include/linux/mfd/88pm88x.h

diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
new file mode 100644
index ..5db6c65b667d
--- /dev/null
+++ b/drivers/mfd/88pm88x.c
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/* interrupt status registers */
+#define PM88X_REG_INT_STATUS1  0x05
+
+#define PM88X_REG_INT_ENA_10x0a
+#define PM88X_INT_ENA1_ONKEY   BIT(0)
+
+enum pm88x_irq_number {
+   PM88X_IRQ_ONKEY,
+
+   PM88X_MAX_IRQ
+};
+
+static struct regmap_irq pm88x_regmap_irqs[] = {
+   REGMAP_IRQ_REG(PM88X_IRQ_ONKEY, 0, PM88X_INT_ENA1_ONKEY),
+};
+
+static struct regmap_irq_chip pm88x_regmap_irq_chip = {
+   .name = "88pm88x",
+   .irqs = pm88x_regmap_irqs,
+   .num_irqs = ARRAY_SIZE(pm88x_regmap_irqs),
+   .num_regs = 4,
+   .status_base = PM88X_REG_INT_STATUS1,
+   .ack_base = PM88X_REG_INT_STATUS1,
+   .unmask_base = PM88X_REG_INT_ENA_1,
+};
+
+static struct reg_sequence pm886_presets[] = {
+   /* disable watchdog */
+   REG_SEQ0(PM88X_REG_WDOG, 0x01),
+   /* GPIO1: DVC, GPIO0: input */
+   REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
+   /* GPIO2: input */
+   REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
+   /* DVC2, DVC1 */
+   REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
+   /* GPIO5V_1:input, GPIO5V_2: input */
+   REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
+   /* output 32 kHz from XO */
+   REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
+   /* OSC_FREERUN = 1, to lock FLL */
+   REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
+   /* XO_LJ = 1, enable low jitter for 32 kHz */
+   REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
+   /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 
5.6V */
+   REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
+   /* set the duty cycle of charger DC/DC to max */
+   REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
+};
+
+static struct resource onkey_resources[] = {
+   DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
+};
+
+static struct mfd_cell pm88x_devs[] = {
+   {
+   .name = "88pm88x-onkey",
+   .num_resources = ARRAY_SIZE(onkey_resources),
+   .resources = onkey_resources,
+   .id = -1,
+   },
+};
+
+static struct pm88x_data pm886_a1_data = {
+   .whoami = PM886_A1_WHOAMI,
+   .presets = pm886_presets,
+   .num_presets = ARRAY_SIZE(pm886_presets),
+};
+
+static const struct regmap_config pm88x_i2c_regmap = {
+   .reg_bits = 8,
+   .val_bits = 8,
+   .max_register = 0xfe,
+};
+
+static int pm88x_power_off_handler(struct sys_off_data *data)
+{
+   struct pm88x_chip *chip = data->cb_data;
+   int ret;
+
+   ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], 
PM88X_REG_MISC_CONFIG1,
+   PM88X_SW_PDOWN, PM88X_SW_PDOWN);
+   if (ret) {
+   dev_err(>client->dev, "Failed to power off the device: 
%d\n", ret);
+   return NOTIFY_BAD;
+   }
+   return NOTIFY_DONE;
+}
+
+static int pm88x_setup_irq(struct pm88x_chip *chip)
+{
+   int ret;
+
+   /* set interrupt clearing mode to clear on write */
+   ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], 
PM88X_REG_MISC_CONFIG2,
+   PM88X_INT_INV | PM88X_INT_CLEAR | PM88X_INT_MASK_MODE,
+   PM88X_INT_WC);
+   if (ret) {
+   dev_err(>client->dev, "Failed to set interrupt clearing 
mode: %d\n", ret);
+   return ret;
+   }
+
+   ret = devm_regmap_add_irq_chip(>client->dev, 
chip->regmaps[PM88X_REGMAP_BASE],
+   chip->client->irq, IRQF_ONESHOT, -1, 
_regmap_irq_chip,
+   >irq_data);
+   if (ret) {
+   dev_err(>client->dev, "Failed to request IRQ: %d\n", ret);
+   return ret;
+   }
+
+   return 0;
+}
+
+static int pm88x_probe(struct i2c_client *client)
+{
+   struct pm88x_chip *chip;
+