Re: [PATCH v8 04/10] reset: eyeq5: add platform driver

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 03:55:25PM +0100, 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.

..

The inclusion block is a semi-random. Please, follow IWYU principle.

+ array_size.h
+ bits.h
+ bug.h
+ container_of.h

> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

+ device.h
+ err.h
+ io.h
+ lockdep.h

+ mod_devicetable.h

> +#include <linux/mutex.h>

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>

Are all of them in use?

> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>

+ types.h

..

> +/* Vendor-provided timeout values. D1 has a long timeout because of LBIST. */
> +#define D0_TIMEOUT_POLL			10
> +#define D1_TIMEOUT_POLL			40000

We use units as suffixes. The above doesn't tell magnitude of timeouts.

Also constants like USEC_PER_MSEC are useful to make code robust.

..

> +	unsigned int val, mask;
> +	int i;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> +			val = readl(priv->base_d0 + D0_SARCR1);
> +			val = !(val & BIT(offset));
> +			if (val == assert)
> +				return 0;
> +			udelay(1);
> +		}
> +		break;

See what iopoll.h gives you.

> +	case 1:
> +		mask = assert ? D1_ACRP_ST_POWER_DOWN : D1_ACRP_ST_ACTIVE;
> +		for (i = 0; i < D1_TIMEOUT_POLL; i++) {
> +			val = readl(priv->base_d1 + 4 * offset);
> +			if (val & mask)
> +				return 0;
> +			udelay(1);
> +		}

Ditto.

> +		break;
> +	case 2:
> +		return 0; /* No busy waiting for domain 2. */
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}

> +	dev_dbg(dev, "%u-%u: timeout\n", domain, offset);

> +	return -ETIMEDOUT;

..

> +static void eq5r_assert_withlock(struct eq5r_private *priv, u32 domain,
> +				 u32 offset)
> +{
> +	void __iomem *reg;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		reg = priv->base_d0 + D0_SARCR0;
> +		writel(readl(reg) & ~BIT(offset), reg);
> +		break;
> +	case 1:
> +		reg = priv->base_d1 + 4 * offset;
> +		writel(readl(reg) | D1_ACRP_PD_REQ, reg);
> +		break;
> +	case 2:
> +		reg = priv->base_d2;
> +		writel(readl(reg) & ~BIT(offset), reg);
> +		break;
> +	default:
> +		WARN_ON(1);

break;

> +	}
> +}
> +
> +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +	eq5r_assert_withlock(priv, domain, offset);
> +	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, true);
> +}
> +
> +static void eq5r_deassert_withlock(struct eq5r_private *priv, u32 domain,
> +				   u32 offset)
> +{
> +	void __iomem *reg;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		reg = priv->base_d0 + D0_SARCR0;
> +		writel(readl(reg) | BIT(offset), reg);
> +		break;
> +	case 1:
> +		reg = priv->base_d1 + 4 * offset;
> +		writel(readl(reg) & ~D1_ACRP_PD_REQ, reg);
> +		break;
> +	case 2:
> +		reg = priv->base_d2;
> +		writel(readl(reg) | BIT(offset), reg);
> +		break;
> +	default:
> +		WARN_ON(1);

break;

> +	}
> +}

..

> +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;

Perhaps

	u32 offset = (id & GENMASK(7, 0)) >> 0;
	u32 domain = (id & GENMASK(31, 8)) >> 8;

for better understanding the split?

> +	dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +	eq5r_deassert_withlock(priv, domain, offset);
> +	return eq5r_busy_wait_withlock(priv, rcdev->dev, domain, offset, false);

withlock is not usual naming pattern, "locked" is shorter and widely used.

> +}
> +
> +static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = rcdev_to_priv(rcdev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;

Ditto.

> +	u32 val;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
> +
> +	guard(mutex)(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		val = readl(priv->base_d0 + D0_SARCR1);
> +		return !(val & BIT(offset));
> +	case 1:
> +		val = readl(priv->base_d1 + 4 * offset);
> +		return !(val & D1_ACRP_ST_ACTIVE);
> +	case 2:
> +		val = readl(priv->base_d2);
> +		return !(val & BIT(offset));
> +	default:
> +		return -EINVAL;
> +	}
> +}

..

> +	struct eq5r_private *priv;
> +	int ret, i;

Why is i signed?

> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base_d0 = devm_platform_ioremap_resource_byname(pdev, "d0");
> +	if (IS_ERR(priv->base_d0))
> +		return PTR_ERR(priv->base_d0);
> +
> +	priv->base_d1 = devm_platform_ioremap_resource_byname(pdev, "d1");
> +	if (IS_ERR(priv->base_d1))
> +		return PTR_ERR(priv->base_d1);
> +
> +	priv->base_d2 = devm_platform_ioremap_resource_byname(pdev, "d2");
> +	if (IS_ERR(priv->base_d2))
> +		return PTR_ERR(priv->base_d2);
> +
> +	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;

It's better to use device_set_node().

> +	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]);

Please, use corresponding hweightXX() API.

> +	ret = devm_reset_controller_register(dev, &priv->rcdev);
> +	if (ret) {

> +		dev_err(dev, "Failed registering reset controller: %d\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	return 0;
> +}

..

> +static struct platform_driver eq5r_driver = {
> +	.probe = eq5r_probe,
> +	.driver = {
> +		.name = "eyeq5-reset",
> +		.of_match_table = eq5r_match_table,
> +	},
> +};

> +

Unneeded blank line.

> +builtin_platform_driver(eq5r_driver);

-- 
With Best Regards,
Andy Shevchenko