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@xxxxxxxxxxx/ > > > > > > 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. -- With Best Regards, Andy Shevchenko
