Re: [PATCH v3 1/8] iio: set default trig->dev.parent

[Gwendal Grignou <gwendal@chromium.org>]

On Tue, Mar 9, 2021 at 2:00 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Mar 9, 2021 at 3:18 AM Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > When allocated with [devm_]iio_trigger_alloc(), set trig device parent to
> > the device the trigger is allocated for by default.
> >
> > It can always be reassigned in the probe routine.
> >
> > Change iio_trigger_alloc() API to add the device pointer to be coherent
> > with devm_iio_trigger_alloc, using similar interface to
> > iio_device_alloc().
>
> Few nit-picks below.
>
> ...
>
> > +static __printf(2, 0)
> > +struct iio_trigger *viio_trigger_alloc(struct device *parent,
>
> > +                                      const char *fmt,
> > +                                      va_list vargs)
>
> Can be one line.
Looking at the rest of the file, when arguments span on multiple
lines, there is only one argument per line: see iio_trigger_read_name
or iio_alloc_pollfunc.
>
> ...
>
> > +/**
> > + * iio_trigger_alloc - Allocate a trigger
> > + * @parent:            Device to allocate iio_trigger for
>
> > + * @fmt:               trigger name format. If it includes format
> > + *                     specifiers, the additional arguments following
> > + *                     format are formatted and inserted in the resulting
> > + *                     string replacing their respective specifiers.
>
> Strange indentation (Everything after the first period should go into
> the main description section). Also inconsistency with capital letters
> at the beginning of field description.
> Yes I have noticed that it's in the original code, but we may do
> better, don't we?
>
> > + * RETURNS:
> > + * Pointer to allocated iio_trigger on success, NULL on failure.
> > + */
>
> ...
>
> > +struct iio_trigger *iio_trigger_alloc(struct device *parent,
> > +                                     const char *fmt, ...)
>
> One line?
Done. Line is over 80 characters, but up to 100 character is now
allowed by checkpatch.pl.
>
> ...
>
> > +struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
> >                                            const char *fmt, ...);
>
> One line?
Done
>
> ...
>
> > +__printf(2, 3) struct iio_trigger *iio_trigger_alloc(struct device *parent,
> > +                                                    const char *fmt, ...);
>
> Perhaps leave __printf() on the first line and keep everything else on
> the second one?
Done, use the same format as devm_iio_trigger_alloc.

>
> --
> With Best Regards,
> Andy Shevchenko