Message-Id: <CZHNS29NK9RR.13G019Y9ZY6IO@bootlin.com>
Date: Thu, 29 Feb 2024 16:13:15 +0100
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Andy Shevchenko" <andriy.shevchenko@...el.com>
Cc: "Gregory CLEMENT" <gregory.clement@...tlin.com>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>, "Rob Herring"
 <robh+dt@...nel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@...aro.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>, "Linus Walleij"
 <linus.walleij@...aro.org>, Rafał Miłecki
 <rafal@...ecki.pl>, "Philipp Zabel" <p.zabel@...gutronix.de>, "Vladimir
 Kondratiev" <vladimir.kondratiev@...ileye.com>,
 <linux-mips@...r.kernel.org>, <linux-clk@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>, <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v8 05/10] pinctrl: eyeq5: add platform driver

Hello,

On Thu Feb 29, 2024 at 12:35 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 28, 2024 at 07:15:12PM +0100, Théo Lebrun wrote:
> > On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:26PM +0100, Théo Lebrun wrote:
>
> ...
>
> > > > +	bool "Mobileye EyeQ5 pinctrl driver"
> > >
> > > Can't be a module?
> > 
> > It theory it could, I however do not see why that would be done. Pinctrl
> > is essential to the platform capabilities. The platform is an embedded
> > one and performance-oriented; boot-time is important and no user will
> > ever want to load pinctrl as a module.
>
> I can argue. The modularization can give a better granularity in the exactly
> embedded world when the memory resource (flash/RAM) is limited or fragmented
> (for one or another reason). Having less weighty kernel at boot makes it smaller
> to fit, for example, faster read only memory block which is not so uncommon.

I can argue back. :-) Granularity brought from modules is useful either
in (1) resource constrained boot context or (2) for peripherals which
some people might want to do without. We are not in case 1 nor case 2.

> The rule of thumb is to make modules if, otherwise, it's not so critical for
> the boot process (and even for some cases we still may have it done as a module
> with help of deferred probe mechanism).

I'd call SoC pin control a critical resource for the boot process.

I also like the simplicity of builtin better for such a resource.
 - If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
   context that we have no reason to support).
 - If we do not allow it and there is a bug, there is no bug.
   Plus, it makes one less choice for people configuring the kernel.

[...]

> > > > +	if (WARN_ON(offset > 31))
> > > > +		return false;
> > >
> > > When this condition can be true?
> > 
> > If there is a bug in the code. Defensive programming.
> > 
> > There is this subtle conversion of pin numbers => offset inside of a
> > bank. If one function forgets doing this then eq5p_test_bit() gets
> > called with a pin number.
> > 
> > In this GPIO series I fixed such a bug in a 10 year old driver:
> > https://lore.kernel.org/lkml/20240228-mbly-gpio-v2-5-3ba757474006@bootlin.com/
> > 
> > The whole "if it can happen it will happen" mantra. We'll get a warning
> > in the logs using pinctrl-eyeq5.
>
> My point here that we have mechanisms to avoid such issues, for example in GPIO
> we have valid_mask field and GPIO library takes care to avoid such conditions
> from happening. Please, double check that you really need these in your driver.
> I prefer to avoid them until it's proven that they are real cases.

Whatever the subsystem does to protect us (like only calling callbacks
with valid IDs), it will not protect us from bugs inside the driver's
callbacks.

I do no see a reason to avoid such code. I do not trust myself to write
perfect code. Its aim is to protect ourselves from our own mistakes. If
such an issue occurs, understanding that this is what happened would be
really hard (especially if it occurs on someone else's boards).

> ...
>
> > > > +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?
> > 
> > Doesn't look like it is. In drivers/pinctrl/devicetree.c:
> > 
> > 	static int dt_to_map_one_config(struct pinctrl *p,
> > 					struct pinctrl_dev *hog_pctldev,
> > 					const char *statename,
> > 					struct device_node *np_config)
> > 	{
> > 		// ...
> > 
> > 		/*
> > 		 * Call pinctrl driver to parse device tree node, and
> > 		 * generate mapping table entries
> > 		 */
> > 		ops = pctldev->desc->pctlops;
> > 		if (!ops->dt_node_to_map) {
> > 			dev_err(p->dev, "pctldev %s doesn't support DT\n",
> > 				dev_name(pctldev->dev));
> > 			return -ENODEV;
> > 		}
> > 
> > 		// ...
> > 	}
> > 
> > And I see nowhere that puts a value if ->dt_node_to_map is empty.
> > 
> > For dt_free_map, it is an optional value. If the field is NULL nothing
> > is done. See dt_free_map() in the same file.
>
> If we drop OF dependency, these fields might not be present in the structure
> (by definition). Compilation won't succeed. Am I mistaken?

struct pinctrl_ops has both ->dt_node_to_map and ->dt_free_map fields in
any case. See include/linux/pinctrl/pinctrl.h which declares the
struct. The function pointers we put are also under no conditional
compilation.

[...]

> > > > +	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?
> > 
> > I'm unsure what helpers you are talking about?
>
> Which give you the MMIO addresses.

Again sorry, but I don't get the question. I see no helper function that
returns an MMIO address in eq5p_pinconf_get(). Two helpers exist to
deal with memory accesses: eq5p_test_bit() and eq5p_update_bits().
Neither are called in this function nor could they be used.

> > If the question is about why multiply before if-condition: I feel like
> > multiplying first allows having the if condition be "offset >= 32".
> > That explicits why we readl HIGH vs LOW regs.
>
> [...]
>
> > > > +	if (arg > 3) {
> > >
> > > Magic number.
> > 
> > Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.
>
> No, the
>
> #define FOO_SELF_EXPLAING	GENMASK(1, 0) // or 3 or 0b11
>
> will.

Will do!

Thanks Andy,

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

Powered by blists - more mailing lists