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

2024-05-31 Thread Karel Balej
Lee Jones, 2024-05-31T11:24:52+01:00:
> Are you planning on seeing to Mark's review comments?

Indeed, I'm hoping that I will be able to send it over the weekend.

K. B.



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

2024-05-31 Thread Lee Jones
On Sat, 04 May 2024, Karel Balej wrote:

> 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:
> v6:
> - Address Lee's comments:
>   - Don't break long line in the power off handler.
>   - Set PLATFORM_DEVID_NONE. This should be safe with respect to
> collisions since there are no known devices with more than one of
> these PMICs, plus given their general purpose nature it is unlikely
> that there would ever be. Also include the corresponding header.
>   - Move all defines to the header.
> - Give the base page's maximum register its real name.
> - Set irq_base to 0.
> v5:
> - Address Mark's feedback:
>   - Move regmap config back out of the header and rename it. Also lower
> its maximum register based on what's actually used in the downstream
> code.
> 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   | 148 
>  drivers/mfd/Kconfig |  12 +++
>  drivers/mfd/Makefile|   1 +
>  include/linux/mfd/88pm886.h |  69 +
>  4 files changed, 230 insertions(+)
>  create mode 100644 drivers/mfd/88pm886.c
>  create mode 100644 include/linux/mfd/88pm886.h

I don't see any more issues.

Are you planning on seeing to Mark's review comments?

-- 
Lee Jones [李琼斯]



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

2024-05-04 Thread 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:
v6:
- Address Lee's comments:
  - Don't break long line in the power off handler.
  - Set PLATFORM_DEVID_NONE. This should be safe with respect to
collisions since there are no known devices with more than one of
these PMICs, plus given their general purpose nature it is unlikely
that there would ever be. Also include the corresponding header.
  - Move all defines to the header.
- Give the base page's maximum register its real name.
- Set irq_base to 0.
v5:
- Address Mark's feedback:
  - Move regmap config back out of the header and rename it. Also lower
its maximum register based on what's actually used in the downstream
code.
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   | 148 
 drivers/mfd/Kconfig |  12 +++
 drivers/mfd/Makefile|   1 +
 include/linux/mfd/88pm886.h |  69 +
 4 files changed, 230 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 ..dbe9efc027d2
--- /dev/null
+++ b/drivers/mfd/88pm886.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+static const struct regmap_config pm886_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+   .max_register = PM886_REG_RTC_SPARE6,
+};
+
+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