Re: [PATCH] media: rc: gpio-ir-recv.c: change platform_data to DT binding
Hi Matus,
Em Tue, 18 Dec 2012 14:41:41 +0100
ujhely...@gmail.com escreveu:
From: Matus Ujhelyi ujhely...@gmail.com
Signed-off-by: Matus Ujhelyi ujhely...@gmail.com
This email lacks a proper description and doesn't c/c the DT maintainer.
I'm not a DT expert, so I won't comment the specific bits there, but
I'm more concerned if this is the right way of doing it.
The thing is that this driver seems to be generic enough to be used not only
by embedded platforms, but also for normal PCI/USB drivers for consumers
hardware, used on x86 machines.
So, it is what we generally call as an ancillary driver, like the I2C drivers
are.
Well, I don't think that a PCI or an USB driver should use DT to pass data to
an ancillary driver like this one. On those non-embedded drivers, the device
detection happens based on the PCI or USB ID, instead of on some DT info.
That's why passing those data through DT seems a bad idea.
IMHO, all those ancillary drivers should be DT-agnostic. So, there must be an
embedded-specific driver that will handle DT and pass the config information
to the driver using the same way a PCI or USB driver would do (currently, via
platform_data).
As an additional benefit, this will help to avoid lots of #ifdef CONFIG_OF
inside the ancillary driver, with is ugly and obfuscates the driver.
Regards,
Mauro
---
drivers/media/rc/gpio-ir-recv.c | 84
++-
1 file changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index ba1a1eb..cfba079 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -58,19 +58,49 @@ err_get_value:
return IRQ_HANDLED;
}
+static const struct of_device_id gpio_ir_of_match[] = {
+ {
+ .compatible = gpio-ir-recv,
+ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_of_matchof_match);
+
+static int __devinit gpio_ir_recv_dt_probe (struct gpio_rc_dev *gpio_dev,
+ struct rc_dev *rcdev, struct platform_device *pdev) {
+
+ struct device_node *node = pdev-dev.of_node;
+
+ if (!node) {
+ pr_err(Missing gpio_nr in the DT\n);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(node, gpio_nr, gpio_dev-gpio_nr)) {
+ pr_err(Missing gpio_nr in the DT\n);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u64(node, allowed_protos,
rcdev-allowed_protos))
+ rcdev-allowed_protos = RC_BIT_ALL;
+
+ if (of_property_read_bool(node, active_low))
+ gpio_dev-active_low = true;
+ else
+ gpio_dev-active_low = false;
+
+ if (of_property_read_string(node, map_name, rcdev-map_name))
+ rcdev-map_name = NULL;
+
+ return 0;
+}
+
static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
{
struct gpio_rc_dev *gpio_dev;
struct rc_dev *rcdev;
const struct gpio_ir_recv_platform_data *pdata =
pdev-dev.platform_data;
- int rc;
-
- if (!pdata)
- return -EINVAL;
-
- if (pdata-gpio_nr 0)
- return -EINVAL;
+ int rc = -EINVAL;
gpio_dev = kzalloc(sizeof(struct gpio_rc_dev), GFP_KERNEL);
if (!gpio_dev)
@@ -82,6 +112,28 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
goto err_allocate_device;
}
+ if (!pdata) {
+
+ if (gpio_ir_recv_dt_probe(gpio_dev, rcdev, pdev))
+ goto err_dt_create_of;
+
+ } else {
+
+ if (pdata-allowed_protos)
+ rcdev-allowed_protos = pdata-allowed_protos;
+ else
+ rcdev-allowed_protos = RC_BIT_ALL;
+ rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
+
+ gpio_dev-gpio_nr = pdata-gpio_nr;
+ gpio_dev-active_low = pdata-active_low;
+
+ }
+
+ if (gpio_dev-gpio_nr 0) {
+ goto err_gpio_nr;
+ }
+
rcdev-priv = gpio_dev;
rcdev-driver_type = RC_DRIVER_IR_RAW;
rcdev-input_name = GPIO_IR_DEVICE_NAME;
@@ -92,20 +144,13 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
rcdev-input_id.version = 0x0100;
rcdev-dev.parent = pdev-dev;
rcdev-driver_name = GPIO_IR_DRIVER_NAME;
- if (pdata-allowed_protos)
- rcdev-allowed_protos = pdata-allowed_protos;
- else
- rcdev-allowed_protos = RC_BIT_ALL;
- rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
gpio_dev-rcdev = rcdev;
- gpio_dev-gpio_nr = pdata-gpio_nr;
- gpio_dev-active_low = pdata-active_low;
- rc = gpio_request(pdata-gpio_nr, gpio-ir-recv);
+ rc = gpio_request(gpio_dev-gpio_nr, gpio-ir-recv);
if (rc 0)
goto err_gpio_request;
- rc = gpio_direction_input(pdata-gpio_nr);
+ rc =
Re: [PATCH] media: rc: gpio-ir-recv.c: change platform_data to DT binding
Hi,
Cc: devicetree-disc...@lists.ozlabs.org
Please Cc this list when introducing any device tree bindings.
And you need documentation for the bindings, so when someone wants
to use the driver they don't have to extract the bindings' semantics
from the code.
On 12/18/2012 02:41 PM, ujhely...@gmail.com wrote:
From: Matus Ujhelyiujhely...@gmail.com
Please provide proper commit text so it is more clear why the patch is
needed.
Also, your patch summary line is very misleading, you're not replacing
platform_data based driver's initialization with device tree based.
This patch is simply _adding_ another initialization method, without
breaking support for platform data, isn't it ?
Signed-off-by: Matus Ujhelyiujhely...@gmail.com
---
drivers/media/rc/gpio-ir-recv.c | 84 ++-
1 file changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index ba1a1eb..cfba079 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -58,19 +58,49 @@ err_get_value:
return IRQ_HANDLED;
}
+static const struct of_device_id gpio_ir_of_match[] = {
+ {
+ .compatible = gpio-ir-recv,
Probably better to make it gpio-ir-receiver.
+ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_of_matchof_match);
I think you didn't try to build this driver as a module, otherwise you
would
have noticed the above typo...
+static int __devinit gpio_ir_recv_dt_probe (struct gpio_rc_dev *gpio_dev,
+ struct rc_dev *rcdev, struct platform_device *pdev) {
+
+ struct device_node *node = pdev-dev.of_node;
+
+ if (!node) {
+ pr_err(Missing gpio_nr in the DT\n);
Really ? :)
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(node, gpio_nr,gpio_dev-gpio_nr)) {
If I'm not mistaken, generic gpios property would be more appropriate
here...
+ pr_err(Missing gpio_nr in the DT\n);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u64(node, allowed_protos,rcdev-allowed_protos))
+ rcdev-allowed_protos = RC_BIT_ALL;
+
+ if (of_property_read_bool(node, active_low))
...and this one would not be needed, since the GPIO's active state could be
passed through generic OF gpio flags, e.g.
enum of_gpio_flags flags;
gpio_dev-gpio_nr = of_get_gpio_flags(node, 0, flags);
gpio_dev-active_low = flags OF_GPIO_ACTIVE_LOW;
+ gpio_dev-active_low = true;
+ else
+ gpio_dev-active_low = false;
+
+ if (of_property_read_string(node, map_name,rcdev-map_name))
+ rcdev-map_name = NULL;
IIRC underscores are discouraged in the property names. What map_name
signifies here, anyway ?
+
+ return 0;
+}
+
static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
{
struct gpio_rc_dev *gpio_dev;
struct rc_dev *rcdev;
const struct gpio_ir_recv_platform_data *pdata =
pdev-dev.platform_data;
- int rc;
-
- if (!pdata)
- return -EINVAL;
-
- if (pdata-gpio_nr 0)
- return -EINVAL;
+ int rc = -EINVAL;
gpio_dev = kzalloc(sizeof(struct gpio_rc_dev), GFP_KERNEL);
if (!gpio_dev)
@@ -82,6 +112,28 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
goto err_allocate_device;
}
+ if (!pdata) {
+
nit: Unnecessary empty line.
+ if (gpio_ir_recv_dt_probe(gpio_dev, rcdev, pdev))
+ goto err_dt_create_of;
+
Ditto.
+ } else {
+
Ditto.
+ if (pdata-allowed_protos)
+ rcdev-allowed_protos = pdata-allowed_protos;
+ else
+ rcdev-allowed_protos = RC_BIT_ALL;
+ rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
+
+ gpio_dev-gpio_nr = pdata-gpio_nr;
+ gpio_dev-active_low = pdata-active_low;
+
Ditto.
+ }
+
+ if (gpio_dev-gpio_nr 0) {
if (gpio_is_valid(gpio_dev-gpio_nr))
+ goto err_gpio_nr;
+ }
+
rcdev-priv = gpio_dev;
rcdev-driver_type = RC_DRIVER_IR_RAW;
rcdev-input_name = GPIO_IR_DEVICE_NAME;
@@ -92,20 +144,13 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
rcdev-input_id.version = 0x0100;
rcdev-dev.parent =pdev-dev;
rcdev-driver_name = GPIO_IR_DRIVER_NAME;
- if (pdata-allowed_protos)
- rcdev-allowed_protos = pdata-allowed_protos;
- else
- rcdev-allowed_protos = RC_BIT_ALL;
- rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
gpio_dev-rcdev = rcdev;
- gpio_dev-gpio_nr = pdata-gpio_nr;
- gpio_dev-active_low = pdata-active_low;
- rc = gpio_request(pdata-gpio_nr, gpio-ir-recv);
+ rc =
[PATCH] media: rc: gpio-ir-recv.c: change platform_data to DT binding
From: Matus Ujhelyi ujhely...@gmail.com
Signed-off-by: Matus Ujhelyi ujhely...@gmail.com
---
drivers/media/rc/gpio-ir-recv.c | 84 ++-
1 file changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index ba1a1eb..cfba079 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -58,19 +58,49 @@ err_get_value:
return IRQ_HANDLED;
}
+static const struct of_device_id gpio_ir_of_match[] = {
+ {
+ .compatible = gpio-ir-recv,
+ },
+};
+MODULE_DEVICE_TABLE(of, gpio_ir_of_matchof_match);
+
+static int __devinit gpio_ir_recv_dt_probe (struct gpio_rc_dev *gpio_dev,
+ struct rc_dev *rcdev, struct platform_device *pdev) {
+
+ struct device_node *node = pdev-dev.of_node;
+
+ if (!node) {
+ pr_err(Missing gpio_nr in the DT\n);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(node, gpio_nr, gpio_dev-gpio_nr)) {
+ pr_err(Missing gpio_nr in the DT\n);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u64(node, allowed_protos,
rcdev-allowed_protos))
+ rcdev-allowed_protos = RC_BIT_ALL;
+
+ if (of_property_read_bool(node, active_low))
+ gpio_dev-active_low = true;
+ else
+ gpio_dev-active_low = false;
+
+ if (of_property_read_string(node, map_name, rcdev-map_name))
+ rcdev-map_name = NULL;
+
+ return 0;
+}
+
static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
{
struct gpio_rc_dev *gpio_dev;
struct rc_dev *rcdev;
const struct gpio_ir_recv_platform_data *pdata =
pdev-dev.platform_data;
- int rc;
-
- if (!pdata)
- return -EINVAL;
-
- if (pdata-gpio_nr 0)
- return -EINVAL;
+ int rc = -EINVAL;
gpio_dev = kzalloc(sizeof(struct gpio_rc_dev), GFP_KERNEL);
if (!gpio_dev)
@@ -82,6 +112,28 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
goto err_allocate_device;
}
+ if (!pdata) {
+
+ if (gpio_ir_recv_dt_probe(gpio_dev, rcdev, pdev))
+ goto err_dt_create_of;
+
+ } else {
+
+ if (pdata-allowed_protos)
+ rcdev-allowed_protos = pdata-allowed_protos;
+ else
+ rcdev-allowed_protos = RC_BIT_ALL;
+ rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
+
+ gpio_dev-gpio_nr = pdata-gpio_nr;
+ gpio_dev-active_low = pdata-active_low;
+
+ }
+
+ if (gpio_dev-gpio_nr 0) {
+ goto err_gpio_nr;
+ }
+
rcdev-priv = gpio_dev;
rcdev-driver_type = RC_DRIVER_IR_RAW;
rcdev-input_name = GPIO_IR_DEVICE_NAME;
@@ -92,20 +144,13 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
rcdev-input_id.version = 0x0100;
rcdev-dev.parent = pdev-dev;
rcdev-driver_name = GPIO_IR_DRIVER_NAME;
- if (pdata-allowed_protos)
- rcdev-allowed_protos = pdata-allowed_protos;
- else
- rcdev-allowed_protos = RC_BIT_ALL;
- rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY;
gpio_dev-rcdev = rcdev;
- gpio_dev-gpio_nr = pdata-gpio_nr;
- gpio_dev-active_low = pdata-active_low;
- rc = gpio_request(pdata-gpio_nr, gpio-ir-recv);
+ rc = gpio_request(gpio_dev-gpio_nr, gpio-ir-recv);
if (rc 0)
goto err_gpio_request;
- rc = gpio_direction_input(pdata-gpio_nr);
+ rc = gpio_direction_input(gpio_dev-gpio_nr);
if (rc 0)
goto err_gpio_direction_input;
@@ -117,7 +162,7 @@ static int __devinit gpio_ir_recv_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, gpio_dev);
- rc = request_any_context_irq(gpio_to_irq(pdata-gpio_nr),
+ rc = request_any_context_irq(gpio_to_irq(gpio_dev-gpio_nr),
gpio_ir_recv_irq,
IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
gpio-ir-recv-irq, gpio_dev);
@@ -131,8 +176,10 @@ err_request_irq:
rc_unregister_device(rcdev);
err_register_rc_device:
err_gpio_direction_input:
- gpio_free(pdata-gpio_nr);
+ gpio_free(gpio_dev-gpio_nr);
err_gpio_request:
+err_dt_create_of:
+err_gpio_nr:
rc_free_device(rcdev);
rcdev = NULL;
err_allocate_device:
@@ -195,6 +242,7 @@ static struct platform_driver gpio_ir_recv_driver = {
#ifdef CONFIG_PM
.pm = gpio_ir_recv_pm_ops,
#endif
+ .of_match_table = of_match_ptr(gpio_ir_of_match),
},
};
module_platform_driver(gpio_ir_recv_driver);
--
1.7.9.5
--
To unsubscribe from this list: send the line
