Re: [PATCH 2/2] hwmon: add MP2845 driver

[Krzysztof Kozlowski]

On Wed, Feb 25, 2026 at 04:56:31PM +0800, wenswang@xxxxxxxx wrote:
> +#define MFR_VIN_OV_UV_SET	0x71
> +#define MFR_OVUV_OCWARN_THRES	0x75
> +#define MFR_TOTAL_OCP_SET	0x76
> +#define MFR_PROTECT_STATUS1	0x80
> +#define MFR_PROTECT_STATUS2 0x81
> +
> +#define MP2845_VIN_LIMIT_UINT	125
> +#define MP2845_READ_VIN_UINT	3125
> +#define MP2845_READ_VIN_DIV	100
> +#define MP2845_READ_IOUT_UINT	3125
> +#define MP2845_READ_IOUT_DIV	100
> +#define MP2845_READ_VOUT_UINT	5
> +#define MP2845_TEMP_UINT	1000
> +
> +#define MFR_READ_VIN	0xA6
> +#define MFR_READ_VOUT	0xA7
> +#define MFR_READ_IOUT	0xA8
> +#define MFR_READ_TEMP	0xA9
> +#define MFR_MFG_ID_SCALE_VI1	0x77
> +#define MFR_MFG_ID_SCALE_VI2	0x78
> +
> +struct mp2845_data {
> +	struct i2c_client *client;
> +	int iout_gain[4];
> +	/* lock for preventing concurrency issue */

This is completely useless comment. The definition of lock is to prevent
concurrency issues. It's like adding a comment to a function: "it is a
function".

You must here explain which data or code logic is protected by this.

> +	struct mutex lock;
> +};

...


> +
> +static int mp2845_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct mp2845_data *data;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA)) {
> +		dev_err(dev, "check failed, smbus byte and/or word data not supported!\n");
> +		return -ENODEV;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(struct mp2845_data), GFP_KERNEL);

sizeof(*)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	mutex_init(&data->lock);
> +	data->client = client;
> +
> +	ret = mp2845_identify_iout_scale(data, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to identify rail1 iout scale, errno = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mp2845_identify_iout_scale(data, 1);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to identify rail2 iout scale, errno = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mp2845_identify_iout_scale(data, 2);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to identify rail3 iout scale, errno = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mp2845_identify_iout_scale(data, 3);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to identify rail4 iout scale, errno = %d\n", ret);
> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 data, &mp2845_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		dev_err(dev, "unable to register mp2845 hwmon device\n");
> +		return PTR_ERR(hwmon_dev);
> +	}
> +
> +	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);

Driver should be silent on success probe. See also coding style and
driver development docs.

Best regards,
Krzysztof