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

[Guenter Roeck]

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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>");
> +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