[PATCH] media: rc: gpio-ir-recv: add support for device tree parsing

[PATCH] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

This patch adds device tree parsing for gpio_ir_recv platform_data and
the mandatory binding documentation. It basically follows what we already
have for e.g. gpio_keys. All required device tree properties are OS
independent but optional properties allow linux specific support for rc
protocols and maps.

There was a similar patch sent by Matus Ujhelyi but that discussion
died after the first reviews.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
Cc: David Hardeman <david@hardeman.nu>
Cc: Trilok Soni <tsoni@codeaurora.org>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-media@vger.kernel.org
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt |   20 ++++++
 drivers/media/rc/gpio-ir-recv.c                    |   68 +++++++++++++++++++-
 2 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
new file mode 100644
index 0000000..937760c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -0,0 +1,20 @@
+Device-Tree bindings for GPIO IR receiver
+
+Required properties:
+	- compatible = "gpio-ir-receiver";
+	- gpios: OF device-tree gpio specification.
+
+Optional properties:
+	- linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
+	    rc protocols.
+	- linux,rc-map-name: Linux specific remote control map name.
+
+Example node:
+
+	ir: ir-receiver {
+		compatible = "gpio-ir-receiver";
+		gpios = <&gpio0 19 1>;
+		/* allow rc protocols 1-4 */
+		linux,allowed-rc-protocols = <0x00000000 0x0000001e>;
+		linux,rc-map-name = "rc-rc6-mce";
+	};
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 4f71a7d..25e09fa 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <media/rc-core.h>
@@ -30,6 +31,63 @@ struct gpio_rc_dev {
 	bool active_low;
 };
 
+#ifdef CONFIG_OF
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static struct gpio_ir_recv_platform_data *
+gpio_ir_recv_get_devtree_pdata(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct gpio_ir_recv_platform_data *pdata;
+	enum of_gpio_flags flags;
+	int gpio;
+
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	if (!of_find_property(np, "gpios", NULL)) {
+		dev_err(dev, "Found gpio-ir-receiver without gpios\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	gpio = of_get_gpio_flags(np, 0, &flags);
+	if (gpio < 0) {
+		if (gpio != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get gpio flags, error: %d\n",
+				gpio);
+		return ERR_PTR(gpio);
+	}
+
+	pdata->gpio_nr = gpio;
+	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? true : false;
+	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
+	of_property_read_u64(np, "linux,allowed-rc-protocols",
+			     &pdata->allowed_protos);
+
+	return pdata;
+}
+
+static struct of_device_id gpio_ir_recv_of_match[] = {
+	{ .compatible = "gpio-ir-receiver", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
+
+#else /* !CONFIG_OF */
+
+static inline struct gpio_ir_recv_platform_data *
+gpio_ir_recv_get_devtree_pdata(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#endif
+
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
 	struct gpio_rc_dev *gpio_dev = dev_id;
@@ -66,8 +124,11 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 					pdev->dev.platform_data;
 	int rc;
 
-	if (!pdata)
-		return -EINVAL;
+	if (!pdata) {
+		pdata = gpio_ir_recv_get_devtree_pdata(&pdev->dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
 
 	if (pdata->gpio_nr < 0)
 		return -EINVAL;
@@ -195,6 +256,9 @@ static struct platform_driver gpio_ir_recv_driver = {
 #ifdef CONFIG_PM
 		.pm	= &gpio_ir_recv_pm_ops,
 #endif
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
+#endif
 	},
 };
 module_platform_driver(gpio_ir_recv_driver);
-- 
1.7.10.4

[PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

This patch adds device tree parsing for gpio_ir_recv platform_data and
the mandatory binding documentation. It basically follows what we already
have for e.g. gpio_keys. All required device tree properties are OS
independent but optional properties allow linux specific support for rc
protocols and maps.

There was a similar patch sent by Matus Ujhelyi but that discussion
died after the first reviews.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
Cc: David Hardeman <david@hardeman.nu>
Cc: Trilok Soni <tsoni@codeaurora.org>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-media@vger.kernel.org
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt |   20 ++++++
 drivers/media/rc/gpio-ir-recv.c                    |   68 +++++++++++++++++++-
 2 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
new file mode 100644
index 0000000..937760c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -0,0 +1,20 @@
+Device-Tree bindings for GPIO IR receiver
+
+Required properties:
+	- compatible = "gpio-ir-receiver";
+	- gpios: OF device-tree gpio specification.
+
+Optional properties:
+	- linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
+	    rc protocols.
+	- linux,rc-map-name: Linux specific remote control map name.
+
+Example node:
+
+	ir: ir-receiver {
+		compatible = "gpio-ir-receiver";
+		gpios = <&gpio0 19 1>;
+		/* allow rc protocols 1-4 */
+		linux,allowed-rc-protocols = <0x00000000 0x0000001e>;
+		linux,rc-map-name = "rc-rc6-mce";
+	};
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 4f71a7d..25e09fa 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <media/rc-core.h>
@@ -30,6 +31,63 @@ struct gpio_rc_dev {
 	bool active_low;
 };
 
+#ifdef CONFIG_OF
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static struct gpio_ir_recv_platform_data *
+gpio_ir_recv_get_devtree_pdata(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct gpio_ir_recv_platform_data *pdata;
+	enum of_gpio_flags flags;
+	int gpio;
+
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	if (!of_find_property(np, "gpios", NULL)) {
+		dev_err(dev, "Found gpio-ir-receiver without gpios\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	gpio = of_get_gpio_flags(np, 0, &flags);
+	if (gpio < 0) {
+		if (gpio != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get gpio flags, error: %d\n",
+				gpio);
+		return ERR_PTR(gpio);
+	}
+
+	pdata->gpio_nr = gpio;
+	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? true : false;
+	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
+	of_property_read_u64(np, "linux,allowed-rc-protocols",
+			     &pdata->allowed_protos);
+
+	return pdata;
+}
+
+static struct of_device_id gpio_ir_recv_of_match[] = {
+	{ .compatible = "gpio-ir-receiver", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
+
+#else /* !CONFIG_OF */
+
+static inline struct gpio_ir_recv_platform_data *
+gpio_ir_recv_get_devtree_pdata(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#endif
+
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
 	struct gpio_rc_dev *gpio_dev = dev_id;
@@ -66,8 +124,11 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 					pdev->dev.platform_data;
 	int rc;
 
-	if (!pdata)
-		return -EINVAL;
+	if (!pdata) {
+		pdata = gpio_ir_recv_get_devtree_pdata(&pdev->dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
 
 	if (pdata->gpio_nr < 0)
 		return -EINVAL;
@@ -195,6 +256,9 @@ static struct platform_driver gpio_ir_recv_driver = {
 #ifdef CONFIG_PM
 		.pm	= &gpio_ir_recv_pm_ops,
 #endif
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
+#endif
 	},
 };
 module_platform_driver(gpio_ir_recv_driver);
-- 
1.7.10.4

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Sylwester Nawrocki]

Hi,

On 02/06/2013 09:03 AM, Sebastian Hesselbarth wrote:
> This patch adds device tree parsing for gpio_ir_recv platform_data and
> the mandatory binding documentation. It basically follows what we already
> have for e.g. gpio_keys. All required device tree properties are OS
> independent but optional properties allow linux specific support for rc
> protocols and maps.
> 
> There was a similar patch sent by Matus Ujhelyi but that discussion
> died after the first reviews.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
...
> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> new file mode 100644
> index 0000000..937760c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> @@ -0,0 +1,20 @@
> +Device-Tree bindings for GPIO IR receiver
> +
> +Required properties:
> +	- compatible = "gpio-ir-receiver";
> +	- gpios: OF device-tree gpio specification.
> +
> +Optional properties:
> +	- linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
> +	    rc protocols.

You likely need to specify in these bindings documentation which bit 
corresponds to which RC protocol.

I'm not very familiar with the RC internals, but why it has to be
specified statically in the device tree, when decoding seems to be
mostly software defined ? I might be missing something though..

Couldn't this be configured at run time, with all protocols allowed
as the default ?

> +	- linux,rc-map-name: Linux specific remote control map name.
> +
> +Example node:
> +
> +	ir: ir-receiver {
> +		compatible = "gpio-ir-receiver";
> +		gpios = <&gpio0 19 1>;
> +		/* allow rc protocols 1-4 */
> +		linux,allowed-rc-protocols = <0x00000000 0x0000001e>;
> +		linux,rc-map-name = "rc-rc6-mce";
> +	};
> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> index 4f71a7d..25e09fa 100644
> --- a/drivers/media/rc/gpio-ir-recv.c
> +++ b/drivers/media/rc/gpio-ir-recv.c
> @@ -16,6 +16,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/gpio.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_device.h>
>  #include <linux/irq.h>
>  #include <media/rc-core.h>
> @@ -30,6 +31,63 @@ struct gpio_rc_dev {
>  	bool active_low;
>  };
>  
> +#ifdef CONFIG_OF
> +/*
> + * Translate OpenFirmware node properties into platform_data
> + */
> +static struct gpio_ir_recv_platform_data *
> +gpio_ir_recv_get_devtree_pdata(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct gpio_ir_recv_platform_data *pdata;
> +	enum of_gpio_flags flags;
> +	int gpio;
> +
> +	if (!np)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (!of_find_property(np, "gpios", NULL)) {

Why do you need this ? Isn't of_get_gpio_flags() sufficient ?

> +		dev_err(dev, "Found gpio-ir-receiver without gpios\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	gpio = of_get_gpio_flags(np, 0, &flags);
> +	if (gpio < 0) {
> +		if (gpio != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get gpio flags, error: %d\n",
> +				gpio);
> +		return ERR_PTR(gpio);
> +	}
> +
> +	pdata->gpio_nr = gpio;
> +	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? true : false;
> +	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
> +	of_property_read_u64(np, "linux,allowed-rc-protocols",
> +			     &pdata->allowed_protos);
> +
> +	return pdata;
> +}
> +
> +static struct of_device_id gpio_ir_recv_of_match[] = {
> +	{ .compatible = "gpio-ir-receiver", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
> +
> +#else /* !CONFIG_OF */
> +
> +static inline struct gpio_ir_recv_platform_data *
> +gpio_ir_recv_get_devtree_pdata(struct device *dev)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +#endif
> +
>  static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>  {
>  	struct gpio_rc_dev *gpio_dev = dev_id;
> @@ -66,8 +124,11 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>  					pdev->dev.platform_data;
>  	int rc;
>  
> -	if (!pdata)
> -		return -EINVAL;
> +	if (!pdata) {
> +		pdata = gpio_ir_recv_get_devtree_pdata(&pdev->dev);

Could assigning to pdev->dev.platform_data be avoided here ?

platform_data is only referenced in probe(), so maybe something like
this would be better:

	const struct gpio_ir_recv_platform_data *pdata = NULL;

	if (pdev->dev.of_node) {
		ret = gpio_ir_recv_parse_dt(&pdev->dev, &pdata);
		if (ret < 0)
			return ret;
	} else {
		pdata = pdev->dev.platform_data;
	}
	if (!pdata)
		return -EINVAL;

> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +	}
>  
>  	if (pdata->gpio_nr < 0)
>  		return -EINVAL;
> @@ -195,6 +256,9 @@ static struct platform_driver gpio_ir_recv_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &gpio_ir_recv_pm_ops,
>  #endif
> +#ifdef CONFIG_OF
> +		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
> +#endif

There is not need for #ifdef here, of_match_ptr() macro was introduced
just to allow to omit #ifdefs.

>  	},
>  };
>  module_platform_driver(gpio_ir_recv_driver);
> 

--

Thanks,
Sylwester

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

On 02/06/2013 02:48 PM, Sylwester Nawrocki wrote:
> On 02/06/2013 09:03 AM, Sebastian Hesselbarth wrote:
>> This patch adds device tree parsing for gpio_ir_recv platform_data and
>> the mandatory binding documentation. It basically follows what we already
>> have for e.g. gpio_keys. All required device tree properties are OS
>> independent but optional properties allow linux specific support for rc
>> protocols and maps.
>>
>> There was a similar patch sent by Matus Ujhelyi but that discussion
>> died after the first reviews.
>>
>> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> ---
> ...
>> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> new file mode 100644
>> index 0000000..937760c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> @@ -0,0 +1,20 @@
>> +Device-Tree bindings for GPIO IR receiver
>> +
>> +Required properties:
>> +	- compatible = "gpio-ir-receiver";
>> +	- gpios: OF device-tree gpio specification.
>> +
>> +Optional properties:
>> +	- linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
>> +	    rc protocols.
>
> You likely need to specify in these bindings documentation which bit
> corresponds to which RC protocol.
>
> I'm not very familiar with the RC internals, but why it has to be
> specified statically in the device tree, when decoding seems to be
> mostly software defined ? I might be missing something though..

Sylwester,

I am not familiar with RC internals either. Maybe somebody with more
insight in media/rc can clarify the specific needs for the rc subsystem.
I was just transferring the DT support approach taken by gpio_keys to
gpio_ir_recv as I will be using it on mach-dove/cubox soon.

> Couldn't this be configured at run time, with all protocols allowed
> as the default ?

Actually, this is how the internal rc code works. If there is nothing
defined for allowed_protocols it assumes that all protocols are supported.
That is why above node properties are optional.

About the binding documentation of allowed_protocols, rc_map, or the
default behavior of current linux code, I don't think they will stay
in-sync for long. I'd rather completely remove those os-specific properties
from DT, but that hits the above statement about the needs of media/rc
subsystem.

>> +	- linux,rc-map-name: Linux specific remote control map name.
>> +
>> +Example node:
>> +
>> +	ir: ir-receiver {
>> +		compatible = "gpio-ir-receiver";
>> +		gpios =<&gpio0 19 1>;
>> +		/* allow rc protocols 1-4 */
>> +		linux,allowed-rc-protocols =<0x00000000 0x0000001e>;
>> +		linux,rc-map-name = "rc-rc6-mce";
>> +	};
>> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
>> index 4f71a7d..25e09fa 100644
>> --- a/drivers/media/rc/gpio-ir-recv.c
>> +++ b/drivers/media/rc/gpio-ir-recv.c
>> @@ -16,6 +16,7 @@
>>   #include<linux/interrupt.h>
>>   #include<linux/gpio.h>
>>   #include<linux/slab.h>
>> +#include<linux/of_gpio.h>
>>   #include<linux/platform_device.h>
>>   #include<linux/irq.h>
>>   #include<media/rc-core.h>
>> @@ -30,6 +31,63 @@ struct gpio_rc_dev {
>>   	bool active_low;
>>   };
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * Translate OpenFirmware node properties into platform_data
>> + */
>> +static struct gpio_ir_recv_platform_data *
>> +gpio_ir_recv_get_devtree_pdata(struct device *dev)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct gpio_ir_recv_platform_data *pdata;
>> +	enum of_gpio_flags flags;
>> +	int gpio;
>> +
>> +	if (!np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	if (!of_find_property(np, "gpios", NULL)) {
>
> Why do you need this ? Isn't of_get_gpio_flags() sufficient ?

Ok. Now that you point at it, I agree that this check and the error
below is not needed. It is in gpio_keys, so that explains why it also
moved in here.

>> +		dev_err(dev, "Found gpio-ir-receiver without gpios\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	gpio = of_get_gpio_flags(np, 0,&flags);
>> +	if (gpio<  0) {
>> +		if (gpio != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get gpio flags, error: %d\n",
>> +				gpio);
>> +		return ERR_PTR(gpio);
>> +	}
>> +
>> +	pdata->gpio_nr = gpio;
>> +	pdata->active_low = (flags&  OF_GPIO_ACTIVE_LOW) ? true : false;
>> +	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
>> +	of_property_read_u64(np, "linux,allowed-rc-protocols",
>> +			&pdata->allowed_protos);
>> +
>> +	return pdata;
>> +}
>> +
>> +static struct of_device_id gpio_ir_recv_of_match[] = {
>> +	{ .compatible = "gpio-ir-receiver", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
>> +
>> +#else /* !CONFIG_OF */
>> +
>> +static inline struct gpio_ir_recv_platform_data *
>> +gpio_ir_recv_get_devtree_pdata(struct device *dev)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +#endif
>> +
>>   static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>>   {
>>   	struct gpio_rc_dev *gpio_dev = dev_id;
>> @@ -66,8 +124,11 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>>   					pdev->dev.platform_data;
>>   	int rc;
>>
>> -	if (!pdata)
>> -		return -EINVAL;
>> +	if (!pdata) {
>> +		pdata = gpio_ir_recv_get_devtree_pdata(&pdev->dev);
>
> Could assigning to pdev->dev.platform_data be avoided here ?
>
> platform_data is only referenced in probe(), so maybe something like
> this would be better:
>
> 	const struct gpio_ir_recv_platform_data *pdata = NULL;
>
> 	if (pdev->dev.of_node) {
> 		ret = gpio_ir_recv_parse_dt(&pdev->dev,&pdata);
> 		if (ret<  0)
> 			return ret;
> 	} else {
> 		pdata = pdev->dev.platform_data;
> 	}
> 	if (!pdata)
> 		return -EINVAL;

Actually, I am not assigning the parsed gpio_ir_recv_platform_data to
pdev->dev.platform_data but pdata ptr instead. Either I don't see the
difference in pointer assignments between your code and mine or you
were mislead from struct gpio_ir_recv_platform_data above.

Anyway, I agree to test for pdev->dev.of_node and call gpio_ir_recv_parse_dt
if set.

>> +		if (IS_ERR(pdata))
>> +			return PTR_ERR(pdata);
>> +	}
>>
>>   	if (pdata->gpio_nr<  0)
>>   		return -EINVAL;
>> @@ -195,6 +256,9 @@ static struct platform_driver gpio_ir_recv_driver = {
>>   #ifdef CONFIG_PM
>>   		.pm	=&gpio_ir_recv_pm_ops,
>>   #endif
>> +#ifdef CONFIG_OF
>> +		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
>> +#endif
>
> There is not need for #ifdef here, of_match_ptr() macro was introduced
> just to allow to omit #ifdefs.

Ok, I will change that.

Thanks for the review!

Sebastian

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Mauro Carvalho Chehab]

Em Wed, 06 Feb 2013 18:18:22 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:

> On 02/06/2013 02:48 PM, Sylwester Nawrocki wrote:
> > On 02/06/2013 09:03 AM, Sebastian Hesselbarth wrote:
> >> This patch adds device tree parsing for gpio_ir_recv platform_data and
> >> the mandatory binding documentation. It basically follows what we already
> >> have for e.g. gpio_keys. All required device tree properties are OS
> >> independent but optional properties allow linux specific support for rc
> >> protocols and maps.
> >>
> >> There was a similar patch sent by Matus Ujhelyi but that discussion
> >> died after the first reviews.
> >>
> >> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> >> ---
> > ...
> >> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> new file mode 100644
> >> index 0000000..937760c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> @@ -0,0 +1,20 @@
> >> +Device-Tree bindings for GPIO IR receiver
> >> +
> >> +Required properties:
> >> +	- compatible = "gpio-ir-receiver";
> >> +	- gpios: OF device-tree gpio specification.
> >> +
> >> +Optional properties:
> >> +	- linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
> >> +	    rc protocols.
> >
> > You likely need to specify in these bindings documentation which bit
> > corresponds to which RC protocol.
> >
> > I'm not very familiar with the RC internals, but why it has to be
> > specified statically in the device tree, when decoding seems to be
> > mostly software defined ? I might be missing something though..
> 
> Sylwester,
> 
> I am not familiar with RC internals either. Maybe somebody with more
> insight in media/rc can clarify the specific needs for the rc subsystem.
> I was just transferring the DT support approach taken by gpio_keys to
> gpio_ir_recv as I will be using it on mach-dove/cubox soon.

The allowed rc protocol field are there for devices with hardware IR
support, where only a limited set of remote protocols can be decoded.

For software decoders RC_BIT_ALL is the proper setup. Users of course
can change it via sysfs at runtime, or a software decoder may be
disabled at compilation time by not selecting its CONFIG_* var.

> > Couldn't this be configured at run time, with all protocols allowed
> > as the default ?
> 
> Actually, this is how the internal rc code works. If there is nothing
> defined for allowed_protocols it assumes that all protocols are supported.
> That is why above node properties are optional.
> 
> About the binding documentation of allowed_protocols, rc_map, or the
> default behavior of current linux code, I don't think they will stay
> in-sync for long. 

Why not? The rc_map name is used either by Kernelspace or by Userspace,
in order to provide the IR keycode name that matches a given keytable.

There's no plans to change it, even in the long term.

Regards,
Mauro.

-- 

Cheers,
Mauro

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

On Fri, Feb 8, 2013 at 6:57 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Wed, 06 Feb 2013 18:18:22 +0100
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:
>> On 02/06/2013 02:48 PM, Sylwester Nawrocki wrote:
>> > On 02/06/2013 09:03 AM, Sebastian Hesselbarth wrote:
>> >> This patch adds device tree parsing for gpio_ir_recv platform_data and
>> >> the mandatory binding documentation. It basically follows what we already
>> >> have for e.g. gpio_keys. All required device tree properties are OS
>> >> independent but optional properties allow linux specific support for rc
>> >> protocols and maps.
>> >>
>> >> There was a similar patch sent by Matus Ujhelyi but that discussion
>> >> died after the first reviews.
>> >>
>> >> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> >> ---
>> > ...
>> >> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> >> new file mode 100644
>> >> index 0000000..937760c
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> >> @@ -0,0 +1,20 @@
>> >> +Device-Tree bindings for GPIO IR receiver
>> >> +
>> >> +Required properties:
>> >> +  - compatible = "gpio-ir-receiver";
>> >> +  - gpios: OF device-tree gpio specification.
>> >> +
>> >> +Optional properties:
>> >> +  - linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
>> >> +      rc protocols.
>> >
>> > You likely need to specify in these bindings documentation which bit
>> > corresponds to which RC protocol.
>> >
>> > I'm not very familiar with the RC internals, but why it has to be
>> > specified statically in the device tree, when decoding seems to be
>> > mostly software defined ? I might be missing something though..
>>
>> Sylwester,
>>
>> I am not familiar with RC internals either. Maybe somebody with more
>> insight in media/rc can clarify the specific needs for the rc subsystem.
>> I was just transferring the DT support approach taken by gpio_keys to
>> gpio_ir_recv as I will be using it on mach-dove/cubox soon.
>
> The allowed rc protocol field are there for devices with hardware IR
> support, where only a limited set of remote protocols can be decoded.
>
> For software decoders RC_BIT_ALL is the proper setup. Users of course
> can change it via sysfs at runtime, or a software decoder may be
> disabled at compilation time by not selecting its CONFIG_* var.

Mauro,

thanks for the clarification! So for v2 of the patch, you all agree on removing
linux,allowed-rc-protocols from device node properties?

>> > Couldn't this be configured at run time, with all protocols allowed
>> > as the default ?
>>
>> Actually, this is how the internal rc code works. If there is nothing
>> defined for allowed_protocols it assumes that all protocols are supported.
>> That is why above node properties are optional.
>>
>> About the binding documentation of allowed_protocols, rc_map, or the
>> default behavior of current linux code, I don't think they will stay
>> in-sync for long.
>
> Why not? The rc_map name is used either by Kernelspace or by Userspace,
> in order to provide the IR keycode name that matches a given keytable.
>
> There's no plans to change it, even in the long term.

Actually, I wasn't referring to changing names or bitmasks but updating
the binding documentation with new allowed protocols or supported map
names.

For linux,rc-map-name property it should be enough to just write that it
relates to linux rc subsystem rc_map name - how to actually
set it to a useful name is documented in rc subsystem. And if the
property is not set at all, DT parsing in gpio_ir_recv assumes the
subsystem (or gpio_ir_recv platform) default, IIRC "rc-none".

I'll respin a v2 without allowed-protocols property soon.

Sebastian

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Mauro Carvalho Chehab]

Em Fri, 8 Feb 2013 19:12:31 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:

> On Fri, Feb 8, 2013 at 6:57 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em Wed, 06 Feb 2013 18:18:22 +0100
> > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:
> >> On 02/06/2013 02:48 PM, Sylwester Nawrocki wrote:
> >> > On 02/06/2013 09:03 AM, Sebastian Hesselbarth wrote:
> >> >> This patch adds device tree parsing for gpio_ir_recv platform_data and
> >> >> the mandatory binding documentation. It basically follows what we already
> >> >> have for e.g. gpio_keys. All required device tree properties are OS
> >> >> independent but optional properties allow linux specific support for rc
> >> >> protocols and maps.
> >> >>
> >> >> There was a similar patch sent by Matus Ujhelyi but that discussion
> >> >> died after the first reviews.
> >> >>
> >> >> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> >> >> ---
> >> > ...
> >> >> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> >> new file mode 100644
> >> >> index 0000000..937760c
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> >> @@ -0,0 +1,20 @@
> >> >> +Device-Tree bindings for GPIO IR receiver
> >> >> +
> >> >> +Required properties:
> >> >> +  - compatible = "gpio-ir-receiver";
> >> >> +  - gpios: OF device-tree gpio specification.
> >> >> +
> >> >> +Optional properties:
> >> >> +  - linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
> >> >> +      rc protocols.
> >> >
> >> > You likely need to specify in these bindings documentation which bit
> >> > corresponds to which RC protocol.
> >> >
> >> > I'm not very familiar with the RC internals, but why it has to be
> >> > specified statically in the device tree, when decoding seems to be
> >> > mostly software defined ? I might be missing something though..
> >>
> >> Sylwester,
> >>
> >> I am not familiar with RC internals either. Maybe somebody with more
> >> insight in media/rc can clarify the specific needs for the rc subsystem.
> >> I was just transferring the DT support approach taken by gpio_keys to
> >> gpio_ir_recv as I will be using it on mach-dove/cubox soon.
> >
> > The allowed rc protocol field are there for devices with hardware IR
> > support, where only a limited set of remote protocols can be decoded.
> >
> > For software decoders RC_BIT_ALL is the proper setup. Users of course
> > can change it via sysfs at runtime, or a software decoder may be
> > disabled at compilation time by not selecting its CONFIG_* var.
> 
> Mauro,
> 
> thanks for the clarification! So for v2 of the patch, you all agree on removing
> linux,allowed-rc-protocols from device node properties?

Yes.
> 
> >> > Couldn't this be configured at run time, with all protocols allowed
> >> > as the default ?
> >>
> >> Actually, this is how the internal rc code works. If there is nothing
> >> defined for allowed_protocols it assumes that all protocols are supported.
> >> That is why above node properties are optional.
> >>
> >> About the binding documentation of allowed_protocols, rc_map, or the
> >> default behavior of current linux code, I don't think they will stay
> >> in-sync for long.
> >
> > Why not? The rc_map name is used either by Kernelspace or by Userspace,
> > in order to provide the IR keycode name that matches a given keytable.
> >
> > There's no plans to change it, even in the long term.
> 
> Actually, I wasn't referring to changing names or bitmasks but updating
> the binding documentation with new allowed protocols or supported map
> names.
> 
> For linux,rc-map-name property it should be enough to just write that it
> relates to linux rc subsystem rc_map name - how to actually
> set it to a useful name is documented in rc subsystem.

It should be one of the names that are there at include/media/rc-map.h.

> And if the
> property is not set at all, DT parsing in gpio_ir_recv assumes the
> subsystem (or gpio_ir_recv platform) default, IIRC "rc-none".

The right default should be "rc-empty", but please use the macro RC_MAP_EMPTY
instead.

> I'll respin a v2 without allowed-protocols property soon.
> 
> Sebastian


-- 

Cheers,
Mauro

Re: [PATCH RESEND] media: rc: gpio-ir-recv: add support for device tree parsing

[Sylwester Nawrocki]

On 02/06/2013 06:18 PM, Sebastian Hesselbarth wrote:
>>> +Optional properties:
>>> +    - linux,allowed-rc-protocols: Linux specific u64 bitmask of allowed
>>> +        rc protocols.
>>
>> You likely need to specify in these bindings documentation which bit
>> corresponds to which RC protocol.
>>
>> I'm not very familiar with the RC internals, but why it has to be
>> specified statically in the device tree, when decoding seems to be
>> mostly software defined ? I might be missing something though..
> 
> Sylwester,
> 
> I am not familiar with RC internals either. Maybe somebody with more
> insight in media/rc can clarify the specific needs for the rc subsystem.
> I was just transferring the DT support approach taken by gpio_keys to
> gpio_ir_recv as I will be using it on mach-dove/cubox soon.
> 
>> Couldn't this be configured at run time, with all protocols allowed
>> as the default ?
> 
> Actually, this is how the internal rc code works. If there is nothing
> defined for allowed_protocols it assumes that all protocols are supported.
> That is why above node properties are optional.
> 
> About the binding documentation of allowed_protocols, rc_map, or the
> default behavior of current linux code, I don't think they will stay
> in-sync for long. I'd rather completely remove those os-specific properties
> from DT, but that hits the above statement about the needs of media/rc
> subsystem.

I think the bindings could define the bitmask, independently of the
RC code. I suppose it is correct to have a list of protocols defined
this way. Since, as Mauro pointed out, it is needed for some hardware
devices that support only selected protocols.
Then you could probably drop the 'linux,' prefix, but I'm not 100% sure
about it.

> Actually, I am not assigning the parsed gpio_ir_recv_platform_data to
> pdev->dev.platform_data but pdata ptr instead. Either I don't see the
> difference in pointer assignments between your code and mine or you
> were mislead from struct gpio_ir_recv_platform_data above.

Sorry, you're right. I think I was just trying to be to quick with this
review.. Your code is correct, however I would probably avoid the ERR_PTR()
pattern as much as possible.

> Anyway, I agree to test for pdev->dev.of_node and call gpio_ir_recv_parse_dt
> if set.

--
Regards,
Sylwester




-- 
Sylwester Nawrocki
실베스터 나브로츠키
Samsung Poland R&D Center

[PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

This patch adds device tree parsing for gpio_ir_recv platform_data and
the mandatory binding documentation. It basically follows what we already
have for e.g. gpio_keys. All required device tree properties are OS
independent but an optional property allow linux specific support for rc
maps.

There was a similar patch sent by Matus Ujhelyi but that discussion
died after the first reviews.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog

v1->v2:
- get rid of ptr returned by _get_devtree_pdata()
- check for of_node instead for NULL pdata
- remove unneccessary double check for gpios property
- remove unneccessary #ifdef CONFIG_OF around match table

Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
Cc: David Hardeman <david@hardeman.nu>
Cc: Trilok Soni <tsoni@codeaurora.org>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Matus Ujhelyi <ujhelyi.m@gmail.com>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-media@vger.kernel.org
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
 drivers/media/rc/gpio-ir-recv.c                    |   57 ++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
new file mode 100644
index 0000000..8589f30
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -0,0 +1,16 @@
+Device-Tree bindings for GPIO IR receiver
+
+Required properties:
+	- compatible = "gpio-ir-receiver";
+	- gpios: OF device-tree gpio specification.
+
+Optional properties:
+	- linux,rc-map-name: Linux specific remote control map name.
+
+Example node:
+
+	ir: ir-receiver {
+		compatible = "gpio-ir-receiver";
+		gpios = <&gpio0 19 1>;
+		linux,rc-map-name = "rc-rc6-mce";
+	};
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 4f71a7d..3c62006 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <media/rc-core.h>
@@ -30,6 +31,50 @@ struct gpio_rc_dev {
 	bool active_low;
 };
 
+#ifdef CONFIG_OF
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static int gpio_ir_recv_get_devtree_pdata(struct device *dev,
+				  struct gpio_ir_recv_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	enum of_gpio_flags flags;
+	int gpio;
+
+	gpio = of_get_gpio_flags(np, 0, &flags);
+	if (gpio < 0) {
+		if (gpio != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get gpio flags, error: %d\n",
+				gpio);
+		return gpio;
+	}
+
+	pdata->gpio_nr = gpio;
+	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW) ? true : false;
+	/* probe() takes care of map_name == NULL or allowed_protos == 0 */
+	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
+	pdata->allowed_protos = 0;
+
+	return 0;
+}
+
+static struct of_device_id gpio_ir_recv_of_match[] = {
+	{ .compatible = "gpio-ir-receiver", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
+
+#else /* !CONFIG_OF */
+
+static inline struct gpio_ir_recv_platform_data *
+gpio_ir_recv_get_devtree_pdata(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#endif
+
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
 	struct gpio_rc_dev *gpio_dev = dev_id;
@@ -66,6 +111,17 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 					pdev->dev.platform_data;
 	int rc;
 
+	if (pdev->dev.of_node) {
+		struct gpio_ir_recv_platform_data *dtpdata =
+			devm_kzalloc(&pdev->dev, sizeof(*dtpdata), GFP_KERNEL);
+		if (!dtpdata)
+			return -ENOMEM;
+		rc = gpio_ir_recv_get_devtree_pdata(&pdev->dev, dtpdata);
+		if (rc)
+			return rc;
+		pdata = dtpdata;
+	}
+
 	if (!pdata)
 		return -EINVAL;
 
@@ -192,6 +248,7 @@ static struct platform_driver gpio_ir_recv_driver = {
 	.driver = {
 		.name   = GPIO_IR_DRIVER_NAME,
 		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
 #ifdef CONFIG_PM
 		.pm	= &gpio_ir_recv_pm_ops,
 #endif
-- 
1.7.10.4

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sylwester Nawrocki]

On 02/08/2013 09:38 PM, Sebastian Hesselbarth wrote:
> This patch adds device tree parsing for gpio_ir_recv platform_data and
> the mandatory binding documentation. It basically follows what we already
> have for e.g. gpio_keys. All required device tree properties are OS
> independent but an optional property allow linux specific support for rc
> maps.
>
> There was a similar patch sent by Matus Ujhelyi but that discussion
> died after the first reviews.
>
> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> ---
> Changelog
>
> v1->v2:
> - get rid of ptr returned by _get_devtree_pdata()
> - check for of_node instead for NULL pdata
> - remove unneccessary double check for gpios property
> - remove unneccessary #ifdef CONFIG_OF around match table
>
> Cc: Grant Likely<grant.likely@secretlab.ca>
> Cc: Rob Herring<rob.herring@calxeda.com>
> Cc: Rob Landley<rob@landley.net>
> Cc: Mauro Carvalho Chehab<mchehab@redhat.com>
> Cc: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> Cc: Benoit Thebaudeau<benoit.thebaudeau@advansee.com>
> Cc: David Hardeman<david@hardeman.nu>
> Cc: Trilok Soni<tsoni@codeaurora.org>
> Cc: Sylwester Nawrocki<s.nawrocki@samsung.com>
> Cc: Matus Ujhelyi<ujhelyi.m@gmail.com>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> ---
>   .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
>   drivers/media/rc/gpio-ir-recv.c                    |   57 ++++++++++++++++++++
>   2 files changed, 73 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>
> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> new file mode 100644
> index 0000000..8589f30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> @@ -0,0 +1,16 @@
> +Device-Tree bindings for GPIO IR receiver
> +
> +Required properties:
> +	- compatible = "gpio-ir-receiver";
> +	- gpios: OF device-tree gpio specification.

Perhaps:
	- compatible: should be "gpio-ir-receiver";
	- gpios: specifies GPIO used for IR signal reception.
?
> +
> +Optional properties:
> +	- linux,rc-map-name: Linux specific remote control map name.
> +
> +Example node:
> +
> +	ir: ir-receiver {
> +		compatible = "gpio-ir-receiver";
> +		gpios =<&gpio0 19 1>;
> +		linux,rc-map-name = "rc-rc6-mce";
> +	};
> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> index 4f71a7d..3c62006 100644
> --- a/drivers/media/rc/gpio-ir-recv.c
> +++ b/drivers/media/rc/gpio-ir-recv.c
> @@ -16,6 +16,7 @@
>   #include<linux/interrupt.h>
>   #include<linux/gpio.h>
>   #include<linux/slab.h>
> +#include<linux/of_gpio.h>
>   #include<linux/platform_device.h>
>   #include<linux/irq.h>
>   #include<media/rc-core.h>
> @@ -30,6 +31,50 @@ struct gpio_rc_dev {
>   	bool active_low;
>   };
>
> +#ifdef CONFIG_OF
> +/*
> + * Translate OpenFirmware node properties into platform_data
> + */
> +static int gpio_ir_recv_get_devtree_pdata(struct device *dev,
> +				  struct gpio_ir_recv_platform_data *pdata)
> +{
> +	struct device_node *np = dev->of_node;
> +	enum of_gpio_flags flags;
> +	int gpio;
> +
> +	gpio = of_get_gpio_flags(np, 0,&flags);
> +	if (gpio<  0) {
> +		if (gpio != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get gpio flags, error: %d\n",
> +				gpio);

			dev_err(dev, "Failed to get gpio flags (%d)\n",	gpio);
?
> +		return gpio;
> +	}
> +
> +	pdata->gpio_nr = gpio;
> +	pdata->active_low = (flags&  OF_GPIO_ACTIVE_LOW) ? true : false;

This could be simplified to:

	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW);

> +	/* probe() takes care of map_name == NULL or allowed_protos == 0 */
> +	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
> +	pdata->allowed_protos = 0;
> +
> +	return 0;
> +}
> +
> +static struct of_device_id gpio_ir_recv_of_match[] = {
> +	{ .compatible = "gpio-ir-receiver", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
> +
> +#else /* !CONFIG_OF */
> +
> +static inline struct gpio_ir_recv_platform_data *
> +gpio_ir_recv_get_devtree_pdata(struct device *dev)

Please check how it compiles with CONFIG_OF disabled ;)

You could also make it:

#define gpio_ir_recv_get_devtree_pdata  (-ENOSYS)

> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +#endif
> +
>   static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>   {
>   	struct gpio_rc_dev *gpio_dev = dev_id;
> @@ -66,6 +111,17 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>   					pdev->dev.platform_data;
>   	int rc;
>
> +	if (pdev->dev.of_node) {
> +		struct gpio_ir_recv_platform_data *dtpdata =

I think you could use pdata here instead, as previously. But I'm fine with
as it is now as well.

> +			devm_kzalloc(&pdev->dev, sizeof(*dtpdata), GFP_KERNEL);
> +		if (!dtpdata)
> +			return -ENOMEM;
> +		rc = gpio_ir_recv_get_devtree_pdata(&pdev->dev, dtpdata);
> +		if (rc)
> +			return rc;
> +		pdata = dtpdata;
> +	}
> +
>   	if (!pdata)
>   		return -EINVAL;
>
> @@ -192,6 +248,7 @@ static struct platform_driver gpio_ir_recv_driver = {
>   	.driver = {
>   		.name   = GPIO_IR_DRIVER_NAME,
>   		.owner  = THIS_MODULE,
> +		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
>   #ifdef CONFIG_PM
>   		.pm	=&gpio_ir_recv_pm_ops,
>   #endif

The patch looks good to me in general, after fixing the empty function
declaration please feel free to add:

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

--

Thanks,
Sylwester

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

On 02/08/2013 10:26 PM, Sylwester Nawrocki wrote:
> On 02/08/2013 09:38 PM, Sebastian Hesselbarth wrote:
>> This patch adds device tree parsing for gpio_ir_recv platform_data and
>> the mandatory binding documentation. It basically follows what we already
>> have for e.g. gpio_keys. All required device tree properties are OS
>> independent but an optional property allow linux specific support for rc
>> maps.
>>
>> There was a similar patch sent by Matus Ujhelyi but that discussion
>> died after the first reviews.
>>
>> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> ---
>> Changelog
>>
>> v1->v2:
>> - get rid of ptr returned by _get_devtree_pdata()
>> - check for of_node instead for NULL pdata
>> - remove unneccessary double check for gpios property
>> - remove unneccessary #ifdef CONFIG_OF around match table
>>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Rob Herring<rob.herring@calxeda.com>
>> Cc: Rob Landley<rob@landley.net>
>> Cc: Mauro Carvalho Chehab<mchehab@redhat.com>
>> Cc: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> Cc: Benoit Thebaudeau<benoit.thebaudeau@advansee.com>
>> Cc: David Hardeman<david@hardeman.nu>
>> Cc: Trilok Soni<tsoni@codeaurora.org>
>> Cc: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Cc: Matus Ujhelyi<ujhelyi.m@gmail.com>
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-media@vger.kernel.org
>> ---
>> .../devicetree/bindings/media/gpio-ir-receiver.txt | 16 ++++++
>> drivers/media/rc/gpio-ir-recv.c | 57 ++++++++++++++++++++
>> 2 files changed, 73 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> new file mode 100644
>> index 0000000..8589f30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> @@ -0,0 +1,16 @@
>> +Device-Tree bindings for GPIO IR receiver
>> +
>> +Required properties:
>> + - compatible = "gpio-ir-receiver";
>> + - gpios: OF device-tree gpio specification.
>
> Perhaps:
> - compatible: should be "gpio-ir-receiver";
> - gpios: specifies GPIO used for IR signal reception.
> ?

Ok, i'll change that.

>> +
>> +Optional properties:
>> + - linux,rc-map-name: Linux specific remote control map name.
>> +
>> +Example node:
>> +
>> + ir: ir-receiver {
>> + compatible = "gpio-ir-receiver";
>> + gpios =<&gpio0 19 1>;
>> + linux,rc-map-name = "rc-rc6-mce";
>> + };
>> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
>> index 4f71a7d..3c62006 100644
>> --- a/drivers/media/rc/gpio-ir-recv.c
>> +++ b/drivers/media/rc/gpio-ir-recv.c
>> @@ -16,6 +16,7 @@
>> #include<linux/interrupt.h>
>> #include<linux/gpio.h>
>> #include<linux/slab.h>
>> +#include<linux/of_gpio.h>
>> #include<linux/platform_device.h>
>> #include<linux/irq.h>
>> #include<media/rc-core.h>
>> @@ -30,6 +31,50 @@ struct gpio_rc_dev {
>> bool active_low;
>> };
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * Translate OpenFirmware node properties into platform_data
>> + */
>> +static int gpio_ir_recv_get_devtree_pdata(struct device *dev,
>> + struct gpio_ir_recv_platform_data *pdata)
>> +{
>> + struct device_node *np = dev->of_node;
>> + enum of_gpio_flags flags;
>> + int gpio;
>> +
>> + gpio = of_get_gpio_flags(np, 0,&flags);
>> + if (gpio< 0) {
>> + if (gpio != -EPROBE_DEFER)
>> + dev_err(dev, "Failed to get gpio flags, error: %d\n",
>> + gpio);
>
> dev_err(dev, "Failed to get gpio flags (%d)\n", gpio);
> ?

Ack.

>> + return gpio;
>> + }
>> +
>> + pdata->gpio_nr = gpio;
>> + pdata->active_low = (flags& OF_GPIO_ACTIVE_LOW) ? true : false;
>
> This could be simplified to:
>
> pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW);

Ack.

>> + /* probe() takes care of map_name == NULL or allowed_protos == 0 */
>> + pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
>> + pdata->allowed_protos = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static struct of_device_id gpio_ir_recv_of_match[] = {
>> + { .compatible = "gpio-ir-receiver", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
>> +
>> +#else /* !CONFIG_OF */
>> +
>> +static inline struct gpio_ir_recv_platform_data *
>> +gpio_ir_recv_get_devtree_pdata(struct device *dev)
>
> Please check how it compiles with CONFIG_OF disabled ;)

Grrrml ;) I'll fix that of course...

> You could also make it:
>
> #define gpio_ir_recv_get_devtree_pdata (-ENOSYS)

Hmm, does that also play with parameter passing of the
CONFIG_OF gpio_ir_recv_get_devtree_pdata() ?

>> +{
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +#endif
>> +
>> static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>> {
>> struct gpio_rc_dev *gpio_dev = dev_id;
>> @@ -66,6 +111,17 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
>> pdev->dev.platform_data;
>> int rc;
>>
>> + if (pdev->dev.of_node) {
>> + struct gpio_ir_recv_platform_data *dtpdata =
>
> I think you could use pdata here instead, as previously. But I'm fine with
> as it is now as well.

Yeah, but pdata is const and I will change it within _get_devtree_pdata().
I could cast the const away when passing it to gpio_ir_recv_get_devtree_pdata()
but it is almost the same amount of code.. and it is cleaner this way.

>
>> + devm_kzalloc(&pdev->dev, sizeof(*dtpdata), GFP_KERNEL);
>> + if (!dtpdata)
>> + return -ENOMEM;
>> + rc = gpio_ir_recv_get_devtree_pdata(&pdev->dev, dtpdata);
>> + if (rc)
>> + return rc;
>> + pdata = dtpdata;
>> + }
>> +
>> if (!pdata)
>> return -EINVAL;
>>
>> @@ -192,6 +248,7 @@ static struct platform_driver gpio_ir_recv_driver = {
>> .driver = {
>> .name = GPIO_IR_DRIVER_NAME,
>> .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(gpio_ir_recv_of_match),
>> #ifdef CONFIG_PM
>> .pm =&gpio_ir_recv_pm_ops,
>> #endif
>
> The patch looks good to me in general, after fixing the empty function
> declaration please feel free to add:
>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thanks for the review (again),

I'll push a v3 with your remarks today or tomorrow.

Sebastian

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sylwester Nawrocki]

On 02/08/2013 10:36 PM, Sebastian Hesselbarth wrote:
>> You could also make it:
>>
>> #define gpio_ir_recv_get_devtree_pdata (-ENOSYS)
>
> Hmm, does that also play with parameter passing of the
> CONFIG_OF gpio_ir_recv_get_devtree_pdata() ?

Oops, should have been:

#define gpio_ir_recv_get_devtree_pdata(dev, pd) (-ENOSYS)

>> #define gpio_ir_recv_get_devtree_pdata (-ENOSYS)
>>> +{
>>> + return ERR_PTR(-ENODEV);
>>> +}
>>> +
>>> +#endif
>>> +
>>> static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>>> {
>>> struct gpio_rc_dev *gpio_dev = dev_id;
>>> @@ -66,6 +111,17 @@ static int gpio_ir_recv_probe(struct
>>> platform_device *pdev)
>>> pdev->dev.platform_data;
>>> int rc;
>>>
>>> + if (pdev->dev.of_node) {
>>> + struct gpio_ir_recv_platform_data *dtpdata =
>>
>> I think you could use pdata here instead, as previously. But I'm fine
>> with
>> as it is now as well.
>
> Yeah, but pdata is const and I will change it within _get_devtree_pdata().
> I could cast the const away when passing it to
> gpio_ir_recv_get_devtree_pdata()
> but it is almost the same amount of code.. and it is cleaner this way.

True, let's leave it intact then.

S.

[PATCH v3] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

This patch adds device tree parsing for gpio_ir_recv platform_data and
the mandatory binding documentation. It basically follows what we already
have for e.g. gpio_keys. All required device tree properties are OS
independent but an optional property allows linux specific support for rc
maps.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
There was a similar patch sent by Matus Ujhelyi but that discussion
died after the first reviews.

Changelog

v1->v2:
- get rid of ptr returned by _get_devtree_pdata()
- check for of_node instead for NULL pdata
- remove unneccessary double check for gpios property
- remove unneccessary #ifdef CONFIG_OF around match table

v2->v3:
- use define for !OF_CONFIG _get_devtree_pdata()
- simplify flags test and of_get_gpio_flags error msg
- fix some typos in binding documentation
- move note about previous patch submission to non-commit area
 
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
Cc: David Hardeman <david@hardeman.nu>
Cc: Trilok Soni <tsoni@codeaurora.org>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Matus Ujhelyi <ujhelyi.m@gmail.com>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-media@vger.kernel.org
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
 drivers/media/rc/gpio-ir-recv.c                    |   52 ++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
new file mode 100644
index 0000000..56e726e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -0,0 +1,16 @@
+Device-Tree bindings for GPIO IR receiver
+
+Required properties:
+	- compatible: should be "gpio-ir-receiver".
+	- gpios: specifies GPIO used for IR signal reception.
+
+Optional properties:
+	- linux,rc-map-name: Linux specific remote control map name.
+
+Example node:
+
+	ir: ir-receiver {
+		compatible = "gpio-ir-receiver";
+		gpios = <&gpio0 19 1>;
+		linux,rc-map-name = "rc-rc6-mce";
+	};
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 4f71a7d..b5cfb44 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <media/rc-core.h>
@@ -30,6 +31,45 @@ struct gpio_rc_dev {
 	bool active_low;
 };
 
+#ifdef CONFIG_OF
+/*
+ * Translate OpenFirmware node properties into platform_data
+ */
+static int gpio_ir_recv_get_devtree_pdata(struct device *dev,
+				  struct gpio_ir_recv_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	enum of_gpio_flags flags;
+	int gpio;
+
+	gpio = of_get_gpio_flags(np, 0, &flags);
+	if (gpio < 0) {
+		if (gpio != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get gpio flags (%d)\n", gpio);
+		return gpio;
+	}
+
+	pdata->gpio_nr = gpio;
+	pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW);
+	/* probe() takes care of map_name == NULL or allowed_protos == 0 */
+	pdata->map_name = of_get_property(np, "linux,rc-map-name", NULL);
+	pdata->allowed_protos = 0;
+
+	return 0;
+}
+
+static struct of_device_id gpio_ir_recv_of_match[] = {
+	{ .compatible = "gpio-ir-receiver", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match);
+
+#else /* !CONFIG_OF */
+
+#define gpio_ir_recv_get_devtree_pdata(dev, pdata)	(-ENOSYS)
+
+#endif
+
 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 {
 	struct gpio_rc_dev *gpio_dev = dev_id;
@@ -66,6 +106,17 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 					pdev->dev.platform_data;
 	int rc;
 
+	if (pdev->dev.of_node) {
+		struct gpio_ir_recv_platform_data *dtpdata =
+			devm_kzalloc(&pdev->dev, sizeof(*dtpdata), GFP_KERNEL);
+		if (!dtpdata)
+			return -ENOMEM;
+		rc = gpio_ir_recv_get_devtree_pdata(&pdev->dev, dtpdata);
+		if (rc)
+			return rc;
+		pdata = dtpdata;
+	}
+
 	if (!pdata)
 		return -EINVAL;
 
@@ -192,6 +243,7 @@ static struct platform_driver gpio_ir_recv_driver = {
 	.driver = {
 		.name   = GPIO_IR_DRIVER_NAME,
 		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_ir_recv_of_match),
 #ifdef CONFIG_PM
 		.pm	= &gpio_ir_recv_pm_ops,
 #endif
-- 
1.7.10.4

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Mauro Carvalho Chehab]

Em Fri,  8 Feb 2013 21:38:07 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:

> This patch adds device tree parsing for gpio_ir_recv platform_data and
> the mandatory binding documentation. It basically follows what we already
> have for e.g. gpio_keys. All required device tree properties are OS
> independent but an optional property allow linux specific support for rc
> maps.
> 
> There was a similar patch sent by Matus Ujhelyi but that discussion
> died after the first reviews.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog
> 
> v1->v2:
> - get rid of ptr returned by _get_devtree_pdata()
> - check for of_node instead for NULL pdata
> - remove unneccessary double check for gpios property
> - remove unneccessary #ifdef CONFIG_OF around match table
> 
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
> Cc: David Hardeman <david@hardeman.nu>
> Cc: Trilok Soni <tsoni@codeaurora.org>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Matus Ujhelyi <ujhelyi.m@gmail.com>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> ---
>  .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
>  drivers/media/rc/gpio-ir-recv.c                    |   57 ++++++++++++++++++++
>  2 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> new file mode 100644
> index 0000000..8589f30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> @@ -0,0 +1,16 @@
> +Device-Tree bindings for GPIO IR receiver
> +
> +Required properties:
> +	- compatible = "gpio-ir-receiver";
> +	- gpios: OF device-tree gpio specification.
> +
> +Optional properties:
> +	- linux,rc-map-name: Linux specific remote control map name.
> +
> +Example node:
> +
> +	ir: ir-receiver {
> +		compatible = "gpio-ir-receiver";
> +		gpios = <&gpio0 19 1>;
> +		linux,rc-map-name = "rc-rc6-mce";

Please change this to:
		linux,rc-map-name = RC_MAP_RC6_MCE;

(as defined at include/media/rc-map.h).

The idea of having those strings defined at the same header file is to:

	- make sure that the same keyboard is spelled at the same way on
all places;

	- avoid people to duplicate IR keytables, using different names;

	- help userspace to get the right table. In the future, the
plan is to remove all keytables from kernelspace, keeping there just the
name of the keytable. The existing userspace code (ir-keytables, part
of v4l-utils) use the keytable name to dynamically load the right table
in runtime and to switch the IR core to only handle the protocol that
it is associated with the loaded keytable.

Regards,
Mauro

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sebastian Hesselbarth]

On 02/09/2013 01:03 AM, Mauro Carvalho Chehab wrote:
> Em Fri,  8 Feb 2013 21:38:07 +0100
> Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  escreveu:
>
>> This patch adds device tree parsing for gpio_ir_recv platform_data and
>> the mandatory binding documentation. It basically follows what we already
>> have for e.g. gpio_keys. All required device tree properties are OS
>> independent but an optional property allow linux specific support for rc
>> maps.
>>
>> There was a similar patch sent by Matus Ujhelyi but that discussion
>> died after the first reviews.
>>
>> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> ---
>> Changelog
>>
>> v1->v2:
>> - get rid of ptr returned by _get_devtree_pdata()
>> - check for of_node instead for NULL pdata
>> - remove unneccessary double check for gpios property
>> - remove unneccessary #ifdef CONFIG_OF around match table
>>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Rob Herring<rob.herring@calxeda.com>
>> Cc: Rob Landley<rob@landley.net>
>> Cc: Mauro Carvalho Chehab<mchehab@redhat.com>
>> Cc: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> Cc: Benoit Thebaudeau<benoit.thebaudeau@advansee.com>
>> Cc: David Hardeman<david@hardeman.nu>
>> Cc: Trilok Soni<tsoni@codeaurora.org>
>> Cc: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Cc: Matus Ujhelyi<ujhelyi.m@gmail.com>
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-media@vger.kernel.org
>> ---
>>   .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
>>   drivers/media/rc/gpio-ir-recv.c                    |   57 ++++++++++++++++++++
>>   2 files changed, 73 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> new file mode 100644
>> index 0000000..8589f30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>> @@ -0,0 +1,16 @@
>> +Device-Tree bindings for GPIO IR receiver
>> +
>> +Required properties:
>> +	- compatible = "gpio-ir-receiver";
>> +	- gpios: OF device-tree gpio specification.
>> +
>> +Optional properties:
>> +	- linux,rc-map-name: Linux specific remote control map name.
>> +
>> +Example node:
>> +
>> +	ir: ir-receiver {
>> +		compatible = "gpio-ir-receiver";
>> +		gpios =<&gpio0 19 1>;
>> +		linux,rc-map-name = "rc-rc6-mce";
>
> Please change this to:
> 		linux,rc-map-name = RC_MAP_RC6_MCE;
>
> (as defined at include/media/rc-map.h).

Mauro,

this is not possible in device tree bindings. Device tree properties
can only carry numeric or string types (and some other stuff) but no
OS specific enumerations. So using strings is the only option here.

> The idea of having those strings defined at the same header file is to:

Unfortunately, device tree blobs don't know about linux header files.

That leaves two options:
- allow the user to supply a string of the map in his device tree description
   and risk that there may be a broken map name
- remove linux,rc-map-name from DT binding and let the user configure in
   from user space (which is propably best choice anyway)

I tried both, DT supplied map name and ir-keytable from userspace
both work fine.

Sebastian

> 	- make sure that the same keyboard is spelled at the same way on
> all places;
>
> 	- avoid people to duplicate IR keytables, using different names;
>
> 	- help userspace to get the right table. In the future, the
> plan is to remove all keytables from kernelspace, keeping there just the
> name of the keytable. The existing userspace code (ir-keytables, part
> of v4l-utils) use the keytable name to dynamically load the right table
> in runtime and to switch the IR core to only handle the protocol that
> it is associated with the loaded keytable.

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Mauro Carvalho Chehab]

Em Sat, 09 Feb 2013 01:45:42 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> escreveu:

> On 02/09/2013 01:03 AM, Mauro Carvalho Chehab wrote:
> > Em Fri,  8 Feb 2013 21:38:07 +0100
> > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  escreveu:
> >
> >> This patch adds device tree parsing for gpio_ir_recv platform_data and
> >> the mandatory binding documentation. It basically follows what we already
> >> have for e.g. gpio_keys. All required device tree properties are OS
> >> independent but an optional property allow linux specific support for rc
> >> maps.
> >>
> >> There was a similar patch sent by Matus Ujhelyi but that discussion
> >> died after the first reviews.
> >>
> >> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> >> ---
> >> Changelog
> >>
> >> v1->v2:
> >> - get rid of ptr returned by _get_devtree_pdata()
> >> - check for of_node instead for NULL pdata
> >> - remove unneccessary double check for gpios property
> >> - remove unneccessary #ifdef CONFIG_OF around match table
> >>
> >> Cc: Grant Likely<grant.likely@secretlab.ca>
> >> Cc: Rob Herring<rob.herring@calxeda.com>
> >> Cc: Rob Landley<rob@landley.net>
> >> Cc: Mauro Carvalho Chehab<mchehab@redhat.com>
> >> Cc: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> >> Cc: Benoit Thebaudeau<benoit.thebaudeau@advansee.com>
> >> Cc: David Hardeman<david@hardeman.nu>
> >> Cc: Trilok Soni<tsoni@codeaurora.org>
> >> Cc: Sylwester Nawrocki<s.nawrocki@samsung.com>
> >> Cc: Matus Ujhelyi<ujhelyi.m@gmail.com>
> >> Cc: devicetree-discuss@lists.ozlabs.org
> >> Cc: linux-doc@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-media@vger.kernel.org
> >> ---
> >>   .../devicetree/bindings/media/gpio-ir-receiver.txt |   16 ++++++
> >>   drivers/media/rc/gpio-ir-recv.c                    |   57 ++++++++++++++++++++
> >>   2 files changed, 73 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> new file mode 100644
> >> index 0000000..8589f30
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
> >> @@ -0,0 +1,16 @@
> >> +Device-Tree bindings for GPIO IR receiver
> >> +
> >> +Required properties:
> >> +	- compatible = "gpio-ir-receiver";
> >> +	- gpios: OF device-tree gpio specification.
> >> +
> >> +Optional properties:
> >> +	- linux,rc-map-name: Linux specific remote control map name.
> >> +
> >> +Example node:
> >> +
> >> +	ir: ir-receiver {
> >> +		compatible = "gpio-ir-receiver";
> >> +		gpios =<&gpio0 19 1>;
> >> +		linux,rc-map-name = "rc-rc6-mce";
> >
> > Please change this to:
> > 		linux,rc-map-name = RC_MAP_RC6_MCE;
> >
> > (as defined at include/media/rc-map.h).
> 
> Mauro,
> 
> this is not possible in device tree bindings. Device tree properties
> can only carry numeric or string types (and some other stuff) but no
> OS specific enumerations. So using strings is the only option here.
> 
> > The idea of having those strings defined at the same header file is to:
> 
> Unfortunately, device tree blobs don't know about linux header files.
> 
> That leaves two options:
> - allow the user to supply a string of the map in his device tree description
>    and risk that there may be a broken map name
> - remove linux,rc-map-name from DT binding and let the user configure in
>    from user space (which is propably best choice anyway)
> 
> I tried both, DT supplied map name and ir-keytable from userspace
> both work fine.

IMO, the first option is better, e. g. letting the device tree have the
string there.

Regards,
Mauro

Re: [PATCH v2] media: rc: gpio-ir-recv: add support for device tree parsing

[Sylwester Nawrocki]

On 02/09/2013 01:45 AM, Sebastian Hesselbarth wrote:
>>> new file mode 100644
>>> index 0000000..8589f30
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
>>> @@ -0,0 +1,16 @@
>>> +Device-Tree bindings for GPIO IR receiver
>>> +
>>> +Required properties:
>>> + - compatible = "gpio-ir-receiver";
>>> + - gpios: OF device-tree gpio specification.
>>> +
>>> +Optional properties:
>>> + - linux,rc-map-name: Linux specific remote control map name.
>>> +
>>> +Example node:
>>> +
>>> + ir: ir-receiver {
>>> + compatible = "gpio-ir-receiver";
>>> + gpios =<&gpio0 19 1>;
>>> + linux,rc-map-name = "rc-rc6-mce";
>>
>> Please change this to:
>> linux,rc-map-name = RC_MAP_RC6_MCE;
>>
>> (as defined at include/media/rc-map.h).
>
> Mauro,
>
> this is not possible in device tree bindings. Device tree properties
> can only carry numeric or string types (and some other stuff) but no
> OS specific enumerations. So using strings is the only option here.
>
>> The idea of having those strings defined at the same header file is to:
>
> Unfortunately, device tree blobs don't know about linux header files.

I suppose this will change when it will be possible to run C pre-processor
on *.dts files. This is still under discussion though [1] and for the
device tree there will likely be separate copies of the header files
needed. Thus I guess explicit string names for now need to be used.

[1] http://www.spinics.net/lists/kernel/msg1458360.html