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
