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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 24 Jan 2024 08:03:52 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 Gregory CLEMENT <gregory.clement@...tlin.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Linus Walleij <linus.walleij@...aro.org>, Rafał Miłecki
 <rafal@...ecki.pl>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
 linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Tawfik Bayouk <tawfik.bayouk@...ileye.com>, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

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