Re: [PATCH 2/2] PM / devfreq: add Airoha SoC devfreq driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The driver is missing the `MODULE_DEVICE_TABLE(of, ...)` macro for its OF match table, preventing automatic module loading.
- [Medium] The `airoha_devfreq_target` callback fails to resolve the requested frequency to a valid OPP and does not update `*freq` with the actual applied frequency.
- [Low] Multiple function definitions end with an illegal trailing semicolon.
--

--- Patch [2]: [PATCH 2/2] PM / devfreq: add Airoha SoC devfreq driver ---
commit 541990b8210ffb6d5026b9bcd5630443b99f63ec
Author: Christian Marangi <ansuelsmth@gmail.com>

PM / devfreq: add Airoha SoC devfreq driver

This commit adds a simple Airoha SoC devfreq driver. It registers a
passive governor to scale various internal peripheral clocks based on
the CPU frequency requirements.

> diff --git a/drivers/devfreq/airoha-devfreq.c b/drivers/devfreq/airoha-devfreq.c
> new file mode 100644
> index 0000000000000..cb6d8b49e4a40
> --- /dev/null
> +++ b/drivers/devfreq/airoha-devfreq.c

[ ... ]

> +static int airoha_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct airoha_devfreq_data *data = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(data->clk);
> +
> +	return 0;
> +};

[Severity: Low]
This isn't a bug, but should this function definition end with a semicolon?

> +
> +static int airoha_devfreq_target(struct device *dev, unsigned long *freq,
> +				 u32 flags)
> +{
> +	struct airoha_devfreq_data *data = dev_get_drvdata(dev);
> +
> +	return clk_set_rate(data->clk, *freq);
> +};

[Severity: Medium]
Does this function need to resolve the requested frequency to a valid OPP
and update the freq pointer with the actual applied frequency?

If the governor or PM QoS requests a frequency that doesn't exactly match
an OPP, applying it directly without resolving (e.g., using
devfreq_recommended_opp) leaves the unrounded frequency in the freq pointer.

The devfreq core tracks frequency statistics using exact matches against
its freq_table.

Won't this cause devfreq_get_freq_level() lookups to fail
in devfreq_update_status(), resulting in spamming the kernel log with
'Couldn't update frequency transition information' warnings?

Also, similar to airoha_devfreq_get_cur_freq(), this function definition
has a trailing semicolon that can probably be removed.

[ ... ]

> +static int airoha_devfreq_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	return PTR_ERR_OR_ZERO(devfreq);
> +};
> +
> +static const struct of_device_id airoha_devfreq_match_table[] = {
> +	{ .compatible = "airoha,devfreq" },
> +	{}
> +};

[Severity: Medium]
Is this missing a MODULE_DEVICE_TABLE(of, airoha_devfreq_match_table)
declaration?

Without it, the modpost tool won't extract the compatible strings to
modules.alias.

Will user-space module loaders be able to automatically load this driver
when the hardware node is detected during boot or hotplug?

> +
> +static struct platform_driver airoha_devfreq_driver = {
> +	.probe		= airoha_devfreq_probe,
> +	.driver		= {
> +		.name   = "airoha-devfreq",
> +		.of_match_table = airoha_devfreq_match_table,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810143057.651478-1-ansuelsmth@gmail.com?part=2