Re: [PATCH v4 1/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson <warthog618@gmail.com>]

On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Store the debounce period for a requested line locally, rather than in
> > the debounce_period_us field in the gpiolib struct gpio_desc.
> >
> > Add a global tree of lines containing supplemental line information
> > to make the debounce period available to be reported by the
> > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy@kernel.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> >  1 file changed, 132 insertions(+), 22 deletions(-)
> >
> > +static inline bool line_is_supplemental(struct line *line)
>
> Under v2 I suggested naming this line_has_suppinfo(). Any reason not
> to do it? I think it's more logical than saying "line is
> supplemental". The latter makes it seem as if certain line objects
> would "supplement" some third party with something. What this really
> checks is: does this line contain additional information.
>


My bad - responded to your first comment and then missed the rest.

Agreed - the naming could be better. Will fix for v5.

> > +{
> > +       return READ_ONCE(line->debounce_period_us);
> > +}
> > +
> > +static void line_set_debounce_period(struct line *line,
> > +                                    unsigned int debounce_period_us)
> > +{
> > +       bool was_suppl = line_is_supplemental(line);
> > +
> > +       WRITE_ONCE(line->debounce_period_us, debounce_period_us);
> > +
> > +       if (line_is_supplemental(line) == was_suppl)
> > +               return;
> > +
> > +       if (was_suppl)
> > +               supinfo_erase(line);
> > +       else
> > +               supinfo_insert(line);
>
> Could you add a comment here saying it's called with the config mutex
> taken as at first glance it looks racy but actually isn't?
>

Sure.  Though it is also covered by the gdev->sem you added, right?
So the config_mutex is now redundant?
Should I document it is covered by both?
Or drop the config_mutex entirely?

And you wanted some comments to explain the logic?
I thought this is a common "has it changed" pattern, and so didn't
require additional explanation, but I guess not as common as I thought.

Cheers,
Kent.