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

[Théo Lebrun <theo.lebrun@xxxxxxxxxxx>]

Hi,

On Wed Jan 24, 2024 at 11:54 AM CET, Philipp Zabel wrote:
> On Di, 2024-01-23 at 19:46 +0100, Théo Lebrun wrote:
> [...]
> > diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
> > new file mode 100644
> > index 000000000000..2217e42e140b
> > --- /dev/null
> > +++ b/drivers/reset/reset-eyeq5.c
> > @@ -0,0 +1,383 @@
> [...]
>
> > +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
>
> rcdev is contained in priv, you can just use container_of instead of
> chasing pointers around.

That's right. Fixed with this local macro:

#define rcdev_to_priv(rcdev) container_of(rcdev, struct eq5r_private, rcdev)

> > +	u32 offset = id & GENMASK(7, 0);
> > +	u32 domain = id >> 8;
> > +	int ret;
> > +
> > +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> > +		return -EINVAL;
>
> Reset controls with domain >= EQ5R_DOMAIN_COUNT are already weeded out
> during request by of_xlate, so this check is not necessary.

It was some defensive programming. I've removed this precautionary
condition from the places it appeared.

>
> > +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> > +
> > +	mutex_lock(&priv->mutexes[domain]);
> > +	_eq5r_assert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true);
> > +	mutex_unlock(&priv->mutexes[domain]);
> > +
> > +	return ret;
>
> Consider using guard(mutex)(&priv->mutexes[domain]) from
> linux/cleanup.h to automatically unlock on return.

Done. I had never used that __cleanup attr feature. It simplifies
returns.

>
> [...]
> > +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id)
>
> Is this used by anything? If unused, I'd prefer this not to be
> implemented. If it is used, is no delay required between assert and
> deassert by any consumer?

Not really, it follows what is done in the downstream vendor kernel.
I've had a quick look in this kernel and I don't see any consumer of
the API. For the moment I'll remove it.

>
> > +{
> > +	struct device *dev = rcdev->dev;
> > +	struct eq5r_private *priv = dev_get_drvdata(dev);
> > +	u32 offset = id & GENMASK(7, 0);
> > +	u32 domain = id >> 8;
> > +	int ret;
> > +
> > +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> > +		return -EINVAL;
> > +
> > +	dev_dbg(dev, "%u-%u: reset request\n", domain, offset);
> > +
> > +	mutex_lock(&priv->mutexes[domain]);
> > +
> > +	_eq5r_assert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, dev, domain, offset, true);
> > +	if (ret) /* don't let an error disappear silently */
> > +		dev_warn(dev, "%u-%u: reset assert failed: %d\n",
> > +			 domain, offset, ret);
>
> Why not return the error though?

The goal was to still run through the deassert even if the assert
returned an error. Goal was to address potential edge case of assert
returning an error but still taking place, in which case we want to try
to deassert to put the peripheral in a de-asserted state (as before the
call).

Not a concern anymore as the function is being removed.

>
> > +	_eq5r_deassert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, dev, domain, offset, false);
> > +
> > +	mutex_unlock(&priv->mutexes[domain]);
> > +
> > +	return ret;
> > +}
> [...]
> > +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);
>
> Using devm_kzalloc() avoids leaking this on error return or driver
> unbind.

Done, thanks.

>
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	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))
> > +		return PTR_ERR(priv->olb);
> > +
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> > +		mutex_init(&priv->mutexes[i]);
> > +
> > +	priv->rcdev.ops = &eq5r_ops;
> > +	priv->rcdev.owner = THIS_MODULE;
> > +	priv->rcdev.dev = dev;
> > +	priv->rcdev.of_node = np;
> > +	priv->rcdev.of_reset_n_cells = 2;
> > +	priv->rcdev.of_xlate = eq5r_of_xlate;
> > +
> > +	priv->rcdev.nr_resets = 0;
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> > +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> > +
> > +	ret = reset_controller_register(&priv->rcdev);
>
> Similarly, use devm_reset_controller_register() or disable driver
> unbind with suppress_bind_attrs.

Switched to the devres version, thanks.


Thanks Philipp,

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