Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] regulator: sy7636a: registering hwmon device outside drivers/hwmon/
--

commit 3011502577fb90233215473b5182522faa4b0058
Author: Pablo Mazzini <pmazzini@gmail.com>

regulator: sy7636a: report the temperature

This commit removes the dedicated sy7636a hardware monitoring driver and
moves temperature reporting into the sy7636a regulator driver, reading the
thermistor register only when the rails are enabled.

> diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
> index 5705599d1b55e..816df7716c0e0 100644
> --- a/drivers/regulator/sy7636a-regulator.c
> +++ b/drivers/regulator/sy7636a-regulator.c

[ ... ]

> @@ -147,6 +148,52 @@ static const struct regulator_desc desc = {
>  	.of_match = of_match_ptr("vcom"),
>  };
>  
> +static int sy7636a_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *temp)
> +{
> +	struct sy7636a_data *data = dev_get_drvdata(dev);
> +	unsigned int mode, val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, SY7636A_REG_OPERATION_MODE_CRL, &mode);
> +	if (ret)
> +		return ret;
> +
> +	/* the thermistor is only sampled while the rails are on */
> +	if (!(mode & SY7636A_OPERATION_MODE_CRL_ONOFF))
> +		return -ENODATA;
> +
> +	ret = regmap_read(data->regmap, SY7636A_REG_TERMISTOR_READOUT, &val);
> +	if (ret)
> +		return ret;
> +
> +	*temp = (s8)val * 1000;
> +
> +	return 0;
> +}

[ ... ]

> @@ -235,6 +282,15 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
>  		return PTR_ERR(rdev);
>  	}
>  
> +	if (IS_REACHABLE(CONFIG_HWMON)) {
> +		struct device *hwmon_dev;
> +
> +		hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "sy7636a", data,
> +								 &sy7636a_hwmon_chip_info, NULL);
> +		if (IS_ERR(hwmon_dev))
> +			dev_notice(&pdev->dev, "failed to register hwmon\n");
> +	}

[Severity: Medium]
Should the hardware monitoring driver remain in drivers/hwmon/ rather than
calling devm_hwmon_device_register_with_info() directly from
sy7636a_regulator_probe() in drivers/regulator/sy7636a-regulator.c?

Hardware monitoring drivers are expected to reside in drivers/hwmon/, and
when a chip's primary functionality is in another subsystem, its hardware
monitoring support should be implemented as an auxiliary or subdevice driver
in drivers/hwmon/ rather than registering a hwmon device directly from
outside drivers/hwmon/.

While the follow-up commit ("regulator: sy7636a: bind to the i2c device
directly") removes the mfd cell table in drivers/mfd/simple-mfd-i2c.c,
sy7636a_hwmon_read() only accesses the shared regmap via regmap_read() and
does not depend on internal regulator driver state. Could the temperature
reading check be kept in drivers/hwmon/sy7636a-hwmon.c (for instance via an
auxiliary device registered by the regulator driver)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026003918.155321-1-pmazzini@gmail.com?part=5