[PATCH 0/5] net: rfkill: gpio: replace platform data with build-in property

The first patch adds a few helper macros for build-in property
creation and the second makes it possible to get the rfkill type
index based on name. The rest deal with rfkill-gpio.

Cheers,


Heikki Krogerus (5):
  device property: helper macros for property entry creation
  net: rfkill: add rfkill_find_type function
  net: rfkill: gpio: get the name and type from device property
  ARM: tegra: use build-in device properties with rfkill_gpio
  net: rfkill: gpio: remove rfkill_gpio_platform_data

 arch/arm/mach-tegra/board-paz00.c | 17 +++++++-----
 include/linux/property.h          | 35 ++++++++++++++++++++++++
 include/linux/rfkill-gpio.h       | 37 -------------------------
 include/linux/rfkill.h            | 15 +++++++++++
 net/rfkill/Kconfig                |  3 +--
 net/rfkill/core.c                 | 57 ++++++++++++++++++++-------------------
 net/rfkill/rfkill-gpio.c          | 24 ++++++++---------
 7 files changed, 101 insertions(+), 87 deletions(-)
 delete mode 100644 include/linux/rfkill-gpio.h

-- 
2.4.6

Marcos for easier creation of build-in property entries.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/property.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..204d899 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h

@@ -152,6 +152,41 @@ struct property_entry {
 	} value;
 };
 
+#define PROP_ENTRY_U8(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U8, \
+	.nval = 1, \
+	.value.u8_data = _val_, \
+}
+
+#define PROP_ENTRY_U16(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U16, \
+	.nval = 1, \
+	.value.u16_data = _val_, \
+}
+
+#define PROP_ENTRY_U32(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U32, \
+	.nval = 1, \
+	.value.u32_data = _val_, \
+}
+
+#define PROP_ENTRY_U64(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U64, \
+	.nval = 1, \
+	.value.u64_data = _val_, \
+}
+
+#define PROP_ENTRY_STRING(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_STRING, \
+	.nval = 1, \
+	.value.str = (const char **)_val_, \
+}
+
 /**
  * struct property_set - Collection of "built-in" device properties.
  * @fwnode: Handle to be pointed to by the fwnode field of struct device.

-- 
2.4.6

This prepares the driver for removal of platform data.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 net/rfkill/rfkill-gpio.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index d5d58d9..07323c3 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c

@@ -81,7 +81,6 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 	if (!id)
 		return -ENODEV;
 
-	rfkill->name = dev_name(dev);
 	rfkill->type = (unsigned)id->driver_data;
 
 	return acpi_dev_add_driver_gpios(ACPI_COMPANION(dev),

@@ -93,12 +92,21 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct rfkill_gpio_data *rfkill;
 	struct gpio_desc *gpio;
+	const char *type_name;
 	int ret;
 
 	rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL);
 	if (!rfkill)
 		return -ENOMEM;
 
+	device_property_read_string(&pdev->dev, "name", &rfkill->name);
+	device_property_read_string(&pdev->dev, "type", &type_name);
+
+	if (!rfkill->name)
+		rfkill->name = dev_name(&pdev->dev);
+
+	rfkill->type = rfkill_find_type(type_name);
+
 	if (ACPI_HANDLE(&pdev->dev)) {
 		ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill);
 		if (ret)

@@ -124,10 +132,8 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 
 	rfkill->shutdown_gpio = gpio;
 
-	/* Make sure at-least one of the GPIO is defined and that
-	 * a name is specified for this instance
-	 */
-	if ((!rfkill->reset_gpio && !rfkill->shutdown_gpio) || !rfkill->name) {
+	/* Make sure at-least one GPIO is defined for this instance */
+	if (!rfkill->reset_gpio && !rfkill->shutdown_gpio) {
 		dev_err(&pdev->dev, "invalid platform data\n");
 		return -EINVAL;
 	}

-- 
2.4.6

Helper for finding the type based on name. Useful if the
type needs to be determined based on device property.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/rfkill.h | 15 +++++++++++++
 net/rfkill/core.c      | 57 +++++++++++++++++++++++++-------------------------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index d901078..02f563c 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h

@@ -212,6 +212,15 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw);
  * @rfkill: rfkill struct to query
  */
 bool rfkill_blocked(struct rfkill *rfkill);
+
+/**
+ * rfkill_find_type - Helpper for finding rfkill type by name
+ * @name: the name of the type
+ *
+ * Returns enum rfkill_type that conrresponds the name.
+ */
+enum rfkill_type rfkill_find_type(const char *name);
+
 #else /* !RFKILL */
 static inline struct rfkill * __must_check
 rfkill_alloc(const char *name,

@@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill *rfkill)
 {
 	return false;
 }
+
+static inline enum rfkill_type rfkill_find_type(const char *name)
+{
+	return 0;
+}
+
 #endif /* RFKILL || RFKILL_MODULE */
 
 

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index f12149a..53d7a97e 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c

@@ -574,6 +574,33 @@ void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
 }
 EXPORT_SYMBOL(rfkill_set_states);
 
+static const char *rfkill_types[NUM_RFKILL_TYPES] = {
+	[RFKILL_TYPE_WLAN]	= "wlan",
+	[RFKILL_TYPE_BLUETOOTH]	= "bluetooth",
+	[RFKILL_TYPE_UWB]	= "ultrawideband",
+	[RFKILL_TYPE_WIMAX]	= "wimax",
+	[RFKILL_TYPE_WWAN]	= "wwan",
+	[RFKILL_TYPE_GPS]	= "gps",
+	[RFKILL_TYPE_FM]	= "fm",
+	[RFKILL_TYPE_NFC]	= "nfc",
+};
+
+enum rfkill_type rfkill_find_type(const char *name)
+{
+	int i;
+
+	BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1);
+
+	if (!name)
+		return RFKILL_TYPE_ALL;
+
+	for (i = 1; i < NUM_RFKILL_TYPES; i++)
+		if (!strcmp(name, rfkill_types[i]))
+			return i;
+	return RFKILL_TYPE_ALL;
+}
+EXPORT_SYMBOL(rfkill_find_type);
+
 static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {

@@ -583,38 +610,12 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(name);
 
-static const char *rfkill_get_type_str(enum rfkill_type type)
-{
-	BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1);
-
-	switch (type) {
-	case RFKILL_TYPE_WLAN:
-		return "wlan";
-	case RFKILL_TYPE_BLUETOOTH:
-		return "bluetooth";
-	case RFKILL_TYPE_UWB:
-		return "ultrawideband";
-	case RFKILL_TYPE_WIMAX:
-		return "wimax";
-	case RFKILL_TYPE_WWAN:
-		return "wwan";
-	case RFKILL_TYPE_GPS:
-		return "gps";
-	case RFKILL_TYPE_FM:
-		return "fm";
-	case RFKILL_TYPE_NFC:
-		return "nfc";
-	default:
-		BUG();
-	}
-}
-
 static ssize_t type_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	struct rfkill *rfkill = to_rfkill(dev);
 
-	return sprintf(buf, "%s\n", rfkill_get_type_str(rfkill->type));
+	return sprintf(buf, "%s\n", rfkill_types[rfkill->type]);
 }
 static DEVICE_ATTR_RO(type);
 

@@ -760,7 +761,7 @@ static int rfkill_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (error)
 		return error;
 	error = add_uevent_var(env, "RFKILL_TYPE=%s",
-			       rfkill_get_type_str(rfkill->type));
+			       rfkill_types[rfkill->type]);
 	if (error)
 		return error;
 	spin_lock_irqsave(&rfkill->lock, flags);

-- 
2.4.6

No more users for it.

Signed-off-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/rfkill-gpio.h | 37 -------------------------------------
 net/rfkill/Kconfig          |  3 +--
 net/rfkill/rfkill-gpio.c    |  8 --------
 3 files changed, 1 insertion(+), 47 deletions(-)
 delete mode 100644 include/linux/rfkill-gpio.h

diff --git a/include/linux/rfkill-gpio.h b/include/linux/rfkill-gpio.h
deleted file mode 100644
index 20bcb55..0000000
--- a/include/linux/rfkill-gpio.h
+++ /dev/null

@@ -1,37 +0,0 @@
-/*
- * Copyright (c) 2011, NVIDIA Corporation.
- *
- * 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.,
- * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- */
-
-
-#ifndef __RFKILL_GPIO_H
-#define __RFKILL_GPIO_H
-
-#include <linux/types.h>
-#include <linux/rfkill.h>
-
-/**
- * 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
- */
-
-struct rfkill_gpio_platform_data {
-	char			*name;
-	enum rfkill_type	type;
-};
-
-#endif /* __RFKILL_GPIO_H */

diff --git a/net/rfkill/Kconfig b/net/rfkill/Kconfig
index 4c10e7e..6320890 100644
--- a/net/rfkill/Kconfig
+++ b/net/rfkill/Kconfig

@@ -40,5 +40,4 @@ config RFKILL_GPIO
 	default n
 	help
 	  If you say yes here you get support of a generic gpio RFKILL
-	  driver. The platform should fill in the appropriate fields in the
-	  rfkill_gpio_platform_data structure and pass that to the driver.
+	  driver.

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 07323c3..69d92e1 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c

@@ -27,8 +27,6 @@
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 
-#include <linux/rfkill-gpio.h>
-
 struct rfkill_gpio_data {
 	const char		*name;
 	enum rfkill_type	type;

@@ -89,7 +87,6 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 
 static int rfkill_gpio_probe(struct platform_device *pdev)
 {
-	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct rfkill_gpio_data *rfkill;
 	struct gpio_desc *gpio;
 	const char *type_name;

@@ -111,11 +108,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 		ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill);
 		if (ret)
 			return ret;
-	} else if (pdata) {
-		rfkill->name = pdata->name;
-		rfkill->type = pdata->type;
-	} else {
-		return -ENODEV;
 	}
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);

-- 
2.4.6

Pass the rfkill name and type to the device with properties
instead of driver specific platform data.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
CC: Alexandre Courbot <redacted>
CC: Thierry Reding <redacted>
CC: Stephen Warren <redacted>
---
 arch/arm/mach-tegra/board-paz00.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
index fbe74c6..5d8d3a4 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c

@@ -17,23 +17,25 @@
  *
  */
 
+#include <linux/property.h>
 #include <linux/gpio/machine.h>
 #include <linux/platform_device.h>
-#include <linux/rfkill-gpio.h>
 
 #include "board.h"
 
-static struct rfkill_gpio_platform_data wifi_rfkill_platform_data = {
-	.name	= "wifi_rfkill",
-	.type	= RFKILL_TYPE_WLAN,
+static struct property_entry wifi_rfkill_prop[] = {
+	PROP_ENTRY_STRING("name", "wifi_rfkill"),
+	PROP_ENTRY_STRING("type", "wlan"),
+	{ },
+};
+
+static struct property_set wifi_rfkill_pset = {
+	.properties = wifi_rfkill_prop,
 };
 
 static struct platform_device wifi_rfkill_device = {
 	.name	= "rfkill_gpio",
 	.id	= -1,
-	.dev	= {
-		.platform_data = &wifi_rfkill_platform_data,
-	},
 };
 
 static struct gpiod_lookup_table wifi_gpio_lookup = {

@@ -47,6 +49,7 @@ static struct gpiod_lookup_table wifi_gpio_lookup = {
 
 void __init tegra_paz00_wifikill_init(void)
 {
+	device_add_property_set(&wifi_rfkill_device.dev, &wifi_rfkill_pset);
 	gpiod_add_lookup_table(&wifi_gpio_lookup);
 	platform_device_register(&wifi_rfkill_device);
 }

-- 
2.4.6

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

PROP_ prefix is too generic.
Maybe DEVPROP_ ? At least for the latter no records in the current
sources.

+	.name = _name_, \
+	.type = DEV_PROP_U8, \
+	.nval = 1, \
+	.value.u8_data = _val_, \
+}
+
+#define PROP_ENTRY_U16(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U16, \
+	.nval = 1, \
+	.value.u16_data = _val_, \
+}
+
+#define PROP_ENTRY_U32(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U32, \
+	.nval = 1, \
+	.value.u32_data = _val_, \
+}
+
+#define PROP_ENTRY_U64(_name_, _val_) { \
+	.name = _name_, \
+	.type = DEV_PROP_U64, \
+	.nval = 1, \
+	.value.u64_data = _val_, \
+}
+
+#define PROP_ENTRY_STRING(_name_, _val_) { \

…_STRING_ARRAY I can notice.

+	.name = _name_, \
+	.type = DEV_PROP_STRING, \
+	.nval = 1, \
+	.value.str = (const char **)_val_, \
+}
+
 /**
  * struct property_set - Collection of "built-in" device properties.
  * @fwnode: Handle to be pointed to by the fwnode field of struct 
device.

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

Hmm… Besides 0 is implicitly casted to enum type the issue with enums
that you rather have to supply existing enum entry. I would suggest to
add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases.

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

On Wed, 2015-08-05 at 17:02 +0300, Andy Shevchenko wrote:

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

[]

quoted

+#define PROP_ENTRY_STRING(_name_, _val_) { \

…_STRING_ARRAY I can notice.

s / can / can't /

quoted

+	.name = _name_, \
+	.type = DEV_PROP_STRING, \
+	.nval = 1, \
+	.value.str = (const char **)_val_, \
+}
+
 /**
  * struct property_set - Collection of "built-in" device 
properties.
  * @fwnode: Handle to be pointed to by the fwnode field of struct 
device.

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

Shouldn't we leave the error path and modify to check if we have device
property set set?

 	}
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

On Wed, Aug 05, 2015 at 05:15:28PM +0300, Andy Shevchenko wrote:

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

No more users for it.

Signed-off-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/rfkill-gpio.h | 37 -----------------------------------
--
 net/rfkill/Kconfig          |  3 +--
 net/rfkill/rfkill-gpio.c    |  8 --------
 3 files changed, 1 insertion(+), 47 deletions(-)
 delete mode 100644 include/linux/rfkill-gpio.h

diff --git a/include/linux/rfkill-gpio.h b/include/linux/rfkill
-gpio.h
deleted file mode 100644
index 20bcb55..0000000
--- a/include/linux/rfkill-gpio.h
+++ /dev/null

@@ -1,37 +0,0 @@
-/*
- * Copyright (c) 2011, NVIDIA Corporation.
- *
- * 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.,
- * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- */
-
-
-#ifndef __RFKILL_GPIO_H
-#define __RFKILL_GPIO_H
-
-#include <linux/types.h>
-#include <linux/rfkill.h>
-
-/**
- * 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
- */
-
-struct rfkill_gpio_platform_data {
-	char			*name;
-	enum rfkill_type	type;
-};
-
-#endif /* __RFKILL_GPIO_H */

diff --git a/net/rfkill/Kconfig b/net/rfkill/Kconfig
index 4c10e7e..6320890 100644
--- a/net/rfkill/Kconfig
+++ b/net/rfkill/Kconfig

@@ -40,5 +40,4 @@ config RFKILL_GPIO
 	default n
 	help
 	  If you say yes here you get support of a generic gpio 

RFKILL
-	  driver. The platform should fill in the appropriate fields 
in the
-	  rfkill_gpio_platform_data structure and pass that to the 
driver.
+	  driver.

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 07323c3..69d92e1 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c

@@ -27,8 +27,6 @@
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 
-#include <linux/rfkill-gpio.h>
-
 struct rfkill_gpio_data {
 	const char		*name;
 	enum rfkill_type	type;

@@ -89,7 +87,6 @@ static int rfkill_gpio_acpi_probe(struct device 

*dev,
 
 static int rfkill_gpio_probe(struct platform_device *pdev)
 {
-	struct rfkill_gpio_platform_data *pdata = pdev
->dev.platform_data;
 	struct rfkill_gpio_data *rfkill;
 	struct gpio_desc *gpio;
 	const char *type_name;

@@ -111,11 +108,6 @@ static int rfkill_gpio_probe(struct 

platform_device *pdev)
 		ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill);
 		if (ret)
 			return ret;
-	} else if (pdata) {
-		rfkill->name = pdata->name;
-		rfkill->type = pdata->type;
-	} else {
-		return -ENODEV;

Shouldn't we leave the error path and modify to check if we have device
property set set?

We already check them before this point. After this ACPI will be the
only "special" case where we know the needed information does not come
from device property and needs separate handling. Otherwise, if the
device properties are not set, we cracefully fail.


Thanks,

-- 
heikki

On Wed, Aug 05, 2015 at 05:02:18PM +0300, Andy Shevchenko wrote:

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

Marcos for easier creation of build-in property entries.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/property.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..204d899 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h

@@ -152,6 +152,41 @@ struct property_entry {
 	} value;
 };
 
+#define PROP_ENTRY_U8(_name_, _val_) { \

PROP_ prefix is too generic.
Maybe DEVPROP_ ? At least for the latter no records in the current
sources.

I disagree with that. IMO this kind of macros should ideally resemble
the structure name they are used to fill (struct property_entry in
this case). And there are already definitions for DEV_PROP_* to
describe the types, so using something like DEVPROP_* here is just
confusing.

If PROP_ENTRY_* is really not good enough, we can change them
PROPERTY_ENTRY_*. But is PROP_ENTRY_* really so bad?

Rafael, what is your opinion?


Thanks,

-- 
heikki

On Wed, Aug 05, 2015 at 05:07:29PM +0300, Andy Shevchenko wrote:

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

Helper for finding the type based on name. Useful if the
type needs to be determined based on device property.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/rfkill.h | 15 +++++++++++++
 net/rfkill/core.c      | 57 +++++++++++++++++++++++++---------------
----------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index d901078..02f563c 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h

@@ -212,6 +212,15 @@ void rfkill_set_states(struct rfkill *rfkill, 

bool sw, bool hw);
  * @rfkill: rfkill struct to query
  */
 bool rfkill_blocked(struct rfkill *rfkill);
+
+/**
+ * rfkill_find_type - Helpper for finding rfkill type by name
+ * @name: the name of the type
+ *
+ * Returns enum rfkill_type that conrresponds the name.
+ */
+enum rfkill_type rfkill_find_type(const char *name);
+
 #else /* !RFKILL */
 static inline struct rfkill * __must_check
 rfkill_alloc(const char *name,

@@ -268,6 +277,12 @@ static inline bool rfkill_blocked(struct rfkill 

*rfkill)
 {
 	return false;
 }
+
+static inline enum rfkill_type rfkill_find_type(const char *name)
+{
+	return 0;

Hmm… Besides 0 is implicitly casted to enum type the issue with enums
that you rather have to supply existing enum entry. I would suggest to
add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases.

Why would you add a new type just for this? You do realize it would
require adding specific handling all over the place? RFKILL_TYPE_ALL
(0) is already handled as an invalid type. Confused?

I'll change this and return RFKILL_TYPE_ALL instead of 0.


Thanks,

-- 
heikki

On Thu, 2015-08-06 at 11:30 +0300, Heikki Krogerus wrote:

quoted

quoted

On Wed, Aug 05, 2015 at 05:07:29PM +0300, Andy Shevchenko wrote:

quoted

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

quoted

+static inline enum rfkill_type rfkill_find_type(const char
*name)
+{
+	return 0;

Hmm… Besides 0 is implicitly casted to enum type the issue with 
enums
that you rather have to supply existing enum entry. I would suggest 
to
add RFKILL_TYPE_UNKNOWN if _ALL is reserved for some use cases.

Why would you add a new type just for this? You do realize it would
require adding specific handling all over the place? RFKILL_TYPE_ALL
(0) is already handled as an invalid type.

It was my thought as well (see *if* in my previous comment).

 Confused?

A bit, yes.

I'll change this and return RFKILL_TYPE_ALL instead of 0.

Excellent!

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

Hello.

On 8/5/2015 4:39 PM, Heikki Krogerus wrote:

Helper for finding the type based on name. Useful if the
type needs to be determined based on device property.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
  include/linux/rfkill.h | 15 +++++++++++++
  net/rfkill/core.c      | 57 +++++++++++++++++++++++++-------------------------
  2 files changed, 44 insertions(+), 28 deletions(-)

[...]

 > +}
 > +

    Shouldn't it return some member of 'enum rfkill_type' instead?

[...]

MBR, Sergei

On Thursday, August 06, 2015 10:48:48 AM Heikki Krogerus wrote:

On Wed, Aug 05, 2015 at 05:02:18PM +0300, Andy Shevchenko wrote:

quoted

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

Marcos for easier creation of build-in property entries.

Signed-off-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/property.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..204d899 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h

@@ -152,6 +152,41 @@ struct property_entry {
 	} value;
 };
 
+#define PROP_ENTRY_U8(_name_, _val_) { \

PROP_ prefix is too generic.
Maybe DEVPROP_ ? At least for the latter no records in the current
sources.

I disagree with that. IMO this kind of macros should ideally resemble
the structure name they are used to fill (struct property_entry in
this case). And there are already definitions for DEV_PROP_* to
describe the types, so using something like DEVPROP_* here is just
confusing.

If PROP_ENTRY_* is really not good enough, we can change them
PROPERTY_ENTRY_*. But is PROP_ENTRY_* really so bad?

Rafael, what is your opinion?

I would prefer PROPERTY_ENTRY_ to be honest.  It's not like we need to save
characters here.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

+static const char *rfkill_types[NUM_RFKILL_TYPES] = {
+	[RFKILL_TYPE_WLAN]	= "wlan",
+	[RFKILL_TYPE_BLUETOOTH]	= "bluetooth",
+	[RFKILL_TYPE_UWB]	= "ultrawideband",
+	[RFKILL_TYPE_WIMAX]	= "wimax",
+	[RFKILL_TYPE_WWAN]	= "wwan",
+	[RFKILL_TYPE_GPS]	= "gps",
+	[RFKILL_TYPE_FM]	= "fm",
+	[RFKILL_TYPE_NFC]	= "nfc",
+};
+
+enum rfkill_type rfkill_find_type(const char *name)
+{
+	int i;
+
+	BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1);

That BUILD_BUG_ON() is now less useful - previously it pointed to the
code that needed to change, now you're left wondering if you don't look
up since it isn't quite that obvious from the code what this does.

Something like

	BUILD_BUG_ON(rfkill_types[NUM_RFKILL_TYPES - 1] == NULL);

would be better. As we only add here, that would be safe enough - I've
done something similar in the past that a bit more complicated.

With that and the static inline fixed (which maybe you could even
remove) I'm fine with all these rfkill patches, but I'm not sure how to
merge them since they affect all kinds of other trees. If desired, I
can apply them, but an ACK from the tegra maintainer would be good :)

johannes

Hi,

On Thu, Aug 13, 2015 at 11:27:46AM +0200, Johannes Berg wrote:

On Wed, 2015-08-05 at 16:39 +0300, Heikki Krogerus wrote:

quoted

+static const char *rfkill_types[NUM_RFKILL_TYPES] = {
+	[RFKILL_TYPE_WLAN]	= "wlan",
+	[RFKILL_TYPE_BLUETOOTH]	= "bluetooth",
+	[RFKILL_TYPE_UWB]	= "ultrawideband",
+	[RFKILL_TYPE_WIMAX]	= "wimax",
+	[RFKILL_TYPE_WWAN]	= "wwan",
+	[RFKILL_TYPE_GPS]	= "gps",
+	[RFKILL_TYPE_FM]	= "fm",
+	[RFKILL_TYPE_NFC]	= "nfc",
+};
+
+enum rfkill_type rfkill_find_type(const char *name)
+{
+	int i;
+
+	BUILD_BUG_ON(NUM_RFKILL_TYPES != RFKILL_TYPE_NFC + 1);

That BUILD_BUG_ON() is now less useful - previously it pointed to the
code that needed to change, now you're left wondering if you don't look
up since it isn't quite that obvious from the code what this does.

Something like

	BUILD_BUG_ON(rfkill_types[NUM_RFKILL_TYPES - 1] == NULL);

would be better. As we only add here, that would be safe enough - I've
done something similar in the past that a bit more complicated.

OK, I'll change it.

With that and the static inline fixed (which maybe you could even
remove) I'm fine with all these rfkill patches, but I'm not sure how to
merge them since they affect all kinds of other trees. If desired, I
can apply them, but an ACK from the tegra maintainer would be good :)

Andy and Mika are preparing some changes to the device property
handling. I'll wait for their proposal and prepare next version these
after that.


Thanks,

-- 
heikki