Hi,
I was waiting for the DT support from Chen-Yu before sending these,
but decided it makes no difference when I send them. I'm dropping the
con ID in the second patch because Dan noticed the warning, but of
course it will mean the "gpios" property can be used with DT.
The two last patches just add ACPI IDs for some new Baytrail based
boards.
Heikki Krogerus (4):
net: rfkill: gpio: remove unused and obsolete platform parameters
net: rfkill: gpio: remove gpio names
net: rfkill: gpio: add ACPI ID for GPS module on Lenove Miix2
net: rfkill: gpio: add ACPI IDs for a Broadcom bluetooth chip
include/linux/rfkill-gpio.h | 10 ----------
net/rfkill/rfkill-gpio.c | 38 +++++++-------------------------------
2 files changed, 7 insertions(+), 41 deletions(-)
--
1.9.0.rc3
After upgrading to descriptor based gpios, the gpio numbers
are not used anymore. The power_clk_name and the platform
specific setup and close hooks are not used by anybody, and
we should not encourage use of such things, so removing them.
Signed-off-by: Heikki Krogerus <[email protected]>
---
include/linux/rfkill-gpio.h | 10 ----------
net/rfkill/rfkill-gpio.c | 15 +--------------
2 files changed, 1 insertion(+), 24 deletions(-)
diff --git a/include/linux/rfkill-gpio.h b/include/linux/rfkill-gpio.h
index 4d09f6e..20bcb55 100644
--- a/include/linux/rfkill-gpio.h
+++ b/include/linux/rfkill-gpio.h
@@ -27,21 +27,11 @@
* struct rfkill_gpio_platform_data - platform data for rfkill gpio device.
* for unused gpio's, the expected value is -1.
* @name: name for the gpio rf kill instance
- * @reset_gpio: GPIO which is used for reseting rfkill switch
- * @shutdown_gpio: GPIO which is used for shutdown of rfkill switch
- * @power_clk_name: [optional] name of clk to turn off while blocked
- * @gpio_runtime_close: clean up platform specific gpio configuration
- * @gpio_runtime_setup: set up platform specific gpio configuration
*/
struct rfkill_gpio_platform_data {
char *name;
- int reset_gpio;
- int shutdown_gpio;
- const char *power_clk_name;
enum rfkill_type type;
- void (*gpio_runtime_close)(struct platform_device *);
- int (*gpio_runtime_setup)(struct platform_device *);
};
#endif /* __RFKILL_GPIO_H */
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index bd2a5b9..0adda44 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -87,7 +87,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
{
struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
struct rfkill_gpio_data *rfkill;
- const char *clk_name = NULL;
struct gpio_desc *gpio;
int ret;
int len;
@@ -101,7 +100,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
if (ret)
return ret;
} else if (pdata) {
- clk_name = pdata->power_clk_name;
rfkill->name = pdata->name;
rfkill->type = pdata->type;
} else {
@@ -120,7 +118,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
snprintf(rfkill->reset_name, len + 6 , "%s_reset", rfkill->name);
snprintf(rfkill->shutdown_name, len + 9, "%s_shutdown", rfkill->name);
- rfkill->clk = devm_clk_get(&pdev->dev, clk_name);
+ rfkill->clk = devm_clk_get(&pdev->dev, NULL);
gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
if (!IS_ERR(gpio)) {
@@ -146,14 +144,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
return -EINVAL;
}
- if (pdata && pdata->gpio_runtime_setup) {
- ret = pdata->gpio_runtime_setup(pdev);
- if (ret) {
- dev_err(&pdev->dev, "can't set up gpio\n");
- return ret;
- }
- }
-
rfkill->rfkill_dev = rfkill_alloc(rfkill->name, &pdev->dev,
rfkill->type, &rfkill_gpio_ops,
rfkill);
@@ -174,10 +164,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
static int rfkill_gpio_remove(struct platform_device *pdev)
{
struct rfkill_gpio_data *rfkill = platform_get_drvdata(pdev);
- struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
- if (pdata && pdata->gpio_runtime_close)
- pdata->gpio_runtime_close(pdev);
rfkill_unregister(rfkill->rfkill_dev);
rfkill_destroy(rfkill->rfkill_dev);
--
1.9.0.rc3
This adds ACPI IDs for Broadcom bluetooth chip BCM43241 used
on various Baytrail based boards such as Lenovo Miix 2 and
Asus Transformer Book T100TA.
Signed-off-by: Heikki Krogerus <[email protected]>
---
net/rfkill/rfkill-gpio.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index ec38884..3a38861 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -157,6 +157,9 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
}
static const struct acpi_device_id rfkill_acpi_match[] = {
+ { "BCM2E1A", RFKILL_TYPE_BLUETOOTH },
+ { "BCM2E39", RFKILL_TYPE_BLUETOOTH },
+ { "BCM2E3D", RFKILL_TYPE_BLUETOOTH },
{ "BCM4752", RFKILL_TYPE_GPS },
{ "LNV4752", RFKILL_TYPE_GPS },
{ },
--
1.9.0.rc3
On Lenovo Miix 2 8", BCM4752 is renamed LNV4752.
Signed-off-by: Heikki Krogerus <[email protected]>
---
net/rfkill/rfkill-gpio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index ad5e354..ec38884 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -158,6 +158,7 @@ static int rfkill_gpio_remove(struct platform_device *pdev)
static const struct acpi_device_id rfkill_acpi_match[] = {
{ "BCM4752", RFKILL_TYPE_GPS },
+ { "LNV4752", RFKILL_TYPE_GPS },
{ },
};
--
1.9.0.rc3
There is no use for them in this driver. This will fix a
static checker warning..
net/rfkill/rfkill-gpio.c:144 rfkill_gpio_probe()
warn: variable dereferenced before check 'rfkill->name'
This will also make sure that when DT support is added,
"gpios" property can be used as no con_id labels are
provided.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
---
net/rfkill/rfkill-gpio.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 0adda44..ad5e354 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -36,8 +36,6 @@ struct rfkill_gpio_data {
struct gpio_desc *shutdown_gpio;
struct rfkill *rfkill_dev;
- char *reset_name;
- char *shutdown_name;
struct clk *clk;
bool clk_enabled;
@@ -89,7 +87,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
struct rfkill_gpio_data *rfkill;
struct gpio_desc *gpio;
int ret;
- int len;
rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL);
if (!rfkill)
@@ -106,21 +103,9 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
return -ENODEV;
}
- len = strlen(rfkill->name);
- rfkill->reset_name = devm_kzalloc(&pdev->dev, len + 7, GFP_KERNEL);
- if (!rfkill->reset_name)
- return -ENOMEM;
-
- rfkill->shutdown_name = devm_kzalloc(&pdev->dev, len + 10, GFP_KERNEL);
- if (!rfkill->shutdown_name)
- return -ENOMEM;
-
- snprintf(rfkill->reset_name, len + 6 , "%s_reset", rfkill->name);
- snprintf(rfkill->shutdown_name, len + 9, "%s_shutdown", rfkill->name);
-
rfkill->clk = devm_clk_get(&pdev->dev, NULL);
- gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
+ gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0);
if (!IS_ERR(gpio)) {
ret = gpiod_direction_output(gpio, 0);
if (ret)
@@ -128,7 +113,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
rfkill->reset_gpio = gpio;
}
- gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1);
+ gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1);
if (!IS_ERR(gpio)) {
ret = gpiod_direction_output(gpio, 0);
if (ret)
--
1.9.0.rc3
On 02/20/2014 05:51 AM, Heikki Krogerus wrote:
> There is no use for them in this driver. This will fix a
> static checker warning..
Didn't you remove the use:
- gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
+ gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0);
doesn't that parameter get put into the sysfs GPIO debug file, so people
can see which GPIOs are used for what?
Hi,
On Fri, Feb 21, 2014 at 12:38 AM, Stephen Warren <[email protected]> wrote:
> On 02/20/2014 05:51 AM, Heikki Krogerus wrote:
>> There is no use for them in this driver. This will fix a
>> static checker warning..
>
> Didn't you remove the use:
>
> - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
> + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0);
>
> doesn't that parameter get put into the sysfs GPIO debug file, so people
> can see which GPIOs are used for what?
That's correct. However using con_id to pass this results in different
behavior across DT and ACPI. A better way is to export the labeling
function so consumers can set meaningful labels themselves.
Cheers
ChenYu
On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
> Hi,
>
> On Fri, Feb 21, 2014 at 12:38 AM, Stephen Warren <[email protected]> wrote:
>> On 02/20/2014 05:51 AM, Heikki Krogerus wrote:
>>> There is no use for them in this driver. This will fix a
>>> static checker warning..
>>
>> Didn't you remove the use:
>>
>> - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
>> + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0);
>>
>> doesn't that parameter get put into the sysfs GPIO debug file, so people
>> can see which GPIOs are used for what?
>
> That's correct. However using con_id to pass this results in different
> behavior across DT and ACPI. A better way is to export the labeling
> function so consumers can set meaningful labels themselves.
But this code is the consumer of those GPIOs. IF the parameter to
devm_gpiod_get_index() isn't intended to be used, why does it exist?
Hi Heikki,
Am Donnerstag, 20. Februar 2014, 14:51:34 schrieb Heikki Krogerus:
> After upgrading to descriptor based gpios, the gpio numbers
> are not used anymore. The power_clk_name and the platform
> specific setup and close hooks are not used by anybody, and
> we should not encourage use of such things, so removing them.
arch/arm/mach-tegra/board-paz00.c is still using platform data. Is there some
prerequisite patch I'm missing (3.14-rc3) or how can this file be converted?
We are waiting for DT support to arrive so we can finally remove this file.
Marc
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> include/linux/rfkill-gpio.h | 10 ----------
> net/rfkill/rfkill-gpio.c | 15 +--------------
> 2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/include/linux/rfkill-gpio.h b/include/linux/rfkill-gpio.h
> index 4d09f6e..20bcb55 100644
> --- a/include/linux/rfkill-gpio.h
> +++ b/include/linux/rfkill-gpio.h
> @@ -27,21 +27,11 @@
> * struct rfkill_gpio_platform_data - platform data for rfkill gpio device.
> * for unused gpio's, the expected value is -1.
> * @name: name for the gpio rf kill instance
> - * @reset_gpio: GPIO which is used for reseting rfkill switch
> - * @shutdown_gpio: GPIO which is used for shutdown of rfkill switch
> - * @power_clk_name: [optional] name of clk to turn off while blocked
> - * @gpio_runtime_close: clean up platform specific gpio configuration
> - * @gpio_runtime_setup: set up platform specific gpio configuration
> */
>
> struct rfkill_gpio_platform_data {
> char *name;
> - int reset_gpio;
> - int shutdown_gpio;
> - const char *power_clk_name;
> enum rfkill_type type;
> - void (*gpio_runtime_close)(struct platform_device *);
> - int (*gpio_runtime_setup)(struct platform_device *);
> };
>
> #endif /* __RFKILL_GPIO_H */
> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
> index bd2a5b9..0adda44 100644
> --- a/net/rfkill/rfkill-gpio.c
> +++ b/net/rfkill/rfkill-gpio.c
> @@ -87,7 +87,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
> {
> struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
> struct rfkill_gpio_data *rfkill;
> - const char *clk_name = NULL;
> struct gpio_desc *gpio;
> int ret;
> int len;
> @@ -101,7 +100,6 @@ static int rfkill_gpio_probe(struct platform_device
> *pdev) if (ret)
> return ret;
> } else if (pdata) {
> - clk_name = pdata->power_clk_name;
> rfkill->name = pdata->name;
> rfkill->type = pdata->type;
> } else {
> @@ -120,7 +118,7 @@ static int rfkill_gpio_probe(struct platform_device
> *pdev) snprintf(rfkill->reset_name, len + 6 , "%s_reset", rfkill->name);
> snprintf(rfkill->shutdown_name, len + 9, "%s_shutdown", rfkill->name);
>
> - rfkill->clk = devm_clk_get(&pdev->dev, clk_name);
> + rfkill->clk = devm_clk_get(&pdev->dev, NULL);
>
> gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0);
> if (!IS_ERR(gpio)) {
> @@ -146,14 +144,6 @@ static int rfkill_gpio_probe(struct platform_device
> *pdev) return -EINVAL;
> }
>
> - if (pdata && pdata->gpio_runtime_setup) {
> - ret = pdata->gpio_runtime_setup(pdev);
> - if (ret) {
> - dev_err(&pdev->dev, "can't set up gpio\n");
> - return ret;
> - }
> - }
> -
> rfkill->rfkill_dev = rfkill_alloc(rfkill->name, &pdev->dev,
> rfkill->type, &rfkill_gpio_ops,
> rfkill);
> @@ -174,10 +164,7 @@ static int rfkill_gpio_probe(struct platform_device
> *pdev) static int rfkill_gpio_remove(struct platform_device *pdev)
> {
> struct rfkill_gpio_data *rfkill = platform_get_drvdata(pdev);
> - struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
>
> - if (pdata && pdata->gpio_runtime_close)
> - pdata->gpio_runtime_close(pdev);
> rfkill_unregister(rfkill->rfkill_dev);
> rfkill_destroy(rfkill->rfkill_dev);
Hi,
On Fri, Feb 21, 2014 at 02:55:14PM +0100, Marc Dietrich wrote:
> Am Donnerstag, 20. Februar 2014, 14:51:34 schrieb Heikki Krogerus:
> > After upgrading to descriptor based gpios, the gpio numbers
> > are not used anymore. The power_clk_name and the platform
> > specific setup and close hooks are not used by anybody, and
> > we should not encourage use of such things, so removing them.
>
> arch/arm/mach-tegra/board-paz00.c is still using platform data. Is there some
> prerequisite patch I'm missing (3.14-rc3) or how can this file be converted?
> We are waiting for DT support to arrive so we can finally remove this file.
True! It still set's the shutdown_gpio and reset_gpio members. I think
I'll leave the header untouched and just clean net/rfkill/rfkill-gpio.c
in this patch. We can remove the whole header after you guys have moved
to DT.
I'll prepare v2 next week.
Thanks!
--
heikki
On Friday 21 February 2014 16:23:49 Heikki Krogerus wrote:
> Hi,
>
> On Fri, Feb 21, 2014 at 02:55:14PM +0100, Marc Dietrich wrote:
> > Am Donnerstag, 20. Februar 2014, 14:51:34 schrieb Heikki Krogerus:
> > > After upgrading to descriptor based gpios, the gpio numbers
> > > are not used anymore. The power_clk_name and the platform
> > > specific setup and close hooks are not used by anybody, and
> > > we should not encourage use of such things, so removing them.
> >
> > arch/arm/mach-tegra/board-paz00.c is still using platform data. Is there
> > some prerequisite patch I'm missing (3.14-rc3) or how can this file be
> > converted? We are waiting for DT support to arrive so we can finally
> > remove this file.
> True! It still set's the shutdown_gpio and reset_gpio members. I think
> I'll leave the header untouched and just clean net/rfkill/rfkill-gpio.c
> in this patch. We can remove the whole header after you guys have moved
> to DT.
Why? Just update the board file to remove those entries. I just checked that
it's working fine without. So if you don't mind, add a patch to remove the
entries as the first patch in your series.
Thanks
Marc
On Sat, Feb 22, 2014 at 11:32:04PM +0100, Marc Dietrich wrote:
> On Friday 21 February 2014 16:23:49 Heikki Krogerus wrote:
> > On Fri, Feb 21, 2014 at 02:55:14PM +0100, Marc Dietrich wrote:
> > > arch/arm/mach-tegra/board-paz00.c is still using platform data. Is there
> > > some prerequisite patch I'm missing (3.14-rc3) or how can this file be
> > > converted? We are waiting for DT support to arrive so we can finally
> > > remove this file.
> > True! It still set's the shutdown_gpio and reset_gpio members. I think
> > I'll leave the header untouched and just clean net/rfkill/rfkill-gpio.c
> > in this patch. We can remove the whole header after you guys have moved
> > to DT.
>
> Why? Just update the board file to remove those entries. I just checked that
> it's working fine without. So if you don't mind, add a patch to remove the
> entries as the first patch in your series.
I was hoping I wouldn't have to touch arch/arm/mach-tegra/board-paz00.c
any more. But OK, I'll add a patch and remove the entries.
Thanks,
--
heikki
Hi,
Am Montag, 24. Februar 2014, 10:38:20 schrieb Heikki Krogerus:
> On Sat, Feb 22, 2014 at 11:32:04PM +0100, Marc Dietrich wrote:
> > On Friday 21 February 2014 16:23:49 Heikki Krogerus wrote:
> > > On Fri, Feb 21, 2014 at 02:55:14PM +0100, Marc Dietrich wrote:
> > > > arch/arm/mach-tegra/board-paz00.c is still using platform data. Is
> > > > there
> > > > some prerequisite patch I'm missing (3.14-rc3) or how can this file be
> > > > converted? We are waiting for DT support to arrive so we can finally
> > > > remove this file.
> > >
> > > True! It still set's the shutdown_gpio and reset_gpio members. I think
> > > I'll leave the header untouched and just clean net/rfkill/rfkill-gpio.c
> > > in this patch. We can remove the whole header after you guys have moved
> > > to DT.
> >
> > Why? Just update the board file to remove those entries. I just checked
> > that it's working fine without. So if you don't mind, add a patch to
> > remove the entries as the first patch in your series.
>
> I was hoping I wouldn't have to touch arch/arm/mach-tegra/board-paz00.c
> any more. But OK, I'll add a patch and remove the entries.
hey, someone has to do it :-) I wouldn't mind to do it myself, but you are
trying to break it, so I think it's your reponsibility to fix it.
Marc
On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <[email protected]> wrote:
> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
>> That's correct. However using con_id to pass this results in different
>> behavior across DT and ACPI. A better way is to export the labeling
>> function so consumers can set meaningful labels themselves.
>
> But this code is the consumer of those GPIOs. IF the parameter to
> devm_gpiod_get_index() isn't intended to be used, why does it exist?
Kerneldoc says:
/**
* gpiod_get_index - obtain a GPIO from a multi-index GPIO function
* @dev: GPIO consumer, can be NULL for system-global GPIOs
* @con_id: function within the GPIO consumer
* @idx: index of the GPIO to obtain in the consumer
*
Basically it is just exposing the fact that of_find_gpio() and
acpi_find_gpio() both take a con_id as argument.
If we drill into this, we find that it is used to conjure the
arbitrary string before the gpios in the DT case, like:
foo-gpios = <...>;
As in tegra30-beaver.dts...
sdhci@78000000 {
status = "okay";
cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
bus-width = <4>;
};
Instead of passing the GPIOs as index 0,1,2 they are named
and I do admit this has a nice "things are under control" aspect
to it.
In the ACPI case the con_id is not used for anything.
So it is basically there to satisfy the habit in some device
tree bindings to name gpio arrays instead of just passing gpios = <...>;
(The latter should be encouraged going forward.)
As DT is ABI we need to keep this forever, and driver may need to
look for foo-gpios=<> and gpios=<> alike for backward compatibility
if we'd change it, sweet isn't it? :-)
I don't quite like this, as it is adding stupid nonsens arguments for
ACPI GPIO producers (which only take an index AFAICT), but it is a
first sacrifice on the altar of trying to mask the differences between DT
and ACPI probe paths about which I have mixed feelings.
Yours,
LInus Walleij
On 02/25/2014 02:13 AM, Linus Walleij wrote:
> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <[email protected]> wrote:
>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
>
>>> That's correct. However using con_id to pass this results in different
>>> behavior across DT and ACPI. A better way is to export the labeling
>>> function so consumers can set meaningful labels themselves.
...
> As in tegra30-beaver.dts...
>
> sdhci@78000000 {
> status = "okay";
> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
> bus-width = <4>;
> };
>
> Instead of passing the GPIOs as index 0,1,2 they are named
> and I do admit this has a nice "things are under control" aspect
> to it.
>
> In the ACPI case the con_id is not used for anything.
>
> So it is basically there to satisfy the habit in some device
> tree bindings to name gpio arrays instead of just passing gpios = <...>;
> (The latter should be encouraged going forward.)
Do you really want to switch from named GPIO lookups to index-based GPIO
lookups? Index-based lookups make it much harder to extend the DT
binding in a backwards-compatible fashion, especially in the face of
optional GPIOs (of which all of CD, WP, power are).
If we switch to a single gpios property, I'd assert we should still do
named-based lookups using a parallel gpio-names property, just like most
(all?) other resource types now support. If we do that, we'll still need
the name parameter.
Please know that no one should not consider me an authority on ACPI at this time. But, I have some comments / context / thoughts below.
Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client. Folks can use [email protected] to avoid outlook-isms from me in the future.
> -----Original Message-----
> From: Linus Walleij [mailto:[email protected]]
> Sent: Tuesday, February 25, 2014 1:14 AM
> To: Stephen Warren; Alexandre Courbot; Grant Likely;
> [email protected]
> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
>
> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
> <[email protected]> wrote:
> > On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
>
> >> That's correct. However using con_id to pass this results in
> >> different behavior across DT and ACPI. A better way is to export the
> >> labeling function so consumers can set meaningful labels themselves.
> >
> > But this code is the consumer of those GPIOs. IF the parameter to
> > devm_gpiod_get_index() isn't intended to be used, why does it exist?
>
> Kerneldoc says:
>
> /**
> * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> * @dev: GPIO consumer, can be NULL for system-global GPIOs
> * @con_id: function within the GPIO consumer
> * @idx: index of the GPIO to obtain in the consumer
> *
>
> Basically it is just exposing the fact that of_find_gpio() and
> acpi_find_gpio() both take a con_id as argument.
>
> If we drill into this, we find that it is used to conjure the arbitrary string
> before the gpios in the DT case, like:
>
> foo-gpios = <...>;
>
> As in tegra30-beaver.dts...
>
> sdhci@78000000 {
> status = "okay";
> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
> bus-width = <4>;
> };
>
> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
> this has a nice "things are under control" aspect to it.
[Gross, Mark] FWIW I don't think this is as "under control" as you do. Those names in the above sdhci example are derived from a specific SDHCI tegra spec sheet or schematic. Those names likely come from the data sheet for the controller. I would like SDHCI kernel drivers to work pretty much the same for all compatible controllers. The set of compatible controllers have spec sheets that use different naming conventions for their pins and thus a name space pollution of the SDHCI enumeration ABI will be a problem.
>
> In the ACPI case the con_id is not used for anything.
>
> So it is basically there to satisfy the habit in some device tree bindings to
> name gpio arrays instead of just passing gpios = <...>; (The latter should be
> encouraged going forward.)
[Gross, Mark] This is in fact just an idiom of the ACPI ABI inherited from writing linux drivers that work on systems with BIOS/FW developed for MS-Windows. For the devices I work on we have a number of driver teams filing bugs against the BIOS/FW to add named GPIO objects to device name spaces such that they can directly query for the GPIO their driver needs and avoid assuming the 3rd GPIO is the special one they need for whatever...
So if you have the luxury of being able to influence (file bugs against or write) the platform enumeration ABI then with ACPI you can have a named gpio today. Note: there is an expectation that the _PRP feature to go into the next version of ACPI and that should enable a trivial 1-1 mapping (when _prp is used by the ACPI platform) between the different platform enumeration API's (DT and ACPI).
Still, from a platform enumeration ABI point of view I see the arbitrariness of indexes not much worse than the arbitrary naming of pins off of spec sheets or schematics. Given that the authors of those specs/layouts come up with a new naming schemes every time they log into their workstations (especially for the schematics). I do admit names are a little better but still have the scaling issue WRT namespaces and arbitrary naming by HW engineers over time.
>
> As DT is ABI we need to keep this forever, and driver may need to look for
> foo-gpios=<> and gpios=<> alike for backward compatibility if we'd change it,
> sweet isn't it? :-)
>
> I don't quite like this, as it is adding stupid nonsens arguments for ACPI GPIO
> producers (which only take an index AFAICT), but it is a first sacrifice on the
> altar of trying to mask the differences between DT and ACPI probe paths
> about which I have mixed feelings.
[Gross, Mark] I don't have mixed feelings. I want to see converged probe paths that can handle both ACPI and DT platform ABI's seamlessly so we can reuse drivers across architectures effectively. I think the _PRP will help with that even though its use assumes you have influence over the platform FW and this will not help with supporting of legacy BIOS's will burden us with gpio index assumptions.
--mark
>
> Yours,
> LInus Walleij
On 02/27/2014 10:38 AM, Gross, Mark wrote:
> Please know that no one should not consider me an authority on ACPI at this time. But, I have some comments / context / thoughts below.
>
> Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client. Folks can use [email protected] to avoid outlook-isms from me in the future.
>
>> -----Original Message-----
>> From: Linus Walleij [mailto:[email protected]]
>> Sent: Tuesday, February 25, 2014 1:14 AM
>> To: Stephen Warren; Alexandre Courbot; Grant Likely;
>> [email protected]
>> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
>> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
>> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
>>
>> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
>> <[email protected]> wrote:
>>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
>>
>>>> That's correct. However using con_id to pass this results in
>>>> different behavior across DT and ACPI. A better way is to export the
>>>> labeling function so consumers can set meaningful labels themselves.
>>>
>>> But this code is the consumer of those GPIOs. IF the parameter to
>>> devm_gpiod_get_index() isn't intended to be used, why does it exist?
>>
>> Kerneldoc says:
>>
>> /**
>> * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
>> * @dev: GPIO consumer, can be NULL for system-global GPIOs
>> * @con_id: function within the GPIO consumer
>> * @idx: index of the GPIO to obtain in the consumer
>> *
>>
>> Basically it is just exposing the fact that of_find_gpio() and
>> acpi_find_gpio() both take a con_id as argument.
>>
>> If we drill into this, we find that it is used to conjure the arbitrary string
>> before the gpios in the DT case, like:
>>
>> foo-gpios = <...>;
>>
>> As in tegra30-beaver.dts...
>>
>> sdhci@78000000 {
>> status = "okay";
>> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
>> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
>> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
>> bus-width = <4>;
>> };
>>
>> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
>> this has a nice "things are under control" aspect to it.
>
> [Gross, Mark] FWIW I don't think this is as "under control" as you do. Those
> names in the above sdhci example are derived from a specific SDHCI
tegra spec
> sheet or schematic. Those names likely come from the data sheet for
> the controller.
The names of the properties are fixed and defined by the DT binding for
the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they
will be the same in every DT file that uses that Tegra SDHCI compatible
value (the compatible property isn't show above, because the above
fragment is a board.dts file, and the compatible value gets inherited
from the soc.dtsi file). There won't be any variation at all,
irrespective of what signal names exist in a particular board schematic.
If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI
controller, I would hope it would use the exact same names for the GPIO
signals.
On Thu, Feb 27, 2014 at 10:47:58AM -0700, Stephen Warren wrote:
> On 02/27/2014 10:38 AM, Gross, Mark wrote:
> > Please know that no one should not consider me an authority on ACPI at this
> > time. But, I have some comments / context / thoughts below.
> >
> > Also I apologize in advance for any email formatting issues caused by
> > replying to this via my work exchange account / outlook client. Folks can
> > use [email protected] to avoid outlook-isms from me in the future.
> >
> >> -----Original Message-----
> >> From: Linus Walleij [mailto:[email protected]]
> >> Sent: Tuesday, February 25, 2014 1:14 AM
> >> To: Stephen Warren; Alexandre Courbot; Grant Likely;
> >> [email protected]
> >> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland
> >> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark
> >> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names
> >>
> >> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren
> >> <[email protected]> wrote:
> >>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote:
> >>
> >>>> That's correct. However using con_id to pass this results in
> >>>> different behavior across DT and ACPI. A better way is to export the
> >>>> labeling function so consumers can set meaningful labels themselves.
> >>>
> >>> But this code is the consumer of those GPIOs. IF the parameter to
> >>> devm_gpiod_get_index() isn't intended to be used, why does it exist?
> >>
> >> Kerneldoc says:
> >>
> >> /**
> >> * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> >> * @dev: GPIO consumer, can be NULL for system-global GPIOs
> >> * @con_id: function within the GPIO consumer
> >> * @idx: index of the GPIO to obtain in the consumer
> >> *
> >>
> >> Basically it is just exposing the fact that of_find_gpio() and
> >> acpi_find_gpio() both take a con_id as argument.
> >>
> >> If we drill into this, we find that it is used to conjure the arbitrary string
> >> before the gpios in the DT case, like:
> >>
> >> foo-gpios = <...>;
> >>
> >> As in tegra30-beaver.dts...
> >>
> >> sdhci@78000000 {
> >> status = "okay";
> >> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
> >> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
> >> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
> >> bus-width = <4>;
> >> };
> >>
> >> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit
> >> this has a nice "things are under control" aspect to it.
> >
> > [Gross, Mark] FWIW I don't think this is as "under control" as you do. Those
> > names in the above sdhci example are derived from a specific SDHCI
> tegra spec
> > sheet or schematic. Those names likely come from the data sheet for
> > the controller.
>
> The names of the properties are fixed and defined by the DT binding for
> the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they
> will be the same in every DT file that uses that Tegra SDHCI compatible
> value (the compatible property isn't show above, because the above
> fragment is a board.dts file, and the compatible value gets inherited
> from the soc.dtsi file). There won't be any variation at all,
> irrespective of what signal names exist in a particular board schematic.
>
> If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI
> controller, I would hope it would use the exact same names for the GPIO
> signals.
me to!
--mark