Received-SPF: pass (google.com: domain of linux-kernel+bounces-110318-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Thu, 21 Mar 2024 17:12:52 +0100
Message-Id: <CZZK759UU6G7.MFPYOI0HBB6I@matfyz.cz>
Cc: "Rob Herring" <robh+dt@kernel.org>,
        "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@linaro.org>,
        "Conor Dooley" <conor+dt@kernel.org>,
        "Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
        "Liam Girdwood"
 <lgirdwood@gmail.com>,
        "Mark Brown" <broonie@kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-input@vger.kernel.org>,
        =?utf-8?q?Duje_Mihanovi=C4=87?=
 <duje.mihanovic@skole.hr>,
        <~postmarketos/upstreaming@lists.sr.ht>, <phone-devel@vger.kernel.org>
Subject: Re: [RFC PATCH v4 2/5] mfd: add driver for Marvell 88PM886 PMIC
To: "Lee Jones" <lee@kernel.org>
From: "Karel Balej" <balejk@matfyz.cz>
References: <20240311160110.32185-1-karelb@gimli.ms.mff.cuni.cz>
 <20240311160110.32185-3-karelb@gimli.ms.mff.cuni.cz>
 <20240321154211.GA13211@google.com>
In-Reply-To: <20240321154211.GA13211@google.com>

Lee Jones, 2024-03-21T15:42:11+00:00:
> On Mon, 11 Mar 2024, Karel Balej wrote:
>
> > From: Karel Balej <balejk@matfyz.cz>
> >=20
> > 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.
> >=20
> > Signed-off-by: Karel Balej <balejk@matfyz.cz>
> > ---
> >=20
> > 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_subregmap=
s.
> >       - Do not store regmap pointers as an array as there is now only o=
ne
> >         regmap. Also drop the corresponding enum.
> >       - Move regmap_config to the header as it is needed in the regulat=
ors
> >         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 registe=
red
> >       all at once in the regulators driver.
> >     - Reword commit message.
> >     - Make device table static and remove comma after the sentinel to s=
ignal
> >       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 mo=
re
> >         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.
> >=20
> >  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 =3D {
> > +	.reg_bits =3D 8,
> > +	.val_bits =3D 8,
> > +	.max_register =3D 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 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.

Thank you, best regards,
K. B.