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

[Philipp Zabel <p.zabel@pengutronix.de>]

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.

> +	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.

> +	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.

[...]
> +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?

> +{
> +	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?

> +	_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.

> +	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.

regards
Philipp