Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 9 Aug 2022 09:52:23 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     cy_huang <u0084500@gmail.com>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        broonie@kernel.org, dmitry.torokhov@gmail.com, lgirdwood@gmail.com,
        cy_huang@richtek.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Lee Jones <lee@kernel.org>
Subject: Re: [PATCH v3 2/3] mfd: rt5120: Add Richtek PMIC support
Message-ID: <YvIgR9RBDMp9VJ6B@google.com>
References: <1657780937-20891-1-git-send-email-u0084500@gmail.com>
 <1657780937-20891-3-git-send-email-u0084500@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1657780937-20891-3-git-send-email-u0084500@gmail.com>
Precedence: bulk

On Thu, 14 Jul 2022, cy_huang wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add Richtek RT5120 PMIC I2C driver.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> Since v3:
> - Use a 'dev' variable and dev_err_probe to decrease the LOC in mfd.
> 
> ---
>  drivers/mfd/Kconfig  |  12 +++++
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/rt5120.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/mfd/rt5120.c

Mostly good, couple of nits.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456..866619c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1127,6 +1127,18 @@ config MFD_RT5033
>  	  sub-devices like charger, fuel gauge, flash LED, current source,
>  	  LDO and Buck.
>  
> +config MFD_RT5120
> +	tristate "Richtek RT5120 Power Management IC"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  The enables support for Richtek RT5120 PMIC. It includes four high
> +	  efficiency buck converters and one LDO voltage regulator. The device
> +	  is targeted at providing the CPU voltage, memory, I/O and peripheral
> +	  power rails in home entertainment devices.
> +
>  config MFD_RC5T583
>  	bool "Ricoh RC5T583 Power Management system device"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf..27e8add 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -234,6 +234,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT4831)	+= rt4831.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
> +obj-$(CONFIG_MFD_RT5120)	+= rt5120.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
> diff --git a/drivers/mfd/rt5120.c b/drivers/mfd/rt5120.c
> new file mode 100644
> index 00000000..12372fa
> --- /dev/null
> +++ b/drivers/mfd/rt5120.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0+

No Copyright?

> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +
> +#define RT5120_REG_INTENABLE	0x1D
> +#define RT5120_REG_INTSTAT	0x1E
> +#define RT5120_REG_FZCMODE	0x44
> +
> +#define RT5120_INT_HOTDIE	0
> +#define RT5120_INT_PWRKEY_REL	5
> +#define RT5120_INT_PWRKEY_PRESS	6
> +
> +static const struct regmap_range rt5120_rd_yes_ranges[] = {
> +	regmap_reg_range(0x03, 0x13),
> +	regmap_reg_range(0x1c, 0x20),
> +	regmap_reg_range(0x44, 0x44)
> +};
> +
> +static const struct regmap_range rt5120_wr_yes_ranges[] = {
> +	regmap_reg_range(0x06, 0x13),
> +	regmap_reg_range(0x1c, 0x20),
> +	regmap_reg_range(0x44, 0x44)
> +};
> +
> +static const struct regmap_access_table rt5120_rd_table = {
> +	.yes_ranges = rt5120_rd_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(rt5120_rd_yes_ranges),
> +};
> +
> +static const struct regmap_access_table rt5120_wr_table = {
> +	.yes_ranges = rt5120_wr_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(rt5120_wr_yes_ranges),
> +};
> +
> +static const struct regmap_config rt5120_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT5120_REG_FZCMODE,
> +
> +	.wr_table = &rt5120_wr_table,
> +	.rd_table = &rt5120_rd_table,
> +};
> +
> +static const struct regmap_irq rt5120_irqs[] = {
> +	REGMAP_IRQ_REG_LINE(RT5120_INT_HOTDIE, 8),
> +	REGMAP_IRQ_REG_LINE(RT5120_INT_PWRKEY_REL, 8),
> +	REGMAP_IRQ_REG_LINE(RT5120_INT_PWRKEY_PRESS, 8)
> +};
> +
> +static const struct regmap_irq_chip rt5120_irq_chip = {
> +	.name = "rt5120-pmic",
> +	.status_base = RT5120_REG_INTSTAT,
> +	.mask_base = RT5120_REG_INTENABLE,
> +	.ack_base = RT5120_REG_INTSTAT,
> +	.mask_invert = true,
> +	.use_ack = true,
> +	.num_regs = 1,
> +	.irqs = rt5120_irqs,
> +	.num_irqs = ARRAY_SIZE(rt5120_irqs),
> +};
> +
> +static const struct resource rt5120_regulator_resources[] = {
> +	DEFINE_RES_IRQ(RT5120_INT_HOTDIE)
> +};
> +
> +static const struct resource rt5120_pwrkey_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(RT5120_INT_PWRKEY_PRESS, "pwrkey-press"),
> +	DEFINE_RES_IRQ_NAMED(RT5120_INT_PWRKEY_REL, "pwrkey-release")
> +};
> +
> +static const struct mfd_cell rt5120_devs[] = {
> +	MFD_CELL_RES("rt5120-regulator", rt5120_regulator_resources),
> +	MFD_CELL_OF("rt5120-pwrkey", rt5120_pwrkey_resources, NULL, 0, 0,
> +		    "richtek,rt5120-pwrkey")

This can be on one line if you want.

> +};
> +
> +static int rt5120_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct regmap *regmap;
> +	struct regmap_irq_chip_data *irq_data;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &rt5120_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = devm_regmap_add_irq_chip(dev, regmap, i2c->irq, IRQF_ONESHOT, 0,
> +				       &rt5120_irq_chip, &irq_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add irq chip\n");

"IRQ"

> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rt5120_devs,
> +				    ARRAY_SIZE(rt5120_devs), NULL, 0,
> +				    regmap_irq_get_domain(irq_data));
> +}
> +
> +static const struct of_device_id rt5120_device_match_table[] = {
> +	{ .compatible = "richtek,rt5120" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rt5120_device_match_table);
> + 
> +static struct i2c_driver rt5120_driver = {
> +	.driver = {
> +		.name = "rt5120",
> +		.of_match_table = rt5120_device_match_table,
> +	},
> +	.probe_new = rt5120_probe,
> +};
> +module_i2c_driver(rt5120_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_DESCRIPTION("Richtek RT5120 I2C driver");
> +MODULE_LICENSE("GPL v2");

-- 
DEPRECATED: Please use lee@kernel.org