Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Mika Westerberg]

On Thu, Oct 23, 2014 at 11:51:59PM +0200, Rafael J. Wysocki wrote:
> OK, let's try to take that a bit farther. :-)
> 
> With the (untested) patch below applied (which is a replacement for the one
> sent previously), the driver can use static tables like these:
> 
> static struct acpi_gpio_params reset_gpios = { 0, 0, false };
> statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
> 
> static struct acpi_gpio_mapping my_gpio_mapping[] = {
> 	{ "reset-gpios", &reset_gpios, 1 },
> 	{ "shutdown-gpios", &shutdown_gpios, 1 },
> 	NULL,
> };
> 

This is even better :-)

I tried this with few changes:

 - constify the mapping parameter
 - check for NULL adev->driver_gpios in acpi_get_driver_gpio_data()
 - check for gm->name instead of gm in acpi_get_driver_gpio_data()

It works just fine. For reference here are my changes to rkill-gpio.c:

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 0f62326c0f5e..ec8f5ac53520 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -63,6 +63,15 @@ static const struct rfkill_ops rfkill_gpio_ops = {
 	.set_block = rfkill_gpio_set_power,
 };
 
+static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
+static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_rfkill_default_gpios[] = {
+	{ "reset-gpios", &reset_gpios, 1 },
+	{ "shutdown-gpios", &shutdown_gpios, 1 },
+	{ },
+};
+
 static int rfkill_gpio_acpi_probe(struct device *dev,
 				  struct rfkill_gpio_data *rfkill)
 {
@@ -75,7 +84,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 	rfkill->name = dev_name(dev);
 	rfkill->type = (unsigned)id->driver_data;
 
-	return 0;
+	return acpi_dev_add_driver_gpios(ACPI_COMPANION(dev),
+					 acpi_rfkill_default_gpios);
 }
 
 static int rfkill_gpio_probe(struct platform_device *pdev)
@@ -110,7 +120,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 		rfkill->reset_gpio = gpio;
 	}
 
-	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
+	gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
 	if (!IS_ERR(gpio)) {
 		ret = gpiod_direction_output(gpio, 0);
 		if (ret)
@@ -150,6 +160,8 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
 	rfkill_unregister(rfkill->rfkill_dev);
 	rfkill_destroy(rfkill->rfkill_dev);
 
+	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
+
 	return 0;
 }
 
And here are the changes on top of your patch:

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 691501b53c5e..4151cd774f2f 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -288,7 +288,7 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 }
 
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios)
+			      const struct acpi_gpio_mapping *gpios)
 {
 	if (adev && gpios) {
 		adev->driver_gpios = gpios;
@@ -302,11 +302,14 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 				      const char *name, int index,
 				      struct acpi_reference_args *args)
 {
-	struct acpi_gpio_mapping *gm;
+	const struct acpi_gpio_mapping *gm;
 
-	for (gm = adev->driver_gpios; gm; gm++)
+	if (!adev->driver_gpios)
+		return false;
+
+	for (gm = adev->driver_gpios; gm->name; gm++)
 		if (!strcmp(name, gm->name) && gm->data && index < gm->size) {
-			struct acpi_gpio_params *par = gm->data + index;
+			const struct acpi_gpio_params *par = gm->data + index;
 
 			args->adev = adev;
 			args->args[0] = par->crs_entry_index;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 2de7a2614c61..98d992062270 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -368,7 +368,7 @@ struct acpi_device {
 	struct acpi_scan_handler *handler;
 	struct acpi_hotplug_context *hp;
 	struct acpi_driver *driver;
-	struct acpi_gpio_mapping *driver_gpios;
+	const struct acpi_gpio_mapping *driver_gpios;
 	void *driver_data;
 	struct device dev;
 	unsigned int physical_node_count;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 26b7a639f4b9..23f9c55d0331 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -680,13 +680,13 @@ struct acpi_gpio_params {
 
 struct acpi_gpio_mapping {
 	const char *name;
-	struct acpi_gpio_params *data;
+	const struct acpi_gpio_params *data;
 	unsigned int size;
 };
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios);
+			      const struct acpi_gpio_mapping *gpios);
 
 static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
 {
@@ -695,7 +695,7 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
 }
 #else
 static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-			      struct acpi_gpio_mapping *gpios)
+			      const struct acpi_gpio_mapping *gpios)
 {
 	return -ENXIO;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/