Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> support of other platforms from Mobileye. It belongs to a syscon region
> called OLB.
> 
> Existing pins and their function live statically in the driver code
> rather than in the devicetree, see compatible match data.
> 

..

> +static int eq5p_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent_np = of_get_parent(np);
> +	const struct eq5p_match *match = of_device_get_match_data(dev);
> +	struct pinctrl_dev *pctldev;
> +	struct eq5p_pinctrl *pctrl;
> +	int ret;
> +
> +	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
> +	if (!pctrl)
> +		return -ENOMEM;
> +
> +	pctrl->olb = ERR_PTR(-ENODEV);
> +	if (parent_np)
> +		pctrl->olb = syscon_node_to_regmap(parent_np);
> +	if (IS_ERR(pctrl->olb))
> +		pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
> +	if (IS_ERR(pctrl->olb))
> +		return PTR_ERR(pctrl->olb);

No, we talked about this, you got comments on this. There is no
mobileye,olb. You cannot have undocumented properties.

> +
> +	pctrl->regs = match->regs;
> +	pctrl->funcs = match->funcs;
> +	pctrl->nfuncs = match->nfuncs;
> +
> +	pctrl->desc.name = dev_name(dev);
> +	pctrl->desc.pins = match->pins;
> +	pctrl->desc.npins = match->npins;
> +	pctrl->desc.pctlops = &eq5p_pinctrl_ops;
> +	pctrl->desc.pmxops = &eq5p_pinmux_ops;
> +	pctrl->desc.confops = &eq5p_pinconf_ops;
> +	pctrl->desc.owner = THIS_MODULE;
> +
> +	ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed registering pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pinctrl_enable(pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed enabling pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "probed\n");

I am pretty sure you got comments for these. Drop such debugs from all
of your code. Current and future.

> +
> +	return 0;
> +}
> +
> +static const struct eq5p_match eq5p_match_a = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C0,
> +		[EQ5P_PU] = 0x0C4,
> +		[EQ5P_DS_LOW] = 0x0D0,
> +		[EQ5P_DS_HIGH] = 0x0D4,
> +		[EQ5P_IOCR] = 0x0B0,
> +	},
> +	.pins = eq5p_pins_a,
> +	.npins = ARRAY_SIZE(eq5p_pins_a),
> +	.funcs = eq5p_functions_a,
> +	.nfuncs = ARRAY_SIZE(eq5p_functions_a),
> +};
> +
> +static const struct eq5p_match eq5p_match_b = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C8,
> +		[EQ5P_PU] = 0x0CC,
> +		[EQ5P_DS_LOW] = 0x0D8,
> +		[EQ5P_DS_HIGH] = 0x0DC,
> +		[EQ5P_IOCR] = 0x0B4,
> +	},
> +	.pins = eq5p_pins_b,
> +	.npins = ARRAY_SIZE(eq5p_pins_b),
> +	.funcs = eq5p_functions_b,
> +	.nfuncs = ARRAY_SIZE(eq5p_functions_b),
> +};
> +
> +static const struct of_device_id eq5p_match[] = {
> +	{ .compatible = "mobileye,eyeq5-a-pinctrl", .data = &eq5p_match_a },
> +	{ .compatible = "mobileye,eyeq5-b-pinctrl", .data = &eq5p_match_b },
> +	{},
> +};
> +
> +static struct platform_driver eq5p_driver = {
> +	.driver = {
> +		.name = "eyeq5-pinctrl",
> +		.of_match_table = eq5p_match,
> +	},
> +	.probe = eq5p_probe,
> +};
> +
> +static int __init eq5p_init(void)
> +{
> +	return platform_driver_register(&eq5p_driver);
> +}
> +core_initcall(eq5p_init);

No, pins are not a core_initcall. This could be arch_initcall, but
considering you depend on the parent this must be module driver.

Even from this dependency point of view your initcalls are totally wrong
and will lead to issues.

Best regards,
Krzysztof