Re: [PATCH 2/2] hwmon: add Axiado AX3000 and AX3005 PWM fan controller driver

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

On Thu, Aug 06, 2026 at 02:01:30AM -0700, Petar Stepanovic wrote:
> +#define PWM_MAX	255

That define has a name that looks much more generic than it actually is.
Please call it AX_TACH_PWM_VAL_MAX or something similar.

> [...]
> +static int axiado_pwm_fan_read(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long *val)
> +{
> +	struct axiado_pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		if (attr != hwmon_pwm_input)
> +			return -EOPNOTSUPP;
> +
> +		scoped_guard(mutex, &ctx->pwm_lock)
> +			*val = ctx->pwm_value;

If there is a hwmon lock that serializes calls to hwmon_ops::read and
hwmon_ops::write, .pwm_lock isn't needed. (Unless I miss something,
please double check. Also I'm unsure if there is such a hwmon
serialization.)

> [...]
> +static const struct of_device_id axiado_pwm_fan_match[] = {
> +	{ .compatible = "axiado,ax3000-pwm-fan" },
> +	{ },

No trailing , after the list terminator please.

> +};
> +MODULE_DEVICE_TABLE(of, axiado_pwm_fan_match);
> +
> +static struct platform_driver axiado_pwm_fan_driver = {
> +	.probe		= axiado_pwm_fan_probe,
> +	.shutdown	= axiado_pwm_fan_shutdown,
> +	.driver	= {
> +		.name		= "axiado-pwm-fan",
> +		.pm		= pm_sleep_ptr(&axiado_pwm_fan_pm),
> +		.of_match_table	= axiado_pwm_fan_match,
> +	},
> +};
> +
> +module_platform_driver(axiado_pwm_fan_driver);

Typically `module_platform_driver` follows the struct platform_driver
without a newline.

> +MODULE_AUTHOR("Axiado Corporation");
> +MODULE_DESCRIPTION("Axiado PWM fan controller driver");
> +MODULE_LICENSE("GPL");
> +

Trailing newline at end of file.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]