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 <balejk@xxxxxxxxx>
> > >
> > > 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 <balejk@xxxxxxxxx>
> > > ---
> > >
> > > 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 000000000000..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 <linux/i2c.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#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_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?
>
> 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?
> > 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 the regulators driver).
>
> I have added the remaining regulators to the driver just today and plan
> to send the first non-RFC version of the series after I test it, likely
> over the weekend. Hopefully the regulators patch will finally get some
> attention then.
Don't forget that the merge-window is open.
Many maintainers take a well deserved nap during this period.
--
Lee Jones [李琼斯]
