Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 8:00 AM CET, Krzysztof Kozlowski wrote:
> On 23/01/2024 19:46, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
> > region called OLB. It might grow to add later support of other
> > platforms from Mobileye.

[...]

> > +static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev,
> > +			   u32 domain, u32 offset, bool assert)
> > +{
> > +	unsigned int val, mask;
> > +	int i;
> > +
> > +	lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > +	switch (domain) {
> > +	case 0:
> > +		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> > +			regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
> > +			val = !(val & BIT(offset));
> > +			if (val == assert)
> > +				return 0;
> > +			__udelay(1);
>
> What is even "__udelay"? It is the first use in drivers. Please use
> common methods, like fsleep or udelay... but actually you should rather
> use regmap_read_poll_timeout() or some variants instead of open-coding it.

udelay is an alias to __udelay on MIPS, which is why this didn't look
odd to me. Fixed.

[...]

> > +static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset)
>
> Drop leading _ and name the function in some informative way.

Fixed by turning `_eq5r_assert` into `eq5r_assert_withlock`, and co.

[...]

> > +
> > +static int eq5r_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);
> > +	struct eq5r_private *priv;
> > +	int ret, i;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
>
> You leak parent.

Fixed in all three clk+reset+pinctrl drivers. They all had this issue.

>
> > +
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	priv->olb = ERR_PTR(-ENODEV);
> > +	if (parent_np) {
> > +		priv->olb = syscon_node_to_regmap(parent_np);
> > +		of_node_put(parent_np);
> > +	}
> > +	if (IS_ERR(priv->olb))
>
> Also here
>
> > +		return PTR_ERR(priv->olb);
>
> This looks over-complicated. First, you cannot just
> dev_get_regmap(pdev->dev.parent)?

No dev_get_regmap() cannot be used as it doesn't pick up syscon regmaps.
I've just tried it.

However I've simplified the logic, it looks better now.

	static int eq5r_probe(struct platform_device *pdev)
	{
		struct device_node *parent_np;

		/* ... */

		parent_np = of_get_parent(np);
		if (!parent_np)
			return -ENODEV;

		priv->olb = syscon_node_to_regmap(parent_np);
		of_node_put(parent_np);
		if (IS_ERR(priv->olb))
			return PTR_ERR(priv->olb);

		/* ... */
	}

[...]

> > +static struct platform_driver eq5r_driver = {
> > +	.probe = eq5r_probe,
> > +	.driver = {
> > +		.name = "eyeq5-reset",
> > +		.of_match_table = eq5r_match_table,
> > +	},
> > +};
> > +
> > +static int __init eq5r_init(void)
> > +{
> > +	return platform_driver_register(&eq5r_driver);
> > +}
> > +
> > +arch_initcall(eq5r_init);
>
> This is does not look like arch code, but driver or subsys. Use regular
> module_driver. I see there is such pattern in reset but I doubt this is
> something good.

Indeed I've moved things to using the builtin_platform_driver() macro.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com