lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <29f30aae-ffad-4a42-909e-b05f9cf360b5@kernel.org>
Date: Mon, 16 Sep 2024 22:06:37 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: André Draszik <andre.draszik@...aro.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Michael Walle <mwalle@...nel.org>
Cc: Peter Griffin <peter.griffin@...aro.org>,
 Tudor Ambarus <tudor.ambarus@...aro.org>,
 Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator
 driver

On 16/09/2024 18:48, André Draszik wrote:
> The MAX20339 is an overvoltage protection (OVP) device which also
> integrates two load switches with resistor programmable current
> limiting and includes ESD protection for the USB Type-C signal pins.
> 
> This driver exposes the main path and the two the load switches via the

...

> +
> +
> +static irqreturn_t max20339_irq(int irqno, void *data)
> +{
> +	struct max20339_irq_data *max20339 = data;
> +	struct device *dev = max20339->dev;
> +	struct regmap *regmap = max20339->regmap;
> +	u8 status[6];
> +	int ret;
> +
> +	ret = regmap_bulk_read(regmap, MAX20339_STATUS1, status,
> +			       ARRAY_SIZE(status));
> +	if (ret) {
> +		dev_err(dev, "Failed to read IRQ status: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	dev_dbg(dev,
> +		"INT1 2 3: %#.2x %#.2x %#.2x STATUS1 2 3: %#.2x %#.2x %#.2x\n",
> +		status[3], status[4], status[5], status[0], status[1],
> +		status[2]);

You should not have prints, even debugs, in interrupt handlers. This can
flood the dmesg.

> +
> +	if (!status[3] && !status[4] && !status[5])
> +		return IRQ_NONE;
> +
> +	/* overall status */
> +	if (status[3] & status[0] & MAX20339_THMFAULT) {
> +		dev_warn(dev, "Thermal fault\n");
> +		for (int i = 0; i < ARRAY_SIZE(max20339->rdevs); ++i)
> +			regulator_notifier_call_chain(max20339->rdevs[i],
> +						      REGULATOR_EVENT_OVER_TEMP,
> +						      NULL);
> +	}
> +
> +	/* INSW status */
> +	if ((status[3] & MAX20339_VINVALID)
> +	    && !(status[0] & MAX20339_VINVALID)) {
> +		dev_warn(dev, "Vin over- or undervoltage\n");

Same with all these. What happens if interrupt is triggered constantly?


> +		regulator_notifier_call_chain(max20339->rdevs[MAX20339_REGULATOR_INSW],
> +					      (REGULATOR_EVENT_UNDER_VOLTAGE_WARN
> +					       | REGULATOR_EVENT_OVER_VOLTAGE_WARN),
> +					      NULL);
> +	}
> +
> +	if (status[3] & status[0] & MAX20339_INOVFAULT) {
> +		dev_warn(dev, "Over voltage on INput\n");
> +		regulator_notifier_call_chain(max20339->rdevs[MAX20339_REGULATOR_INSW],
> +					      REGULATOR_EVENT_OVER_VOLTAGE_WARN,
> +					      NULL);
> +	}
> +


...

> +
> +static int max20339_lsw_get_error_flags(struct regulator_dev *rdev,
> +					unsigned int *flags)
> +{
> +	struct max20339_regulator *data = rdev_get_drvdata(rdev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(rdev_get_regmap(rdev), data->status_reg, &val);
> +	if (ret) {
> +		dev_err(rdev_get_dev(rdev),
> +			"Failed to read MAX20339_STATUS%d: %d\n",
> +			data->status_reg, ret);
> +		return ret;
> +	}
> +
> +	*flags = 0;
> +
> +	if (val & MAX20339_LSWxSHORTFAULT)
> +		*flags |= REGULATOR_ERROR_OVER_CURRENT;
> +
> +	if (val & MAX20339_LSWxOVFAULT)
> +		*flags |= REGULATOR_ERROR_OVER_VOLTAGE_WARN;
> +
> +	if (val & MAX20339_LSWxOCFAULT)
> +		*flags |= REGULATOR_ERROR_OVER_CURRENT;
> +
> +	return 0;
> +}
> +
> +static int max20339_lsw_dt_parse(struct device_node *np,
> +				 const struct regulator_desc *desc,
> +				 struct regulator_config *cfg)
> +{
> +	struct max20339_regulator *data = cfg->driver_data;
> +
> +	/* we turn missing properties into a fatal issue during probe() */

Your binding does not look in sync with above.

> +	return of_property_read_u32(np, "shunt-resistor-micro-ohms",
> +				    &data->shunt_micro_ohm);
> +}
> +
> +static const struct regulator_ops max20339_lsw_ops = {
> +	.enable = regulator_enable_regmap,
> +	.disable = regulator_disable_regmap,
> +	.is_enabled = max20339_lsw_is_enabled,
> +
> +	.set_over_current_protection = max20339_lsw_set_ocp,
> +	.set_over_voltage_protection = max20339_lsw_set_ovp,
> +
> +	.get_error_flags = max20339_lsw_get_error_flags,
> +};
> +
> +#define MAX20339_LSW_DESC(_sfx, _enable_mask, _csel_mask, _ovp_mask, _status_reg) { \
> +	.desc = {                                                 \
> +		.name = "max20339-" _sfx,                         \
> +		\
> +		.ops = &max20339_lsw_ops,                         \
> +		.type = REGULATOR_VOLTAGE,                        \
> +		.supply_name = _sfx,                              \
> +		\
> +		.enable_reg = MAX20339_SWCNTL,                    \
> +		.enable_mask = _enable_mask,                      \
> +		\
> +		.csel_reg = MAX20339_SWILIMDIV,                   \
> +		.csel_mask = _csel_mask,                          \
> +		\
> +		.regulators_node = of_match_ptr("regulators"),    \
> +		.of_match = of_match_ptr(_sfx),                   \
> +		.of_parse_cb = max20339_lsw_dt_parse,             \
> +		\
> +		.enable_time = 11000 + 1100,                      \
> +		.off_on_delay = 13,                               \
> +		.poll_enabled_time = 2420,                        \
> +		\
> +		.owner = THIS_MODULE,                             \
> +	},                                                        \
> +	.ovp_mask = _ovp_mask,                                    \
> +	.status_reg = _status_reg,                                \
> +}
> +
> +

Here and in few other places - just one blank line.

> +static struct max20339_regulator max20339_regulators[MAX20339_N_REGULATORS] = {

This can be const and then use container_of instead of rdev_get_drvdata().

See:
https://lore.kernel.org/all/20240909-regulator-const-v1-17-8934704a5787@linaro.org/

> +	[MAX20339_REGULATOR_INSW] = {
> +		.desc = {
> +			.name = "max20339-insw",
> +
> +			.ops = &max20339_insw_ops,
> +			.type = REGULATOR_VOLTAGE,
> +			.supply_name = "insw",
> +
> +			.volt_table = max20339_insw_volt_table,
> +			.n_voltages = ARRAY_SIZE(max20339_insw_volt_table),
> +
> +			.enable_reg = MAX20339_IN_CTR,
> +			.enable_mask = MAX20339_IN_CTR_INSWEN,
> +			.enable_val = FIELD_PREP_CONST(MAX20339_IN_CTR_INSWEN,
> +						  MAX20339_IN_CTR_INSWEN_AUTO),
> +
> +			.active_discharge_reg = MAX20339_IN_CTR,
> +			.active_discharge_mask = MAX20339_IN_CTR_INDISEN,
> +			.active_discharge_on = MAX20339_IN_CTR_INDISEN,
> +			.active_discharge_off = 0,
> +
> +			.regulators_node = of_match_ptr("regulators"),
> +			.of_match = of_match_ptr("insw"),
> +
> +			.enable_time = 15000,
> +			.off_on_delay = 1,
> +			.poll_enabled_time = 3000,
> +
> +			.owner = THIS_MODULE,
> +		}
> +	},
> +	[MAX20339_REGULATOR_LSW1] = MAX20339_LSW_DESC("lsw1",
> +						MAX20339_SWCNTL_LSW1EN,
> +						MAX20339_SWILIMDIV_LSW1ILIMDIV,
> +						MAX20339_SWCNTL_LSW1OVEN,
> +						MAX20339_STATUS2),
> +	[MAX20339_REGULATOR_LSW2] = MAX20339_LSW_DESC("lsw2",
> +						MAX20339_SWCNTL_LSW2EN,
> +						MAX20339_SWILIMDIV_LSW2ILIMDIV,
> +						MAX20339_SWCNTL_LSW2OVEN,
> +						MAX20339_STATUS3),
> +};
> +
> +static int max20339_setup_irq(struct i2c_client *client,
> +			      struct regmap *regmap,
> +			      struct regulator_dev *rdevs[])
> +{
> +	u8 enabled_irqs[3];
> +	struct max20339_irq_data *max20339;
> +	int ret;
> +	unsigned long irq_flags;
> +
> +	/* the IRQ is optional */
> +	if (!client->irq) {
> +		enabled_irqs[0] = enabled_irqs[1] = enabled_irqs[2] = 0;
> +	} else {
> +		enabled_irqs[0] = (MAX20339_VINVALID | MAX20339_THMFAULT
> +				   | MAX20339_INOVFAULT);
> +		enabled_irqs[1] = (MAX20339_LSWxSHORTFAULT
> +				   | MAX20339_LSWxOVFAULT
> +				   | MAX20339_LSWxOCFAULT);
> +		enabled_irqs[2] = (MAX20339_LSWxSHORTFAULT
> +				   | MAX20339_LSWxOVFAULT
> +				   | MAX20339_LSWxOCFAULT);
> +
> +		max20339 = devm_kzalloc(&client->dev, sizeof(*max20339),
> +					GFP_KERNEL);
> +		if (!max20339)
> +			return -ENOMEM;
> +	}
> +
> +	ret = regmap_bulk_write(regmap, MAX20339_INTMASK1,
> +				enabled_irqs, ARRAY_SIZE(enabled_irqs));
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "error configuring INTMASK1..3\n");
> +
> +	if (!client->irq)
> +		return 0;
> +
> +	max20339->dev = &client->dev;
> +	max20339->regmap = regmap;
> +	memcpy(max20339->rdevs, rdevs, sizeof(max20339->rdevs));
> +
> +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;

Why shared?

> +	irq_flags |= irqd_get_trigger_type(irq_get_irq_data(client->irq));
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq,

Shared interrupts should not be devm. It leads to tricky cases during
removal. If you investigated the code and you are 100% sure there is no
issue, please write a short comment in the code confirming that. Or just
don't use devm.

> +					NULL, max20339_irq, irq_flags,
> +					"max20339", max20339);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "IRQ setup failed\n");
> +
> +	return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_GPIO_REGMAP)
> +static int max20339_gpio_regmap_xlate(struct gpio_regmap *const gpio,
> +				      unsigned int base, unsigned int offset,
> +				      unsigned int *const reg,
> +				      unsigned int *const mask)
> +{
> +	if (offset != 0)
> +		return -EINVAL;
> +
> +	*reg = base;
> +	*mask = MAX20339_VINVALID;
> +	return 0;
> +}
> +
> +static int max20339_setup_gpio(struct i2c_client *client,
> +			       struct regmap *regmap)
> +{
> +	struct fwnode_handle *fwnode;
> +	static const char * const names[] = { "vin" };
> +	int ret;
> +
> +	fwnode = gpiochip_node_get_first(&client->dev);
> +	if (!fwnode) {
> +		dev_info(&client->dev, "Skipping gpio chip initialization\n");
> +		return 0;
> +	}
> +
> +	ret = PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&client->dev,
> +			&(struct gpio_regmap_config) {
> +				.parent = &client->dev,
> +				.regmap = regmap,
> +				.fwnode = fwnode,
> +				.ngpio = ARRAY_SIZE(names),
> +				.names = names,
> +				.reg_dat_base = MAX20339_STATUS1,
> +				.reg_mask_xlate = max20339_gpio_regmap_xlate
> +			}));

That's not really readable. Please split PTR_ERR_OR_ZERO.

> +	fwnode_handle_put(fwnode);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to initialize gpio chip\n");
> +
> +	return 0;
> +}
> +#else /* CONFIG_GPIO_REGMAP */
> +static int max20339_setup_gpio(struct i2c_client *client,
> +			       struct regmap *regmap)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_GPIO_REGMAP */
> +
> +static int max20339_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	struct regmap *regmap;
> +	struct regulator_config config = {};
> +	struct regulator_dev *rdev;
> +	struct regulator_dev *rdevs[MAX20339_N_REGULATORS];
> +
> +	/*
> +	 * either "dig-supply" is needed, or alternatively "in-supply" will
> +	 * supply power
> +	 */
> +	ret = devm_regulator_get_enable_optional(&client->dev, "dig");
> +	if (ret) {
> +		if (ret == -ENODEV)
> +			ret = devm_regulator_get_enable_optional(&client->dev,
> +								 "insw");
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "failed to get regulator");
> +	}
> +
> +	regmap = devm_regmap_init_i2c(client, &max20339_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err_probe(&client->dev, PTR_ERR(regmap),

return dev_err_probe

> +			      "regmap init failed\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	for (int i = 0; i < ARRAY_SIZE(max20339_regulators); ++i) {
> +		config.dev = &client->dev;
> +		config.regmap = regmap;
> +		config.driver_data = &max20339_regulators[i];
> +
> +		rdev = devm_regulator_register(config.dev,
> +					       &max20339_regulators[i].desc,
> +					       &config);
> +		if (IS_ERR(rdev))
> +			return dev_err_probe(&client->dev, PTR_ERR(rdev),
> +					     "failed to register MAX20339 regulator %s\n",
> +					     max20339_regulators[i].desc.name);
> +
> +		/*
> +		 * For the LSWs, we really need to know the shunts' values
> +		 * (from DT). Fail if missing.
> +		 */
> +		if (max20339_regulators[i].desc.csel_mask
> +		    && !max20339_regulators[i].shunt_micro_ohm)
> +			return dev_err_probe(&client->dev, -EINVAL,
> +					     "property 'shunt-resistor-micro-ohms' not found\n");
> +
> +		rdevs[i] = rdev;
> +
> +		dev_info(&client->dev, "registered MAX20339 regulator %s\n",
> +			 max20339_regulators[i].desc.name);
> +	}
> +
> +	ret = max20339_setup_irq(client, regmap, rdevs);
> +	if (ret < 0)
> +		return ret;
> +
> +	return max20339_setup_gpio(client, regmap);
> +}
> +
> +static const struct i2c_device_id max20339_i2c_id[] = {
> +	{ "max20339", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max20339_i2c_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id max20339_of_id[] = {
> +	{ .compatible = "maxim,max20339", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max20339_of_id);
> +#endif
> +
> +static struct i2c_driver max20339_i2c_driver = {
> +	.driver = {
> +		.name = "max20339",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +		.of_match_table = of_match_ptr(max20339_of_id),

Drop of_match_ptr and earlier #ifdef. Not much benefits and limits usage
to OF-systems.

Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ