Re: [PATCH v3 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver

[Marek Vasut <marek.vasut@mailbox.org>]

On 8/5/26 5:20 PM, Geert Uytterhoeven wrote:

Hello Geert,

[...]

> +DEFINE_MDLC_RESET_WRAPPER(reset)
> +DEFINE_MDLC_RESET_WRAPPER(assert)
> +DEFINE_MDLC_RESET_WRAPPER(deassert)
> +DEFINE_MDLC_RESET_WRAPPER(status)
> +
> +static const struct reset_control_ops r8a78000_mdlc_reset_ops = {
> +	.reset = r8a78000_mdlc_reset,
> +	.assert = r8a78000_mdlc_assert,
> +	.deassert = r8a78000_mdlc_deassert,
> +	.status = r8a78000_mdlc_status,
> +};

Please keep the list sorted alphabetically.

> +static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
> +				    struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct r8a78000_mdlc_priv *priv;
> +	struct of_phandle_args pd_spec;
> +	const struct mod_map *map;
> +	unsigned int id;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "power-domains",
> +					 "#power-domain-cells", 0, &pd_spec);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (pd_spec.args_count != 2) {
> +		of_node_put(pd_spec.np);

Maybe some "goto error" fail path which does of_node_put() wouldn't hurt 
here, since the of_node_put() is duplicated in here multiple times.

> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &r8a78000_mdlc_lock) {
> +		hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> +			if (priv->np == pd_spec.np)
> +				break;
> +		}
> +	}
> +
> +	if (!priv) {
> +		dev_err(dev, "%s: MDLC %pOF not found\n", __func__, pd_spec.np);
> +		of_node_put(pd_spec.np);
> +		return -ENODEV;
> +	}
> +
> +	id = pd_spec.args[1];
> +	of_node_put(pd_spec.np);
> +
> +	map = mod_map_find(priv->mod_map, id);
> +	if (!map) {
> +		dev_err(dev, "Unknown module 0x%x\n", id);
> +		return -ENOENT;
> +	}
> +
> +	dev_dbg(dev, "Ignoring HW module 0x%x\n", id);
> +	return 0;
> +}

[...]

> +static int r8a78000_mdlc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct r8a78000_mdlc_priv *priv;
> +	const struct mdlc_info *info;
> +	struct resource *res;
> +	int ret;
> +
> +	ret = r8a78000_genpd_always_on_singleton(dev);
> +	if (ret)
> +		return ret;
> +
> +	info = of_device_get_match_data(dev);
> +	if (!info)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->np = np;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	for (; info->base; info++) {
> +		if (info->base == res->start)
> +			break;
> +	}
> +
> +	if (!info->base) {
> +		dev_dbg(dev, "Unsupported MDLC instance 0x%pa\n", &res->start);

return dev_err_probe() might be more appropriate here, even if that 
would be more chatty until the support for various MDLC nodes lands.

Some dev_info_probe() would be even better, but we don't have that.

[...]