Hello, On Wed Jan 24, 2024 at 8:03 AM CET, Krzysztof Kozlowski wrote: > 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. Clearly and I fully agree. It's a versioning issue on my side. It's been fixed (again, oops). > > > + > > + 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. Same thing, it must have been lost in the same fixup patch as the previous mistake. [...] > > +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. Same as reset: moved to using the builtin_platform_driver() macro. No need for it to be module as it cannot be one. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com