Received-SPF: pass (google.com: domain of linux-kernel+bounces-110269-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 21 Mar 2024 15:42:11 +0000
From: Lee Jones <lee@kernel.org>
To: Karel Balej <karelb@gimli.ms.mff.cuni.cz>
Cc: Karel Balej <balejk@matfyz.cz>, 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,
	Duje =?utf-8?Q?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
Message-ID: <20240321154211.GA13211@google.com>
References: <20240311160110.32185-1-karelb@gimli.ms.mff.cuni.cz>
 <20240311160110.32185-3-karelb@gimli.ms.mff.cuni.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240311160110.32185-3-karelb@gimli.ms.mff.cuni.cz>

On Mon, 11 Mar 2024, Karel Balej wrote:

> From: Karel Balej <balejk@matfyz.cz>
> 
> 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@matfyz.cz>
> ---
> 
> 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?

> +#endif /* __MFD_88PM886_H */

What would you like me to do with this RFC patch?

-- 
Lee Jones [李琼斯]