Received-SPF: pass (google.com: domain of linux-kernel+bounces-3968-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 18 Dec 2023 23:40:51 +0800
From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
	linus.walleij@linaro.org, andy@kernel.org
Subject: Re: [PATCH v4 1/5] gpiolib: cdev: relocate debounce_period_us from
 struct gpio_desc
Message-ID: <ZYBoA25z76uutBBI@rigel>
References: <20231216001652.56276-1-warthog618@gmail.com>
 <20231216001652.56276-2-warthog618@gmail.com>
 <CAMRc=McBVeQ=yRpGRsnPEULfPx15PBO3kiGscdS4s6-d0URc3w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=McBVeQ=yRpGRsnPEULfPx15PBO3kiGscdS4s6-d0URc3w@mail.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.