[v4,2/5] drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support

Message ID 20230201103534.108136-3-okan.sahin@analog.com
State New
Headers
Series Add MAX77541/MAX77540 PMIC Support |

Commit Message

Sahin, Okan Feb. 1, 2023, 10:35 a.m. UTC
  Regulator driver for both MAX77541 and MAX77540.
The MAX77541 is a high-efficiency step-down converter
with two 3A switching phases for single-cell Li+ battery
and 5VDC systems.

The MAX77540 is a high-efficiency step-down converter
with two 3A switching phases.

Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
 drivers/regulator/Kconfig              |   9 ++
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/max77541-regulator.c | 145 +++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 drivers/regulator/max77541-regulator.c
  

Comments

Mark Brown Feb. 1, 2023, 2:30 p.m. UTC | #1
On Wed, Feb 01, 2023 at 01:35:15PM +0300, Okan Sahin wrote:
> Regulator driver for both MAX77541 and MAX77540.
> The MAX77541 is a high-efficiency step-down converter
> with two 3A switching phases for single-cell Li+ battery
> and 5VDC systems.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> +	if (max77541->chip->id == MAX77540)
> +		desc = max77540_regulators_desc;
> +	else if (max77541->chip->id == MAX77541)
> +		desc = max77541_regulators_desc;
> +	else
> +		return -EINVAL;

Write this as a switch statement for extensibility.

Otherwise this looks good.
  
Andy Shevchenko Feb. 2, 2023, 1:30 p.m. UTC | #2
On Wed, Feb 01, 2023 at 01:35:15PM +0300, Okan Sahin wrote:
> Regulator driver for both MAX77541 and MAX77540.
> The MAX77541 is a high-efficiency step-down converter
> with two 3A switching phases for single-cell Li+ battery
> and 5VDC systems.
> 
> The MAX77540 is a high-efficiency step-down converter
> with two 3A switching phases.

...

With

> +	struct device *dev = &pdev->dev;

to be here the following can be shortened

> +	struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);

	struct max77541 *max77541 = dev_get_drvdata(dev->parent);

> +	struct regulator_config config = {};
> +	const struct regulator_desc *desc;
> +	struct device *dev = &pdev->dev;
> +	struct regulator_dev *rdev;
> +	int i;

> +	config.dev = pdev->dev.parent;

	config.dev = dev->parent;

...

> +static const struct platform_device_id max77541_regulator_platform_id[] = {
> +	{ "max77540-regulator", },
> +	{ "max77541-regulator", },

Inner commas are not required.

> +	{  /* sentinel */  }
> +};
  
Sahin, Okan Feb. 20, 2023, 2:58 p.m. UTC | #3
Wed, 1 Feb 2023 5:30 PM
Mark Brown <broonie@kernel.org> wrote:

>On Wed, Feb 01, 2023 at 01:35:15PM +0300, Okan Sahin wrote:
>> Regulator driver for both MAX77541 and MAX77540.
>> The MAX77541 is a high-efficiency step-down converter with two 3A
>> switching phases for single-cell Li+ battery and 5VDC systems.
>
>Please submit patches using subject lines reflecting the style for the subsystem,
>this makes it easier for people to identify relevant patches.
>Look at what existing commits in the area you're changing are doing and make
>sure your subject lines visually resemble what they're doing.
>There's no need to resubmit to fix this alone.
>
>> +	if (max77541->chip->id == MAX77540)
>> +		desc = max77540_regulators_desc;
>> +	else if (max77541->chip->id == MAX77541)
>> +		desc = max77541_regulators_desc;
>> +	else
>> +		return -EINVAL;
>
>Write this as a switch statement for extensibility.
>
>Otherwise this looks good.

Hi Mark,

Thank you for your feedback.  I am not sure that I fully understand your feedback. What do you mean by "reflecting style for the subsystem". For example, this patch includes files modified or added under regulator directory as stated in the subject line. Do I need to say anything about mfd as regulator is subdevice?

Regards,
Okan Sahin
  
Andy Shevchenko Feb. 20, 2023, 3:30 p.m. UTC | #4
On Mon, Feb 20, 2023 at 02:58:47PM +0000, Sahin, Okan wrote:
> Wed, 1 Feb 2023 5:30 PM
> Mark Brown <broonie@kernel.org> wrote:
> >On Wed, Feb 01, 2023 at 01:35:15PM +0300, Okan Sahin wrote:

> >Please submit patches using subject lines reflecting the style for the subsystem,
> >this makes it easier for people to identify relevant patches.
> >Look at what existing commits in the area you're changing are doing and make
> >sure your subject lines visually resemble what they're doing.
> >There's no need to resubmit to fix this alone.
> >
> >> +	if (max77541->chip->id == MAX77540)
> >> +		desc = max77540_regulators_desc;
> >> +	else if (max77541->chip->id == MAX77541)
> >> +		desc = max77541_regulators_desc;
> >> +	else
> >> +		return -EINVAL;
> >
> >Write this as a switch statement for extensibility.
> >
> >Otherwise this looks good.

> Thank you for your feedback.  I am not sure that I fully understand your
> feedback. What do you mean by "reflecting style for the subsystem". For
> example, this patch includes files modified or added under regulator
> directory as stated in the subject line. Do I need to say anything about mfd
> as regulator is subdevice?

It's about Subject.

Just run `git log --online --no-merges -- drivers/regulator/max77*` and
look closely at it.
  

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 070e4403c6c2..1e416c195af9 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -556,6 +556,15 @@  config REGULATOR_MAX597X
 	  The MAX5970/5978 is a smart switch with no output regulation, but
 	  fault protection and voltage and current monitoring capabilities.
 
+config REGULATOR_MAX77541
+	tristate "Analog Devices MAX77541/77540 Regulator"
+	depends on MFD_MAX77541
+	help
+	  This driver controls a Analog Devices MAX77541/77540 regulators
+	  via I2C bus. Both MAX77540 and MAX77541 are dual-phase
+	  high-efficiency buck converter. Say Y here to
+	  enable the regulator driver.
+
 config REGULATOR_MAX77620
 	tristate "Maxim 77620/MAX20024 voltage regulator"
 	depends on MFD_MAX77620 || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 5962307e1130..c19efc7cfbef 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -68,6 +68,7 @@  obj-$(CONFIG_REGULATOR_LTC3676) += ltc3676.o
 obj-$(CONFIG_REGULATOR_MAX14577) += max14577-regulator.o
 obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
 obj-$(CONFIG_REGULATOR_MAX597X) += max597x-regulator.o
+obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o
 obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o
 obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8649)	+= max8649.o
diff --git a/drivers/regulator/max77541-regulator.c b/drivers/regulator/max77541-regulator.c
new file mode 100644
index 000000000000..0cf3b041451f
--- /dev/null
+++ b/drivers/regulator/max77541-regulator.c
@@ -0,0 +1,145 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI Regulator driver for the MAX77540 and MAX77541
+ */
+
+#include <linux/i2c.h>
+#include <linux/mfd/max77541.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+static const struct regulator_ops max77541_buck_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.list_voltage		= regulator_list_voltage_pickable_linear_range,
+	.get_voltage_sel	= regulator_get_voltage_sel_pickable_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_pickable_regmap,
+};
+
+static const struct linear_range max77540_buck_ranges[] = {
+	/* Ranges when VOLT_SEL bits are 0x00 */
+	REGULATOR_LINEAR_RANGE(500000, 0x00, 0x8B, 5000),
+	REGULATOR_LINEAR_RANGE(1200000, 0x8C, 0xFF, 0),
+	/* Ranges when VOLT_SEL bits are 0x40 */
+	REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+	REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+	/* Ranges when VOLT_SEL bits are  0x80 */
+	REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+	REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const struct linear_range max77541_buck_ranges[] = {
+	/* Ranges when VOLT_SEL bits are 0x00 */
+	REGULATOR_LINEAR_RANGE(300000, 0x00, 0xB3, 5000),
+	REGULATOR_LINEAR_RANGE(1200000, 0xB4, 0xFF, 0),
+	/* Ranges when VOLT_SEL bits are 0x40 */
+	REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+	REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+	/* Ranges when VOLT_SEL bits are  0x80 */
+	REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+	REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const unsigned int max77541_buck_volt_range_sel[] = {
+	0x00, 0x00, 0x40, 0x40, 0x80, 0x80,
+};
+
+#define MAX77540_BUCK(_id, _ops)					\
+	{	.id = MAX77541_BUCK ## _id,				\
+		.name = "buck"#_id,					\
+		.of_match = "buck"#_id,					\
+		.regulators_node = "regulators",			\
+		.enable_reg = MAX77541_REG_EN_CTRL,			\
+		.enable_mask = MAX77541_BIT_M ## _id ## _EN,		\
+		.ops = &(_ops),						\
+		.type = REGULATOR_VOLTAGE,				\
+		.linear_ranges = max77540_buck_ranges,			\
+		.n_linear_ranges = ARRAY_SIZE(max77540_buck_ranges),	\
+		.vsel_reg = MAX77541_REG_M ## _id ## _VOUT,		\
+		.vsel_mask = MAX77541_BITS_MX_VOUT,			\
+		.vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1,	\
+		.vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG,		\
+		.linear_range_selectors = max77541_buck_volt_range_sel, \
+		.owner = THIS_MODULE,					\
+	}
+
+#define MAX77541_BUCK(_id, _ops)					\
+	{	.id = MAX77541_BUCK ## _id,				\
+		.name = "buck"#_id,					\
+		.of_match = "buck"#_id,					\
+		.regulators_node = "regulators",			\
+		.enable_reg = MAX77541_REG_EN_CTRL,			\
+		.enable_mask = MAX77541_BIT_M ## _id ## _EN,		\
+		.ops = &(_ops),						\
+		.type = REGULATOR_VOLTAGE,				\
+		.linear_ranges = max77541_buck_ranges,			\
+		.n_linear_ranges = ARRAY_SIZE(max77541_buck_ranges),	\
+		.vsel_reg = MAX77541_REG_M ## _id ## _VOUT,		\
+		.vsel_mask = MAX77541_BITS_MX_VOUT,			\
+		.vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1,	\
+		.vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG,		\
+		.linear_range_selectors = max77541_buck_volt_range_sel, \
+		.owner = THIS_MODULE,					\
+	}
+
+static const struct regulator_desc max77540_regulators_desc[] = {
+	MAX77540_BUCK(1, max77541_buck_ops),
+	MAX77540_BUCK(2, max77541_buck_ops),
+};
+
+static const struct regulator_desc max77541_regulators_desc[] = {
+	MAX77541_BUCK(1, max77541_buck_ops),
+	MAX77541_BUCK(2, max77541_buck_ops),
+};
+
+static int max77541_regulator_probe(struct platform_device *pdev)
+{
+	struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_config config = {};
+	const struct regulator_desc *desc;
+	struct device *dev = &pdev->dev;
+	struct regulator_dev *rdev;
+	int i;
+
+	config.dev = pdev->dev.parent;
+
+	if (max77541->chip->id == MAX77540)
+		desc = max77540_regulators_desc;
+	else if (max77541->chip->id == MAX77541)
+		desc = max77541_regulators_desc;
+	else
+		return -EINVAL;
+
+	for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
+		rdev = devm_regulator_register(dev, &desc[i], &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(rdev),
+					     "Failed to register regulator\n");
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id max77541_regulator_platform_id[] = {
+	{ "max77540-regulator", },
+	{ "max77541-regulator", },
+	{  /* sentinel */  }
+};
+MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
+
+static struct platform_driver max77541_regulator_driver = {
+	.driver = {
+		.name = "max77541-regulator",
+	},
+	.probe = max77541_regulator_probe,
+	.id_table = max77541_regulator_platform_id,
+};
+module_platform_driver(max77541_regulator_driver);
+
+MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>");
+MODULE_DESCRIPTION("MAX77540/MAX77541 regulator driver");
+MODULE_LICENSE("GPL");