Re: [PATCH v2] mux: convert to use fwnode interface

[Alvin Šipraga]

Hi Fabio,

On Sat, Sep 26, 2026 at 12:00:03PM +0200, Fabio wrote:
> > > @@ -566,39 +569,40 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> > >   	}
> > >   	if (state)
> > > -		ret = of_parse_phandle_with_args(np,
> > > -						 "mux-states", "#mux-state-cells",
> > > -						 index, &args);
> > > +		ret = fwnode_property_get_reference_args(fwnode, "mux-states",
> > > +							 "#mux-state-cells", 0,
> > > +							 index, &args);
> > >   	else
> > > -		ret = of_parse_phandle_with_args(np,
> > > -						 "mux-controls", "#mux-control-cells",
> > > -						 index, &args);
> > > +		ret = fwnode_property_get_reference_args(fwnode,
> > > +							 "mux-controls", "#mux-control-cells",
> > > +							 0, index, &args);
> > > +
> > >   	if (ret) {
> > >   		if (optional && ret == -ENOENT)
> > >   			return NULL;
> > > -		dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
> > > -			np, state ? "state" : "control",
> > > -			mux_name ?: "", index);
> > > +		dev_err(dev, "%pfw: failed to get mux-%s %s(%i)\n",
> > > +			fwnode, state ? "state" : "control", mux_name ?: "",
> > > +			index);
> > >   		return ERR_PTR(ret);
> > >   	}
> > > -	mux_chip = of_find_mux_chip_by_node(args.np);
> > > -	of_node_put(args.np);
> > > +	mux_chip = mux_chip_find_by_fwnode(args.fwnode);
> > > +	fwnode_handle_put(args.fwnode);
> > 
> > I'm actually not sure why there was an of_node_put() here in the first
> > place. It seems wrong to me. Any idea what it's for?
> > 
> > I want to give a Reviewed-by here for the rest of the code, but if the
> > above _put() erroneous, it might be better to add a patch before this
> > one to remove it (and cc the stable list with that patch).
> 
> I don't think it's wrong, because fwnode_property_get_reference_args() documents that:
> 
>     The caller is responsible for calling fwnode_handle_put() on the returned
>     @args->fwnode pointer.
> 
> What is wrong though, is passing args.fwnode to the dev_err() calls below after it was freed.
> Or that's what I'm inclined to say; I doubt put() would free everything but fwnode's name.
> If I had to guess, fwnode_handle_put is (erroneously) where it is to avoid gotos/calling mux_chip_find_by_fwnode()
> in the sad path.

Yeah, I think you're right, my bad!

> If you agree with the above, I'll be happy to write a patch to fix it.

That would be great, I will give it a review when you send. Thank you!

Kind regards,
Alvin