Message-ID: <3a9441f01e82dfcbdf146a809ba4a6f9604c63d7.camel@gmail.com>
Date: Wed, 29 Oct 2025 09:55:53 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Joan-Na-adi <joan.na.devcode@...il.com>, Liam Girdwood
 <lgirdwood@...il.com>
Cc: Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski
	 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, Joan Na
	 <joan.na@...log.com>
Subject: Re: [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator
 driver

On Wed, 2025-10-29 at 11:32 +0900, Joan-Na-adi wrote:
> From: Joan Na <joan.na@...log.com>
> 
> Add support for the Maxim Integrated MAX77675 PMIC regulator.
> 
> The MAX77675 is a compact, highly efficient SIMO (Single Inductor Multiple Output)
> power management IC that provides four programmable buck-boost switching regulators
> with only one inductor. It supports up to 700mA total output current and operates
> from a single-cell Li-ion battery.
> 
> An integrated power-up sequencer and I2C interface allow flexible startup
> configuration and runtime control.
> 
> Signed-off-by: Joan Na <joan.na@...log.com>
> ---

Hi Joan,

Some comments from me... 

>  drivers/regulator/Kconfig              |   9 +
>  drivers/regulator/Makefile             |   1 +
>  drivers/regulator/max77675-regulator.c | 861 +++++++++++++++++++++++++
>  drivers/regulator/max77675-regulator.h | 260 ++++++++
>  4 files changed, 1131 insertions(+)
>  create mode 100644 drivers/regulator/max77675-regulator.c
>  create mode 100644 drivers/regulator/max77675-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index d84f3d054c59..93131446e402 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -649,6 +649,15 @@ config REGULATOR_MAX77650
>  	  Semiconductor. This device has a SIMO with three independent
>  	  power rails and an LDO.
> 
> +config REGULATOR_MAX77675
> +	tristate "Maxim MAX77675 regulator driver"
> +	depends on I2C

Looking at your code, I would expected OF to be a dependency as well.

> +	select REGMAP_I2C
> +	help
> +	  This driver controls the Maxim MAX77675 power regulator via I2C.
> +	  It supports four programmable buck-boost outputs.
> +	  Say Y here to enable the regulator driver
> +
>  config REGULATOR_MAX77857
>  	tristate "ADI MAX77857/MAX77831 regulator support"
>  	depends on I2C
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index b3101376029d..cdd99669cd24 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MAX77503) += max77503-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_MAX77675) += max77675-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8649)	+= max8649.o
>  obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o
>  obj-$(CONFIG_REGULATOR_MAX8893) += max8893.o
> diff --git a/drivers/regulator/max77675-regulator.c b/drivers/regulator/max77675-regulator.c
> new file mode 100644
> index 000000000000..c1281f07fe43
> --- /dev/null
> +++ b/drivers/regulator/max77675-regulator.c
> @@ -0,0 +1,861 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Analog Devices, Inc.
> + * ADI regulator driver for MAX77675.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/bitfield.h>

You're clearly missing (at least) mod_devicetable.h. I know that at least clang allows you to get
iwyu,

> +
> +#include "max77675-regulator.h"

Why do we need the header file? Just include everything in the source code unless you expect to
share something with another module (which I dunno)?

> +
> +struct max77675_regulator_pdata {
> +	u8   fps_slot;
> +	bool fixed_slew_rate;
> +};
> +

I would get rid of the _pdata suffix. Implies some legacy way of doing things (but kind of a
nitpick)

> +struct max77675_config {
> +	u8   en_mode;
> +	u8   voltage_change_latency;
> +	u8   drv_sbb_strength;
> +	u8   dvs_slew_rate;
> +	u8   debounce_time;
> +	u8   manual_reset_time;
> +	bool en_pullup_disable;
> +	bool bias_low_power_request;
> +	bool simo_int_ldo_always_on;
> +};
> +
> +struct max77675_regulator {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct max77675_config config;
> +	struct max77675_regulator_pdata pdata[MAX77675_ID_NUM_MAX];
> +};
> +
> +/**
> + * Set latency mode.
> + *
> + * @param maxreg Pointer to max77675 device structure.
> + * @param enable true to enable latency mode, false to disable.
> + */
> +static int max77675_set_latency_mode(struct max77675_regulator *maxreg, bool enable)
> +{
> +	return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B,
> +				  MAX77675_LAT_MODE_BIT,
> +				  FIELD_PREP(MAX77675_LAT_MODE_BIT, enable));
> +}
> +

I would drop these one liner wrappers. Personally, I don't see a big benefit on it.

> +/**
> + * Set DVS slew rate mode.
> + *
> + * @param maxreg Pointer to max77675 device structure.
> + * @param enable true to use DVS-controlled slew rate, false for fixed 2mV/us.
> + */
> +static int max77675_set_dvs_slew_rate(struct max77675_regulator *maxreg, bool enable)
> +{
> +	return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B,
> +				  MAX77675_DVS_SLEW_BIT,
> +				  FIELD_PREP(MAX77675_DVS_SLEW_BIT, enable));
> +}
> +

Ditto for all other places.

...

> 
> +
> +/**
> + * Set debounce time for EN pin.
> + *
> + * @param maxreg Pointer to max77675 device structure.
> + * @param debounce_30ms true for 30ms, false for 100us
> + */
> +static int max77675_set_debounce_time(struct max77675_regulator *maxreg, bool debounce_30ms)
> +{
> +	return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A,
> +				  MAX77675_DBEN_EN_BIT,
> +				  FIELD_PREP(MAX77675_DBEN_EN_BIT, debounce_30ms));
> +}
> +
> +static int max77675_regulator_get_fps_src(struct max77675_regulator *maxreg, int id)
> +{
> +	unsigned int reg_addr;
> +	unsigned int val;
> +	int ret;
> +
> +	switch (id) {
> +	case MAX77675_ID_SBB0:
> +		reg_addr = MAX77675_REG_CNFG_SBB0_B;
> +		break;
> +	case MAX77675_ID_SBB1:
> +		reg_addr = MAX77675_REG_CNFG_SBB1_B;
> +		break;
> +	case MAX77675_ID_SBB2:
> +		reg_addr = MAX77675_REG_CNFG_SBB2_B;
> +		break;
> +	case MAX77675_ID_SBB3:
> +		reg_addr = MAX77675_REG_CNFG_SBB3_B;
> +		break;
> +	default:
> +		dev_err(maxreg->dev, "Invalid regulator id: %d\n", id);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(maxreg->regmap, reg_addr, &val);
> +	if (ret < 0) {
> +		dev_err(maxreg->dev, "Failed to read FPS source (reg 0x%02x): %d\n",
> +			reg_addr, ret);
> +		return ret;
> +	}
> +
> +	return val & MAX77675_EN_SBB_MASK;

Ok, this works since the mask is 0x7. However, FIELD_GET() would make it more
readable and easy to review. I mean, I would not need to go and see the mask value.

> +}
> +
> +static int max77675_regulator_set_fps_src(struct max77675_regulator *maxreg, int id, u8 fps_src)
> +{
> +	unsigned int reg_addr;
> +	int ret;
> +
> +	switch (id) {
> +	case MAX77675_ID_SBB0:
> +		reg_addr = MAX77675_REG_CNFG_SBB0_B;
> +		break;
> +	case MAX77675_ID_SBB1:
> +		reg_addr = MAX77675_REG_CNFG_SBB1_B;
> +		break;
> +	case MAX77675_ID_SBB2:
> +		reg_addr = MAX77675_REG_CNFG_SBB2_B;
> +		break;
> +	case MAX77675_ID_SBB3:
> +		reg_addr = MAX77675_REG_CNFG_SBB3_B;
> +		break;
> +	default:
> +		dev_err(maxreg->dev, "Invalid regulator id: %d\n", id);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(maxreg->regmap, reg_addr,
> +				 MAX77675_EN_SBB_MASK, fps_src);
> +	if (ret < 0) {
> +		dev_err(maxreg->dev, "Failed to set FPS source (reg 0x%02x): %d\n",
> +			reg_addr, ret);
> +		return ret;
> +	}

I would drop the log and just do return regmap_update_bits(). Up to you...

> +
> +	return 0;
> +}
> +
> +/**
> + * max77675_set_sbb_slew_rate_fixed - Set the slew rate for a specific SBB regulator channel
> + *
> + * @maxreg: Pointer to the max77675 regulator structure
> + * @id: Regulator channel ID (ID_SBB0 ~ ID_SBB3)
> + * @fixed: Slew rate value (true = 2mV/us, false = use DVS_SLEW)
> + *
> + * This function configures the slew rate control source for the specified SBB channel by
> + * updating the corresponding bits in the CNFG_SBB_TOP_B register.
> + *
> + * Return: 0 on success, negative error code on failure (e.g., invalid channel ID).
> + */
> +static int max77675_set_sbb_slew_rate_fixed(struct max77675_regulator *maxreg, int id, bool
> fixed)
> +{
> +	u8 mask, value;
> +	u8 slew_src_ctrl_bit = fixed ? 0 : 1;
> +
> +	switch (id) {
> +	case MAX77675_ID_SBB0:
> +		mask = MAX77675_SR_SBB0_BIT;
> +		value = FIELD_PREP(MAX77675_SR_SBB0_BIT, slew_src_ctrl_bit);
> +		break;
> +
> +	case MAX77675_ID_SBB1:
> +		mask = MAX77675_SR_SBB1_BIT;
> +		value = FIELD_PREP(MAX77675_SR_SBB1_BIT, slew_src_ctrl_bit);
> +		break;
> +
> +	case MAX77675_ID_SBB2:
> +		mask = MAX77675_SR_SBB2_BIT;
> +		value = FIELD_PREP(MAX77675_SR_SBB2_BIT, slew_src_ctrl_bit);
> +		break;
> +
> +	case MAX77675_ID_SBB3:
> +		mask = MAX77675_SR_SBB3_BIT;
> +		value = FIELD_PREP(MAX77675_SR_SBB3_BIT, slew_src_ctrl_bit);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, mask, value);
> +}
> +
> +static int max77675_init_regulator(struct max77675_regulator *maxreg, int id)
> +{
> +	struct max77675_regulator_pdata *rpdata = &maxreg->pdata[id];
> +	int ret;
> +
> +	if (rpdata->fps_slot == MAX77675_FPS_DEF) {
> +		ret = max77675_regulator_get_fps_src(maxreg, id);
> +		if (ret < 0) {
> +			dev_err(maxreg->dev, "Failed to read FPS source for ID %d\n", id);
> +			return ret;
> +		}
> +		rpdata->fps_slot = ret;
> +	} else {
> +		ret = max77675_regulator_set_fps_src(maxreg, id, rpdata->fps_slot);
> +		if (ret)
> +			dev_warn(maxreg->dev, "Failed to set FPS source for ID %d\n", id);
> +	}
> +
> +	ret = max77675_set_sbb_slew_rate_fixed(maxreg, id, rpdata->fixed_slew_rate);
> +	if (ret)
> +		dev_warn(maxreg->dev, "Failed to set slew rate for ID %d\n", id);

Do we really want to treat this as a warning (as FPS)? If so, I would expect a proper
comment explaining why we can afford it.

> +
> +	return 0;
> +}
> +
> +static int max77675_of_parse_cb(struct device_node *np,
> +				const struct regulator_desc *desc,
> +				struct regulator_config *config)
> +{
> +	struct max77675_regulator *maxreg = config->driver_data;
> +	struct max77675_regulator_pdata *rpdata = &maxreg->pdata[desc->id];
> +	u32 pval;
> +	int ret;
> +
> +	/* Parse FPS slot from DT */
> +	ret = of_property_read_u32(np, "maxim,fps-slot", &pval);
> +	rpdata->fps_slot = (!ret) ? (u8)pval : MAX77675_FPS_DEF;
> +

So, can we get any arbitrary value for pval? I see we you have an enum in
the bindings so make sure we properly validate it. Same for all other
properties. The bindings also have this as a string and here you have a u32?
Not going to work. You need of_property_read_string() + match_string().


Also, "maxim,"? For some time now it's "adi,".

> +	/* Parse slew rate control source */
> +	rpdata->fixed_slew_rate = of_property_read_bool(np, "maxim,fixed-slew-rate");
> +
> +	/* Apply parsed configuration */
> +	return max77675_init_regulator(maxreg, desc->id);
> +}
> +
> +static int max77675_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> +{
> +	struct max77675_regulator *maxreg = rdev_get_drvdata(rdev);
> +	unsigned int int_flags;
> +	int id = rdev_get_id(rdev);
> +	int ret;
> +
> +	ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_flags);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to read INT_GLBL: %d\n", ret);
> +		return ret;
> +	}
> +
> +	*flags = 0;
> +
> +	switch (id) {
> +	case MAX77675_ID_SBB0:
> +		if (int_flags & MAX77675_INT_SBB0_F_BIT)
> +			*flags |= REGULATOR_ERROR_FAIL;
> +		break;
> +	case MAX77675_ID_SBB1:
> +		if (int_flags & MAX77675_INT_SBB1_F_BIT)
> +			*flags |= REGULATOR_ERROR_FAIL;
> +		break;
> +	case MAX77675_ID_SBB2:
> +		if (int_flags & MAX77675_INT_SBB2_F_BIT)
> +			*flags |= REGULATOR_ERROR_FAIL;
> +		break;
> +	case MAX77675_ID_SBB3:
> +		if (int_flags & MAX77675_INT_SBB3_F_BIT)
> +			*flags |= REGULATOR_ERROR_FAIL;
> +		break;
> +	default:
> +		dev_warn(maxreg->dev, "Unsupported regulator ID: %d\n", id);
> +		break;

Should not be a warning. Also wonder if it can happen at all?

> +	}
> +
> +	if (int_flags & MAX77675_INT_TJAL2_R_BIT) {
> +		/* TJAL2 interrupt: Over-temperature condition (above 120 degree) */
> +		*flags |= REGULATOR_ERROR_OVER_TEMP;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regulator_ops max77675_regulator_ops = {
> +	.list_voltage         = regulator_list_voltage_linear,
> +	.enable               = regulator_enable_regmap,
> +	.disable              = regulator_disable_regmap,
> +	.is_enabled           = regulator_is_enabled_regmap,
> +	.map_voltage          = regulator_map_voltage_linear,
> +	.set_voltage_sel      = regulator_set_voltage_sel_regmap,
> +	.get_voltage_sel      = regulator_get_voltage_sel_regmap,
> +	.set_active_discharge = regulator_set_active_discharge_regmap,
> +	.get_error_flags      = max77675_get_error_flags,
> +};
> +
> +static struct regulator_desc max77675_regulators[MAX77675_ID_NUM_MAX] = {
> +	{
> +		.name                  = "sbb0",
> +		.of_match              = of_match_ptr("sbb0"),
> +		.regulators_node       = of_match_ptr("regulators"),

I wonder if we need of_match_ptr() given that the whole thing depends on OF.

...

> +
> +static int max77675_apply_config(struct max77675_regulator *maxreg)
> +{
> +	const struct max77675_config *config = &maxreg->config;
> +	int ret;
> +
> +	ret = max77675_set_en_mode(maxreg, config->en_mode);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set EN mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_latency_mode(maxreg, config->voltage_change_latency);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set latency mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_drv_sbb_strength(maxreg, config->drv_sbb_strength);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set drive strength: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_dvs_slew_rate(maxreg, config->dvs_slew_rate);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set DVS slew rate: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_debounce_time(maxreg, config->debounce_time);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set EN debounce time: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_manual_reset_time(maxreg, config->manual_reset_time);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set manual reset time: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_en_pullup_disable(maxreg, config->en_pullup_disable);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set EN pull-up disable: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_bias_low_power_request(maxreg, config->bias_low_power_request);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set bias low-power request: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = max77675_set_simo_int_ldo_always_on(maxreg, config->simo_int_ldo_always_on);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to set SIMO internal LDO always-on: %d\n", ret);
> +		return ret;
> +	}

This seems to called during probe. All the above can be return dev_err_probe()...

> +
> +	return 0;
> +}
> +
> +static u8 max77675_parse_voltage_change_latency(struct device_node *np)
> +{
> +	u32 val;
> +
> +	if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) {
> +		switch (val) {
> +		case 10:
> +			return MAX77675_LOW_LATENCY_MODE;
> +		case 100:
> +			return MAX77675_HIGH_LATENCY_MODE;
> +		default:
> +			break;

The above is wrong. You're ignoring invalid values being given and overwrite them
with defaults. The pattern is:

val = MAX77675_HIGH_LATENCY_MODE;
if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) {
	...
	default:
		return dev_err_probe(dev, -EINVAL, ...);
}

You can also do:

ret = of_property_read_u32(...);
/* Not 100% sure if -EINVAL is the error code for a missing property */
if (ret && ret != -EINVAL)
	return ret;
if (!ret) {
	...
}

> +		}
> +	}
> +
> +	/* default: high latency */
> +	return MAX77675_HIGH_LATENCY_MODE;
> +}
> +
> +static u8 max77675_parse_en_mode(struct device_node *np)
> +{
> +	const char *str;
> +
> +	if (!of_property_read_string(np, "maxim,en-mode", &str)) {
> +		if (!strcasecmp(str, "push-button"))
> +			return MAX77675_EN_PUSH_BUTTON;
> +		else if (!strcasecmp(str, "slide-switch"))
> +			return MAX77675_EN_SLIDE_SWITCH;
> +		else if (!strcasecmp(str, "logic"))
> +			return MAX77675_EN_LOGIC;

redundant else if(). Moreover, this looks like it could use match_string()

> +	}
> +
> +	/* default : slide-switch */
> +	return MAX77675_EN_SLIDE_SWITCH;
> +}
> +
> +static u8 max77675_parse_manual_reset_time(struct device_node *np)
> +{
> +	u32 val;
> +
> +	if (!of_property_read_u32(np, "reset-time-sec", &val)) {
> +		switch (val) {
> +		case 4:
> +			return MAX77675_MRT_4S;
> +		case 8:
> +			return MAX77675_MRT_8S;
> +		case 12:
> +			return MAX77675_MRT_12S;
> +		case 16:
> +			return MAX77675_MRT_16S;
> +		default:
> +			break;

Ditto.

...

> +
> +static int max77675_parse_config(struct max77675_regulator *maxreg)
> +{
> +	struct device_node *np = maxreg->dev->of_node;
> +	struct max77675_config *config = &maxreg->config;
> +	int ret;
> +
> +	/* EN pin mode: push-button, slide-switch, or logic */
> +	config->en_mode = max77675_parse_en_mode(np);
> +
> +	/* latency mode */
> +	config->voltage_change_latency = max77675_parse_voltage_change_latency(np);
> +
> +	/* drive strength */
> +	config->drv_sbb_strength = max77675_parse_drv_sbb_strength(np);
> +
> +	/* drv slew rate */
> +	config->dvs_slew_rate = max77675_parse_dvs_slew_rate(np);
> +
> +	/* Debounce time for EN pin */
> +	config->debounce_time = max77675_parse_debounce_time_us(np);
> +
> +	/* Manual reset time for EN pin */
> +	config->manual_reset_time = max77675_parse_manual_reset_time(np);

Seems to me that all of the above will need some error handling.

> +
> +	/* Disable internal pull-up resistor on EN pin */
> +	config->en_pullup_disable = of_property_read_bool(np, "maxim,en-pullup-disable");
> +
> +	/* Request low-power mode for main bias */
> +	config->bias_low_power_request = of_property_read_bool(np, "maxim,bias-low-power-
> request");
> +
> +	/* Force internal LDO to always supply 1.8V */
> +	config->simo_int_ldo_always_on = of_property_read_bool(np, "maxim,simo-int-ldo-always-
> on");
> +
> +	ret = max77675_apply_config(maxreg);
> +
> +	return ret;
> +}
> +
> +static int max77675_init_event(struct max77675_regulator *maxreg)
> +{
> +	unsigned int ercflag, int_glbl;
> +	int ret;
> +
> +	ret = regmap_read(maxreg->regmap, MAX77675_REG_ERCF_GLBL, &ercflag);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to read CID register: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_glbl);
> +	if (ret) {
> +		dev_err(maxreg->dev, "Failed to read INT_GLBL register: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (ercflag & MAX77675_SFT_CRST_F_BIT)
> +		dev_info(maxreg->dev, "Software Cold Reset Flag is set\n");
> +
> +	if (ercflag & MAX77675_SFT_OFF_F_BIT)
> +		dev_info(maxreg->dev, "Software Off Flag is set\n");
> +
> +	if (ercflag & MAX77675_MRST_BIT)
> +		dev_info(maxreg->dev, "Manual Reset Timer Flag is set\n");
> +
> +	if (ercflag & MAX77675_UVLO_BIT)
> +		dev_info(maxreg->dev, "Undervoltage Lockout Flag is set\n");
> +
> +	if (ercflag & MAX77675_OVLO_BIT)
> +		dev_info(maxreg->dev, "Overvoltage Lockout Flag is set\n");
> +
> +	if (ercflag & MAX77675_TOVLD_BIT)
> +		dev_info(maxreg->dev, "Thermal Overload Flag is set\n");
> +
> +	if (int_glbl & MAX77675_INT_SBB3_F_BIT)
> +		dev_info(maxreg->dev, "SBB3 Channel Fault Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_SBB2_F_BIT)
> +		dev_info(maxreg->dev, "SBB2 Channel Fault Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_SBB1_F_BIT)
> +		dev_info(maxreg->dev, "SBB1 Channel Fault Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_SBB0_F_BIT)
> +		dev_info(maxreg->dev, "SBB0 Channel Fault Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_TJAL2_R_BIT)
> +		dev_info(maxreg->dev, "Thermal Alarm 2 Rising Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_TJAL1_R_BIT)
> +		dev_info(maxreg->dev, "Thermal Alarm 1 Rising Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_EN_R_BIT)
> +		dev_info(maxreg->dev, "nEN Rising Edge Interrupt occurred\n");
> +
> +	if (int_glbl & MAX77675_INT_EN_F_BIT)
> +		dev_info(maxreg->dev, "nEN Falling Edge Interrupt occurred\n");

All of the above looks like dev_dbg() to me.

> +
> +	return 0;
> +}
> +
> +static int max77675_regulator_probe(struct i2c_client *client)
> +{
> +	struct max77675_regulator *maxreg;
> +	struct regulator_config config = {};
> +	struct device_node *regulators_np;
> +	int i, ret;
> +
> +	maxreg = devm_kzalloc(&client->dev, sizeof(*maxreg), GFP_KERNEL);
> +	if (!maxreg)
> +		return -ENOMEM;
> +
> +	maxreg->dev = &client->dev;
> +
> +	maxreg->regmap = devm_regmap_init_i2c(client, &max77675_regmap_config);
> +	if (IS_ERR(maxreg->regmap))
> +		return dev_err_probe(maxreg->dev,
> +				     PTR_ERR(maxreg->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = max77675_init_event(maxreg);
> +	if (ret)
> +		return dev_err_probe(maxreg->dev, ret, "Failed to init event\n");
> +
> +	ret = max77675_parse_config(maxreg);
> +	if (ret)
> +		return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n");
> +
> +	config.dev = &client->dev;
> +	config.regmap = maxreg->regmap;
> +	config.driver_data = maxreg;
> +
> +	regulators_np = of_get_child_by_name(client->dev.of_node, "regulators");

The above can actually be:

struct device_node *regulators_np __free(device_node) = of_get_child_by_name(...)

and then no need to care about of_node_put(). You need cleanup.h

> +	if (!regulators_np) {
> +		dev_err(maxreg->dev, "No 'regulators' subnode found in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < MAX77675_ID_NUM_MAX; i++) {
> +		const struct regulator_desc *desc = &max77675_regulators[i];
> +		struct regulator_dev *rdev;
> +
> +		config.of_node = of_get_child_by_name(regulators_np, desc->name);
> +		if (!config.of_node) {
> +			dev_warn(maxreg->dev, "No DT node for regulator %s\n", desc->name);
> +			continue;
> +		}
> +
> +		rdev = devm_regulator_register(&client->dev, desc, &config);
> +		of_node_put(config.of_node);
> +		if (IS_ERR(rdev)) {
> +			of_node_put(regulators_np);
> +			return dev_err_probe(maxreg->dev, PTR_ERR(rdev),
> +				"Failed to register regulator %d (%s): %d\n",
> +				i, desc->name, ret);
> +		}
> +	}
> +
> +	of_node_put(regulators_np);
> +	i2c_set_clientdata(client, maxreg);

I do not see i2c_get_clientdata() anywhere. I suspect you can drop the above.

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max77675_i2c_id[] = {
> +	{ "max77675", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77675_i2c_id);
> +
> +static const struct of_device_id __maybe_unused max77675_of_match[] = {
> +	{ .compatible = "maxim,max77675", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max77675_of_match);
> +
> +static struct i2c_driver max77675_regulator_driver = {
> +	.driver = {
> +		.name = "max77675",
> +		.of_match_table = of_match_ptr(max77675_of_match),

I guess we can drop of_match_ptr() if we agree that we depend on OF

> +	},
> +	.probe = max77675_regulator_probe,
> +	.id_table = max77675_i2c_id,
> +};
> +
> +module_i2c_driver(max77675_regulator_driver);
> +
> +MODULE_DESCRIPTION("MAX77675 Regulator Driver");
> +MODULE_AUTHOR("Joan Na <joan.na@...log.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/regulator/max77675-regulator.h b/drivers/regulator/max77675-regulator.h
> new file mode 100644
> index 000000000000..0aaa30a630ca
> --- /dev/null
> +++ b/drivers/regulator/max77675-regulator.h

As said, drop the header.

- Nuno Sá

Powered by blists - more mailing lists