Received-SPF: pass (google.com: domain of linux-kernel+bounces-83806-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 27 Feb 2024 20:14:57 +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 05/10] pinctrl: eyeq5: add platform driver
Message-ID: <Zd4moVd_-bY6Z_kL@smile.fi.intel.com>
References: <20240227-mbly-clk-v8-0-c57fbda7664a@bootlin.com>
 <20240227-mbly-clk-v8-5-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-5-c57fbda7664a@bootlin.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Feb 27, 2024 at 03:55:26PM +0100, Th?o Lebrun wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> support of other platforms from Mobileye. It belongs to a syscon region
> called OLB.
> 
> Existing pins and their function live statically in the driver code
> rather than in the devicetree, see compatible match data.

..

> +config PINCTRL_EYEQ5
> +	bool "Mobileye EyeQ5 pinctrl driver"

Can't be a module?

> +	depends on OF

It's even not needed for this software as far as I can tell from the code.

> +	depends on MACH_EYEQ5 || COMPILE_TEST
> +	select PINMUX
> +	select GENERIC_PINCONF
> +	select MFD_SYSCON
> +	default MACH_EYEQ5
> +	help
> +	  Pin controller driver for the Mobileye EyeQ5 platform. It does both
> +	  pin config & pin muxing. It does not handle GPIO.
> +
> +	  Pin muxing supports two functions for each pin: first is GPIO, second
> +	  is pin-dependent. Pin config is about bias & drive strength.

..

> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>

Semi-random list of the inclusions. Please, fix it.
While doing that, group out pinctrl/* ones as it's done in other drivers.

> +#include "core.h"
> +#include "pinctrl-utils.h"

..

> +struct eq5p_function {
> +	const char		*name;
> +	const char * const	*groups;
> +	unsigned int		ngroups;
> +};

We have struct pinfunction, use it instead.

..

> +static const char * const gpio_groups[] = {
> +	/* Bank A */
> +	"PA0", "PA1", "PA2", "PA3", "PA4", "PA5", "PA6", "PA7", "PA8", "PA9",
> +	"PA10", "PA11", "PA12", "PA13", "PA14", "PA15", "PA16", "PA17", "PA18",
> +	"PA19", "PA20", "PA21", "PA22", "PA23", "PA24", "PA25", "PA26", "PA27",
> +	"PA28",

For all arrays like this, please split them on 4/8/10/16 items per line as it's
much easier to count and refer by index when reading the code.

> +	/* Bank B */
> +	"PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
> +	"PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
> +	"PB19", "PB20", "PB21", "PB22",
> +};

..

> +#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) }

Use PINCTRL_PINFUNCTION() instead.

..

> +static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
> +			  enum eq5p_bank bank, enum eq5p_regs reg, int offset)
> +{
> +	u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);

> +	if (WARN_ON(offset > 31))
> +		return false;

When this condition can be true?

> +	return (val & BIT(offset)) != 0;
> +}

..

> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *config);

Can't you avoid forward declarations?

..

> +	if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {

What's wrong with positive conditional?


> +	} else {

> +	}

..

> +static const struct pinctrl_ops eq5p_pinctrl_ops = {
> +	.get_groups_count	= eq5p_pinctrl_get_groups_count,
> +	.get_group_name		= eq5p_pinctrl_get_group_name,
> +	.get_group_pins		= eq5p_pinctrl_get_group_pins,
> +	.pin_dbg_show		= eq5p_pinctrl_pin_dbg_show,

> +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_pin,
> +	.dt_free_map		= pinctrl_utils_free_map,

ifdef is missing for these... But the question is, isn't these a default when
OF is in use?

> +};

..

> +	dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
> +		group_name);

Drop __func__ from all debug messages. With Dynamic Debug enabled (which is
often the case) we can do it at run-time).

..

> +	mask = BIT(offset);
> +	val = is_gpio ? 0 : U32_MAX;

I think you meant something else (semantically) than U32_MAX.
Perhaps GENMASK(31, 0)?

..

> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *config)
> +{
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int offset = eq5p_pin_to_offset(pin);
> +	enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> +	u32 val_ds, arg = 0;

What's arg assignment for?

> +	bool pd, pu;
> +
> +	pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
> +	pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		arg = !(pd || pu);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		arg = pd;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = pu;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		offset *= 2; /* two bits per pin */
> +		if (offset >= 32) {
> +			val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> +			offset -= 32;
> +		} else {
> +			val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> +		}

I'm wondering why you can't use your helpers before multiplication?

> +		arg = (val_ds >> offset) & 0b11;

GENMASK(1, 0)

> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +	return 0;
> +}

..

> +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> +					   unsigned int pin, u32 arg)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int offset = eq5p_pin_to_offset(pin);
> +	enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> +	unsigned int reg;
> +	u32 mask, val;
> +
> +	if (arg > 3) {

Magic number.

> +		dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> +		return -EINVAL;
> +	}
> +
> +	offset *= 2; /* two bits per pin */
> +
> +	if (offset >= 32) {
> +		reg = EQ5P_DS_HIGH;
> +		offset -= 32;
> +	} else {
> +		reg = EQ5P_DS_LOW;
> +	}

> +	mask = 0b11 << offset;
> +	val = arg << offset;
> +	eq5p_update_bits(pctrl, bank, reg, mask, val);

Similar comments as per previous function.

> +	return 0;
> +}

..

> +static const struct of_device_id eq5p_match[] = {
> +	{ .compatible = "mobileye,eyeq5-pinctrl" },
> +	{},

No comma in the terminator entry.

> +};

No MODULE_DEVICE_TABLE()?

> +static struct platform_driver eq5p_driver = {
> +	.driver = {
> +		.name = "eyeq5-pinctrl",
> +		.of_match_table = eq5p_match,
> +	},
> +	.probe = eq5p_probe,
> +};

> +

Unneeded blank line.

> +builtin_platform_driver(eq5p_driver);

-- 
With Best Regards,
Andy Shevchenko