Re: [PATCH 1/5] staging: drivers: mfd: Add MAX77541/MAX77540 PMIC Support

From: Jonathan Cameron
Date: Sun Dec 11 2022 - 07:24:15 EST

Next message: Russell King (Oracle): "Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA"
Previous message: Yuwei Guan: "Re: [PATCH] f2fs: continuous counting for 'issued' in __issue_discard_cmd_orderly()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 7 Dec 2022 12:08:40 +0300
Okan Sahin <okan.sahin@xxxxxxxxxx> wrote:

> This patch adds MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
>
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
>
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
>
> They have same regmap except for ADC part of MAX77541.
>
> Signed-off-by: Okan Sahin <okan.sahin@xxxxxxxxxx>

I'll try not to duplicate the points Andy has already made, but there
may be some overlap!

> ---
> MAINTAINERS | 7 ++
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77541.c | 236 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77541.h | 150 ++++++++++++++++++++++

Huh? It's not in staging, so just change the patch titles!

> 5 files changed, 407 insertions(+)
> create mode 100644 drivers/mfd/max77541.c
> create mode 100644 include/linux/mfd/max77541.h
>
or MAX77620 and MAX20024 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..bf21228f5742 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -161,6 +161,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o
> obj-$(CONFIG_MFD_DA9150) += da9150-core.o
>
> obj-$(CONFIG_MFD_MAX14577) += max14577.o
> +obj-$(CONFIG_MFD_MAX77541) += max77541.o
> obj-$(CONFIG_MFD_MAX77620) += max77620.o
> obj-$(CONFIG_MFD_MAX77650) += max77650.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> diff --git a/drivers/mfd/max77541.c b/drivers/mfd/max77541.c
> new file mode 100644
> index 000000000000..97a2df3ce0b6
> --- /dev/null
> +++ b/drivers/mfd/max77541.c
> @@ -0,0 +1,236 @@

...

> +
> +static int max77541_pmic_setup(struct max77541_dev *me)
> +{
> + struct regmap *regmap = me->regmap;
> + struct device *dev = me->dev;
> + unsigned int val;
> + int ret;
> +
> + ret = max77541_pmic_irq_init(me);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
> +
> + ret = regmap_read(regmap, MAX77541_REG_INT_SRC, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, MAX77541_REG_TOPSYS_INT, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, MAX77541_REG_BUCK_INT, &val);
> + if (ret)
> + return ret;
> +
> + ret = device_init_wakeup(dev, true);
> + if (ret)
> + return dev_err_probe(dev, ret, "Unable to init wakeup\n");
> +
> + switch (me->type) {
> + case MAX77540:
once you've switched to using a chip_info structure instead this block just becomes

return devm_mfd_add_devices(dev, -1, me->chip_info.devs, me->chip_info.num_devs,
NULL, 0, NULL);

> + return devm_mfd_add_devices(dev, -1, max77540_devs, ARRAY_SIZE(max77540_devs),
> + NULL, 0, NULL);
> + case MAX77541:
> + return devm_mfd_add_devices(dev, -1, max77541_devs, ARRAY_SIZE(max77541_devs),
> + NULL, 0, NULL);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max77541_i2c_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *chip_id;
> + struct max77541_dev *me;
> + const void *chip_data;
> +
> + me = devm_kzalloc(&client->dev, sizeof(*me), GFP_KERNEL);
> + if (!me)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, me);
> + me->dev = &client->dev;
> + me->i2c = client;
> +
> + chip_id = to_i2c_driver(client->dev.driver)->id_table;
> +
> + chip_data = device_get_match_data(me->dev);
> + if (!chip_data) {
> + chip_data = (void *)i2c_match_id(chip_id, client)->driver_data;

Ah. This is why your enum doesn't have 0 in it. Solve that by using a poitner
to a chip_info structure rather than an enum value

> + if (!chip_data)
> + return dev_err_probe(me->dev, -ENODEV, "Unable to find device\n");

Message is rather misleading. More a case of unknown device.

> + }
> +
> + me->type = (enum dev_type)chip_data;
> + me->regmap = devm_regmap_init_i2c(client, &max77541_regmap_config);
> + if (IS_ERR(me->regmap))
> + return dev_err_probe(me->dev, PTR_ERR(me->regmap),
> + "Failed to allocate register map\n");
> +
> + return max77541_pmic_setup(me);
> +}
> +
> +static const struct of_device_id max77541_of_id[] = {
> + {
> + .compatible = "adi,max77540",
> + .data = (void *)MAX77540,
> + },
> + {
> + .compatible = "adi,max77541",
> + .data = (void *)MAX77541,
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> +
> +static const struct i2c_device_id max77541_i2c_id[] = {
> + { "max77540", MAX77540 },
> + { "max77541", MAX77541 },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77541_i2c_id);
> +
> +static struct i2c_driver max77541_i2c_driver = {
> + .driver = {
> + .name = "max77541",
> + .of_match_table = max77541_of_id,
> + },
> + .probe_new = max77541_i2c_probe,
> + .id_table = max77541_i2c_id,
> +};
> +
> +module_i2c_driver(max77541_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAX7740/MAX7741 MFD Driver");
> +MODULE_AUTHOR("Okan Sahin <okan.sahin@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

We almost never assign module versions any more. They aren't useful
given we must maintain backwards compatibility of userspace interfaces anyway.

> diff --git a/include/linux/mfd/max77541.h b/include/linux/mfd/max77541.h
> new file mode 100644
> index 000000000000..6f2753300227
> --- /dev/null
> +++ b/include/linux/mfd/max77541.h
> @@ -0,0 +1,150 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __MAX77541_MFD_H__
> +#define __MAX77541_MFD_H__
> +
> +/* REGISTERS */
> +
> +/* GLOBAL CONFIG1 */
> +#define MAX77541_REG_INT_SRC 0x00
> +#define MAX77541_REG_INT_SRC_M 0x01
> +#define MAX77541_REG_TOPSYS_INT 0x02
> +#define MAX77541_REG_TOPSYS_INT_M 0x03
> +#define MAX77541_REG_TOPSYS_STAT 0x04
> +#define MAX77541_REG_DEVICE_CFG1 0x06
> +#define MAX77541_REG_DEVICE_CFG2 0x07
> +#define MAX77541_REG_TOPSYS_CFG 0x08
> +#define MAX77541_REG_PROT_CFG 0x09
> +#define MAX77541_REG_EN_CTRL 0x0B
> +
> +/* GLOBAL CONFIG2 */

Hmm. A register called CFG1 is CONFIG2?
That's novel ;)

> +#define MAX77541_REG_GLB_CFG1 0x11
> +
> +/* BUCK CONFIG */
> +#define MAX77541_REG_BUCK_INT 0x20
> +#define MAX77541_REG_BUCK_INT_M 0x21
> +#define MAX77541_REG_BUCK_STAT 0x22
> +
> +/* BUCK1 */
> +#define MAX77541_REG_M1_VOUT 0x23
> +#define MAX77541_REG_M1_CFG1 0x25
> +#define MAX77541_REG_M1_CFG2 0x26
> +#define MAX77541_REG_M1_CFG3 0x27
> +
> +/* BUCK2 */
> +#define MAX77541_REG_M2_VOUT 0x33
> +#define MAX77541_REG_M2_CFG1 0x35
> +#define MAX77541_REG_M2_CFG2 0x36
> +#define MAX77541_REG_M2_CFG3 0x37
> +
> +#define MAX77541_REG_NOT_AVAILABLE 0xFF
> +
> +/* INTERRUPT MASKS*/
> +#define MAX77541_REG_INT_SRC_MASK 0x00
> +#define MAX77541_REG_TOPSYS_INT_MASK 0x00
> +#define MAX77541_REG_BUCK_INT_MASK 0x00
> +
> +/*BITS OF REGISTERS*/
> +
> +#define MAX77541_BIT_INT_SRC_TOPSYS BIT(0)
> +#define MAX77541_BIT_INT_SRC_BUCK BIT(1)
> +
> +#define MAX77541_BIT_TOPSYS_INT_TJ_120C BIT(0)
> +#define MAX77541_BIT_TOPSYS_INT_TJ_140C BIT(1)
> +#define MAX77541_BIT_TOPSYS_INT_TSHDN BIT(2)
> +#define MAX77541_BIT_TOPSYS_INT_UVLO BIT(3)
> +#define MAX77541_BIT_TOPSYS_INT_ALT_SWO BIT(4)
> +#define MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET BIT(5)
> +
> +#define MAX77541_BIT_BUCK_INT_M1_POK_FLT BIT(0)
> +#define MAX77541_BIT_BUCK_INT_M2_POK_FLT BIT(1)
> +#define MAX77541_BIT_BUCK_INT_M1_SCFLT BIT(4)
> +#define MAX77541_BIT_BUCK_INT_M2_SCFLT BIT(5)
> +
> +#define MAX77541_BITS_DEVICE_CFG1_SEL1_LATCH GENMASK(4, 0)
> +#define MAX77541_BITS_DEVICE_CFG2_SEL2_LATCH GENMASK(4, 0)
> +
> +#define MAX77541_BIT_TOPSYS_CFG_DIS_ALT_IN BIT(0)
> +
> +#define MAX77541_BITS_PROT_CFG_POK_TO GENMASK(1, 0)
> +#define MAX77541_BIT_PROT_CFG_EN_FTMON BIT(2)
> +
> +#define MAX77541_BIT_M1_EN BIT(0)
> +#define MAX77541_BIT_M2_EN BIT(1)
> +#define MAX77541_BIT_M1_LPM BIT(4)
> +#define MAX77541_BIT_M2_LPM BIT(5)
> +
> +#define MAX77541_BITS_GLB_CFG1_SSTOP_SR GENMASK(2, 0)

Naming of BITS is unusual for what I think is a field mask.
MSK or MASK is more common. Cn use that for the single bit
fields as well.

> +#define MAX77541_BITS_GLB_CFG1_SSTRT_SR GENMASK(5, 3)
> +
> +#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
> +
> +#define MAX77541_BITS_MX_CFG1_RU_SR GENMASK(2, 0)
> +#define MAX77541_BITS_MX_CFG1_RD_SR GENMASK(5, 3)
> +#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
> +
> +#define MAX77541_BIT_MX_CFG2_FPWM BIT(0)
> +#define MAX77541_BIT_MX_CFG2_FSREN BIT(1)
> +#define MAX77541_BITS_MX_CFG2_SS_PAT GENMASK(3, 2)
> +#define MAX77541_BITS_MX_CFG2_SS_FREQ GENMASK(5, 4)
> +#define MAX77541_BITS_MX_CFG2_SS_ENV GENMASK(7, 6)
> +
> +#define MAX77541_BITS_MX_CFG3_ILIM GENMASK(1, 0)
> +#define MAX77541_BITS_MX_CFG3_FREQ GENMASK(3, 2)
> +#define MAX77541_BIT_MX_CFG3_FTRAK BIT(4)
> +#define MAX77541_BIT_MX_CFG3_RESRESH BIT(5)
> +#define MAX77541_BIT_MX_CFG3_ADIS1 BIT(6)
> +#define MAX77541_BIT_MX_CFG3_ADIS100 BIT(7)
> +
> +#define MAX77541_MAX_REGULATORS 2
> +
> +/* ADC */
> +#define MAX77541_REG_ADC_INT 0x70
> +#define MAX77541_REG_ADC_MSK 0x71
> +
> +#define MAX77541_REG_ADC_DATA_CH1 0x72
> +#define MAX77541_REG_ADC_DATA_CH2 0x73
> +#define MAX77541_REG_ADC_DATA_CH3 0x74
> +#define MAX77541_REG_ADC_DATA_CH6 0x77
> +
> +#define MAX77541_BIT_ADC_INT_CH1_I BIT(0)
> +#define MAX77541_BIT_ADC_INT_CH2_I BIT(1)
> +#define MAX77541_BIT_ADC_INT_CH3_I BIT(2)
> +#define MAX77541_BIT_ADC_INT_CH6_I BIT(5)
> +
> +#define MAX77541_REGMAP_IRQ_REG(_mask) \
> + { .mask = (_mask), }

Define is longer than the code. Just use the code everywhere
you current use this macro.

> +
> +enum dev_type {
> + MAX77540 = 1,
If you need to index from 1, add a comment on why.

> + MAX77541 = 2,
> +};
> +
> +enum max77541_regulators {
> + MAX77541_BUCK1 = 1,
> + MAX77541_BUCK2,
> +};
> +
> +enum mx_range {

Prefix this with max77541
Also, probably better to introduce it in the patch where it is used.
For now I've no idea why we need a reserved value for example!

> + LOW_RANGE,
> + MID_RANGE,
> + HIGH_RANGE,
> + RESERVED
> +};
> +
> +struct max77541_dev {

> + u8 type;

Rather than using an enum (incidentally the type of this should be a
named enum, not a u8) much better to define a
'chip info' structure that describes the different specific features of
each chip. That approach ends up much more extensible as new devices
are added to the driver than a constant increase in switch statements.

> +};
> +
> +#endif /* __MAX77541_MFD_H__ */

Next message: Russell King (Oracle): "Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA"
Previous message: Yuwei Guan: "Re: [PATCH] f2fs: continuous counting for 'issued' in __issue_discard_cmd_orderly()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]