Re: [PATCH 13/15] gpio: sodaville: use new generic GPIO chip API

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

On Tue, Sep 09, 2025 at 04:47:09PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 09, 2025 at 04:45:31PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 09, 2025 at 08:24:23AM -0500, Bartosz Golaszewski wrote:
> > > On Tue, 9 Sep 2025 15:13:04 +0200, Andy Shevchenko
> > > <andriy.shevchenko@intel.com> said:
> > > > On Tue, Sep 09, 2025 at 01:35:04PM +0200, Bartosz Golaszewski wrote:
> > > >> On Tue, Sep 9, 2025 at 1:31 PM Andy Shevchenko
> > > >> <andriy.shevchenko@intel.com> wrote:
> > > >> > On Tue, Sep 09, 2025 at 11:15:40AM +0200, Bartosz Golaszewski wrote:

...

> > > >> > > +     config = (typeof(config)){
> > > >> >
> > > >> > This looks unusual. Why can't properly formed compound literal be used as in
> > > >> > many other places in the kernel?
> > > >>
> > > >> It is correct C
> > > >
> > > > If it compiles, it doesn't mean it's correct C, it might be non-standard.
> > > > Have you checked with the standard (note, I read that part in the past,
> > > > but I may forgot the details, so I don't know the answer to this)?
> > > 
> > > It's a GNU extension alright
> > 
> > clang, I suppose, also okay with this?
> > 
> > > but it's supported in the kernel as it evaluates
> > > to a simple cast.
> > 
> > There is no cast. And that's make a big difference to what the code tries to do.
> > 
> > > >> and checkpatch doesn't raise any warnings.
> > > >
> > > > checkpatch is far from being useful in the questions like this.
> > > > It false positively complains for for_each*() macros all over
> > > > the kernel, for example.
> > > >
> > > >> It's the
> > > >> same kind of argument as between kmalloc(sizeof(struct foo)) vs
> > > >> kmalloc(sizeof(f)).
> > > >
> > > > Maybe, but it introduces a new style while all other cases use the other,
> > > > _established_ style. So we have a precedent and the form the code is written
> > > > in is against the de facto usage of the compound literals.
> > > 
> > > It may not be *very* common but it's hardly new style:
> > 
> > I think your statement is incorrect see below why.
> > 
> > > $ git grep -P "\(typeof\(.*\)\) ?\{" | wc
> > >     108     529    7315
> > 
> > Not correct. The correct output will be closer to
> > 
> > $ git grep -l -P "\(typeof\(.*\)\) ?\{" | wc -l
> > 15
> > 
> > And if you looked at the output carefully, you see the bug in the RE you used.
> > 
> > So, even closer will be this one:
> > 
> > $ git grep -l -P "=[[:space:]]+\(typeof\(.*\)\) ?\{" | wc -l
> > 7
> > 
> > 2 out of which are related to libeth, effectively makes this 6.

And for of fullness the picture:
$ git grep -l -P "=[[:space:]]+\(struct [^[:space:]]*\) ?\{" | wc -l
501

So, it's 1:100 ratio.

> TBH, I think those 6 all made the same mistake, i.e. thinking of the compound
> literal as a cast. Which is not!
> 
> > No, this is completely non-standard and unusual thing in the kernel.
> > 
> > > >> I guess it's personal taste but I like this version better.
> > > >
> > > > In kernel we also try to be consistent. This add inconsistency. Am I wrong?
> > > >
> > > >> > > +             .dev = &pdev->dev,
> > > >> > > +             .sz = 4,
> > > >> > > +             .dat = sd->gpio_pub_base + GPINR,
> > > >> > > +             .set = sd->gpio_pub_base + GPOUTR,
> > > >> > > +             .dirout = sd->gpio_pub_base + GPOER,
> > > >> > > +     };

-- 
With Best Regards,
Andy Shevchenko