subject:"\[RFC PATCH v3 2\/5\] mfd\: add driver for Marvell 88PM886 PMIC"

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

2024-03-07 Thread Lee Jones

> > > +static int pm886_initialize_subregmaps(struct pm886_chip *chip)
> > > +{
> > > + struct device *dev = >client->dev;
> > > + struct i2c_client *page;
> > > + struct regmap *regmap;
> > > + int err;
> > > +
> > > + /* regulators page */
> > > + page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> > > + chip->client->addr + 
> > > PM886_PAGE_OFFSET_REGULATORS);
> > > + if (IS_ERR(page)) {
> > > + err = PTR_ERR(page);
> > > + dev_err(dev, "Failed to initialize regulators client: %d\n", 
> > > err);
> > > + return err;
> > > + }
> > > + regmap = devm_regmap_init_i2c(page, _i2c_regmap);
> > > + if (IS_ERR(regmap)) {
> > > + err = PTR_ERR(regmap);
> > > + dev_err(dev, "Failed to initialize regulators regmap: %d\n", 
> > > err);
> > > + return err;
> > > + }
> > > + chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
> >
> > Except for the regulator driver, where else is the regulators regmap used?
> 
> Nowhere, at least as of now. So you are saying that I should initialize
> the regmap in the regulator driver?

I am.

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

2024-03-05 Thread Karel Balej

Lee Jones, 2024-03-05T11:44:18+00:00:
> > +static struct mfd_cell pm886_devs[] = {
> > +   {
> > +   .name = "88pm886-onkey",
> > +   .num_resources = ARRAY_SIZE(pm886_onkey_resources),
> > +   .resources = pm886_onkey_resources,
> > +   },
> > +   {
> > +   .name = "88pm886-regulator",
> > +   .id = PM886_REGULATOR_ID_LDO2,
>
> Why doesn't PLATFORM_DEVID_AUTO work for this device?

Because I am using the IDs in the regulator driver to determine which
regulator data to use/which regulator to register.

> > +static int pm886_initialize_subregmaps(struct pm886_chip *chip)
> > +{
> > +   struct device *dev = >client->dev;
> > +   struct i2c_client *page;
> > +   struct regmap *regmap;
> > +   int err;
> > +
> > +   /* regulators page */
> > +   page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> > +   chip->client->addr + 
> > PM886_PAGE_OFFSET_REGULATORS);
> > +   if (IS_ERR(page)) {
> > +   err = PTR_ERR(page);
> > +   dev_err(dev, "Failed to initialize regulators client: %d\n", 
> > err);
> > +   return err;
> > +   }
> > +   regmap = devm_regmap_init_i2c(page, _i2c_regmap);
> > +   if (IS_ERR(regmap)) {
> > +   err = PTR_ERR(regmap);
> > +   dev_err(dev, "Failed to initialize regulators regmap: %d\n", 
> > err);
> > +   return err;
> > +   }
> > +   chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
>
> Except for the regulator driver, where else is the regulators regmap used?

Nowhere, at least as of now. So you are saying that I should initialize
the regmap in the regulator driver?

Thank you,
K. B.

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

2024-03-05 Thread Lee Jones

On Sun, 03 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.
> 
> Only implement basic support to allow for the use of regulators and
> onkey omitting the currently unused register definitions and I2C
> subclients which should thus be added with the subdevice drivers which
> need them.
> 
> Signed-off-by: Karel Balej 
> ---
> 
> Notes:
> 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

I still see 'whoami'.

>   - 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   | 210 
>  drivers/mfd/Kconfig |  12 +++
>  drivers/mfd/Makefile|   1 +
>  include/linux/mfd/88pm886.h |  46 
>  4 files changed, 269 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 ..c17220e1b7e2
> --- /dev/null
> +++ b/drivers/mfd/88pm886.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Header?  Description?  Author?  Copyright?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define PM886_REG_INT_STATUS10x05
> +
> +#define PM886_REG_INT_ENA_1  0x0a
> +#define PM886_INT_ENA1_ONKEY BIT(0)
> +
> +#define PM886_REGMAP_CONF_REG_BITS   8
> +#define PM886_REGMAP_CONF_VAL_BITS   8

These 2 are not commonly defined, just the one below please.

> +#define PM886_REGMAP_CONF_MAX_REG0xfe

> +enum pm886_irq_number {
> + PM886_IRQ_ONKEY,
> +
> + PM886_MAX_IRQ
> +};

Seems superfluous for 1 IRQ and an unused MAX.

Looks like I've mentioned this before.

The IRQ number probably shouldn't be arbitrary either.

> +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[] = {
> + {
> + .name = "88pm886-onkey",
> + .num_resources = ARRAY_SIZE(pm886_onkey_resources),
> + .resources = pm886_onkey_resources,
> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_LDO2,

Why doesn't PLATFORM_DEVID_AUTO work for this device?

> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_LDO15,
> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_BUCK2,
> + },
> +};
> +
> +static const struct regmap_config pm886_i2c_regmap = {
> + .reg_bits = PM886_REGMAP_CONF_REG_BITS,
> + .val_bits = PM886_REGMAP_CONF_VAL_BITS,
> + .max_register = PM886_REGMAP_CONF_MAX_REG,
> +};
> +
/> +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->regmaps[PM886_REGMAP_BASE];
> + struct device *dev = >client->dev;
> + int err;
> +
> + err = regmap_update_bits(regmap,

[RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

2024-03-03 Thread Karel Balej

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.

Only implement basic support to allow for the use of regulators and
onkey omitting the currently unused register definitions and I2C
subclients which should thus be added with the subdevice drivers which
need them.

Signed-off-by: Karel Balej 
---

Notes:
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   | 210 
 drivers/mfd/Kconfig |  12 +++
 drivers/mfd/Makefile|   1 +
 include/linux/mfd/88pm886.h |  46 
 4 files changed, 269 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 ..c17220e1b7e2
--- /dev/null
+++ b/drivers/mfd/88pm886.c
@@ -0,0 +1,210 @@
+// 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_REGMAP_CONF_REG_BITS 8
+#define PM886_REGMAP_CONF_VAL_BITS 8
+#define PM886_REGMAP_CONF_MAX_REG  0xfe
+
+enum pm886_irq_number {
+   PM886_IRQ_ONKEY,
+
+   PM886_MAX_IRQ
+};
+
+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[] = {
+   {
+   .name = "88pm886-onkey",
+   .num_resources = ARRAY_SIZE(pm886_onkey_resources),
+   .resources = pm886_onkey_resources,
+   },
+   {
+   .name = "88pm886-regulator",
+   .id = PM886_REGULATOR_ID_LDO2,
+   },
+   {
+   .name = "88pm886-regulator",
+   .id = PM886_REGULATOR_ID_LDO15,
+   },
+   {
+   .name = "88pm886-regulator",
+   .id = PM886_REGULATOR_ID_BUCK2,
+   },
+};
+
+static const struct regmap_config pm886_i2c_regmap = {
+   .reg_bits = PM886_REGMAP_CONF_REG_BITS,
+   .val_bits = PM886_REGMAP_CONF_VAL_BITS,
+   .max_register = PM886_REGMAP_CONF_MAX_REG,
+};
+
+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->regmaps[PM886_REGMAP_BASE];
+   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_initialize_subregmaps(struct pm886_chip *chip)
+{
+   struct device *dev = >client->dev;
+   struct i2c_client *page;
+   struct regmap *regmap;
+   int err;
+
+   /* regulators page */
+   page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
+

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

[RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC

4 matches

Site Navigation

Mail list logo

Footer information