Re: [PATCH v8 2/3] regulator: Add devres version of of_regulator_get_optional()

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

On Fri, Sep 27, 2024 at 12:38:35PM +0800, Chen-Yu Tsai wrote:
> On Thu, Sep 26, 2024 at 8:26 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Sep 26, 2024 at 04:43:52PM +0800, Chen-Yu Tsai wrote:
> > > On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote:

...

> > > > > +#if IS_ENABLED(CONFIG_OF)
> > > >
> > > > Do we really need this?
> > >
> > > What's the point of going through devres_* stuff if we already know
> > > _of_regulator_get() is going to fail anyway?
> >
> > With devm_add_action*() this will be other way around and there are plenty of
> > APIs done this way. The ifdeffery is simply ugly in the code.
> 
> It's still extra code that doesn't get compiled out.

Are you sure? In case of the stub the compiler should go with a "dead code
elimination" optimisation and get rid of most of it (yes, I admit that it might
be the overhead for the exporting a function which returns a constant).

> > > Also, _of_regulator_get() does not have a stub version for !CONFIG_OF.
> >
> > So, what prevents us from adding it?
> 
> Because there's no need if the only internal user isn't using it.
> 
> I could also move them over to of_regulator.c.
> 
> _of_regulator_get() stays internal to that file, and devm_regulator_release()
> gets exposed instead.
> 
> Does that sound better?

This sounds good to me!

...

> > > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev,
> > > > > +                                                                         struct device_node *node,
> > > > > +                                                                         const char *id)
> > > >
> > > > I don't know the conventions here, but I find better to have it as
> > > >
> > > > static inline __must_check struct regulator *
> > > > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id)
> > > >
> > > > Similar to other stubs and declarations.
> > >
> > > I don't think there are any conventions. This file already has three types:
> > >
> > > 1. Wrap the line with the function name on the second line
> > > 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis.
> > > 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of
> > >    tabs.
> > >
> > > I prefer the way I have put them.
> >
> > The way you put it despite relaxed limit is slightly harder to read.
> > I don't remember many headers that do so-o indented parameters. Besides
> > your way defers the burden of resplit to the future in case one more parameter
> > needs to be added which will excess the 100 limit.
> >
> > Also __must_check is somehow misplaced in my opinion (talking from my
> > experience and this can be simply checked by grepping other headers).
> 
> Seems correct to me. It's between the return type and the function name.
> From the coding style doc:
> 
>  __init void * __must_check action(enum magic value, size_t size, u8 count,
>                                    char *fmt, ...) __printf(4, 5) __malloc;
> 
> The preferred order of elements for a function prototype is:
> 
> - storage class (below, ``static __always_inline``, noting that
> ``__always_inline``
>   is technically an attribute but is treated like ``inline``)
> - storage class attributes (here, ``__init`` -- i.e. section
> declarations, but also
>   things like ``__cold``)
> - return type (here, ``void *``)
> - return type attributes (here, ``__must_check``)
> - function name (here, ``action``)
> - function parameters (here, ``(enum magic value, size_t size, u8
> count, char *fmt, ...)``,
>   noting that parameter names should always be included)
> - function parameter attributes (here, ``__printf(4, 5)``)
> - function behavior attributes (here, ``__malloc``)
> 
> > That said, I prefer the way I suggested or something alike.
> 
> Two people arguing over style that is not clearly specified in the coding
> style doc is probably wasting time. I'll use what `clang-format` gave:
> 
> static inline struct regulator *__must_check of_regulator_get_optional(
>        struct device *dev, struct device_node *node, const char *id)
> static inline struct regulator *__must_check devm_of_regulator_get_optional(
>        struct device *dev, struct device_node *node, const char *id)

With all my hatred towards this clang-format "feature", i.e. open ended
parenthesis, this looks better than your original variant.

-- 
With Best Regards,
Andy Shevchenko