| Message ID | 1355838101-972-1-git-send-email-ujhelyi.m@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matus, Em Tue, 18 Dec 2012 14:41:41 +0100 ujhelyi.m@gmail.com escreveu: > From: Matus Ujhelyi <ujhelyi.m@gmail.com> > > Signed-off-by: Matus Ujhelyi <ujhelyi.m@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 = 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); Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Cc: devicetree-discuss@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, ujhelyi.m@gmail.com wrote: > From: Matus Ujhelyi<ujhelyi.m@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 Ujhelyi<ujhelyi.m@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 = 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); Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);