linux-kernel - Re: [PATCH v1] hwmon: Add driver for MPS MPQ8785 Synchronous Step-Down Converter

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: <81860c27-43ac-4e55-a653-e7f5597ffa93@roeck-us.net>
Date: Sat, 27 Jan 2024 16:00:25 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Charles Hsu <ythsu0511@...il.com>
Cc: jdelvare@...e.com, corbet@....net, Delphine_CC_Chiu@...ynn.com,
	linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
	Charles.Hsu@...ntatw.com
Subject: Re: [PATCH v1] hwmon: Add driver for MPS MPQ8785 Synchronous
 Step-Down Converter

On Fri, Jan 26, 2024 at 03:52:13PM +0800, Charles Hsu wrote:
> Add support for mpq8785 device from Monolithic Power Systems, Inc.
> (MPS) vendor. This is synchronous step-down controller.
> 

"(MPS) vendor" above has no value.

I find no reference that this chip actually exists. Sorry, but I can not
apply such patches without confirmation that the chip actually exists.

> Signed-off-by: Charles Hsu <ythsu0511@...il.com>
> ---
> Change in v1:
>     Initial patchset.

A change log or v1 tag is not needed for the first version of a patch
or patch series.

> ---
..
> +		PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_IOUT |
> +		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,

I am not too happy that all those drivers claim to have no output status
registers. It always makes me wonder if the definitions are just copied
from one driver to the next.

> +static const struct of_device_id __maybe_unused mpq8785_of_match[] = {
> +	{ .compatible = "mps,mpq8785", },
> +	{}

checkpatch says:

WARNING: DT compatible string "mps,mpq8785" appears un-documented -- check ./Documentation/devicetree/bindings/
#293: FILE: drivers/hwmon/pmbus/mpq8785.c:37:
+	{ .compatible = "mps,mpq8785", },

I don't care about the also missing entry in MAINTAINERS,
but the device needs to be be documented in
Documentation/devicetree/bindings/trivial-devices.yaml.

> +};
> +
> +MODULE_DEVICE_TABLE(of, mpq8785_of_match);
> +
> +static struct i2c_driver mpq8785_driver = {
> +	.driver = {
> +		   .name = "mpq8785",
> +		   .of_match_table = of_match_ptr(mpq8785_of_match),
> +	},
> +	.probe_new = mpq8785_probe,
> +};
> +
> +module_i2c_driver(mpq8785_driver);
> +
> +MODULE_AUTHOR("Charles Hsu <ythsu0511@...il.com>");
> +MODULE_DESCRIPTION("MPQ8785 PMIC regulator driver");

Is this copied from the mpq7932 driver ? This driver does not include a
regulator component, so it is a bit misleading to label it as regulator
driver.

Guenter