Re: [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > Here the second chip select is native. However, when using swnodes
> 
> Here, the
> 

Can do.

> > +config GPIO_SWNODE_UNDEFINED
> > +       bool
> 
> But why did you remove the help? Please, put it back.
> 

Sorry didn't realise you still wanted it will pop it back.

> ...
> 
> > +       /*
> > +        * Check for special node that identifies undefined GPIOs, this is
> 
> for a special

Can do.

> 
> > +        * primarily used as a key for internal chip selects in SPI bindings.
> > +        */
> > +       if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
> > +               return ERR_PTR(-ENOENT);
> 
> This is a dead code when the respective config option is not selected.
> Or actually a potential flaw if somebody else names their swnode like
> this.
> 

Can add a check for the config.

> ...
> 
> > +       ret = software_node_register(&swnode_gpio_undefined);
> > +       if (ret < 0)
> > +               pr_err("gpiolib: failed to register swnode: %d\n", ret);
> 
> Instead of this prefix, define pr_fmt()
> 

Little iffy on this, there are other prints in gpiolib that do it
this way as well, I guess I could add a patch to convert
everything but its starting to get a little out of the thrust of
what I am doing here.

> > +       return ret;
> 
> ...
> 
> > --- a/include/linux/gpio/consumer.h
> > +++ b/include/linux/gpio/consumer.h
> 
> Why? I already said that the best place is gpio/property.h as it's
> exactly for swnode related code.
> 

Oh I see that's what you meant by those comments, as per my email
I wasn't exactly sure. Will move them I don't mind where they go.

Thanks,
Charles