| Message ID | 20231003145114.21637-5-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: don't use GPIOLIB global numberspace in helpers | expand |
On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Use the improved variant of pinctrl_gpio_can_use_line() which takes a > pointer to the gpio_chip and a controller-relative offset. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib-cdev.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 31fc71a612c2..54ee075410db 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -2287,8 +2287,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, > * FIXME: find a non-racy way to retrieve this information. Maybe a > * lock common to both frameworks? > */ > - ok_for_pinctrl = > - pinctrl_gpio_can_use_line(gc->base + info->offset); > + ok_for_pinctrl = pinctrl_gpio_can_use_line_new(gc, info->offset); > _new?? In what sense? I agree with the change in principle, just not comfortable with the naming. Cheers, Kent.
On Tue, Oct 3, 2023 at 6:02 PM Kent Gibson <warthog618@gmail.com> wrote: > On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: ... > I agree with the change in principle, just not comfortable with the naming. +1 here. I proposed some names, have you seen my comment(s)?
On Tue, Oct 03, 2023 at 06:17:27PM +0300, Andy Shevchenko wrote: > On Tue, Oct 3, 2023 at 6:02 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: > > ... > > > I agree with the change in principle, just not comfortable with the naming. > > +1 here. I proposed some names, have you seen my comment(s)? > I have now - any of those work for me. Whichever is consistent with what we are using for gpiochip functions in gpiolib would make most sense to me. Cheers, Kent.
On Tue, Oct 3, 2023 at 5:24 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Oct 03, 2023 at 06:17:27PM +0300, Andy Shevchenko wrote: > > On Tue, Oct 3, 2023 at 6:02 PM Kent Gibson <warthog618@gmail.com> wrote: > > > On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: > > > > ... > > > > > I agree with the change in principle, just not comfortable with the naming. > > > > +1 here. I proposed some names, have you seen my comment(s)? > > > > I have now - any of those work for me. > Whichever is consistent with what we are using for gpiochip functions in > gpiolib would make most sense to me. > Does it really matter? It's not here to stay, it's temporary and exists only until the whole series is applied - which given that it's limited to gpio and pinctrl, shouldn't take more than one release cycle. There are plenty of examples of this naming convention for temporary symbols - there's even an ongoing effort to replace all .remove() callbacks with .remove_new() which will then be changed back to .remove() treewide. Bart
On Tue, Oct 03, 2023 at 08:07:05PM +0200, Bartosz Golaszewski wrote: > On Tue, Oct 3, 2023 at 5:24 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Oct 03, 2023 at 06:17:27PM +0300, Andy Shevchenko wrote: > > > On Tue, Oct 3, 2023 at 6:02 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: > > > > > > ... > > > > > > > I agree with the change in principle, just not comfortable with the naming. > > > > > > +1 here. I proposed some names, have you seen my comment(s)? > > > > > > > I have now - any of those work for me. > > Whichever is consistent with what we are using for gpiochip functions in > > gpiolib would make most sense to me. > > > > Does it really matter? It's not here to stay, it's temporary and > exists only until the whole series is applied - which given that it's > limited to gpio and pinctrl, shouldn't take more than one release > cycle. > > There are plenty of examples of this naming convention for temporary > symbols - there's even an ongoing effort to replace all .remove() > callbacks with .remove_new() which will then be changed back to > .remove() treewide. > This was the only patch that I was included into, so I didn't realise there was a treewide rename at the end. Even so, using _new suffix for that purpose is poor (well pinctrl_gpio_free_new() did draw a laugh, but other than that...). Perhaps use something specific to the patch series so it is clear what its purpose is? Cheers, Kent.
On Wed, Oct 4, 2023 at 6:16 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Oct 03, 2023 at 08:07:05PM +0200, Bartosz Golaszewski wrote: > > On Tue, Oct 3, 2023 at 5:24 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Tue, Oct 03, 2023 at 06:17:27PM +0300, Andy Shevchenko wrote: > > > > On Tue, Oct 3, 2023 at 6:02 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > On Tue, Oct 03, 2023 at 04:50:42PM +0200, Bartosz Golaszewski wrote: > > > > > > > > ... > > > > > > > > > I agree with the change in principle, just not comfortable with the naming. > > > > > > > > +1 here. I proposed some names, have you seen my comment(s)? > > > > > > > > > > I have now - any of those work for me. > > > Whichever is consistent with what we are using for gpiochip functions in > > > gpiolib would make most sense to me. > > > > > > > Does it really matter? It's not here to stay, it's temporary and > > exists only until the whole series is applied - which given that it's > > limited to gpio and pinctrl, shouldn't take more than one release > > cycle. > > > > There are plenty of examples of this naming convention for temporary > > symbols - there's even an ongoing effort to replace all .remove() > > callbacks with .remove_new() which will then be changed back to > > .remove() treewide. > > > > This was the only patch that I was included into, so I didn't realise > there was a treewide rename at the end. I didn't want to spam 20+ maintainers with the entire series of 36 patches. Should have probably Cc'ed everyone on the cover letter though. > Even so, using _new suffix for that purpose is poor (well > pinctrl_gpio_free_new() did draw a laugh, but other than that...). > Perhaps use something specific to the patch series so it is clear what > its purpose is? > I think Linus will end up applying the entire series to his tree in one go in which case the name really doesn't matter. Do we really need to bikeshed about the name which will exist for as long as it takes to apply the series on his laptop? I much more care about preserving bisectability across the series which it does. Bart
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 31fc71a612c2..54ee075410db 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -2287,8 +2287,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, * FIXME: find a non-racy way to retrieve this information. Maybe a * lock common to both frameworks? */ - ok_for_pinctrl = - pinctrl_gpio_can_use_line(gc->base + info->offset); + ok_for_pinctrl = pinctrl_gpio_can_use_line_new(gc, info->offset); spin_lock_irqsave(&gpio_lock, flags);