Re: [PATCH v3 3/7] pinctrl: pinctrl-generic: add __pinctrl_generic_pins_function_dt_node_to_map()

[Frank Li]

On Wed, Mar 25, 2026 at 10:33:05AM +0000, Conor Dooley wrote:
> On Tue, Mar 24, 2026 at 04:16:10PM -0400, Frank Li wrote:
> > On Fri, Mar 20, 2026 at 09:54:45AM -0400, Frank Li wrote:
> > > On Fri, Mar 20, 2026 at 02:27:21PM +0100, Linus Walleij wrote:
> > > > On Thu, Mar 19, 2026 at 12:04 AM Frank Li <Frank.li@xxxxxxx> wrote:
> > > > > On Mon, Mar 16, 2026 at 10:37:28AM +0100, Linus Walleij wrote:
> > > >
> > > > > > That said: in this case you're just adding a parameter, just add
> > > > > > the parameter and change all of the in-tree users to pass false
> > > > > > or whatever you need, these is just one (1) in-tree user anyway.
> > > > >
> > > > > pinctrl_generic_pins_function_dt_node_to_map() directly feed to
> > > > > .dt_node_to_map() callback, add parameter will impact too much.
> > > >
> > > > Why do you say that. It already has many parameters, one more
> > > > or less doesn't matter. It's not like this call is performance-critical.
> > > > Just change the users.
> > >
> > > In only user drivers/pinctrl/microchip/pinctrl-mpfs-mssio.c,
> > > 	.dt_node_to_map = pinctrl_generic_pins_function_dt_node_to_map;
> > >
> > > pinctrl_generic_pins_function_dt_node_to_map() need match .dt_node_to_map()'s
> > > declear.
> > >
> > > So it can't direct add two parameters in pinctrl_generic_pins_function_dt_node_to_map()
> > > Need simple wrap function, which other in pinctrl-mpfs-mssio.c or in
> > > pinconf.h.
> > >
> > > If add two parameter in .dt_node_to_map(), need change all functions, which
> > > .dt_node_to_map = xxx_to_map(). and OF core part.
> >
> > Linus Walleij:
> > 	Is my explain clear enough? I am preparing respin it?
> >
> > 	is okay use wrap function
> > 	pinctrl_generic_pins_function_dt_node_to_map_ext()?
>
> I don't understand this patch. The function is called
> pinctrl_generic_pins_function_dt_node_to_map(). You have no pins.
> You're adding a parameter to make a function with *pins* in its name not
> use pins. The new function doesn't use pins but has pins in the name.
> At the very least function names should not be misleading.
>
> I was going to suggest pulling out the relevant portions and creating
> some helpers that could be used by multiple different-but-similar
> functions, but I don't actually even think that there's much in common.
> Most damningly I think, you don't actually read either the functions or
> pins properties at all and neither are permitted by your binding.
> So turns out you use neither pins or functions...
>
> You don't actually have any of these properties which runs counter to the
> goal of the function, which is parsing. With this in mind, it feels to me
> like you're trying way too hard to make use of a generic function when the
> right thing to do is probably just have an entirely custom function.
> Maybe that's a custom implementation in your driver, or a new function
> here, but I think writing that will highlight just how little of the
> code would be shared between the existing function and what your
> use-case needs: no pin configuration stuff, no reading of the devicetree
> other than the node names and no dealing with the label pointing to the
> "wrong" place.
>
> I recently bought a spacemit k1 board to go and write a sister function
> to pinctrl_generic_pins_function_dt_node_to_map() that deals with pins
> and groups (because that's a pretty common pattern).
> I would be calling that pinctrl_generic_pinmux_dt_node_to_map(),
> because it that's the property it deals with. I have honestly got no
> idea what to call one for this situation since you don't have any of the
> properties in pinmux-node.yaml. Maybe that's a sign.

At v2, I implemented customize dt_node_to_map(), Linus Walleij think it is
too similar with pinctrl_generic_pins_function_dt_node_to_map(), so ask me
to enhanance and reuse pinctrl_generic_pins_function_dt_node_to_map().

Frank
>
> Cheers,
> Conor.