Re: [PATCH 1/3] pinctrl: cix: Add pin-controller support for sky1

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/08/2025 04:42, Gary Yang wrote:
> +
> +static int sky1_pinctrl_probe_dt(struct platform_device *pdev,
> +				struct sky1_pinctrl *spctl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *child;
> +	struct pinctrl_dev *pctl = spctl->pctl;
> +	u32 nfuncs = 0;
> +	u32 i = 0;
> +	bool flat_funcs;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	flat_funcs = sky1_pinctrl_dt_is_flat_functions(np);
> +	if (flat_funcs) {
> +		nfuncs = 1;
> +	} else {
> +		nfuncs = of_get_child_count(np);
> +		if (nfuncs == 0) {
> +			dev_err(&pdev->dev, "no functions defined\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for (i = 0; i < nfuncs; i++) {
> +		struct function_desc *function;
> +
> +		function = devm_kzalloc(&pdev->dev, sizeof(*function),
> +					GFP_KERNEL);
> +		if (!function)
> +			return -ENOMEM;
> +
> +		mutex_lock(&spctl->mutex);
> +		radix_tree_insert(&pctl->pin_function_tree, i, function);
> +		mutex_unlock(&spctl->mutex);
> +	}
> +	pctl->num_functions = nfuncs;
> +
> +	spctl->group_index = 0;
> +	if (flat_funcs) {
> +		pctl->num_groups = of_get_child_count(np);
> +	} else {
> +		pctl->num_groups = 0;
> +		for_each_child_of_node(np, child)
> +			pctl->num_groups += of_get_child_count(child);
> +	}
> +
> +	if (flat_funcs) {
> +		sky1_pinctrl_parse_functions(np, spctl, 0);
> +	} else {
> +		i = 0;
> +		for_each_child_of_node(np, child)
> +			sky1_pinctrl_parse_functions(child, spctl, i++);
> +	}
> +
> +	return 0;
> +}
> +
> +int sky1_base_pinctrl_probe(struct platform_device *pdev,
> +		      const struct sky1_pinctrl_soc_info *info)
> +{
> +	struct pinctrl_desc *sky1_pinctrl_desc;
> +	struct sky1_pinctrl *spctl;
> +	int ret, i;
> +
> +	if (!info || !info->pins || !info->npins) {
> +		dev_err(&pdev->dev, "wrong pinctrl info\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Create state holders etc for this driver */
> +	spctl = devm_kzalloc(&pdev->dev, sizeof(*spctl), GFP_KERNEL);
> +	if (!spctl)
> +		return -ENOMEM;
> +
> +	spctl->pin_regs = devm_kmalloc_array(&pdev->dev, info->npins,
> +					    sizeof(*spctl->pin_regs),
> +					    GFP_KERNEL);
> +	if (!spctl->pin_regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < info->npins; i++)
> +		spctl->pin_regs[i] = -1;
> +
> +	spctl->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(spctl->base))
> +		return PTR_ERR(spctl->base);
> +
> +	sky1_pinctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*sky1_pinctrl_desc),
> +					GFP_KERNEL);
> +	if (!sky1_pinctrl_desc)
> +		return -ENOMEM;
> +
> +	sky1_pinctrl_desc->name = dev_name(&pdev->dev);
> +	sky1_pinctrl_desc->pins = info->pins;
> +	sky1_pinctrl_desc->npins = info->npins;
> +	sky1_pinctrl_desc->pctlops = &sky1_pctrl_ops;
> +	sky1_pinctrl_desc->pmxops = &sky1_pmx_ops;
> +	sky1_pinctrl_desc->confops = &sky1_pinconf_ops;
> +	sky1_pinctrl_desc->owner = THIS_MODULE;
> +
> +	mutex_init(&spctl->mutex);
> +
> +	spctl->info = info;
> +	spctl->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, spctl);
> +	ret = devm_pinctrl_register_and_init(&pdev->dev,
> +					     sky1_pinctrl_desc, spctl,
> +					     &spctl->pctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not register SKY1 pinctrl driver\n");
> +		return ret;
> +	}
> +
> +	ret = sky1_pinctrl_probe_dt(pdev, spctl);
> +

No blank line here.

> +	if (ret) {
> +		dev_err(&pdev->dev, "fail to probe dt properties\n");

You are printing same error twice. Drop this and just handle error
printing in sky1_pinctrl_probe_dt().
Especially that you now print errors on ENOMEM.

> +		return ret;
> +	}
> +
> +	pinctrl_provide_dummies();
> +	dev_info(&pdev->dev, "initialized SKY1 pinctrl driver\n");


No, please drop. Drivers should be silent on success.

> +
> +	return pinctrl_enable(spctl->pctl);
> +}
> +EXPORT_SYMBOL_GPL(sky1_base_pinctrl_probe);
> +



..

> +
> +static struct platform_driver sky1_pinctrl_driver = {
> +	.driver = {
> +		.name = "sky1-pinctrl",
> +		.of_match_table = of_match_ptr(sky1_pinctrl_of_match),


You have a warning here - please drop of_match_ptr.

> +		.pm = &sky1_pinctrl_pm_ops,
> +	},
> +	.probe = sky1_pinctrl_probe,
> +};
> +
> +static int __init sky1_pinctrl_init(void)
> +{
> +	return platform_driver_register(&sky1_pinctrl_driver);
> +}
> +arch_initcall(sky1_pinctrl_init);
> +
> +MODULE_AUTHOR("Jerry Zhu <Jerry.Zhu@cixtech.com>");
> +MODULE_DESCRIPTION("Cix Sky1 pinctrl driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pinctrl/cix/pinctrl-sky1.h b/drivers/pinctrl/cix/pinctrl-sky1.h
> new file mode 100644
> index 000000000000..09b25dbb6db3
> --- /dev/null
> +++ b/drivers/pinctrl/cix/pinctrl-sky1.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Author: Jerry Zhu <Jerry.Zhu@cixtech.com>
> + */
> +
> +#ifndef __DRIVERS_PINCTRL_SKY1_H
> +#define __DRIVERS_PINCTRL_SKY1_H
> +
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>

Are you sure you use both headers in this header?

Best regards,
Krzysztof