From: Lee Jones <lee@kernel.org>
To: Okan Sahin <okan.sahin@analog.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Marcus Folkesson <marcus.folkesson@gmail.com>,
Marcelo Schmitt <marcelo.schmitt1@gmail.com>,
ChiYuan Huang <cy_huang@richtek.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
William Breathitt Gray <william.gray@linaro.org>,
Caleb Connolly <caleb.connolly@linaro.org>,
Ramona Bolboaca <ramona.bolboaca@analog.com>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-iio@vger.kernel.org
Subject: Re: [PATCH v4 5/5] drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support
Date: Fri, 3 Mar 2023 13:29:42 +0000 [thread overview]
Message-ID: <20230303132942.GN2420672@google.com> (raw)
In-Reply-To: <20230201103534.108136-6-okan.sahin@analog.com>
On Wed, 01 Feb 2023, Okan Sahin wrote:
> 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@analog.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77541.c | 244 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77541.h | 108 ++++++++++++++++
> 4 files changed, 366 insertions(+)
> create mode 100644 drivers/mfd/max77541.c
> create mode 100644 include/linux/mfd/max77541.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..e6bf621cbc8e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -791,6 +791,19 @@ config MFD_MAX14577
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_MAX77541
> + tristate "Analog Devices MAX77541/77540 PMIC Support"
> + depends on I2C=y
> + select MFD_CORE
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + help
> + Say yes here to add support for Analog Devices
60-chars is not a good choice for line wrap. 80 would be better.
> + MAX77541 and MAX77540 Power Management ICs.This
> + driver provides common support for accessing the
> + device;additional drivers must be enabled in order
> + to use the functionality of the device.
Spaces after '.' and ';'.
Please mention the device functionality i.e. the children.
> +
> config MFD_MAX77620
> bool "Maxim Semiconductor 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..f4d8f9e40ada
> --- /dev/null
> +++ b/drivers/mfd/max77541.c
> @@ -0,0 +1,244 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.
> + * Driver for the MAX77540 and MAX77541
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/max77541.h>
> +#include <linux/regmap.h>
Alphabetical.
> +static const struct regmap_config max77541_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static const struct regmap_irq max77541_src_irqs[] = {
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_INT_SRC_TOPSYS),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_INT_SRC_BUCK),
> +};
> +
> +static const struct regmap_irq_chip max77541_src_irq_chip = {
> + .name = "max77541-src",
> + .status_base = MAX77541_REG_INT_SRC,
> + .mask_base = MAX77541_REG_INT_SRC,
MAX77541_REG_INT_SRC_M?
> + .num_regs = 1,
> + .irqs = max77541_src_irqs,
> + .num_irqs = ARRAY_SIZE(max77541_src_irqs),
> +};
> +
> +static const struct regmap_irq max77541_topsys_irqs[] = {
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TJ_120C),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TJ_140C),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TSHDN),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_UVLO),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_ALT_SWO),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET),
> +};
> +
> +static const struct regmap_irq_chip max77541_topsys_irq_chip = {
> + .name = "max77541-topsys",
> + .status_base = MAX77541_REG_TOPSYS_INT,
> + .mask_base = MAX77541_REG_TOPSYS_INT_M,
> + .num_regs = 1,
> + .irqs = max77541_topsys_irqs,
> + .num_irqs = ARRAY_SIZE(max77541_topsys_irqs),
> +};
> +
> +static const struct regmap_irq max77541_buck_irqs[] = {
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M1_POK_FLT),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M2_POK_FLT),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M1_SCFLT),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M2_SCFLT),
> +};
> +
> +static const struct regmap_irq_chip max77541_buck_irq_chip = {
> + .name = "max77541-buck",
> + .status_base = MAX77541_REG_BUCK_INT,
> + .mask_base = MAX77541_REG_BUCK_INT_M,
> + .num_regs = 1,
> + .irqs = max77541_buck_irqs,
> + .num_irqs = ARRAY_SIZE(max77541_buck_irqs),
> +};
> +
> +static const struct regmap_irq max77541_adc_irqs[] = {
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH1_I),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH2_I),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH3_I),
> + MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH6_I),
> +};
> +
> +static const struct regmap_irq_chip max77541_adc_irq_chip = {
> + .name = "max77541-adc",
> + .status_base = MAX77541_REG_ADC_INT,
> + .mask_base = MAX77541_REG_ADC_MSK,
> + .num_regs = 1,
> + .irqs = max77541_adc_irqs,
> + .num_irqs = ARRAY_SIZE(max77541_adc_irqs),
> +};
> +
> +static const struct mfd_cell max77540_devs[] = {
> + MFD_CELL_OF("max77540-regulator", NULL, NULL, 0, 0,
> + "adi,max77540-regulator"),
No line-wrap required here.
Please use 100-chars throughout the C file.
> +};
> +
> +static const struct mfd_cell max77541_devs[] = {
> + MFD_CELL_OF("max77541-regulator", NULL, NULL, 0, 0,
> + "adi,max77541-regulator"),
> + MFD_CELL_OF("max77541-adc", NULL, NULL, 0, 0,
> + NULL),
> +};
> +
> +static const struct chip_info chip[] = {
Why do you need this require sub-structure?
> + [MAX77540] = {
> + .id = MAX77540,
> + .n_devs = ARRAY_SIZE(max77540_devs),
> + .devs = max77540_devs,
> + },
> + [MAX77541] = {
> + .id = MAX77541,
> + .n_devs = ARRAY_SIZE(max77541_devs),
> + .devs = max77541_devs,
> + },
> +};
> +
> +static int max77541_pmic_irq_init(struct device *dev)
> +{
> + struct max77541 *max77541 = dev_get_drvdata(dev);
> + int irq = max77541->i2c->irq;
> + int ret;
> +
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_src_irq_chip,
> + &max77541->irq_data);
> + if (ret)
> + return ret;
> +
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_topsys_irq_chip,
> + &max77541->irq_topsys);
> + if (ret)
> + return ret;
> +
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_buck_irq_chip,
> + &max77541->irq_buck);
> + if (ret)
> + return ret;
> +
> + if (max77541->chip->id == MAX77541) {
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_adc_irq_chip,
> + &max77541->irq_adc);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int max77541_pmic_setup(struct device *dev)
> +{
> + struct max77541 *max77541 = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + ret = max77541_pmic_irq_init(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
> +
> + ret = regmap_read(max77541->regmap, MAX77541_REG_INT_SRC, &val);
> + if (ret)
> + return ret;
What are you doing with 'val' before over-writing it in the next line?
> + ret = regmap_read(max77541->regmap, MAX77541_REG_TOPSYS_INT, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(max77541->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");
> +
> + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> + max77541->chip->devs,
> + max77541->chip->n_devs,
> + NULL, 0, NULL);
We usually place these at the bottom of .probe()
> +}
> +
> +static const struct i2c_device_id max77541_i2c_id[];
> +
> +static int max77541_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct max77541 *max77541;
> +
> + max77541 = devm_kzalloc(&client->dev, sizeof(*max77541), GFP_KERNEL);
"dev"
> + if (!max77541)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, max77541);
> + max77541->i2c = client;
> +
> + if (dev->of_node)
> + max77541->chip = of_device_get_match_data(dev);
> + else
> + max77541->chip = (struct chip_info *)
> + i2c_match_id(max77541_i2c_id,
> + client)->driver_data;
> + if (!max77541->chip)
> + return -EINVAL;
> +
> + max77541->regmap = devm_regmap_init_i2c(client,
> + &max77541_regmap_config);
> + if (IS_ERR(max77541->regmap))
> + return dev_err_probe(dev, PTR_ERR(max77541->regmap),
> + "Failed to allocate register map\n");
> +
> + return max77541_pmic_setup(dev);
> +}
> +
> +static const struct of_device_id max77541_of_id[] = {
> + {
> + .compatible = "adi,max77540",
> + .data = &chip[MAX77540],
> + },
> + {
> + .compatible = "adi,max77541",
> + .data = &chip[MAX77541],
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> +
> +static const struct i2c_device_id max77541_i2c_id[] = {
> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
Just 'MAX77540' is fine.
> + { /* sentinel */ }
Remove the comment, we know how terminators work.
Same comments for max77541_of_id.
> +};
> +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,
The "i2c" parts here are superfluous.
> +};
> +module_i2c_driver(max77541_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAX7740/MAX7741 MFD Driver");
Drop all mention of MFD from the code.
> +MODULE_AUTHOR("Okan Sahin <okan.sahin@analog.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");
> diff --git a/include/linux/mfd/max77541.h b/include/linux/mfd/max77541.h
> new file mode 100644
> index 000000000000..986f0f71c196
> --- /dev/null
> +++ b/include/linux/mfd/max77541.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __MAX77541_MFD_H__
> +#define __MAX77541_MFD_H__
__MFD_<DEVICE>__
> +#include <linux/bits.h>
> +#include <linux/types.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_EN_CTRL 0x0B
> +
> +/* BUCK CONFIG */
> +#define MAX77541_REG_BUCK_INT 0x20
> +#define MAX77541_REG_BUCK_INT_M 0x21
> +
> +#define MAX77541_REG_M1_VOUT 0x23
> +#define MAX77541_REG_M1_CFG1 0x25
> +
> +#define MAX77541_REG_M2_VOUT 0x33
> +#define MAX77541_REG_M2_CFG1 0x35
> +
> +/* INTERRUPT MASKS*/
Inconsistent.
> +#define MAX77541_REG_INT_SRC_MASK 0x00
> +#define MAX77541_REG_TOPSYS_INT_MASK 0x00
> +#define MAX77541_REG_BUCK_INT_MASK 0x00
> +
> +/*BITS OF REGISTERS*/
Spaces.
> +#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)
It's nice when these are listed under the their registers, like:
#define MAX77541_REG_TOPSYS_INT 0x02
#define MAX77541_REG_TOPSYS_INT_M 0x03
#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_BIT_M1_EN BIT(0)
> +#define MAX77541_BIT_M2_EN BIT(1)
> +
> +#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
> +#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
> +
> +/* ADC */
Just one space each side is fine.
> +#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_MAX_REGULATORS 2
> +
> +#define MAX77541_REGMAP_IRQ_REG(_mask) \
> + { .mask = (_mask) }
Same line.
Although, I'm not sure this adds to anything in the code.
Use REGMAP_IRQ_REG() or just the .mask line in place.
> +enum max7754x_ids {
> + MAX77540,
> + MAX77541,
> +};
> +
> +enum max77541_regulators {
> + MAX77541_BUCK1 = 1,
> + MAX77541_BUCK2,
> +};
> +
> +struct chip_info {
> + enum max7754x_ids id;
> + int n_devs;
> + const struct mfd_cell *devs;
> +};
> +
> +struct regmap;
> +struct regmap_irq_chip_data;
> +struct i2c_client;
> +
> +struct max77541 {
> + enum max7754x_ids id;
Why do you need this at all?
You definitely don't need it twice.
> + const struct chip_info *chip;
> +
> + struct regmap_irq_chip_data *irq_data;
> + struct regmap_irq_chip_data *irq_buck;
> + struct regmap_irq_chip_data *irq_topsys;
> + struct regmap_irq_chip_data *irq_adc;
> +
> + struct i2c_client *i2c;
> + struct regmap *regmap;
These are the important ones. Please place them at the top.
> +};
> +
> +#endif /* __MAX77541_MFD_H__ */
> --
> 2.30.2
>
--
Lee Jones [李琼斯]
prev parent reply other threads:[~2023-03-03 13:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-01 10:35 [PATCH v4 0/5] Add MAX77541/MAX77540 PMIC Support Okan Sahin
2023-02-01 10:35 ` [PATCH v4 1/5] dt-bindings: regulator: Add ADI MAX77541/MAX77540 Regulator Okan Sahin
2023-02-01 14:46 ` Krzysztof Kozlowski
2023-02-01 10:35 ` [PATCH v4 2/5] drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support Okan Sahin
2023-02-01 14:30 ` Mark Brown
2023-02-20 14:58 ` Sahin, Okan
2023-02-20 15:30 ` Andy Shevchenko
2023-02-02 13:30 ` Andy Shevchenko
2023-02-01 10:35 ` [PATCH v4 3/5] drivers: iio: adc: Add ADI MAX77541 ADC Support Okan Sahin
2023-02-01 14:44 ` Krzysztof Kozlowski
2023-02-02 13:37 ` Andy Shevchenko
2023-02-01 10:35 ` [PATCH v4 4/5] dt-bindings: mfd: Add ADI MAX77541/MAX77540 Okan Sahin
2023-02-01 14:47 ` Krzysztof Kozlowski
2023-02-01 10:35 ` [PATCH v4 5/5] drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support Okan Sahin
2023-03-03 13:29 ` Lee Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230303132942.GN2420672@google.com \
--to=lee@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=broonie@kernel.org \
--cc=caleb.connolly@linaro.org \
--cc=cy_huang@richtek.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcelo.schmitt1@gmail.com \
--cc=marcus.folkesson@gmail.com \
--cc=okan.sahin@analog.com \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=ramona.bolboaca@analog.com \
--cc=robh+dt@kernel.org \
--cc=william.gray@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.