Re: [hwmon-next PATCH v4 4/4] hwmon: (adt7470) Add thermal zone sensor support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The driver implements PWM functionality directly rather than using an auxiliary device.

Pre-existing issues:
- [High] The driver holds a mutex for up to 2 seconds while sleeping during temperature reads, which will severely block thermal core polling.
- [Medium] The driver implements its own lock (`data->lock`) for extra sysfs attributes instead of using the required `hwmon_lock()` API.
--

--- Patch [4]: [hwmon-next PATCH v4 4/4] hwmon: (adt7470) Add thermal zone sensor support ---
commit a0151226eff53f9134558a65871681b2e48deb5c
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Add thermal zone sensor support

This patch registers the ADT7470 temperature channels as thermal zone sensors.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 883838fdcd7db..6cc246aa12851 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -1349,6 +1349,8 @@ static const struct hwmon_ops adt7470_hwmon_ops = {
>  };
>  
>  static const struct hwmon_channel_info * const adt7470_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_REGISTER_TZ),

[Severity: High]
This is a pre-existing issue, but does registering the thermal zone cause the
thermal core to severely block on reading temperatures?

The adt7470_update_thread repeatedly acquires data->lock and calls
adt7470_read_temperatures(), which sleeps for up to 2000 ms:

drivers/hwmon/adt7470.c:adt7470_read_temperatures() {
    ...
    res = msleep_interruptible((data->num_temp_sensors >= 0 ?
                                data->num_temp_sensors * 200 :
                                TEMP_COLLECTION_TIME));
    ...
}

Concurrently, the kernel thermal subsystem automatically polls the temperature
because this patch registers the sensors as a thermal zone.

The thermal core calls adt7470_update_device(), which blocks on data->lock.
Blocking the thermal core's system-wide freezable workqueue for up to 2
seconds per read causes significant delays for other thermal zones and
background tasks.

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but does the driver violate the subsystem
locking rules by implementing its own lock for sysfs attributes?

drivers/hwmon/adt7470.c:pwm_min_store() {
    ...
    mutex_lock(&data->lock);
    data->pwm_min[attr->index] = temp;
    ...
}

The hardware monitoring subsystem guidelines strictly state that drivers must
use hwmon_lock() and hwmon_unlock() for attributes registered by other means.

[Severity: Medium]
Is pwm functionality implemented directly within the hardware monitoring
driver rather than using an auxiliary device?

drivers/hwmon/adt7470.c:adt7470_probe() {
    ...
    chip = devm_pwmchip_alloc(dev, ADT7470_PWM_COUNT, 0);
    ...
    err = devm_pwmchip_add(dev, chip);
    ...
}

The hwmon subsystem guide explicitly mandates that secondary functionalities
must be implemented as an auxiliary device, with the driver residing in the
appropriate subsystem directory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4