[v8,05/10] pinctrl: eyeq5: add platform driver

  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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/pinctrl/Kconfig         |  15 ++
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-eyeq5.c | 577 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 593 insertions(+)
  

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

Hello,

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:
> > 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?

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.

>
> > +	depends on OF
>
> It's even not needed for this software as far as I can tell from the code.

Indeed looks like it. Will try that out and remove the dependency if it
works as expected.

[...]

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

Here is my new list:

#include <linux/array_size.h>
#include <linux/bits.h>
#include <linux/bug.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/io.h>
#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/types.h>

#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>

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

[...]

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

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.

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

Yes, will do so.

>
> ...
>
> > +	if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
>
> What's wrong with positive conditional?

Nothing. In my mind GPIO was first, other was second. Will change.

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

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.

[...]

> > +	mask = BIT(offset);
> > +	val = is_gpio ? 0 : U32_MAX;
>
> I think you meant something else (semantically) than U32_MAX.
> Perhaps GENMASK(31, 0)?

To me the semantic of U32_MAX is the same. I see where you are coming
from. A better alternative however would be:

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

That way the desire is clear and the code is simpler.

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

No reason indeed. Will remove the assignment.

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

I'm unsure what helpers you are talking about?

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.

[...]

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

Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.

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

So GENMASK(1, 0) rather than 0b11. Or GENMASK(offset+1, offset).

Something else?

>
> > +	return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id eq5p_match[] = {
> > +	{ .compatible = "mobileye,eyeq5-pinctrl" },
> > +	{},
>
> No comma in the terminator entry.
>
> > +};
>
> No MODULE_DEVICE_TABLE()?

It is an oversight. Will be added.

Thanks for the review Andy.

Have a nice day,

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

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.

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

[...]

> > > +#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.
> 
> Here is my new list:
> 
> #include <linux/array_size.h>
> #include <linux/bits.h>
> #include <linux/bug.h>
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> #include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> 
> #include <linux/pinctrl/pinconf-generic.h>
> #include <linux/pinctrl/pinconf.h>
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/pinctrl/pinmux.h>
> 
> #include "core.h"
> #include "pinctrl-utils.h"

Thanks, looks much better to me!

[...]

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

..

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

[...]

> > > +	mask = BIT(offset);
> > > +	val = is_gpio ? 0 : U32_MAX;
> >
> > I think you meant something else (semantically) than U32_MAX.
> > Perhaps GENMASK(31, 0)?
> 
> To me the semantic of U32_MAX is the same.

Not at all. When we speak hardware we speak bits, bitfields, etc. We almost
never speaks software types and their limits (u32 is a pure software concept).

> I see where you are coming
> from. A better alternative however would be:
> 
> 	mask = BIT(offset);
> 	val = is_gpio ? 0 : mask;
> 
> That way the desire is clear and the code is simpler.

Yes, please follow the latter, much better than integer limits.

..

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

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

..

> > Similar comments as per previous function.
> 
> So GENMASK(1, 0) rather than 0b11. Or GENMASK(offset+1, offset).

Definitely not the latter one, it will generate awful code.

> Something else?

if it was another comment, I don't see in the context here...
  

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
  

On Thu, Feb 29, 2024 at 04:13:15PM +0100, Théo Lebrun wrote:
> 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.

The problem is that you reduce the flexibility. Nobody prevents you from having
it built-in while tristate. But completely different situation when it's bool.

So my argument still stays. I think new code shouldn't be boolean by default.
The only exceptional cases can do that (like PMIC driver or critical clock one).

[...]

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

Perfect is enemy of good. ;)

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

Yes, but we usually don't put a dead code into the kernel. So, can you confirm
that warning can appear IRL? If yes, there is another red flag or question:
why WARN()? This is easily becomes a panic and/or reboot (depending to the kernel
command line) and hence may give unresponsive system. Was this considered?

..

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

Indeed, I mixed it with something else (probably GPIO library and one of its
core structures) where it's the case.
  

Hi folks,

lots of discussion here, lazy Reviewed-by from me, but Andy often (thank God!)
catches things I just miss.

On Thu, Feb 29, 2024 at 4:32 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:

> > > 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.
>
> The problem is that you reduce the flexibility. Nobody prevents you from having
> it built-in while tristate. But completely different situation when it's bool.
>
> So my argument still stays. I think new code shouldn't be boolean by default.
> The only exceptional cases can do that (like PMIC driver or critical clock one).

I think bool is helpful for users if:

- The system cannot boot without the pin control driver

- The system cannot mount root from a storage medium without the pin control
  driver. Initramfs doesn't count for embedded systems, many of these use things
  like OpenWrt that does not use initramfs the way Debian or Fedora etc does.

This SoC is obviously for the deeply embedded usecase. If this SoC has root
on flash or eMMC and cannot access either without pin control, it is helpful
for users to have this as bool so they don't shoot themselves in the foot with
Kconfig.

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

I think I asked for this code in my review, because I felt unsafe about offset.

Maybe it's not such a big problem, but, this twoliner is also not a big deal.

Yours,
Linus Walleij