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

[Théo Lebrun]

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@xxxxxxxxxxx/

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