| Message ID | 1376501221-22416-1-git-send-email-srinivas.kandagatla@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@st.com> > > 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@st.com> > --- > 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@st.com> > + * > + * 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 linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Mark for your comments. On 15/08/13 09:49, Mark Rutland wrote: > On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> >> >> 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? We need an additional boolean property for TX enable. + Two more configurable parameters for TX sub-carrier frequency and duty-cycle. By default driver can set the sub carrier frequency to be 38Khz and duty cycle as 50%. However I don't have strong use case to make these configurable. So, I think just one boolean property to enable tx should be Ok. > >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com> >> --- >> 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? COMMS block is a collection of communication peripherals, and IRB(Infra red blaster) is always part of the COMMS block. May renaming the compatible to "st,comms-rc" might be more clear. > > What's the full name of this unit as it appears in documentation? It is always refered as the Communication sub-system (COMMS) which is a collection of communication peripherals like UART, I2C, SPI, IRB. > >> + - 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? Its both, some boards have IR and others UHF receivers, So it becomes board choice here. And also the driver has different register set for UHF receiver and IR receiver. This options selects which registers to use depending on mode of reception. > > 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". Am ok with either approaches. > >> + - 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@st.com> >> + * >> + * 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. Clock is coming for the device tree only. I can add the clock and pinctrl bindings to the documentation if it makes it more clear. > >> + 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. > rc_unregister_device does call rc_free_device as well. >> + 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. Am ok to move to string property for this. > >> + >> + 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: Yep.. I missed this one... > > /* > * 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? As its freed in rc_unregister_device(). Thanks, srini > >> +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 linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/08/13 14:30, Pawel Moll wrote: > On Wed, 2013-08-14 at 18:27 +0100, Srinivas KANDAGATLA wrote: >> +Device-Tree bindings for ST IR and UHF receiver >> + >> +Required properties: >> + - compatible: should be "st,rc". >> + - st,uhfmode: boolean property to indicate if reception is in UHF. >> + - 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>; >> + }; > > So is "st,uhfmode" required or optional after all? If the former, the > example is wrong (doesn't specify required property). But as far as I > understand it's really optional... You are right, I will move this to optional properties section. Thanks, srini > > Pawe? > > > -- 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
On Thu, Aug 15, 2013 at 01:57:13PM +0100, Srinivas KANDAGATLA wrote: > Thanks Mark for your comments. > > On 15/08/13 09:49, Mark Rutland wrote: > > On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: > >> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> > >> > >> 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? > > We need an additional boolean property for TX enable. Why? Becuase you might not have something wired up for Tx? What needs to be present physically for Tx support? > > + > > Two more configurable parameters for TX sub-carrier frequency and > duty-cycle. By default driver can set the sub carrier frequency to be > 38Khz and duty cycle as 50%. > However I don't have strong use case to make these configurable. > > So, I think just one boolean property to enable tx should be Ok. > > > > >> > >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com> > >> --- > >> 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? > COMMS block is a collection of communication peripherals, and IRB(Infra > red blaster) is always part of the COMMS block. > > May renaming the compatible to "st,comms-rc" might be more clear. Given the actual name of the hardware seems to include "irb", I think "irb" makes more sense than "rc" in the compatible string. Is there no more specific name than "Infra Red Blaster"? > > > > > What's the full name of this unit as it appears in documentation? > It is always refered as the Communication sub-system (COMMS) which is a > collection of communication peripherals like UART, I2C, SPI, IRB. Ok, are those separate IP blocks within a container, or is anything shared? Does the COMMS block have any registers shared between those units, for example? > > > > >> + - 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? > > Its both, some boards have IR and others UHF receivers, So it becomes > board choice here. When you say it's both, what do you mean? Is it possible for a unit to be wired with both IR and UHF support simultaneously, even if the Linux driver can't handle that? The dt should encode information about the hardware, not the way you intend to use the hardware at the present moment in time. > And also the driver has different register set for UHF receiver and IR > receiver. This options selects which registers to use depending on mode > of reception. Ok, but that's an implementation issue. If you described that IR *may* be used, and UHF *may* be used, the driver can choose what to do based on that. > > > > > 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". > > Am ok with either approaches. It sounds like we may need separate properties as mentioned above, or a supported-modes list, perhaps. > > > > >> + - 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@st.com> > >> + * > >> + * 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. > Clock is coming for the device tree only. > > I can add the clock and pinctrl bindings to the documentation if it > makes it more clear. If we need clocks and pinctrl, they should be described form the start. Given we already have the infrastructure, there's no reason not to. Not doing so will only lead to headaches later as we try to keep bindings stable. > > > > >> + 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. > > > rc_unregister_device does call rc_free_device as well. I see. That's a somewhat confusing API design, but that's not down to you. Thanks, Mark. -- 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
On Wed, 2013-08-14 at 18:27 +0100, Srinivas KANDAGATLA wrote: > +Device-Tree bindings for ST IR and UHF receiver > + > +Required properties: > + - compatible: should be "st,rc". > + - st,uhfmode: boolean property to indicate if reception is in UHF. > + - 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>; > + }; So is "st,uhfmode" required or optional after all? If the former, the example is wrong (doesn't specify required property). But as far as I understand it's really optional... Pawe? -- 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
On 15/08/13 14:29, Mark Rutland wrote: > On Thu, Aug 15, 2013 at 01:57:13PM +0100, Srinivas KANDAGATLA wrote: >> Thanks Mark for your comments. >> >> On 15/08/13 09:49, Mark Rutland wrote: >>> On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: >>>> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> >>>> >>>> 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? >> >> We need an additional boolean property for TX enable. > > Why? Becuase you might not have something wired up for Tx? Yes. > > What needs to be present physically for Tx support? An IR transmitter LEDs need to be physically connected. Also driver need to know about this to setup the IP to do tx. > >> >> + >> >> Two more configurable parameters for TX sub-carrier frequency and >> duty-cycle. By default driver can set the sub carrier frequency to be >> 38Khz and duty cycle as 50%. >> However I don't have strong use case to make these configurable. >> >> So, I think just one boolean property to enable tx should be Ok. >> >> >>>> +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? >> COMMS block is a collection of communication peripherals, and IRB(Infra >> red blaster) is always part of the COMMS block. >> >> May renaming the compatible to "st,comms-rc" might be more clear. > > Given the actual name of the hardware seems to include "irb", I think > "irb" makes more sense than "rc" in the compatible string. Is there no > more specific name than "Infra Red Blaster"? There is'nt, its always referred as IRB. I will rename the compatible to "st,comms-irb" does this sound Ok? > >> >>> >>> What's the full name of this unit as it appears in documentation? >> It is always refered as the Communication sub-system (COMMS) which is a >> collection of communication peripherals like UART, I2C, SPI, IRB. > > Ok, are those separate IP blocks within a container, or is anything > shared? Does the COMMS block have any registers shared between those > units, for example? No, registers are not shared across the IP blocks. > >> >>> >>>> + - 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? >> >> Its both, some boards have IR and others UHF receivers, So it becomes >> board choice here. > > When you say it's both, what do you mean? Is it possible for a unit to > be wired with both IR and UHF support simultaneously, even if the Linux > driver can't handle that? > I meant that this property is physical property of the device(board wired up to either IR or UHF receiver) and also choice on how the IP is configured. > The dt should encode information about the hardware, not the way you > intend to use the hardware at the present moment in time. Ok. > >> And also the driver has different register set for UHF receiver and IR >> receiver. This options selects which registers to use depending on mode >> of reception. > > Ok, but that's an implementation issue. If you described that IR *may* > be used, and UHF *may* be used, the driver can choose what to do based > on that. > >> >>> >>> 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". >> >> Am ok with either approaches. > > It sounds like we may need separate properties as mentioned above, or a > supported-modes list, perhaps. I think having rx-mode and tx-mode properties makes more sense rather than supported-modes. like: rx-mode = "uhf"; or rx-mode = "infrared"; Similarly tx-mode. > >> >>> >>>> + - 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@st.com> >>>> + * >>>> + * 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. >> Clock is coming for the device tree only. >> >> I can add the clock and pinctrl bindings to the documentation if it >> makes it more clear. > > If we need clocks and pinctrl, they should be described form the start. > Given we already have the infrastructure, there's no reason not to. Not > doing so will only lead to headaches later as we try to keep bindings > stable. > Ok, I will document them in bindings. -- 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
On Thu, Aug 15, 2013 at 03:06:14PM +0100, Srinivas KANDAGATLA wrote: > On 15/08/13 14:29, Mark Rutland wrote: > > On Thu, Aug 15, 2013 at 01:57:13PM +0100, Srinivas KANDAGATLA wrote: > >> Thanks Mark for your comments. > >> > >> On 15/08/13 09:49, Mark Rutland wrote: > >>> On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: > >>>> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> > >>>> > >>>> 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? > >> > >> We need an additional boolean property for TX enable. > > > > Why? Becuase you might not have something wired up for Tx? > Yes. > > > > What needs to be present physically for Tx support? > An IR transmitter LEDs need to be physically connected. Ok. > Also driver need to know about this to setup the IP to do tx. > > > >> > >> + > >> > >> Two more configurable parameters for TX sub-carrier frequency and > >> duty-cycle. By default driver can set the sub carrier frequency to be > >> 38Khz and duty cycle as 50%. > >> However I don't have strong use case to make these configurable. > >> > >> So, I think just one boolean property to enable tx should be Ok. > >> > >> > >>>> +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? > >> COMMS block is a collection of communication peripherals, and IRB(Infra > >> red blaster) is always part of the COMMS block. > >> > >> May renaming the compatible to "st,comms-rc" might be more clear. > > > > Given the actual name of the hardware seems to include "irb", I think > > "irb" makes more sense than "rc" in the compatible string. Is there no > > more specific name than "Infra Red Blaster"? > > There is'nt, its always referred as IRB. > > I will rename the compatible to "st,comms-irb" does this sound Ok? That sounds fine. > > > >> > >>> > >>> What's the full name of this unit as it appears in documentation? > >> It is always refered as the Communication sub-system (COMMS) which is a > >> collection of communication peripherals like UART, I2C, SPI, IRB. > > > > Ok, are those separate IP blocks within a container, or is anything > > shared? Does the COMMS block have any registers shared between those > > units, for example? > No, registers are not shared across the IP blocks. Good to know. > > > >> > >>> > >>>> + - 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? > >> > >> Its both, some boards have IR and others UHF receivers, So it becomes > >> board choice here. Ok, so we can never have both wired up simultaneously? Any board will have only one of the two wired up (and the other will be unusable becasue it's not wired up). > > > > When you say it's both, what do you mean? Is it possible for a unit to > > be wired with both IR and UHF support simultaneously, even if the Linux > > driver can't handle that? > > > I meant that this property is physical property of the device(board > wired up to either IR or UHF receiver) and also choice on how the IP is > configured. > > > The dt should encode information about the hardware, not the way you > > intend to use the hardware at the present moment in time. > Ok. > > > >> And also the driver has different register set for UHF receiver and IR > >> receiver. This options selects which registers to use depending on mode > >> of reception. > > > > Ok, but that's an implementation issue. If you described that IR *may* > > be used, and UHF *may* be used, the driver can choose what to do based > > on that. > > > >> > >>> > >>> 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". > >> > >> Am ok with either approaches. > > > > It sounds like we may need separate properties as mentioned above, or a > > supported-modes list, perhaps. > > I think having rx-mode and tx-mode properties makes more sense rather > than supported-modes. > > like: > rx-mode = "uhf"; > > or > > rx-mode = "infrared"; > > Similarly tx-mode. That makes sense to me. > > > > >> > >>> > >>>> + - 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@st.com> > >>>> + * > >>>> + * 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. > >> Clock is coming for the device tree only. > >> > >> I can add the clock and pinctrl bindings to the documentation if it > >> makes it more clear. > > > > If we need clocks and pinctrl, they should be described form the start. > > Given we already have the infrastructure, there's no reason not to. Not > > doing so will only lead to headaches later as we try to keep bindings > > stable. > > > Ok, I will document them in bindings. > Cheers. Mark. -- 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
On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@st.com> > > 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. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com> > --- > 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". > + - st,uhfmode: boolean property to indicate if reception is in UHF. > + - 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>; > + }; > + > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig > index 5a79c33..548a705 100644 > --- a/drivers/media/rc/Kconfig > +++ b/drivers/media/rc/Kconfig > @@ -321,4 +321,14 @@ config IR_GPIO_CIR > To compile this driver as a module, choose M here: the module will > be called gpio-ir-recv. > > +config RC_ST > + tristate "ST remote control receiver" > + depends on ARCH_STI && LIRC > + help > + Say Y here if you want support for ST remote control driver > + which allows both IR and UHF RX. IR RX receiver is the default mode. > + The driver passes raw pluse and space information to the LIRC decoder. > + > + If you're not sure, select N here. > + > endif #RC_DEVICES > diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile > index 56bacf0..f4eb32c 100644 > --- a/drivers/media/rc/Makefile > +++ b/drivers/media/rc/Makefile > @@ -30,3 +30,4 @@ obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o > obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o > obj-$(CONFIG_IR_IGUANA) += iguanair.o > obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o > +obj-$(CONFIG_RC_ST) += st_rc.o > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c > new file mode 100644 > index 0000000..712a2fb > --- /dev/null > +++ b/drivers/media/rc/st_rc.c > @@ -0,0 +1,371 @@ > +/* > + * Copyright (C) 2013 STMicroelectronics Limited > + * Author: Srinivas Kandagatla <srinivas.kandagatla@st.com> > + * > + * 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; > +}; > + > +/* Registers */ > +#define IRB_SAMPLE_RATE_COMM 0x64 /* sample freq divisor*/ > +#define IRB_CLOCK_SEL 0x70 /* clock select */ > +#define IRB_CLOCK_SEL_STATUS 0x74 /* clock status */ > +/* IRB IR/UHF receiver registers */ > +#define IRB_RX_ON 0x40 /* pulse time capture */ > +#define IRB_RX_SYS 0X44 /* sym period capture */ > +#define IRB_RX_INT_EN 0x48 /* IRQ enable (R/W) */ > +#define IRB_RX_INT_STATUS 0x4C /* IRQ status (R/W) */ > +#define IRB_RX_EN 0x50 /* Receive enablei */ > +#define IRB_MAX_SYM_PERIOD 0x54 /* max sym value */ > +#define IRB_RX_INT_CLEAR 0x58 /* overrun status */ > +#define IRB_RX_STATUS 0x6C /* receive status */ > +#define IRB_RX_NOISE_SUPPR 0x5C /* noise suppression */ > +#define IRB_RX_POLARITY_INV 0x68 /* polarity inverter */ > + > +/* IRQ set: Enable full FIFO 1 -> bit 3; > + * Enable overrun IRQ 1 -> bit 2; > + * Enable last symbol IRQ 1 -> bit 1: > + * Enable RX interrupt 1 -> bit 0; > + */ > +#define IRB_RX_INTS 0x0f > +#define IRB_RX_OVERRUN_INT 0x04 > + /* maximum symbol period (microsecs),timeout to detect end of symbol train */ > +#define MAX_SYMB_TIME 0x5000 > +#define IRB_SAMPLE_FREQ 10000000 > +#define IRB_FIFO_NOT_EMPTY 0xff00 > +#define IRB_OVERFLOW 0x4 > +#define IRB_TIMEOUT 0xffff > +#define IR_ST_NAME "st-rc" > + > +static void st_rc_send_lirc_timeout(struct rc_dev *rdev) > +{ > + DEFINE_IR_RAW_EVENT(ev); > + ev.timeout = true; > + ev.pulse = false; > + ir_raw_event_store(rdev, &ev); > +} > + > +/** > + * RX graphical example to better understand the difference between ST IR block > + * output and standard definition used by LIRC (and most of the world!) > + * > + * mark mark > + * |-IRB_RX_ON-| |-IRB_RX_ON-| > + * ___ ___ ___ ___ ___ ___ _ > + * | | | | | | | | | | | | | > + * | | | | | | space 0 | | | | | | space 1 | > + * _____| |__| |__| |____________________________| |__| |__| |_____________| > + * > + * |--------------- IRB_RX_SYS -------------|------ IRB_RX_SYS -------| > + * > + * |------------- encoding bit 0 -----------|---- encoding bit 1 -----| > + * > + * ST hardware returns mark (IRB_RX_ON) and total symbol time (IRB_RX_SYS), so > + * convert to standard mark/space we have to calculate space=(IRB_RX_SYS-mark) > + * The mark time represents the amount of time the carrier (usually 36-40kHz) > + * is detected.The above examples shows Pulse Width Modulation encoding where > + * bit 0 is represented by space>mark. > + */ > + > +static irqreturn_t st_rc_rx_interrupt(int irq, void *data) > +{ > + unsigned int symbol, mark = 0; > + struct st_rc_device *dev = data; > + int last_symbol = 0; > + u32 status; > + DEFINE_IR_RAW_EVENT(ev); > + > + if (dev->irq_wake) > + pm_wakeup_event(dev->dev, 0); > + > + status = readl(dev->rx_base + IRB_RX_STATUS); > + > + while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) { > + u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); > + if (unlikely(int_status & IRB_RX_OVERRUN_INT)) { You should call ir_raw_event_reset() so that the in-kernel decoders realise that the IR stream is not contiguous. > + /* discard the entire collection in case of errors! */ > + dev_info(dev->dev, "IR RX overrun\n"); > + writel(IRB_RX_OVERRUN_INT, > + dev->rx_base + IRB_RX_INT_CLEAR); > + continue; > + } > + > + symbol = readl(dev->rx_base + IRB_RX_SYS); > + mark = readl(dev->rx_base + IRB_RX_ON); > + > + if (symbol == IRB_TIMEOUT) > + last_symbol = 1; > + > + /* Ignore any noise */ > + if ((mark > 2) && (symbol > 1)) { > + symbol -= mark; > + if (dev->overclocking) { /* adjustments to timings */ > + symbol *= dev->sample_mult; > + symbol /= dev->sample_div; > + mark *= dev->sample_mult; > + mark /= dev->sample_div; > + } > + > + ev.duration = US_TO_NS(mark); > + ev.pulse = true; > + ir_raw_event_store(dev->rdev, &ev); > + > + if (!last_symbol) { > + ev.duration = US_TO_NS(symbol); > + ev.pulse = false; > + ir_raw_event_store(dev->rdev, &ev); Make sure you call ir_raw_event_handle() once a while (maybe every time the interrupt handler is called?) to prevent the ir kfifo from overflowing in case of very long IR. ir_raw_event_store() just adds new edges to the kfifo() but does not flush them to the decoders or lirc. > + } else { > + st_rc_send_lirc_timeout(dev->rdev); > + ir_raw_event_handle(dev->rdev); > + } > + } > + last_symbol = 0; > + status = readl(dev->rx_base + IRB_RX_STATUS); > + } > + > + writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR); > + > + return IRQ_HANDLED; > +} > + > +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); > + 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); > + return 0; > +} > + > +static int st_rc_open(struct rc_dev *rdev) > +{ > + struct st_rc_device *dev = rdev->priv; > + unsigned long flags; > + local_irq_save(flags); > + /* enable interrupts and receiver */ > + writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_EN); > + writel(0x01, dev->rx_base + IRB_RX_EN); > + local_irq_restore(flags); > + > + return 0; > +} > + > +static void st_rc_close(struct rc_dev *rdev) > +{ > + struct st_rc_device *dev = rdev->priv; > + /* disable interrupts and receiver */ > + writel(0x00, dev->rx_base + IRB_RX_EN); > + writel(0x00, dev->rx_base + IRB_RX_INT_EN); > +} > + > +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"); > + > + 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; allowed_protos is a bit field, so you need one of the RC_BIT_ types; you should use RC_BIT_ALL so that in-kernel IR decoders can be used for decoding if desired. > + 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"; Please also set the timeout and rx_resolution according to the hardware. > + > + /* 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. */ > + 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; > +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; > +} > + > +#ifdef CONFIG_PM > +static int st_rc_suspend(struct device *dev) > +{ > + struct st_rc_device *rc_dev = dev_get_drvdata(dev); > + > + if (device_may_wakeup(dev)) { > + if (!enable_irq_wake(rc_dev->irq)) > + rc_dev->irq_wake = 1; > + else > + return -EINVAL; > + } else { > + pinctrl_pm_select_sleep_state(dev); > + writel(0x00, rc_dev->rx_base + IRB_RX_EN); > + writel(0x00, rc_dev->rx_base + IRB_RX_INT_EN); > + clk_disable_unprepare(rc_dev->sys_clock); > + } > + > + return 0; > +} > + > +static int st_rc_resume(struct device *dev) > +{ > + struct st_rc_device *rc_dev = dev_get_drvdata(dev); > + struct rc_dev *rdev = rc_dev->rdev; > + > + if (rc_dev->irq_wake) { > + disable_irq_wake(rc_dev->irq); > + rc_dev->irq_wake = 0; > + } else { > + pinctrl_pm_select_default_state(dev); > + st_rc_hardware_init(rc_dev); > + if (rdev->users) { > + writel(IRB_RX_INTS, rc_dev->rx_base + IRB_RX_INT_EN); > + writel(0x01, rc_dev->rx_base + IRB_RX_EN); > + } > + } > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(st_rc_pm_ops, st_rc_suspend, st_rc_resume); > +#endif > + > +#ifdef CONFIG_OF > +static struct of_device_id st_rc_match[] = { > + { .compatible = "st,rc", }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, st_rc_match); > +#endif > + > +static struct platform_driver st_rc_driver = { > + .driver = { > + .name = IR_ST_NAME, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(st_rc_match), > +#ifdef CONFIG_PM > + .pm = &st_rc_pm_ops, > +#endif > + }, > + .probe = st_rc_probe, > + .remove = st_rc_remove, > +}; > + > +module_platform_driver(st_rc_driver); > + > +MODULE_DESCRIPTION("RC Transceiver driver for STMicroelectronics platforms"); > +MODULE_AUTHOR("STMicroelectronics (R&D) Ltd"); > +MODULE_LICENSE("GPL"); > -- > 1.7.6.5 > > -- > 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 -- 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
Thanks Sean for the comments. On 16/08/13 09:38, Sean Young wrote: > On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: [...] >> >> 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 >> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c [...] >> +static irqreturn_t st_rc_rx_interrupt(int irq, void *data) >> +{ >> + unsigned int symbol, mark = 0; >> + struct st_rc_device *dev = data; >> + int last_symbol = 0; >> + u32 status; >> + DEFINE_IR_RAW_EVENT(ev); >> + >> + if (dev->irq_wake) >> + pm_wakeup_event(dev->dev, 0); >> + >> + status = readl(dev->rx_base + IRB_RX_STATUS); >> + >> + while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) { >> + u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); >> + if (unlikely(int_status & IRB_RX_OVERRUN_INT)) { > > You should call ir_raw_event_reset() so that the in-kernel decoders > realise that the IR stream is not contiguous. Yep... It makes sense.. I will add this. > >> + /* discard the entire collection in case of errors! */ >> + dev_info(dev->dev, "IR RX overrun\n"); >> + writel(IRB_RX_OVERRUN_INT, >> + dev->rx_base + IRB_RX_INT_CLEAR); >> + continue; >> + } >> + >> + symbol = readl(dev->rx_base + IRB_RX_SYS); >> + mark = readl(dev->rx_base + IRB_RX_ON); >> + >> + if (symbol == IRB_TIMEOUT) >> + last_symbol = 1; >> + >> + /* Ignore any noise */ >> + if ((mark > 2) && (symbol > 1)) { >> + symbol -= mark; >> + if (dev->overclocking) { /* adjustments to timings */ >> + symbol *= dev->sample_mult; >> + symbol /= dev->sample_div; >> + mark *= dev->sample_mult; >> + mark /= dev->sample_div; >> + } >> + >> + ev.duration = US_TO_NS(mark); >> + ev.pulse = true; >> + ir_raw_event_store(dev->rdev, &ev); >> + >> + if (!last_symbol) { >> + ev.duration = US_TO_NS(symbol); >> + ev.pulse = false; >> + ir_raw_event_store(dev->rdev, &ev); > > Make sure you call ir_raw_event_handle() once a while (maybe every time > the interrupt handler is called?) to prevent the ir kfifo from > overflowing in case of very long IR. ir_raw_event_store() just adds > new edges to the kfifo() but does not flush them to the decoders or > lirc. I agree, but Am not sure it will really help in this case because, we are going to stay in this interrupt handler till we get a last_symbol(full key press/release event).. So calling ir_raw_event_store mulitple times might not help because the ir_raw_event kthread(which is clearing kfifo) which is only scheduled after returning from this interrupt. Correct me if Am wrong. > >> + } else { >> + st_rc_send_lirc_timeout(dev->rdev); >> + ir_raw_event_handle(dev->rdev); >> + } >> + } >> + last_symbol = 0; >> + status = readl(dev->rx_base + IRB_RX_STATUS); >> + } >> + >> + writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR); >> + >> + return IRQ_HANDLED; >> +} >> + [...] >> +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"); >> + >> + 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; > > allowed_protos is a bit field, so you need one of the RC_BIT_ types; > you should use RC_BIT_ALL so that in-kernel IR decoders can be used > for decoding if desired. Ok, I miss interpreted the TYPE .. I will fix this in v2. > >> + 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"; > > Please also set the timeout and rx_resolution according to the hardware. Ok, I will add this info as well. Thanks, srini > -- 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
On Fri, Aug 16, 2013 at 11:53:48AM +0100, Srinivas KANDAGATLA wrote: > Thanks Sean for the comments. > On 16/08/13 09:38, Sean Young wrote: > > On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: > [...] > >> + /* discard the entire collection in case of errors! */ > >> + dev_info(dev->dev, "IR RX overrun\n"); > >> + writel(IRB_RX_OVERRUN_INT, > >> + dev->rx_base + IRB_RX_INT_CLEAR); > >> + continue; > >> + } > >> + > >> + symbol = readl(dev->rx_base + IRB_RX_SYS); > >> + mark = readl(dev->rx_base + IRB_RX_ON); > >> + > >> + if (symbol == IRB_TIMEOUT) > >> + last_symbol = 1; > >> + > >> + /* Ignore any noise */ > >> + if ((mark > 2) && (symbol > 1)) { > >> + symbol -= mark; > >> + if (dev->overclocking) { /* adjustments to timings */ > >> + symbol *= dev->sample_mult; > >> + symbol /= dev->sample_div; > >> + mark *= dev->sample_mult; > >> + mark /= dev->sample_div; > >> + } > >> + > >> + ev.duration = US_TO_NS(mark); > >> + ev.pulse = true; > >> + ir_raw_event_store(dev->rdev, &ev); > >> + > >> + if (!last_symbol) { > >> + ev.duration = US_TO_NS(symbol); > >> + ev.pulse = false; > >> + ir_raw_event_store(dev->rdev, &ev); > > > > Make sure you call ir_raw_event_handle() once a while (maybe every time > > the interrupt handler is called?) to prevent the ir kfifo from > > overflowing in case of very long IR. ir_raw_event_store() just adds > > new edges to the kfifo() but does not flush them to the decoders or > > lirc. > I agree, but Am not sure it will really help in this case because, we > are going to stay in this interrupt handler till we get a > last_symbol(full key press/release event).. So calling > ir_raw_event_store mulitple times might not help because the > ir_raw_event kthread(which is clearing kfifo) which is only scheduled > after returning from this interrupt. If I read it correctly, then this is only true if the fifo contains an IRB_TIMEOUT symbol. If not yet, then the interrupt handlers is not waiting around for those symbols to arrive. Thanks Sean -- 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
On 16/08/13 12:40, Sean Young wrote: > On Fri, Aug 16, 2013 at 11:53:48AM +0100, Srinivas KANDAGATLA wrote: >> Thanks Sean for the comments. >> On 16/08/13 09:38, Sean Young wrote: >>> On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote: >> [...] >>>> + /* discard the entire collection in case of errors! */ >>>> + dev_info(dev->dev, "IR RX overrun\n"); >>>> + writel(IRB_RX_OVERRUN_INT, >>>> + dev->rx_base + IRB_RX_INT_CLEAR); >>>> + continue; >>>> + } >>>> + >>>> + symbol = readl(dev->rx_base + IRB_RX_SYS); >>>> + mark = readl(dev->rx_base + IRB_RX_ON); >>>> + >>>> + if (symbol == IRB_TIMEOUT) >>>> + last_symbol = 1; >>>> + >>>> + /* Ignore any noise */ >>>> + if ((mark > 2) && (symbol > 1)) { >>>> + symbol -= mark; >>>> + if (dev->overclocking) { /* adjustments to timings */ >>>> + symbol *= dev->sample_mult; >>>> + symbol /= dev->sample_div; >>>> + mark *= dev->sample_mult; >>>> + mark /= dev->sample_div; >>>> + } >>>> + >>>> + ev.duration = US_TO_NS(mark); >>>> + ev.pulse = true; >>>> + ir_raw_event_store(dev->rdev, &ev); >>>> + >>>> + if (!last_symbol) { >>>> + ev.duration = US_TO_NS(symbol); >>>> + ev.pulse = false; >>>> + ir_raw_event_store(dev->rdev, &ev); >>> >>> Make sure you call ir_raw_event_handle() once a while (maybe every time >>> the interrupt handler is called?) to prevent the ir kfifo from >>> overflowing in case of very long IR. ir_raw_event_store() just adds >>> new edges to the kfifo() but does not flush them to the decoders or >>> lirc. >> I agree, but Am not sure it will really help in this case because, we >> are going to stay in this interrupt handler till we get a >> last_symbol(full key press/release event).. So calling >> ir_raw_event_store mulitple times might not help because the >> ir_raw_event kthread(which is clearing kfifo) which is only scheduled >> after returning from this interrupt. > > If I read it correctly, then this is only true if the fifo contains an > IRB_TIMEOUT symbol. If not yet, then the interrupt handlers is not > waiting around for those symbols to arrive. Yes, as we are clearing the fifo and fifo size is 64 words its always highly possible that It will contain IRB_TIMEOUT for that interrupt. Thanks, srini > > Thanks > Sean > -- 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/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". + - st,uhfmode: boolean property to indicate if reception is in UHF. + - 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>; + }; + diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig index 5a79c33..548a705 100644 --- a/drivers/media/rc/Kconfig +++ b/drivers/media/rc/Kconfig @@ -321,4 +321,14 @@ config IR_GPIO_CIR To compile this driver as a module, choose M here: the module will be called gpio-ir-recv. +config RC_ST + tristate "ST remote control receiver" + depends on ARCH_STI && LIRC + help + Say Y here if you want support for ST remote control driver + which allows both IR and UHF RX. IR RX receiver is the default mode. + The driver passes raw pluse and space information to the LIRC decoder. + + If you're not sure, select N here. + endif #RC_DEVICES diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile index 56bacf0..f4eb32c 100644 --- a/drivers/media/rc/Makefile +++ b/drivers/media/rc/Makefile @@ -30,3 +30,4 @@ obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o obj-$(CONFIG_IR_IGUANA) += iguanair.o obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o +obj-$(CONFIG_RC_ST) += st_rc.o diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c new file mode 100644 index 0000000..712a2fb --- /dev/null +++ b/drivers/media/rc/st_rc.c @@ -0,0 +1,371 @@ +/* + * Copyright (C) 2013 STMicroelectronics Limited + * Author: Srinivas Kandagatla <srinivas.kandagatla@st.com> + * + * 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; +}; + +/* Registers */ +#define IRB_SAMPLE_RATE_COMM 0x64 /* sample freq divisor*/ +#define IRB_CLOCK_SEL 0x70 /* clock select */ +#define IRB_CLOCK_SEL_STATUS 0x74 /* clock status */ +/* IRB IR/UHF receiver registers */ +#define IRB_RX_ON 0x40 /* pulse time capture */ +#define IRB_RX_SYS 0X44 /* sym period capture */ +#define IRB_RX_INT_EN 0x48 /* IRQ enable (R/W) */ +#define IRB_RX_INT_STATUS 0x4C /* IRQ status (R/W) */ +#define IRB_RX_EN 0x50 /* Receive enablei */ +#define IRB_MAX_SYM_PERIOD 0x54 /* max sym value */ +#define IRB_RX_INT_CLEAR 0x58 /* overrun status */ +#define IRB_RX_STATUS 0x6C /* receive status */ +#define IRB_RX_NOISE_SUPPR 0x5C /* noise suppression */ +#define IRB_RX_POLARITY_INV 0x68 /* polarity inverter */ + +/* IRQ set: Enable full FIFO 1 -> bit 3; + * Enable overrun IRQ 1 -> bit 2; + * Enable last symbol IRQ 1 -> bit 1: + * Enable RX interrupt 1 -> bit 0; + */ +#define IRB_RX_INTS 0x0f +#define IRB_RX_OVERRUN_INT 0x04 + /* maximum symbol period (microsecs),timeout to detect end of symbol train */ +#define MAX_SYMB_TIME 0x5000 +#define IRB_SAMPLE_FREQ 10000000 +#define IRB_FIFO_NOT_EMPTY 0xff00 +#define IRB_OVERFLOW 0x4 +#define IRB_TIMEOUT 0xffff +#define IR_ST_NAME "st-rc" + +static void st_rc_send_lirc_timeout(struct rc_dev *rdev) +{ + DEFINE_IR_RAW_EVENT(ev); + ev.timeout = true; + ev.pulse = false; + ir_raw_event_store(rdev, &ev); +} + +/** + * RX graphical example to better understand the difference between ST IR block + * output and standard definition used by LIRC (and most of the world!) + * + * mark mark + * |-IRB_RX_ON-| |-IRB_RX_ON-| + * ___ ___ ___ ___ ___ ___ _ + * | | | | | | | | | | | | | + * | | | | | | space 0 | | | | | | space 1 | + * _____| |__| |__| |____________________________| |__| |__| |_____________| + * + * |--------------- IRB_RX_SYS -------------|------ IRB_RX_SYS -------| + * + * |------------- encoding bit 0 -----------|---- encoding bit 1 -----| + * + * ST hardware returns mark (IRB_RX_ON) and total symbol time (IRB_RX_SYS), so + * convert to standard mark/space we have to calculate space=(IRB_RX_SYS-mark) + * The mark time represents the amount of time the carrier (usually 36-40kHz) + * is detected.The above examples shows Pulse Width Modulation encoding where + * bit 0 is represented by space>mark. + */ + +static irqreturn_t st_rc_rx_interrupt(int irq, void *data) +{ + unsigned int symbol, mark = 0; + struct st_rc_device *dev = data; + int last_symbol = 0; + u32 status; + DEFINE_IR_RAW_EVENT(ev); + + if (dev->irq_wake) + pm_wakeup_event(dev->dev, 0); + + status = readl(dev->rx_base + IRB_RX_STATUS); + + while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) { + u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); + if (unlikely(int_status & IRB_RX_OVERRUN_INT)) { + /* discard the entire collection in case of errors! */ + dev_info(dev->dev, "IR RX overrun\n"); + writel(IRB_RX_OVERRUN_INT, + dev->rx_base + IRB_RX_INT_CLEAR); + continue; + } + + symbol = readl(dev->rx_base + IRB_RX_SYS); + mark = readl(dev->rx_base + IRB_RX_ON); + + if (symbol == IRB_TIMEOUT) + last_symbol = 1; + + /* Ignore any noise */ + if ((mark > 2) && (symbol > 1)) { + symbol -= mark; + if (dev->overclocking) { /* adjustments to timings */ + symbol *= dev->sample_mult; + symbol /= dev->sample_div; + mark *= dev->sample_mult; + mark /= dev->sample_div; + } + + ev.duration = US_TO_NS(mark); + ev.pulse = true; + ir_raw_event_store(dev->rdev, &ev); + + if (!last_symbol) { + ev.duration = US_TO_NS(symbol); + ev.pulse = false; + ir_raw_event_store(dev->rdev, &ev); + } else { + st_rc_send_lirc_timeout(dev->rdev); + ir_raw_event_handle(dev->rdev); + } + } + last_symbol = 0; + status = readl(dev->rx_base + IRB_RX_STATUS); + } + + writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR); + + return IRQ_HANDLED; +} + +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); + 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); + return 0; +} + +static int st_rc_open(struct rc_dev *rdev) +{ + struct st_rc_device *dev = rdev->priv; + unsigned long flags; + local_irq_save(flags); + /* enable interrupts and receiver */ + writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_EN); + writel(0x01, dev->rx_base + IRB_RX_EN); + local_irq_restore(flags); + + return 0; +} + +static void st_rc_close(struct rc_dev *rdev) +{ + struct st_rc_device *dev = rdev->priv; + /* disable interrupts and receiver */ + writel(0x00, dev->rx_base + IRB_RX_EN); + writel(0x00, dev->rx_base + IRB_RX_INT_EN); +} + +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"); + + 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. */ + 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; +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; +} + +#ifdef CONFIG_PM +static int st_rc_suspend(struct device *dev) +{ + struct st_rc_device *rc_dev = dev_get_drvdata(dev); + + if (device_may_wakeup(dev)) { + if (!enable_irq_wake(rc_dev->irq)) + rc_dev->irq_wake = 1; + else + return -EINVAL; + } else { + pinctrl_pm_select_sleep_state(dev); + writel(0x00, rc_dev->rx_base + IRB_RX_EN); + writel(0x00, rc_dev->rx_base + IRB_RX_INT_EN); + clk_disable_unprepare(rc_dev->sys_clock); + } + + return 0; +} + +static int st_rc_resume(struct device *dev) +{ + struct st_rc_device *rc_dev = dev_get_drvdata(dev); + struct rc_dev *rdev = rc_dev->rdev; + + if (rc_dev->irq_wake) { + disable_irq_wake(rc_dev->irq); + rc_dev->irq_wake = 0; + } else { + pinctrl_pm_select_default_state(dev); + st_rc_hardware_init(rc_dev); + if (rdev->users) { + writel(IRB_RX_INTS, rc_dev->rx_base + IRB_RX_INT_EN); + writel(0x01, rc_dev->rx_base + IRB_RX_EN); + } + } + + return 0; +} + +static SIMPLE_DEV_PM_OPS(st_rc_pm_ops, st_rc_suspend, st_rc_resume); +#endif + +#ifdef CONFIG_OF +static struct of_device_id st_rc_match[] = { + { .compatible = "st,rc", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, st_rc_match); +#endif + +static struct platform_driver st_rc_driver = { + .driver = { + .name = IR_ST_NAME, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(st_rc_match), +#ifdef CONFIG_PM + .pm = &st_rc_pm_ops, +#endif + }, + .probe = st_rc_probe, + .remove = st_rc_remove, +}; + +module_platform_driver(st_rc_driver); + +MODULE_DESCRIPTION("RC Transceiver driver for STMicroelectronics platforms"); +MODULE_AUTHOR("STMicroelectronics (R&D) Ltd"); +MODULE_LICENSE("GPL");