diff mbox

net: rfkill: gpio: Add default GPIO driver mappings for ACPI

Message ID 1414404914-163837-1-git-send-email-mika.westerberg@linux.intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mika Westerberg Oct. 27, 2014, 10:15 a.m. UTC
The driver uses devm_gpiod_get_index(..., index) so that the index refers
directly to the GpioIo resource under the ACPI device. The problem with
this is that if the ordering changes we get wrong GPIOs.

With ACPI 5.1 _DSD we can now use names instead to reference GPIOs
analogous to Device Tree. However, we still have systems out there that do
not provide _DSD at all. These systems must be supported as well.

Luckily we now have acpi_dev_add_driver_gpios() that can be used to provide
mappings for systems where _DSD is not provided and still take advantage of
_DSD if it exists.

This patch changes the driver to create default GPIO mappings if we are
running on ACPI system.

While there we can drop the indices completely and use devm_gpiod_get()
with name instead.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Hi,

This patch is based on top of linux-pm/device-properties [1] and following
patch from Rafael [2].

Johannes, John,

If you are happy with the patch, can you ACK it so that we can merge it
with the rest of the device-properties patches. Thanks.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
[2] https://lkml.org/lkml/2014/10/24/690

 net/rfkill/rfkill-gpio.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Johannes Berg Oct. 27, 2014, 11:23 a.m. UTC | #1
On Mon, 2014-10-27 at 12:15 +0200, Mika Westerberg wrote:
> The driver uses devm_gpiod_get_index(..., index) so that the index refers
> directly to the GpioIo resource under the ACPI device. The problem with
> this is that if the ordering changes we get wrong GPIOs.
> 
> With ACPI 5.1 _DSD we can now use names instead to reference GPIOs
> analogous to Device Tree. However, we still have systems out there that do
> not provide _DSD at all. These systems must be supported as well.
> 
> Luckily we now have acpi_dev_add_driver_gpios() that can be used to provide
> mappings for systems where _DSD is not provided and still take advantage of
> _DSD if it exists.
> 
> This patch changes the driver to create default GPIO mappings if we are
> running on ACPI system.
> 
> While there we can drop the indices completely and use devm_gpiod_get()
> with name instead.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi,
> 
> This patch is based on top of linux-pm/device-properties [1] and following
> patch from Rafael [2].
> 
> Johannes, John,
> 
> If you are happy with the patch, can you ACK it so that we can merge it
> with the rest of the device-properties patches. Thanks.

Acked-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John W. Linville Oct. 27, 2014, 3:21 p.m. UTC | #2
On Mon, Oct 27, 2014 at 12:23:55PM +0100, Johannes Berg wrote:
> On Mon, 2014-10-27 at 12:15 +0200, Mika Westerberg wrote:
> > The driver uses devm_gpiod_get_index(..., index) so that the index refers
> > directly to the GpioIo resource under the ACPI device. The problem with
> > this is that if the ordering changes we get wrong GPIOs.
> > 
> > With ACPI 5.1 _DSD we can now use names instead to reference GPIOs
> > analogous to Device Tree. However, we still have systems out there that do
> > not provide _DSD at all. These systems must be supported as well.
> > 
> > Luckily we now have acpi_dev_add_driver_gpios() that can be used to provide
> > mappings for systems where _DSD is not provided and still take advantage of
> > _DSD if it exists.
> > 
> > This patch changes the driver to create default GPIO mappings if we are
> > running on ACPI system.
> > 
> > While there we can drop the indices completely and use devm_gpiod_get()
> > with name instead.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Hi,
> > 
> > This patch is based on top of linux-pm/device-properties [1] and following
> > patch from Rafael [2].
> > 
> > Johannes, John,
> > 
> > If you are happy with the patch, can you ACK it so that we can merge it
> > with the rest of the device-properties patches. Thanks.
> 
> Acked-by: Johannes Berg <johannes@sipsolutions.net>
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Acked-by: John W. Linville <linville@tuxdriver.com>
Linus Walleij Oct. 30, 2014, 3:20 p.m. UTC | #3
On Mon, Oct 27, 2014 at 11:15 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> The driver uses devm_gpiod_get_index(..., index) so that the index refers
> directly to the GpioIo resource under the ACPI device. The problem with
> this is that if the ordering changes we get wrong GPIOs.
>
> With ACPI 5.1 _DSD we can now use names instead to reference GPIOs
> analogous to Device Tree. However, we still have systems out there that do
> not provide _DSD at all. These systems must be supported as well.
>
> Luckily we now have acpi_dev_add_driver_gpios() that can be used to provide
> mappings for systems where _DSD is not provided and still take advantage of
> _DSD if it exists.
>
> This patch changes the driver to create default GPIO mappings if we are
> running on ACPI system.
>
> While there we can drop the indices completely and use devm_gpiod_get()
> with name instead.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi,
>
> This patch is based on top of linux-pm/device-properties [1] and following
> patch from Rafael [2].
>
> Johannes, John,
>
> If you are happy with the patch, can you ACK it so that we can merge it
> with the rest of the device-properties patches. Thanks.
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> [2] https://lkml.org/lkml/2014/10/24/690
(...)
> +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 },
> +       { },
> +};

Jaysis, it could use a comment above the map telling the poor code
reader what is actually going on.

Other than that, this is what we have to live with I guess so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 0f62326c0f5e..2a4717967502 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)
@@ -102,7 +112,7 @@  static int rfkill_gpio_probe(struct platform_device *pdev)
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);
 
-	gpio = devm_gpiod_get_index(&pdev->dev, "reset", 0);
+	gpio = devm_gpiod_get(&pdev->dev, "reset");
 	if (!IS_ERR(gpio)) {
 		ret = gpiod_direction_output(gpio, 0);
 		if (ret)
@@ -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(&pdev->dev, "shutdown");
 	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;
 }