Re: [PATCH] media: st-rc: Add ST remote control driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxx>
> 
> This patch adds support to ST RC driver, which is basically a IR/UHF
> receiver and transmitter. This IP is common across all the ST parts for
> settop box platforms. IRB is embedded in ST COMMS IP block.
> It supports both Rx & Tx functionality.
> 
> In this driver adds only Rx functionality via LIRC codec.

Is there anything that additionally needs to be in the dt to support Tx
functionality?

> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxx>
> ---
> Hi Chehab,
> 
> This is a very simple rc driver for IRB controller found in STi ARM CA9 SOCs.
> STi ARM SOC support went in 3.11 recently.
> This driver is a raw driver which feeds data to lirc codec for the user lircd
> to decode the keys.
> 
> This patch is based on git://linuxtv.org/media_tree.git master branch.
> 
> Comments?
> 
> Thanks,
> srini
> 
>  Documentation/devicetree/bindings/media/st-rc.txt |   18 +
>  drivers/media/rc/Kconfig                          |   10 +
>  drivers/media/rc/Makefile                         |    1 +
>  drivers/media/rc/st_rc.c                          |  371 +++++++++++++++++++++
>  4 files changed, 400 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/st-rc.txt
>  create mode 100644 drivers/media/rc/st_rc.c
> 
> diff --git a/Documentation/devicetree/bindings/media/st-rc.txt b/Documentation/devicetree/bindings/media/st-rc.txt
> new file mode 100644
> index 0000000..57f9ee8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/st-rc.txt
> @@ -0,0 +1,18 @@
> +Device-Tree bindings for ST IR and UHF receiver
> +
> +Required properties:
> +       - compatible: should be "st,rc".

That "rc" should be made more specific, and it seems like this is a
subset of a larger block of IP (the ST COMMS IP block). Is this really a
standalone piece of hardware, or is it always in the larger comms block?

What's the full name of this unit as it appears in documentation?

> +       - st,uhfmode: boolean property to indicate if reception is in UHF.

That's not a very clear name. Is this a physical property of the device
(i.e. it's wired to either an IR receiver or a UHF receiver), or is this
a choice as to how it's used at runtime?

If it's fixed property of how the device is wired, it might make more
sense to have a string mode property that's either "uhf" or "infared".

> +       - reg: base physical address of the controller and length of memory
> +       mapped  region.
> +       - interrupts: interrupt number to the cpu. The interrupt specifier
> +       format depends on the interrupt controller parent.
> +
> +Example node:
> +
> +       rc: rc@fe518000 {
> +               compatible      = "st,rc";
> +               reg             = <0xfe518000 0x234>;
> +               interrupts      =  <0 203 0>;
> +       };
> +

[...]

> +++ b/drivers/media/rc/st_rc.c
> @@ -0,0 +1,371 @@
> +/*
> + * Copyright (C) 2013 STMicroelectronics Limited
> + * Author: Srinivas Kandagatla <srinivas.kandagatla@xxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/kernel.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <media/rc-core.h>
> +#include <linux/pinctrl/consumer.h>
> +
> +struct st_rc_device {
> +       struct device                   *dev;
> +       int                             irq;
> +       int                             irq_wake;
> +       struct clk                      *sys_clock;
> +       void                            *base;  /* Register base address */
> +       void                            *rx_base;/* RX Register base address */
> +       struct rc_dev                   *rdev;
> +       bool                            overclocking;
> +       int                             sample_mult;
> +       int                             sample_div;
> +       bool                            rxuhfmode;
> +};

[...]

> +static void st_rc_hardware_init(struct st_rc_device *dev)
> +{
> +       int baseclock, freqdiff;
> +       unsigned int rx_max_symbol_per = MAX_SYMB_TIME;
> +       unsigned int rx_sampling_freq_div;
> +
> +       clk_prepare_enable(dev->sys_clock);

This clock should be defined in the binding.

> +       baseclock = clk_get_rate(dev->sys_clock);
> +
> +       /* IRB input pins are inverted internally from high to low. */
> +       writel(1, dev->rx_base + IRB_RX_POLARITY_INV);
> +
> +       rx_sampling_freq_div = baseclock / IRB_SAMPLE_FREQ;
> +       writel(rx_sampling_freq_div, dev->base + IRB_SAMPLE_RATE_COMM);
> +
> +       freqdiff = baseclock - (rx_sampling_freq_div * IRB_SAMPLE_FREQ);
> +       if (freqdiff) { /* over clocking, workout the adjustment factors */
> +               dev->overclocking = true;
> +               dev->sample_mult = 1000;
> +               dev->sample_div = baseclock / (10000 * rx_sampling_freq_div);
> +               rx_max_symbol_per = (rx_max_symbol_per * 1000)/dev->sample_div;
> +       }
> +
> +       writel(rx_max_symbol_per, dev->rx_base + IRB_MAX_SYM_PERIOD);
> +}
> +
> +static int st_rc_remove(struct platform_device *pdev)
> +{
> +       struct st_rc_device *rc_dev = platform_get_drvdata(pdev);
> +       clk_disable_unprepare(rc_dev->sys_clock);
> +       rc_unregister_device(rc_dev->rdev);

Is a call to rc_free_device() not necessary?

There are separate calls to rc_allocate_device() and rc_register_device
in the probe function, and an rc_free_device() in its failure path.

> +       return 0;
> +}

[...]

> +static int st_rc_probe(struct platform_device *pdev)
> +{
> +       int ret = -EINVAL;
> +       struct rc_dev *rdev;
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       struct st_rc_device *rc_dev;
> +       struct device_node *np = pdev->dev.of_node;
> +
> +       rc_dev = devm_kzalloc(dev, sizeof(struct st_rc_device), GFP_KERNEL);
> +       rdev = rc_allocate_device();
> +
> +       if (!rc_dev || !rdev)
> +               return -ENOMEM;
> +
> +       if (np)
> +               rc_dev->rxuhfmode = of_property_read_bool(np, "st,uhfmode");

I see of_property_read_bool has an implicit NULL check on np via
__of_find_property, though I'm not sure if we want people to rely on
that.

I don't think a boolean property is the best way of encoding this,
regardless.

> +
> +       rc_dev->sys_clock = devm_clk_get(dev, NULL);
> +       if (IS_ERR(rc_dev->sys_clock)) {
> +               dev_err(dev, "System clock not found\n");
> +               ret = PTR_ERR(rc_dev->sys_clock);
> +               goto err;
> +       }
> +
> +       rc_dev->irq = platform_get_irq(pdev, 0);
> +       if (rc_dev->irq < 0) {
> +               ret = rc_dev->irq;
> +               goto clkerr;
> +       }
> +
> +       ret = -ENODEV;
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               goto clkerr;
> +
> +       rc_dev->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(rc_dev->base))
> +               goto clkerr;
> +
> +       if (rc_dev->rxuhfmode)
> +               rc_dev->rx_base = rc_dev->base + 0x40;
> +       else
> +               rc_dev->rx_base = rc_dev->base;
> +
> +       rc_dev->dev = dev;
> +       platform_set_drvdata(pdev, rc_dev);
> +       st_rc_hardware_init(rc_dev);
> +
> +       rdev->driver_type = RC_DRIVER_IR_RAW;
> +       rdev->allowed_protos = RC_TYPE_LIRC;
> +       rdev->priv = rc_dev;
> +       rdev->open = st_rc_open;
> +       rdev->close = st_rc_close;
> +       rdev->driver_name = IR_ST_NAME;
> +       rdev->map_name = RC_MAP_LIRC;
> +       rdev->input_name = "ST Remote Control Receiver";
> +
> +       /* enable wake via this device */
> +       device_set_wakeup_capable(dev, true);
> +       device_set_wakeup_enable(dev, true);
> +
> +       ret = rc_register_device(rdev);
> +       if (ret < 0)
> +               goto clkerr;
> +
> +       rc_dev->rdev = rdev;
> +       if (devm_request_irq(dev, rc_dev->irq, st_rc_rx_interrupt,
> +                       IRQF_NO_SUSPEND, IR_ST_NAME, rc_dev) < 0) {
> +               dev_err(dev, "IRQ %d register failed\n", rc_dev->irq);
> +               ret = -EINVAL;
> +               goto rcerr;
> +       }
> +
> +       /* for LIRC_MODE_MODE2 or LIRC_MODE_PULSE or LIRC_MODE_RAW
> +        * lircd expects a long space first before a signal train to sync. */

Comment should be like:

	/*
	 * Multi-line comment.
	 * Preceding and trailing / 
	 */

> +       st_rc_send_lirc_timeout(rdev);
> +
> +       dev_info(dev, "setup in %s mode\n", rc_dev->rxuhfmode ? "UHF" : "IR");
> +
> +       return ret;
> +rcerr:
> +       rc_unregister_device(rdev);
> +       rdev = NULL;

Why? Doesn't that mean rdev never gets freed?

> +clkerr:
> +       clk_disable_unprepare(rc_dev->sys_clock);
> +err:
> +       rc_free_device(rdev);
> +       dev_err(dev, "Unable to register device (%d)\n", ret);
> +       return ret;
> +}

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux