Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, Mar 21, 2024 at 08:14:44PM +0100, Karel Balej wrote: > Mark Brown, 2024-03-21T19:00:24+00:00: > > I would expect that if you have two separate register maps they would > > have separate configurations that describe the corresponding physical > > register maps, as far as I can tell this driver is just making up a > > maximum register number. > Alright, so I should just use a separate config for each regmap and set > the max_register value for each to whatever I can find is actually the > highest used value in the downstream code -- correct? That sounds plausible if you don't have any other information about the register maps. signature.asc Description: PGP signature
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Mark Brown, 2024-03-21T19:00:24+00:00: > On Thu, Mar 21, 2024 at 07:16:43PM +0100, Karel Balej wrote: > > Mark Brown, 2024-03-21T17:48:28+00:00: > > > > > They do according to the downstream driver which is my only reference. > > > > In fact, there the driver defines the configs separately for each regmap > > > > but with the same values. > > > > This is a downstream driver - are you sure it's got the best code > > > quality? > > > No, that is why I have rewritten it and tried to improve on this. But > > like I said, it is my only reference. Is there some other way to verify > > this value (besides perhaps the datasheet)? > > The maximum register is whatever the maximum register we know about for > the device is, the datasheet is generally a good reference there. > > > > I'm not seeing any references to registers with numbers as high as the > > > maximum register that's there in your driver for example. > > > Indeed, I have performed the same check with the same findings. But that > > doesn't necessarily mean that the maximum should be lower, no? > > > Do you have some specific modifications of my code in mind regarding > > this? > > I would expect that if you have two separate register maps they would > have separate configurations that describe the corresponding physical > register maps, as far as I can tell this driver is just making up a > maximum register number. Alright, so I should just use a separate config for each regmap and set the max_register value for each to whatever I can find is actually the highest used value in the downstream code -- correct? Thank you, K. B.
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, Mar 21, 2024 at 07:16:43PM +0100, Karel Balej wrote: > Mark Brown, 2024-03-21T17:48:28+00:00: > > > They do according to the downstream driver which is my only reference. > > > In fact, there the driver defines the configs separately for each regmap > > > but with the same values. > > This is a downstream driver - are you sure it's got the best code > > quality? > No, that is why I have rewritten it and tried to improve on this. But > like I said, it is my only reference. Is there some other way to verify > this value (besides perhaps the datasheet)? The maximum register is whatever the maximum register we know about for the device is, the datasheet is generally a good reference there. > > I'm not seeing any references to registers with numbers as high as the > > maximum register that's there in your driver for example. > Indeed, I have performed the same check with the same findings. But that > doesn't necessarily mean that the maximum should be lower, no? > Do you have some specific modifications of my code in mind regarding > this? I would expect that if you have two separate register maps they would have separate configurations that describe the corresponding physical register maps, as far as I can tell this driver is just making up a maximum register number. signature.asc Description: PGP signature
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Mark Brown, 2024-03-21T17:48:28+00:00: > On Thu, Mar 21, 2024 at 06:32:03PM +0100, Karel Balej wrote: > > Mark Brown, 2024-03-21T17:17:40+00:00: > > > > Do they both genuinely have the same maximum register? > > > They do according to the downstream driver which is my only reference. > > In fact, there the driver defines the configs separately for each regmap > > but with the same values. > > This is a downstream driver - are you sure it's got the best code > quality? No, that is why I have rewritten it and tried to improve on this. But like I said, it is my only reference. Is there some other way to verify this value (besides perhaps the datasheet)? > I'm not seeing any references to registers with numbers as high as the > maximum register that's there in your driver for example. Indeed, I have performed the same check with the same findings. But that doesn't necessarily mean that the maximum should be lower, no? Do you have some specific modifications of my code in mind regarding this? Thank you, K. B.
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, Mar 21, 2024 at 06:32:03PM +0100, Karel Balej wrote: > Mark Brown, 2024-03-21T17:17:40+00:00: > > Do they both genuinely have the same maximum register? > They do according to the downstream driver which is my only reference. > In fact, there the driver defines the configs separately for each regmap > but with the same values. This is a downstream driver - are you sure it's got the best code quality? I'm not seeing any references to registers with numbers as high as the maximum register that's there in your driver for example. signature.asc Description: PGP signature
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Mark Brown, 2024-03-21T17:17:40+00:00:
> On Thu, Mar 21, 2024 at 06:08:16PM +0100, Karel Balej wrote:
> > Mark Brown, 2024-03-21T16:58:44+00:00:
>
> > > > > > > > +static const struct regmap_config pm886_i2c_regmap = {
> > > > > > > > + .reg_bits = 8,
> > > > > > > > + .val_bits = 8,
> > > > > > > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > > > > > > +};
>
> ...
>
> > > You shouldn't be creating two regmaps for the same set of registers,
> > > that just opens the potential for confusion.
>
> > Just the regmap config is the same. Otherwise, each regmap lives at a
> > different I2C address.
>
> Do they both genuinely have the same maximum register?
They do according to the downstream driver which is my only reference.
In fact, there the driver defines the configs separately for each regmap
but with the same values.
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, Mar 21, 2024 at 06:08:16PM +0100, Karel Balej wrote:
> Mark Brown, 2024-03-21T16:58:44+00:00:
> > > > > > > +static const struct regmap_config pm886_i2c_regmap = {
> > > > > > > + .reg_bits = 8,
> > > > > > > + .val_bits = 8,
> > > > > > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > > > > > +};
...
> > You shouldn't be creating two regmaps for the same set of registers,
> > that just opens the potential for confusion.
> Just the regmap config is the same. Otherwise, each regmap lives at a
> different I2C address.
Do they both genuinely have the same maximum register?
signature.asc
Description: PGP signature
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Mark Brown, 2024-03-21T16:58:44+00:00:
> On Thu, Mar 21, 2024 at 05:55:17PM +0100, Karel Balej wrote:
> > Lee Jones, 2024-03-21T16:20:45+00:00:
> > > On Thu, 21 Mar 2024, Karel Balej wrote:
>
> > > > > > +static const struct regmap_config pm886_i2c_regmap = {
> > > > > > + .reg_bits = 8,
> > > > > > + .val_bits = 8,
> > > > > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > > > > +};
>
> > > > > Why is this in here?
>
> > > > Because since I moved the regulators regmap initialization into the
> > > > regulators driver, I need to access it from there.
>
> > > So move it into the regulators driver?
>
> > It is used in the MFD driver too for the base regmap.
>
> You shouldn't be creating two regmaps for the same set of registers,
> that just opens the potential for confusion.
Just the regmap config is the same. Otherwise, each regmap lives at a
different I2C address.
Regards,
K. B.
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, Mar 21, 2024 at 05:55:17PM +0100, Karel Balej wrote:
> Lee Jones, 2024-03-21T16:20:45+00:00:
> > On Thu, 21 Mar 2024, Karel Balej wrote:
> > > > > +static const struct regmap_config pm886_i2c_regmap = {
> > > > > + .reg_bits = 8,
> > > > > + .val_bits = 8,
> > > > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > > > +};
> > > > Why is this in here?
> > > Because since I moved the regulators regmap initialization into the
> > > regulators driver, I need to access it from there.
> > So move it into the regulators driver?
> It is used in the MFD driver too for the base regmap.
You shouldn't be creating two regmaps for the same set of registers,
that just opens the potential for confusion.
signature.asc
Description: PGP signature
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Lee Jones, 2024-03-21T16:20:45+00:00:
> On Thu, 21 Mar 2024, Karel Balej wrote:
>
> > Lee Jones, 2024-03-21T15:42:11+00:00:
> > > On Mon, 11 Mar 2024, Karel Balej wrote:
> > > > diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> > > > new file mode 100644
> > > > index ..a5e6524bb19d
> > > > --- /dev/null
> > > > +++ b/include/linux/mfd/88pm886.h
> > > > @@ -0,0 +1,38 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +#ifndef __MFD_88PM886_H
> > > > +#define __MFD_88PM886_H
> > > > +
> > > > +#include
> > > > +#include
> > > > +
> > > > +#define PM886_A1_CHIP_ID 0xa1
> > > > +
> > > > +#define PM886_REGMAP_CONF_MAX_REG 0xfe
> > > > +
> > > > +#define PM886_REG_ID 0x00
> > > > +
> > > > +#define PM886_REG_STATUS1 0x01
> > > > +#define PM886_ONKEY_STS1 BIT(0)
> > > > +
> > > > +#define PM886_REG_MISC_CONFIG1 0x14
> > > > +#define PM886_SW_PDOWN BIT(5)
> > > > +
> > > > +#define PM886_REG_MISC_CONFIG2 0x15
> > > > +#define PM886_INT_INV BIT(0)
> > > > +#define PM886_INT_CLEARBIT(1)
> > > > +#define PM886_INT_RC 0x00
> > > > +#define PM886_INT_WC BIT(1)
> > > > +#define PM886_INT_MASK_MODEBIT(2)
> > > > +
> > > > +struct pm886_chip {
> > > > + struct i2c_client *client;
> > > > + unsigned int chip_id;
> > > > + struct regmap *regmap;
> > > > +};
> > > > +
> > > > +static const struct regmap_config pm886_i2c_regmap = {
> > > > + .reg_bits = 8,
> > > > + .val_bits = 8,
> > > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > > +};
> > >
> > > Why is this in here?
> >
> > Because since I moved the regulators regmap initialization into the
> > regulators driver, I need to access it from there.
>
> So move it into the regulators driver?
It is used in the MFD driver too for the base regmap.
K. B.
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Thu, 21 Mar 2024, Karel Balej wrote:
> Lee Jones, 2024-03-21T15:42:11+00:00:
> > On Mon, 11 Mar 2024, Karel Balej wrote:
> >
> > > From: Karel Balej
> > >
> > > Marvell 88PM886 is a PMIC which provides various functions such as
> > > onkey, battery, charger and regulators. It is found for instance in the
> > > samsung,coreprimevelte smartphone with which this was tested. Implement
> > > basic support to allow for the use of regulators and onkey.
> > >
> > > Signed-off-by: Karel Balej
> > > ---
> > >
> > > Notes:
> > > RFC v4:
> > > - Use MFD_CELL_* macros.
> > > - Address Lee's feedback:
> > > - Do not define regmap_config.val_bits and .reg_bits.
> > > - Drop everything regulator related except mfd_cell (regmap
> > > initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
> > > - Do not store regmap pointers as an array as there is now only one
> > > regmap. Also drop the corresponding enum.
> > > - Move regmap_config to the header as it is needed in the regulators
> > > driver.
> > > - pm886_chip.whoami -> chip_id
> > > - Reword chip ID mismatch error message and print the ID as
> > > hexadecimal.
> > > - Fix includes in include/linux/88pm886.h.
> > > - Drop the pm886_irq_number enum and define the (for the moment)
> > > only
> > > IRQ explicitly.
> > > - Have only one MFD cell for all regulators as they are now registered
> > > all at once in the regulators driver.
> > > - Reword commit message.
> > > - Make device table static and remove comma after the sentinel to
> > > signal
> > > that nothing should come after it.
> > > RFC v3:
> > > - Drop onkey cell .of_compatible.
> > > - Rename LDO page offset and regmap to REGULATORS.
> > > RFC v2:
> > > - Remove some abstraction.
> > > - Sort includes alphabetically and add linux/of.h.
> > > - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
> > > - Use more temporaries and break long lines.
> > > - Do not initialize ret in probe.
> > > - Use the wakeup-source DT property.
> > > - Rename ret to err.
> > > - Address Lee's comments:
> > > - Drop patched in presets for base regmap and related defines.
> > > - Use full sentences in comments.
> > > - Remove IRQ comment.
> > > - Define regmap_config member values.
> > > - Rename data to sys_off_data.
> > > - Add _PMIC suffix to Kconfig.
> > > - Use dev_err_probe.
> > > - Do not store irq_data.
> > > - s/WHOAMI/CHIP_ID
> > > - Drop LINUX part of include guard name.
> > > - Merge in the regulator series modifications in order to have more
> > > devices and modify the commit message accordingly. Changes with
> > > respect to the original regulator series patches:
> > > - ret -> err
> > > - Add temporary for dev in pm88x_initialize_subregmaps.
> > > - Drop of_compatible for the regulators.
> > > - Do not duplicate LDO regmap for bucks.
> > > - Rewrite commit message.
> > >
> > > drivers/mfd/88pm886.c | 149
> > > drivers/mfd/Kconfig | 12 +++
> > > drivers/mfd/Makefile| 1 +
> > > include/linux/mfd/88pm886.h | 38 +
> > > 4 files changed, 200 insertions(+)
> > > create mode 100644 drivers/mfd/88pm886.c
> > > create mode 100644 include/linux/mfd/88pm886.h
> >
> > Looks mostly okay.
> >
> > > diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> > > new file mode 100644
> > > index ..a5e6524bb19d
> > > --- /dev/null
> > > +++ b/include/linux/mfd/88pm886.h
> > > @@ -0,0 +1,38 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +#ifndef __MFD_88PM886_H
> > > +#define __MFD_88PM886_H
> > > +
> > > +#include
> > > +#include
> > > +
> > > +#define PM886_A1_CHIP_ID 0xa1
> > > +
> > > +#define PM886_REGMAP_CONF_MAX_REG0xfe
> > > +
> > > +#define PM886_REG_ID 0x00
> > > +
> > > +#define PM886_REG_STATUS10x01
> > > +#define PM886_ONKEY_STS1 BIT(0)
> > > +
> > > +#define PM886_REG_MISC_CONFIG1 0x14
> > > +#define PM886_SW_PDOWN BIT(5)
> > > +
> > > +#define PM886_REG_MISC_CONFIG2 0x15
> > > +#define PM886_INT_INVBIT(0)
> > > +#define PM886_INT_CLEAR BIT(1)
> > > +#define PM886_INT_RC 0x00
> > > +#define PM886_INT_WC BIT(1)
> > > +#define PM886_INT_MASK_MODE BIT(2)
> > > +
> > > +struct pm886_chip {
> > > + struct i2c_client *client;
> > > + unsigned int chip_id;
> > > + struct regmap *regmap;
> > > +};
> > > +
> > > +static const struct regmap_config pm886_i2c_regmap = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > > +};
> >
> > Why is this in here?
>
>
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
Lee Jones, 2024-03-21T15:42:11+00:00:
> On Mon, 11 Mar 2024, Karel Balej wrote:
>
> > From: Karel Balej
> >
> > Marvell 88PM886 is a PMIC which provides various functions such as
> > onkey, battery, charger and regulators. It is found for instance in the
> > samsung,coreprimevelte smartphone with which this was tested. Implement
> > basic support to allow for the use of regulators and onkey.
> >
> > Signed-off-by: Karel Balej
> > ---
> >
> > Notes:
> > RFC v4:
> > - Use MFD_CELL_* macros.
> > - Address Lee's feedback:
> > - Do not define regmap_config.val_bits and .reg_bits.
> > - Drop everything regulator related except mfd_cell (regmap
> > initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
> > - Do not store regmap pointers as an array as there is now only one
> > regmap. Also drop the corresponding enum.
> > - Move regmap_config to the header as it is needed in the regulators
> > driver.
> > - pm886_chip.whoami -> chip_id
> > - Reword chip ID mismatch error message and print the ID as
> > hexadecimal.
> > - Fix includes in include/linux/88pm886.h.
> > - Drop the pm886_irq_number enum and define the (for the moment) only
> > IRQ explicitly.
> > - Have only one MFD cell for all regulators as they are now registered
> > all at once in the regulators driver.
> > - Reword commit message.
> > - Make device table static and remove comma after the sentinel to signal
> > that nothing should come after it.
> > RFC v3:
> > - Drop onkey cell .of_compatible.
> > - Rename LDO page offset and regmap to REGULATORS.
> > RFC v2:
> > - Remove some abstraction.
> > - Sort includes alphabetically and add linux/of.h.
> > - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
> > - Use more temporaries and break long lines.
> > - Do not initialize ret in probe.
> > - Use the wakeup-source DT property.
> > - Rename ret to err.
> > - Address Lee's comments:
> > - Drop patched in presets for base regmap and related defines.
> > - Use full sentences in comments.
> > - Remove IRQ comment.
> > - Define regmap_config member values.
> > - Rename data to sys_off_data.
> > - Add _PMIC suffix to Kconfig.
> > - Use dev_err_probe.
> > - Do not store irq_data.
> > - s/WHOAMI/CHIP_ID
> > - Drop LINUX part of include guard name.
> > - Merge in the regulator series modifications in order to have more
> > devices and modify the commit message accordingly. Changes with
> > respect to the original regulator series patches:
> > - ret -> err
> > - Add temporary for dev in pm88x_initialize_subregmaps.
> > - Drop of_compatible for the regulators.
> > - Do not duplicate LDO regmap for bucks.
> > - Rewrite commit message.
> >
> > drivers/mfd/88pm886.c | 149
> > drivers/mfd/Kconfig | 12 +++
> > drivers/mfd/Makefile| 1 +
> > include/linux/mfd/88pm886.h | 38 +
> > 4 files changed, 200 insertions(+)
> > create mode 100644 drivers/mfd/88pm886.c
> > create mode 100644 include/linux/mfd/88pm886.h
>
> Looks mostly okay.
>
> > diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> > new file mode 100644
> > index ..a5e6524bb19d
> > --- /dev/null
> > +++ b/include/linux/mfd/88pm886.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_88PM886_H
> > +#define __MFD_88PM886_H
> > +
> > +#include
> > +#include
> > +
> > +#define PM886_A1_CHIP_ID 0xa1
> > +
> > +#define PM886_REGMAP_CONF_MAX_REG 0xfe
> > +
> > +#define PM886_REG_ID 0x00
> > +
> > +#define PM886_REG_STATUS1 0x01
> > +#define PM886_ONKEY_STS1 BIT(0)
> > +
> > +#define PM886_REG_MISC_CONFIG1 0x14
> > +#define PM886_SW_PDOWN BIT(5)
> > +
> > +#define PM886_REG_MISC_CONFIG2 0x15
> > +#define PM886_INT_INV BIT(0)
> > +#define PM886_INT_CLEARBIT(1)
> > +#define PM886_INT_RC 0x00
> > +#define PM886_INT_WC BIT(1)
> > +#define PM886_INT_MASK_MODEBIT(2)
> > +
> > +struct pm886_chip {
> > + struct i2c_client *client;
> > + unsigned int chip_id;
> > + struct regmap *regmap;
> > +};
> > +
> > +static const struct regmap_config pm886_i2c_regmap = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = PM886_REGMAP_CONF_MAX_REG,
> > +};
>
> Why is this in here?
Because since I moved the regulators regmap initialization into the
regulators driver, I need to access it from there.
> What would you like me to do with this RFC patch?
I was hoping that you would take this through the MFD tree (after the
regulator subsystem maintainers approve
Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Mon, 11 Mar 2024, Karel Balej wrote:
> From: Karel Balej
>
> Marvell 88PM886 is a PMIC which provides various functions such as
> onkey, battery, charger and regulators. It is found for instance in the
> samsung,coreprimevelte smartphone with which this was tested. Implement
> basic support to allow for the use of regulators and onkey.
>
> Signed-off-by: Karel Balej
> ---
>
> Notes:
> RFC v4:
> - Use MFD_CELL_* macros.
> - Address Lee's feedback:
> - Do not define regmap_config.val_bits and .reg_bits.
> - Drop everything regulator related except mfd_cell (regmap
> initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
> - Do not store regmap pointers as an array as there is now only one
> regmap. Also drop the corresponding enum.
> - Move regmap_config to the header as it is needed in the regulators
> driver.
> - pm886_chip.whoami -> chip_id
> - Reword chip ID mismatch error message and print the ID as
> hexadecimal.
> - Fix includes in include/linux/88pm886.h.
> - Drop the pm886_irq_number enum and define the (for the moment) only
> IRQ explicitly.
> - Have only one MFD cell for all regulators as they are now registered
> all at once in the regulators driver.
> - Reword commit message.
> - Make device table static and remove comma after the sentinel to signal
> that nothing should come after it.
> RFC v3:
> - Drop onkey cell .of_compatible.
> - Rename LDO page offset and regmap to REGULATORS.
> RFC v2:
> - Remove some abstraction.
> - Sort includes alphabetically and add linux/of.h.
> - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
> - Use more temporaries and break long lines.
> - Do not initialize ret in probe.
> - Use the wakeup-source DT property.
> - Rename ret to err.
> - Address Lee's comments:
> - Drop patched in presets for base regmap and related defines.
> - Use full sentences in comments.
> - Remove IRQ comment.
> - Define regmap_config member values.
> - Rename data to sys_off_data.
> - Add _PMIC suffix to Kconfig.
> - Use dev_err_probe.
> - Do not store irq_data.
> - s/WHOAMI/CHIP_ID
> - Drop LINUX part of include guard name.
> - Merge in the regulator series modifications in order to have more
> devices and modify the commit message accordingly. Changes with
> respect to the original regulator series patches:
> - ret -> err
> - Add temporary for dev in pm88x_initialize_subregmaps.
> - Drop of_compatible for the regulators.
> - Do not duplicate LDO regmap for bucks.
> - Rewrite commit message.
>
> drivers/mfd/88pm886.c | 149
> drivers/mfd/Kconfig | 12 +++
> drivers/mfd/Makefile| 1 +
> include/linux/mfd/88pm886.h | 38 +
> 4 files changed, 200 insertions(+)
> create mode 100644 drivers/mfd/88pm886.c
> create mode 100644 include/linux/mfd/88pm886.h
Looks mostly okay.
> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> new file mode 100644
> index ..a5e6524bb19d
> --- /dev/null
> +++ b/include/linux/mfd/88pm886.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_88PM886_H
> +#define __MFD_88PM886_H
> +
> +#include
> +#include
> +
> +#define PM886_A1_CHIP_ID 0xa1
> +
> +#define PM886_REGMAP_CONF_MAX_REG0xfe
> +
> +#define PM886_REG_ID 0x00
> +
> +#define PM886_REG_STATUS10x01
> +#define PM886_ONKEY_STS1 BIT(0)
> +
> +#define PM886_REG_MISC_CONFIG1 0x14
> +#define PM886_SW_PDOWN BIT(5)
> +
> +#define PM886_REG_MISC_CONFIG2 0x15
> +#define PM886_INT_INVBIT(0)
> +#define PM886_INT_CLEAR BIT(1)
> +#define PM886_INT_RC 0x00
> +#define PM886_INT_WC BIT(1)
> +#define PM886_INT_MASK_MODE BIT(2)
> +
> +struct pm886_chip {
> + struct i2c_client *client;
> + unsigned int chip_id;
> + struct regmap *regmap;
> +};
> +
> +static const struct regmap_config pm886_i2c_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = PM886_REGMAP_CONF_MAX_REG,
> +};
Why is this in here?
> +#endif /* __MFD_88PM886_H */
What would you like me to do with this RFC patch?
--
Lee Jones [李琼斯]
[RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
From: Karel Balej
Marvell 88PM886 is a PMIC which provides various functions such as
onkey, battery, charger and regulators. It is found for instance in the
samsung,coreprimevelte smartphone with which this was tested. Implement
basic support to allow for the use of regulators and onkey.
Signed-off-by: Karel Balej
---
Notes:
RFC v4:
- Use MFD_CELL_* macros.
- Address Lee's feedback:
- Do not define regmap_config.val_bits and .reg_bits.
- Drop everything regulator related except mfd_cell (regmap
initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
- Do not store regmap pointers as an array as there is now only one
regmap. Also drop the corresponding enum.
- Move regmap_config to the header as it is needed in the regulators
driver.
- pm886_chip.whoami -> chip_id
- Reword chip ID mismatch error message and print the ID as
hexadecimal.
- Fix includes in include/linux/88pm886.h.
- Drop the pm886_irq_number enum and define the (for the moment) only
IRQ explicitly.
- Have only one MFD cell for all regulators as they are now registered
all at once in the regulators driver.
- Reword commit message.
- Make device table static and remove comma after the sentinel to signal
that nothing should come after it.
RFC v3:
- Drop onkey cell .of_compatible.
- Rename LDO page offset and regmap to REGULATORS.
RFC v2:
- Remove some abstraction.
- Sort includes alphabetically and add linux/of.h.
- Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
- Use more temporaries and break long lines.
- Do not initialize ret in probe.
- Use the wakeup-source DT property.
- Rename ret to err.
- Address Lee's comments:
- Drop patched in presets for base regmap and related defines.
- Use full sentences in comments.
- Remove IRQ comment.
- Define regmap_config member values.
- Rename data to sys_off_data.
- Add _PMIC suffix to Kconfig.
- Use dev_err_probe.
- Do not store irq_data.
- s/WHOAMI/CHIP_ID
- Drop LINUX part of include guard name.
- Merge in the regulator series modifications in order to have more
devices and modify the commit message accordingly. Changes with
respect to the original regulator series patches:
- ret -> err
- Add temporary for dev in pm88x_initialize_subregmaps.
- Drop of_compatible for the regulators.
- Do not duplicate LDO regmap for bucks.
- Rewrite commit message.
drivers/mfd/88pm886.c | 149
drivers/mfd/Kconfig | 12 +++
drivers/mfd/Makefile| 1 +
include/linux/mfd/88pm886.h | 38 +
4 files changed, 200 insertions(+)
create mode 100644 drivers/mfd/88pm886.c
create mode 100644 include/linux/mfd/88pm886.h
diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
new file mode 100644
index ..88f21f813ec1
--- /dev/null
+++ b/drivers/mfd/88pm886.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define PM886_REG_INT_STATUS1 0x05
+
+#define PM886_REG_INT_ENA_10x0a
+#define PM886_INT_ENA1_ONKEY BIT(0)
+
+#define PM886_IRQ_ONKEY0
+
+static struct regmap_irq pm886_regmap_irqs[] = {
+ REGMAP_IRQ_REG(PM886_IRQ_ONKEY, 0, PM886_INT_ENA1_ONKEY),
+};
+
+static struct regmap_irq_chip pm886_regmap_irq_chip = {
+ .name = "88pm886",
+ .irqs = pm886_regmap_irqs,
+ .num_irqs = ARRAY_SIZE(pm886_regmap_irqs),
+ .num_regs = 4,
+ .status_base = PM886_REG_INT_STATUS1,
+ .ack_base = PM886_REG_INT_STATUS1,
+ .unmask_base = PM886_REG_INT_ENA_1,
+};
+
+static struct resource pm886_onkey_resources[] = {
+ DEFINE_RES_IRQ_NAMED(PM886_IRQ_ONKEY, "88pm886-onkey"),
+};
+
+static struct mfd_cell pm886_devs[] = {
+ MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
+ MFD_CELL_NAME("88pm886-regulator"),
+};
+
+static int pm886_power_off_handler(struct sys_off_data *sys_off_data)
+{
+ struct pm886_chip *chip = sys_off_data->cb_data;
+ struct regmap *regmap = chip->regmap;
+ struct device *dev = >client->dev;
+ int err;
+
+ err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG1, PM886_SW_PDOWN,
+ PM886_SW_PDOWN);
+ if (err) {
+ dev_err(dev, "Failed to power off the device: %d\n", err);
+ return NOTIFY_BAD;
+ }
+ return NOTIFY_DONE;
+}
+
+static int pm886_setup_irq(struct pm886_chip *chip,
+ struct regmap_irq_chip_data **irq_data)
+{
+ struct regmap *regmap = chip->regmap;
+ struct device *dev = >client->dev;
+ int err;
+
+ /* Set interrupt clearing mode to
