[PATCH] gpiolib: unexport gpiod_set_transitory()

[PATCH] gpiolib: unexport gpiod_set_transitory()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There are no and never have been any users of gpiod_set_transitory()
outside the core GPIOLIB code. Make it private.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c        | 1 -
 drivers/gpio/gpiolib.h        | 2 ++
 include/linux/gpio/consumer.h | 8 --------
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c78d83990068..490c1cad3b52 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2774,7 +2774,6 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 						      PIN_CONFIG_PERSIST_STATE,
 						      !transitory);
 }
-EXPORT_SYMBOL_GPL(gpiod_set_transitory);
 
 /**
  * gpiod_is_active_low - test whether a GPIO is active-low or not
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 1f6277bbd612..3ccacf3c1288 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -132,6 +132,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 				  struct gpio_array *array_info,
 				  unsigned long *value_bitmap);
 
+int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
+
 extern spinlock_t gpio_lock;
 extern struct list_head gpio_devices;
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 1c4385a00f88..6cc345440a5b 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -159,7 +159,6 @@ int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 
 int gpiod_set_config(struct gpio_desc *desc, unsigned long config);
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce);
-int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 void gpiod_toggle_active_low(struct gpio_desc *desc);
 
 int gpiod_is_active_low(const struct gpio_desc *desc);
@@ -494,13 +493,6 @@ static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned int deboun
 	return -ENOSYS;
 }
 
-static inline int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
-{
-	/* GPIO can never have been requested */
-	WARN_ON(desc);
-	return -ENOSYS;
-}
-
 static inline void gpiod_toggle_active_low(struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
-- 
2.39.2

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Linus Walleij]

On Sun, Sep 3, 2023 at 9:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There are no and never have been any users of gpiod_set_transitory()
> outside the core GPIOLIB code. Make it private.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Well spotted!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Andy Shevchenko]

On Sun, Sep 03, 2023 at 09:06:57PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> There are no and never have been any users of gpiod_set_transitory()
> outside the core GPIOLIB code. Make it private.

And rename to be gpio_desc_...()?

With this done,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The rationale has been explained in the other threads with renaming matters.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Bartosz Golaszewski]

On Mon, Sep 4, 2023 at 12:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sun, Sep 03, 2023 at 09:06:57PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > There are no and never have been any users of gpiod_set_transitory()
> > outside the core GPIOLIB code. Make it private.
>
> And rename to be gpio_desc_...()?
>
> With this done,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> The rationale has been explained in the other threads with renaming matters.
>

I'm not buying this explanation. Public GPIO functions don't have a
monopoly on the gpiod_ prefix. Eventually I'd love to unify the naming
convention for the three important structures that we use:
gpio_device, gpio_chip and gpio_desc, no matter whether they're public
or private as that's already clear from their placement in
include/linux/ or drivers/gpio/.

Bartosz

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

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 01:14:29PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 4, 2023 at 12:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Sep 03, 2023 at 09:06:57PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > There are no and never have been any users of gpiod_set_transitory()
> > > outside the core GPIOLIB code. Make it private.
> >
> > And rename to be gpio_desc_...()?
> >
> > With this done,
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > The rationale has been explained in the other threads with renaming matters.
> 
> I'm not buying this explanation. Public GPIO functions don't have a
> monopoly on the gpiod_ prefix. Eventually I'd love to unify the naming
> convention for the three important structures that we use:
> gpio_device, gpio_chip and gpio_desc, no matter whether they're public
> or private as that's already clear from their placement in
> include/linux/ or drivers/gpio/.

And I would like to avoid adding confusion by mixing internal and external APIs
under the same prefix.

Personally I do not like this change, when gpiod_ is being used. So, you may
override this, you are the maintainer, but then here is the formal NAK from me
(as a user of these APIs internally and externally).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Bartosz Golaszewski]

On Mon, Sep 4, 2023 at 1:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 04, 2023 at 01:14:29PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 4, 2023 at 12:19 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Sun, Sep 03, 2023 at 09:06:57PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > There are no and never have been any users of gpiod_set_transitory()
> > > > outside the core GPIOLIB code. Make it private.
> > >
> > > And rename to be gpio_desc_...()?
> > >
> > > With this done,
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > The rationale has been explained in the other threads with renaming matters.
> >
> > I'm not buying this explanation. Public GPIO functions don't have a
> > monopoly on the gpiod_ prefix. Eventually I'd love to unify the naming
> > convention for the three important structures that we use:
> > gpio_device, gpio_chip and gpio_desc, no matter whether they're public
> > or private as that's already clear from their placement in
> > include/linux/ or drivers/gpio/.
>
> And I would like to avoid adding confusion by mixing internal and external APIs
> under the same prefix.
>
> Personally I do not like this change, when gpiod_ is being used. So, you may
> override this, you are the maintainer, but then here is the formal NAK from me
> (as a user of these APIs internally and externally).

This is still inconsistent with the rest of the public symbols -
especially those in gpio/driver.h. My long-term plan - before making
locking great again - is to limit the usage of any gpiochip_ symbols
to GPIO providers (as the chip is not guaranteed to be valid, unlike
gpio_device) and provide a bunch of gpio_device_ or gpiodev_
interfaces for use by those who *really* need it. Now am I going to
make up two distinct prefixes for public and non-public APIs? That
will be even more confusing IMO.

Just like in C++ you don't make up special names for public vs private
methods except for some deranged coding styles that also require you
to name arguments like "in_foo" and "out_bar".

Bart

Re: [PATCH] gpiolib: unexport gpiod_set_transitory()

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 01:32:38PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 4, 2023 at 1:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> This is still inconsistent with the rest of the public symbols -
> especially those in gpio/driver.h. My long-term plan - before making
> locking great again - is to limit the usage of any gpiochip_ symbols
> to GPIO providers (as the chip is not guaranteed to be valid, unlike
> gpio_device) and provide a bunch of gpio_device_ or gpiodev_
> interfaces for use by those who *really* need it. Now am I going to
> make up two distinct prefixes for public and non-public APIs? That
> will be even more confusing IMO.
> 
> Just like in C++ you don't make up special names for public vs private
> methods except for some deranged coding styles that also require you
> to name arguments like "in_foo" and "out_bar".

Yeah, I understand your point of view on this, but as I said
"disagree and commit" (used to be corporate value at some point :-).

On my side I criticized and proposed... It's your turn what to do
with that, I'm not insisting on my way, I'm just telling I don't like
yours, but I will survive, no hard feelings :-)

-- 
With Best Regards,
Andy Shevchenko