linux-kernel - Re: [PATCH 1/2] hwmon: (pmbus) Add support for MPS multi-phase mp2869a/mp29612a controllers

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9bd05709-7702-4b74-85e1-3df25b57c535@kernel.org>
Date: Tue, 24 Jun 2025 09:48:18 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: tzuhao.wtmh@...il.com, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Naresh Solanki <naresh.solanki@...ements.com>,
 Rodrigo Gobbi <rodrigo.gobbi.7@...il.com>,
 Michal Simek <michal.simek@....com>, Fabio Estevam <festevam@...il.com>,
 Henry Wu <Henry_Wu@...ntatw.com>, Grant Peltier <grantpeltier93@...il.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Cedric Encarnacion <cedricjustine.encarnacion@...log.com>,
 Kim Seer Paller <kimseer.paller@...log.com>,
 Leo Yang <leo.yang.sy0@...il.com>, Ninad Palsule <ninad@...ux.ibm.com>,
 Alex Vdovydchenko <xzeol@...oo.com>,
 John Erasmus Mari Geronimo <johnerasmusmari.geronimo@...log.com>,
 Nuno Sa <nuno.sa@...log.com>, Jerome Brunet <jbrunet@...libre.com>,
 Noah Wang <noahwang.wang@...look.com>,
 Mariel Tinaco <Mariel.Tinaco@...log.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] hwmon: (pmbus) Add support for MPS multi-phase
 mp2869a/mp29612a controllers

On 24/06/2025 09:41, tzuhao.wtmh@...il.com wrote:
> +static int
> +MP2869A_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	switch (reg) {
> +	case PMBUS_VOUT_MODE:
> +		/* Enforce VOUT direct format. */
> +		return PB_VOUT_MODE_DIRECT;
> +	default:
> +		return -ENODATA;
> +	}
> +}
> +
> +static int
> +MP2869A_identify_vout_format(struct i2c_client *client,

Use Linux coding style, so lowercase for variables, types and functions.
Everywhere (except when coding style tells you different, so please read
it).

> +			    struct MP2869A_data *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < data->info.pages; i++) {
> +		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_read_word_data(client, MP2869A_VOUT_MODE);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (ret & MP2869A_VOUT_MODE_MASK) {
> +		case MP2869A_VOUT_MODE_VID:
> +			data->vout_format[i] = vid;
> +			break;
> +		default:
> +		return -EINVAL;
> +		}
> +		}

Messed indentation in multiple places.

> +	return 0;
> +}
> +
> +static struct pmbus_driver_info MP2869A_info = {

This is const.

> +	.pages = MP2869A_PAGE_NUM,
> +	.format[PSC_VOLTAGE_IN] = linear,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_TEMPERATURE] = linear,
> +	.format[PSC_CURRENT_IN] = linear,
> +	.format[PSC_CURRENT_OUT] = linear,
> +	.format[PSC_POWER] = linear,
> +	.m[PSC_VOLTAGE_OUT] = 1,
> +	.b[PSC_VOLTAGE_OUT] = 0,
> +	.R[PSC_VOLTAGE_OUT] = -3,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +		PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> +		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
> +		PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
> +	.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT |
> +		PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT | PMBUS_HAVE_TEMP,
> +	.read_byte_data = MP2869A_read_byte_data,
> +	.read_word_data = MP2869A_read_word_data,
> +};
> +
> +static int mp2869a_probe(struct i2c_client *client)
> +{
> +	struct pmbus_driver_info *info;
> +	struct MP2869A_data *data;
> +	int ret;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct MP2869A_data),

sizeof(*)

> +		GFP_KERNEL);

Misaligned. Run checkpatch --srtict

> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->chip_id = (enum chips)(uintptr_t)i2c_get_match_data(client);

These are just wrong or redundant casts. You need only one cast -
kernel_ulong_t

> +
> +	memcpy(data->max_phases, mp2869a_max_phases[data->chip_id],
> +		sizeof(data->max_phases));

Why you cannot just store the pointer?

> +
> +	memcpy(&data->info, &MP2869A_info, sizeof(*info));

Why you cannot just store the pointer?

> +	info = &data->info;
> +
> +

One blank line, not two

> +	/* Identify vout format. */
> +	ret = MP2869A_identify_vout_format(client, data);
> +	if (ret)
> +		return ret;
> +
> +	/* set the device to page 0 */
> +	i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct of_device_id __maybe_unused mp2869a_of_match[] = {
> +	{ .compatible = "mps,mp2869a", .data = (void *)mp2869a },
> +	{ .compatible = "mps,mp29612a", .data = (void *)mp29612a},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, mp2869a_of_match);
> +
> +static struct i2c_driver mp2869a_driver = {
> +	.driver = {
> +		.name = "mp2869a",
> +		.of_match_table = mp2869a_of_match,
> +	},
> +	.probe = mp2869a_probe,
> +	.id_table = mp2869a_id,
> +};
> +
> +module_i2c_driver(mp2869a_driver);
> +
> +

One blank line, not two. This applies everywhere.

> +MODULE_AUTHOR("Henry Wu <Henry_WU@...ntatw.com>");
> +MODULE_DESCRIPTION("PMBus driver for MPS MP2869A/MP29612A device");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);


Best regards,
Krzysztof