Re: [PATCH 3/5] gpio: make gpiochip_get_desc() gpiolib-private

Guenter Roeck
2014-07-23T06:21:14Z

[-- Attachment #1: Type: text/plain, Size: 6665 bytes --]

On 07/22/2014 10:39 PM, Alexandre Courbot wrote:
> On Wed, Jul 23, 2014 at 12:47 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/22/2014 08:10 PM, Alexandre Courbot wrote:
>>>
>>> On Wed, Jul 23, 2014 at 5:17 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Tue, Jul 22, 2014 at 04:17:41PM +0900, Alexandre Courbot wrote:
>>>>>
>>>>> As GPIO descriptors are not going to remain unique anymore, having this
>>>>> function public is not safe. Restrain its use to gpiolib since we have
>>>>> no user outside of it.
>>>>>
>>>> If I implement a gpio chip driver built as module, and I want to use
>>>> gpiochip_request_own_desc(), how am I supposed to get desc ?
>>>>
>>>> I understand that there is still gpio_to_desc(), but I would have thought
>>>> that
>>>> desc = gpiochip_get_desc(chip, pin);
>>>> would be better than
>>>> desc = gpio_to_desc(chip->base + pin);
>>>>
>>>> Not that it makes much of a difference for me, just asking.
>>>
>>>
>>> Actually I was thinking of changing the prototype of
>>> gpiochip_request_own_desc(), and your comment definitely strenghtens
>>> that idea. gpiochip functions should not work with descriptors,
>>> especially since we are going to switch to a multiple-consumer scheme
>>> where there won't be any canonical descriptor anymore. Thus, how about
>>> turning gpiochip_request_own_desc() into this:
>>>
>>> struct gpio_desc *
>>> gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, const char
>>> *label);
>>>
>>> which would basically do both the gpiochip_get_desc() and former
>>> gpiochip_request_own_desc() in one call. I think it should satisfy
>>> everybody and removes the need to have gpiochip_get_desc() (a not very
>>> useful function by itself) exposed out of gpiolib.
>>>
>>> I will send a patch taking care of this if you agree that makes sense.
>>>
>>
>> I think you also plan to remove the capability to retrieve the chip
>> pointer, don't you ? If so, I won't be able to use the function from
>> the pca953x platform init function, since I won't be able to get the
>> pointer to the gpio chip. Even if you don't remove gpiod_to_chip(),
>> things would become a bit messy, since I would first have to convert
>> a pin to a desc and then to the chip pointer. Anyway, that change
>> would mean that exporting gpiochip_request_own_desc or its replacement
>> won't solve one of the problems addressed by my patch anymore, leaving
>> me more or less in the dark :-(.
>
> Here is why this change is taking place: right now you have a clear
> descriptor/pin mapping, i.e. there is only one descriptor per pin,
> anytime. For various reasons this is going to change soon, and
> descriptors will be allocated the provided to GPIO consumers as an
> abstraction of the pin. Meaning that you cannot really "get the
> descriptor for that pin" anymore. Since gpiochip_request_own_desc()'s
> purpose is precisely to request one descriptor for drivers to use, the
> new prototype makes much more sense IMHO.
>
> Another reason to have it work on a gpio_chip is that the gpio_chip
> pointer is a token to doing certain "priviledged" operations. Like
> obtaining an arbitrary descriptor. If consumers can get a pointer to
> the gpio_chip of a descriptor, this means they can basically do
> anything.
>
I understand, but my problem with pca953x platform initialization
is that the code to do that is outside the gpio directory in platform code.
Even though this is not consumer code, it still needs a means to perform
operations on a gpio pin, whatever those means are.

> Being in the board code, it seems to be that you are in a good
> position to obtain a pointer to the gpio_chip, and without knowing
> better I'd say that's what you should try to do. But maybe I would
> understand your problem better if you could post a small patch of what
> you want to achieve here.
>
Ok, but how do I get the pointer to the gpio chip from platform code
if gpiod_to_chip is gone ?

I attached the relevant parts of a platform file (scu.c), the one utilizing
pca953x initialization code to auto-export gpio pins. It currently uses
gpio_request_one(), which I am trying to replace. I can send you
the complete file if you like, but it is 1,600 bytes long so I figured
that would not help much.

I also attached a patch that tries to replace gpio_request_one with
gpiochip_request_own_desc in a gpio chip driver; maybe that gives
you an idea of that part of the problem.

>>
>> I was thinking about implementing a separate platform driver which
>> would enable me to auto-export (or initialize, if needed) gpio pins
>> from arbitrary gpio drivers to user space. I could make this work
>> with both devicetree data and platform data. Sure, that driver
>> would not have a chance to get accepted upstream, since it would use
>> devicetree data to, in a sense, configure the system, but on the
>> upside it would be independent of gpio API changes, and it would
>> work for _all_ gpio chips, not just for the ones with gpio driver
>> support. Unfortunately this approach doesn't really work either,
>> since exported pin names need to be configured with the chip driver,
>> and can not be selected afterwards when a pin is actually exported.
>>
>> On the other side, would you agree to adding something like
>> gpiod_export_name(), which would export a gpio pin with given name,
>> not using the default or chip->names ? That might help solving
>> at least some of my problems, and I would no longer depend on
>> gpiochip_request_own_desc or any of the related functions.
>
> Isn't that what gpiod_export_link() does?
>
This function assumes the existence of an exported pin,
and the resulting link is not in /sys/class/gpio but elsewhere.
That is not really the idea here; I did not want to create a second
export directory just for the sake of having well defined pin names
in some arbitrary place; the idea was to have those well defined
names in /sys/cass/gpio, and /sys/class/gpio is not available
as target for linking outside the gpio subsystem (if I remember
correctly because gpio_class is static and not exported).

>>
>> For reference, I currently need the ability to auto-export
>> gpio pins to user space for pca953x, ich, and for various
>> to-be-published gpio drivers used by my employer.
>
> Under which criteria are the GPIOs auto-exported? Can't you have the
> board code simply request all the GPIOs as a regular consumer and
> export them to user-space?
>

That is what the above mentioned platform driver would do. On x86 it
would use platform data for the purpose of identifying to-be-exported
pins, on devicetree platforms it would use devicetree data.

Thanks,
Guenter

[-- Attachment #2: patch.sam --]
[-- Type: text/plain, Size: 3064 bytes --]

diff --git a/drivers/gpio/gpio-sam.c b/drivers/gpio/gpio-sam.c
index 62cea48..4d59058 100644
--- a/drivers/gpio/gpio-sam.c
+++ b/drivers/gpio/gpio-sam.c
@@ -25,6 +25,8 @@
#include <linux/sched.h>
#include <linux/mfd/sam.h>

+#include "gpiolib.h"
+
/* gpio status/configuration */
#define SAM_GPIO_NEG_EDGE (1 << 8)
#define SAM_GPIO_NEG_EDGE_EN (1 << 7)
@@ -166,14 +168,7 @@ static void sam_gpio_setup(struct sam_gpio *sam)

chip->dev = sam->dev;
chip->label = dev_name(sam->dev);
- /*
- * Setting the owner results in the module being locked
- * into the kernel if gpio pins are auto-exported.
- * Don't set the gpio chip owner until we find a better
- * solution.
- *
- * chip->owner = THIS_MODULE;
- */
+ chip->owner = THIS_MODULE;
chip->direction_input = sam_gpio_direction_input;
chip->get = sam_gpio_get;
chip->direction_output = sam_gpio_direction_output;
@@ -564,10 +559,9 @@ static int sam_gpio_unexport(struct sam_gpio *sam)

/* un-export all auto-exported pins */
for (i = 0; i < sam->gpio_count; i++) {
- if (sam->export_flags[i] & GPIOF_EXPORT) {
- gpio_unexport(i + sam->gpio.base);
- gpio_free(i + sam->gpio.base);
- }
+ struct gpio_desc *desc = gpiochip_get_desc(&sam->gpio, i);
+ if (sam->export_flags[i] & GPIOF_EXPORT)
+ gpiochip_free_own_desc(desc);
}
return 0;
}
@@ -582,11 +576,32 @@ static int sam_gpio_export(struct sam_gpio *sam)
/* auto-export pins as requested */

for (i = 0; i < sam->gpio_count; i++) {
+ enum of_gpio_flags flags = sam->export_flags[i];
+ struct gpio_desc *desc = gpiochip_get_desc(&sam->gpio, i);
+
/* request and initialize exported pins */
- if (sam->export_flags[i] & GPIOF_EXPORT) {
- ret = gpio_request_one(i + sam->gpio.base,
- sam->export_flags[i],
- sam->names[i]);
+ if (flags & GPIOF_EXPORT) {
+ ret = gpiod_sysfs_set_active_low(desc,
+ flags & GPIOF_ACTIVE_LOW);
+ if (ret)
+ goto error;
+
+ ret = gpiochip_request_own_desc(desc, "sam-export");
+ if (ret)
+ goto error;
+
+ if (flags & GPIOF_DIR_IN) {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ goto error;
+ } else {
+ ret = gpiod_direction_output(desc,
+ flags & GPIOF_OUT_INIT_HIGH);
+ if (ret)
+ goto error;
+ }
+ ret = gpiod_export(desc,
+ flags & GPIOF_EXPORT_CHANGEABLE);
if (ret)
goto error;
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 61de1ef..f011834 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1711,6 +1711,7 @@ int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label)

return __gpiod_request(desc, label);
}
+EXPORT_SYMBOL(gpiochip_request_own_desc);

/**
* gpiochip_free_own_desc - Free GPIO requested by the chip driver
@@ -1724,6 +1725,7 @@ void gpiochip_free_own_desc(struct gpio_desc *desc)
if (desc)
__gpiod_free(desc);
}
+EXPORT_SYMBOL(gpiochip_free_own_desc);

/* Drivers MUST set GPIO direction before making get/set calls. In
* some cases this is done in early boot, before IRQs are enabled.

[-- Attachment #3: scu.c --]
[-- Type: text/x-csrc, Size: 7849 bytes --]

/*
* SCU board driver
*
* Copyright (c) 2012, 2014 Guenter Roeck <linux@roeck-us.net>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/types.h>
#include <linux/string.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/leds.h>
#include <linux/mdio-gpio.h>
#include <linux/platform_device.h>
#include <linux/gpio.h>
#include <linux/err.h>
#include <linux/dmi.h>
#include <linux/i2c.h>
#include <linux/i2c-gpio.h>
#include <linux/version.h>
#include <linux/platform_data/at24.h>
#include <linux/platform_data/pca953x.h>
#include <linux/sysfs.h>
#include <linux/spi/spi.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/netdevice.h>
#include <linux/rtnetlink.h>
#include <net/dsa.h>
#include <asm/processor.h>
#include <asm/byteorder.h>

...

static const char *pca9538_ext0_gpio_names[8] = {
"pca9538_ext0:wireless_ena_1",
"pca9538_ext0:wireless_ena_2",
"pca9538_ext0:wireless_a_radio_disable",
"pca9538_ext0:wireless_a_reset",
"pca9538_ext0:in_spare_1",
"pca9538_ext0:in_spare_2",
"pca9538_ext0:wireless_b_radio_disable",
"pca9538_ext0:wireless_b_reset",
};

static const char *pca9538_ext1_gpio_names[8] = {
"pca9538_ext1:rd_led_on",
"pca9538_ext1:wless_led_on",
"pca9538_ext1:ld_fail_led_on",
"pca9538_ext1:sw_led_on",
"pca9538_ext1:discrete_out_1",
"pca9538_ext1:discrete_out_2",
"pca9538_ext1:discrete_out_3",
"pca9538_ext1:discrete_out_4",
};

static const char *pca9538_ext2_gpio_names[8] = {
"pca9538_ext2:sd_active_1",
"pca9538_ext2:sd_error_1",
"pca9538_ext2:sd_active_2",
"pca9538_ext2:sd_error_2",
"pca9538_ext2:sd_active_3",
"pca9538_ext2:sd_error_3",
"pca9538_ext2:hub_6_reset",
"pca9538_ext2:hub_6_config_status",
};

static const char *pca9538_ext3_gpio_names[8] = {
"pca9538_ext3:sd_active_4",
"pca9538_ext3:sd_error_4",
"pca9538_ext3:sd_active_5",
"pca9538_ext3:sd_error_5",
"pca9538_ext3:sd_active_6",
"pca9538_ext3:sd_error_6",
"pca9538_ext3:hub_2_reset",
"pca9538_ext3:hub_2_config_status",
};

static const char *pca9557_gpio_names[8] = {
"pca9557:sd_card_detect_1",
"pca9557:sd_card_detect_2",
"pca9557:sd_card_detect_3",
"pca9557:sd_card_detect_4",
"pca9557:sd_card_detect_5",
"pca9557:sd_card_detect_6",
"pca9557:spare1",
"pca9557:spare2",
};

static int scu_gpio_common_setup(unsigned gpio_base, unsigned ngpio, u32 mask,
u32 is_input, u32 is_active, u32 active_low)
{
int i;
unsigned long flags;

for (i = 0; i < ngpio; i++) {
if (!(mask & (1 << i)))
continue;
flags = GPIOF_EXPORT_DIR_FIXED;
if (is_input & (1 << i)) {
flags |= GPIOF_DIR_IN;
} else {
flags |= GPIOF_DIR_OUT;
if (is_active & (1 << i))
flags |= GPIOF_INIT_HIGH;
}
if (active_low & (1 << i))
flags |= GPIOF_ACTIVE_LOW;
gpio_request_one(gpio_base + i, flags, NULL);
}
return 0;
}

static int pca9538_ext0_setup(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio, void *context)
{
scu_gpio_common_setup(gpio_base, ngpio, 0xff, 0x33, 0xcc, 0x00);
return 0;
}

static int pca9538_ext1_setup(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio, void *context)
{
scu_gpio_common_setup(gpio_base, ngpio, 0xf0, 0x00, 0x00, 0x00);
return 0;
}

static int pca9538_ext2_setup(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio, void *context)
{
scu_gpio_common_setup(gpio_base, ngpio, 0xc0, 0x80, 0x40, 0x00);
return 0;
}

static int pca9538_ext3_setup(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio, void *context)
{
scu_gpio_common_setup(gpio_base, ngpio, 0xc0, 0x80, 0x40, 0x00);
return 0;
}

static int pca9557_setup(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio, void *context)
{
scu_gpio_common_setup(gpio_base, ngpio, 0x3f, 0x3f, 0x00, 0x3f);
return 0;
}

static void scu_gpio_common_teardown(unsigned gpio_base, int ngpio, u32 mask)
{
int i;

for (i = 0; i < ngpio; i++) {
if (mask & (1 << i)) {
gpio_unexport(gpio_base + i);
gpio_free(gpio_base + i);
}
}
}

static int pca9538_ext0_teardown(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio,
void *context)
{
scu_gpio_common_teardown(gpio_base, ngpio, 0xff);
return 0;
}

static int pca9538_ext1_teardown(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio,
void *context)
{
scu_gpio_common_teardown(gpio_base, ngpio, 0xf0);
return 0;
}

static int pca9538_ext2_teardown(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio,
void *context)
{
scu_gpio_common_teardown(gpio_base, ngpio, 0xc0);
return 0;
}

static int pca9538_ext3_teardown(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio,
void *context)
{
scu_gpio_common_teardown(gpio_base, ngpio, 0xc0);
return 0;
}

static int pca9557_teardown(struct i2c_client *client,
unsigned gpio_base, unsigned ngpio,
void *context)
{
scu_gpio_common_teardown(gpio_base, ngpio, 0x3f);
return 0;
}

static struct pca953x_platform_data scu_pca953x_pdata[] = {
[0] = {.gpio_base = SCU_EXT_GPIO_BASE(0),
.irq_base = -1,
.setup = pca9538_ext0_setup,
.teardown = pca9538_ext0_teardown,
.names = pca9538_ext0_gpio_names},
[1] = {.gpio_base = SCU_EXT_GPIO_BASE(1),
.irq_base = -1,
.setup = pca9538_ext1_setup,
.teardown = pca9538_ext1_teardown,
.names = pca9538_ext1_gpio_names},
[2] = {.gpio_base = SCU_EXT_GPIO_BASE(2),
.irq_base = -1,
.setup = pca9538_ext2_setup,
.teardown = pca9538_ext2_teardown,
.names = pca9538_ext2_gpio_names},
[3] = {.gpio_base = SCU_EXT_GPIO_BASE(3),
.irq_base = -1,
.setup = pca9538_ext3_setup,
.teardown = pca9538_ext3_teardown,
.names = pca9538_ext3_gpio_names},
[4] = {.gpio_base = SCU_EXT_GPIO_BASE(4),
.irq_base = -1,
.setup = pca9557_setup,
.teardown = pca9557_teardown,
.names = pca9557_gpio_names},
};

...

static int scu_instantiate_i2c(struct scu_data *data, int base,
struct i2c_board_info *info, int count)
{
int i;

for (i = 0; i < count; i++) {
data->client[base + i] = i2c_new_device(data->adapter, info);
if (!data->client[base + i]) {
/*
* Unfortunately this call does not tell us
* why it failed. Pick the most likely reason.
*/
return -EBUSY;
}
info++;
}
return 0;
}

static struct i2c_board_info scu_i2c_info_common[] = {
{ I2C_BOARD_INFO("scu_pic", 0x20)},
{ I2C_BOARD_INFO("at24", 0x54),
.platform_data = &at24c08},
{ I2C_BOARD_INFO("ds1682", 0x6b)},
{ I2C_BOARD_INFO("pca9538", 0x71),
.platform_data = &scu_pca953x_pdata[1],},
{ I2C_BOARD_INFO("pca9538", 0x72),
.platform_data = &scu_pca953x_pdata[2],},
{ I2C_BOARD_INFO("pca9538", 0x73),
.platform_data = &scu_pca953x_pdata[3],},
};

...

static int scu_probe(struct platform_device *pdev)
{
struct proc_dir_entry *rave_board_type;
struct device *dev = &pdev->dev;
struct i2c_adapter *adapter;
struct net_device *ndev;
struct scu_data *data;
int i, ret;

...

ret = scu_instantiate_i2c(data, 0, scu_i2c_info_common,
ARRAY_SIZE(scu_i2c_info_common));
...
}