Received-SPF: pass (google.com: domain of linux-kernel+bounces-83721-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 27 Feb 2024 19:27:08 +0200
From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: =?iso-8859-1?Q?Th=E9o?= Lebrun <theo.lebrun@bootlin.com>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Linus Walleij <linus.walleij@linaro.org>,
	=?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>,
	linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Tawfik Bayouk <tawfik.bayouk@mobileye.com>,
	linux-gpio@vger.kernel.org
Subject: Re: [PATCH v8 04/10] reset: eyeq5: add platform driver
Message-ID: <Zd4bbCsY54XEnvJM@smile.fi.intel.com>
References: <20240227-mbly-clk-v8-0-c57fbda7664a@bootlin.com>
 <20240227-mbly-clk-v8-4-c57fbda7664a@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240227-mbly-clk-v8-4-c57fbda7664a@bootlin.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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