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

[Andy Shevchenko <andriy.shevchenko@intel.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...

-- 
With Best Regards,
Andy Shevchenko