Re: [PATCH v3 03/10] gpiolib: implement low-level, shared GPIO support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Oct 29, 2025 at 04:57:02PM +0100, Bartosz Golaszewski wrote:
> On Wed, Oct 29, 2025 at 4:19 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Oct 29, 2025 at 01:39:34PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Oct 29, 2025 at 12:45 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Wed, Oct 29, 2025 at 12:20:39PM +0100, Bartosz Golaszewski wrote:

...

> > > > Besides strcmp_suffix() that already exists in OF core, there are also some
> > > > existing pieces that seems being repeated here (again). Can we reduce amount
> > > > of duplication?
> > >
> > > I'm afraid you need to be more specific here.
> >
> > You can simply browse the file, it's not long to find and think about it.
> > I'm _thinking_ that it's possible to improve the situation overall by
> > try our best of deduplicating (or rather not duplicating) things.
> 
> Sorry, but this is not how reviewing works. You can't just say: "I
> think this can be improved, go figure out what can and fix it, you can
> browse this file for reference". You need to specifically point out
> issues in code and propose alternatives.

Then consider this as a hint and not a review.

...

> > > > > +#if IS_ENABLED(CONFIG_OF)
> > > > > +static int gpio_shared_of_traverse(struct device_node *curr)
> > > > > +{
> > > >
> > > > I believe parts of this code may be resided somewhere in drivers/of/property.c
> > > > or nearby as it has the similar parsing routines.
> > >
> > > I don't think this is a good idea, I want to keep it within the
> > > confines of drivers/gpio/ and the use-case is so specific, there's
> > > really no point in putting parts of it under drivers/of/.
> > >
> > > If I could only iterate over all properties of an fwnode, I'd have
> > > skipped using OF-specific routines altogether.
> >
> > The problem is that every subsystem considers "it's not a good idea" or
> > "historical reasons" or other excuses. Since you are adding OF-specific
> > stuff that has something already done inside OF specific code, why to
> > spread it over the kernel by duplicating in another place(s)?
> 
> Well, point me to the things that have been done already and I'll see
> about reusing them.

I already gave a direction, but if you think the duplication is okay, up to
you. It can be addressed later, just a bit more of technical debt.

-- 
With Best Regards,
Andy Shevchenko