[PATCH v1] media: rc: Add driver for tango IR decoder

[PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

The tango IR decoder supports NEC, RC-5, RC-6 protocols.
NB: Only the NEC decoder was tested.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
  drivers/media/rc/Kconfig    |   5 +
  drivers/media/rc/Makefile   |   1 +
  drivers/media/rc/tango-ir.c | 263 ++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 269 insertions(+)
  create mode 100644 drivers/media/rc/tango-ir.c

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index d9ce8ff55d0c..f84923289964 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -469,6 +469,11 @@ config IR_SIR
  	   To compile this driver as a module, choose M here: the module will
  	   be called sir-ir.
  
+config IR_TANGO
+	tristate "Sigma Designs SMP86xx IR decoder"
+	depends on RC_CORE
+	depends on ARCH_TANGO || COMPILE_TEST
+
  config IR_ZX
  	tristate "ZTE ZX IR remote control"
  	depends on RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 9bc6a3980ed0..643797dc971b 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_IR_SERIAL) += serial_ir.o
  obj-$(CONFIG_IR_SIR) += sir_ir.o
  obj-$(CONFIG_IR_MTK) += mtk-cir.o
  obj-$(CONFIG_IR_ZX) += zx-irdec.o
+obj-$(CONFIG_IR_TANGO) += tango-ir.o
diff --git a/drivers/media/rc/tango-ir.c b/drivers/media/rc/tango-ir.c
new file mode 100644
index 000000000000..fedb8d35ac10
--- /dev/null
+++ b/drivers/media/rc/tango-ir.c
@@ -0,0 +1,263 @@
+/*
+ * Copyright (C) 2015 Mans Rullgard <mans@mansr.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/input.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <media/rc-core.h>
+
+#define IR_NEC_CTRL	0x00
+#define IR_NEC_DATA	0x04
+#define IR_CTRL		0x08
+#define IR_RC5_CLK_DIV	0x0c
+#define IR_RC5_DATA	0x10
+#define IR_INT		0x14
+
+#define NEC_TIME_BASE	560
+#define RC5_TIME_BASE	1778
+
+#define RC6_CTRL	0x00
+#define RC6_CLKDIV	0x04
+#define RC6_DATA0	0x08
+#define RC6_DATA1	0x0c
+#define RC6_DATA2	0x10
+#define RC6_DATA3	0x14
+#define RC6_DATA4	0x18
+
+#define RC6_CARRIER	36000
+#define RC6_TIME_BASE	16
+
+struct tango_ir {
+	void __iomem *rc5_base;
+	void __iomem *rc6_base;
+	struct rc_dev *rc;
+	struct clk *clk;
+};
+
+static void tango_ir_handle_nec(struct tango_ir *ir)
+{
+	u32 v, code;
+	enum rc_proto proto;
+
+	v = readl_relaxed(ir->rc5_base + IR_NEC_DATA);
+	if (!v) {
+		rc_repeat(ir->rc);
+		return;
+	}
+
+	code = ir_nec_bytes_to_scancode(v, v >> 8, v >> 16, v >> 24, &proto);
+	rc_keydown(ir->rc, proto, code, 0);
+}
+
+static void tango_ir_handle_rc5(struct tango_ir *ir)
+{
+	u32 data, field, toggle, addr, cmd, code;
+
+	data = readl_relaxed(ir->rc5_base + IR_RC5_DATA);
+	if (data & BIT(31))
+		return;
+
+	field = data >> 12 & 1;
+	toggle = data >> 11 & 1;
+	addr = data >> 6 & 0x1f;
+	cmd = (data & 0x3f) | (field ^ 1) << 6;
+
+	code = RC_SCANCODE_RC5(addr, cmd);
+	rc_keydown(ir->rc, RC_PROTO_RC5, code, toggle);
+}
+
+static void tango_ir_handle_rc6(struct tango_ir *ir)
+{
+	u32 data0, data1, toggle, mode, addr, cmd, code;
+
+	data0 = readl_relaxed(ir->rc6_base + RC6_DATA0);
+	data1 = readl_relaxed(ir->rc6_base + RC6_DATA1);
+
+	mode = data0 >> 1 & 7;
+	if (mode != 0)
+		return;
+
+	toggle = data0 & 1;
+	addr = data0 >> 16;
+	cmd = data1;
+
+	code = RC_SCANCODE_RC6_0(addr, cmd);
+	rc_keydown(ir->rc, RC_PROTO_RC6_0, code, toggle);
+}
+
+static irqreturn_t tango_ir_irq(int irq, void *dev_id)
+{
+	struct tango_ir *ir = dev_id;
+	unsigned int rc5_stat;
+	unsigned int rc6_stat;
+
+	rc5_stat = readl_relaxed(ir->rc5_base + IR_INT);
+	writel_relaxed(rc5_stat, ir->rc5_base + IR_INT);
+
+	rc6_stat = readl_relaxed(ir->rc6_base + RC6_CTRL);
+	writel_relaxed(rc6_stat, ir->rc6_base + RC6_CTRL);
+
+	if (!(rc5_stat & 3) && !(rc6_stat & BIT(31)))
+		return IRQ_NONE;
+
+	if (rc5_stat & BIT(0))
+		tango_ir_handle_rc5(ir);
+
+	if (rc5_stat & BIT(1))
+		tango_ir_handle_nec(ir);
+
+	if (rc6_stat & BIT(31))
+		tango_ir_handle_rc6(ir);
+
+	return IRQ_HANDLED;
+}
+
+#define DISABLE_NEC	(BIT(4) | BIT(8))
+#define ENABLE_RC5	(BIT(0) | BIT(9))
+#define ENABLE_RC6	(BIT(0) | BIT(7))
+
+static int tango_change_protocol(struct rc_dev *dev, u64 *rc_type)
+{
+	struct tango_ir *ir = dev->priv;
+	u32 rc5_ctrl = DISABLE_NEC;
+	u32 rc6_ctrl = 0;
+
+	if (*rc_type & RC_PROTO_BIT_NEC)
+		rc5_ctrl = 0;
+
+	if (*rc_type & RC_PROTO_BIT_RC5)
+		rc5_ctrl |= ENABLE_RC5;
+
+	if (*rc_type & RC_PROTO_BIT_RC6_0)
+		rc6_ctrl = ENABLE_RC6;
+
+	writel_relaxed_relaxed(rc5_ctrl, ir->rc5_base + IR_CTRL);
+	writel_relaxed_relaxed(rc6_ctrl, ir->rc6_base + RC6_CTRL);
+
+	return 0;
+}
+
+static int tango_ir_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rc_dev *rc;
+	struct tango_ir *ir;
+	struct resource *rc5_res;
+	struct resource *rc6_res;
+	u64 clkrate, clkdiv;
+	int irq, err;
+
+	rc5_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!rc5_res)
+		return -EINVAL;
+
+	rc6_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!rc6_res)
+		return -EINVAL;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return -EINVAL;
+
+	ir = devm_kzalloc(dev, sizeof(*ir), GFP_KERNEL);
+	if (!ir)
+		return -ENOMEM;
+
+	ir->rc5_base = devm_ioremap_resource(dev, rc5_res);
+	if (IS_ERR(ir->rc5_base))
+		return PTR_ERR(ir->rc5_base);
+
+	ir->rc6_base = devm_ioremap_resource(dev, rc6_res);
+	if (IS_ERR(ir->rc6_base))
+		return PTR_ERR(ir->rc6_base);
+
+	ir->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ir->clk))
+		return PTR_ERR(ir->clk);
+
+	rc = devm_rc_allocate_device(dev, RC_DRIVER_SCANCODE);
+	if (!rc)
+		return -ENOMEM;
+
+	rc->driver_name = rc->device_name = "tango-ir";
+	rc->input_phys = "tango-ir/input0";
+	rc->map_name = RC_MAP_EMPTY;
+	rc->allowed_protocols = RC_PROTO_BIT_RC5 | RC_PROTO_BIT_RC6_0 |
+		RC_PROTO_BIT_NEC | RC_PROTO_BIT_NECX | RC_PROTO_BIT_NEC32;
+	rc->change_protocol = tango_change_protocol;
+	rc->priv = ir;
+	ir->rc = rc;
+
+	err = devm_rc_register_device(dev, rc);
+	if (err)
+		return err;
+
+	err = devm_request_irq(dev, irq, tango_ir_irq, IRQF_SHARED, dev_name(dev), ir);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(ir->clk);
+	if (err)
+		return err;
+
+	clkrate = clk_get_rate(ir->clk);
+
+	clkdiv = clkrate * NEC_TIME_BASE;
+	do_div(clkdiv, 1000000);
+
+	writel_relaxed(31 << 24 | 12 << 16 | clkdiv, ir->rc5_base + IR_NEC_CTRL);
+
+	clkdiv = clkrate * RC5_TIME_BASE;
+	do_div(clkdiv, 1000000);
+
+	writel_relaxed(0x110, ir->rc5_base + IR_CTRL);
+	writel_relaxed(clkdiv, ir->rc5_base + IR_RC5_CLK_DIV);
+	writel_relaxed(0x3, ir->rc5_base + IR_INT);
+
+	clkdiv = clkrate * RC6_TIME_BASE;
+	do_div(clkdiv, RC6_CARRIER);
+
+	writel_relaxed(0xc0000000, ir->rc6_base + RC6_CTRL);
+	writel_relaxed((clkdiv >> 2) << 18 | clkdiv, ir->rc6_base + RC6_CLKDIV);
+
+	platform_set_drvdata(pdev, ir);
+
+	return 0;
+}
+
+static int tango_ir_remove(struct platform_device *pdev)
+{
+	struct tango_ir *ir = platform_get_drvdata(pdev);
+	clk_disable_unprepare(ir->clk);
+	return 0;
+}
+
+static const struct of_device_id tango_ir_dt_ids[] = {
+	{ .compatible = "sigma,smp8642-ir" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tango_ir_dt_ids);
+
+static struct platform_driver tango_ir_driver = {
+	.probe	= tango_ir_probe,
+	.remove	= tango_ir_remove,
+	.driver	= {
+		.name		= "tango-ir",
+		.of_match_table	= tango_ir_dt_ids,
+	},
+};
+module_platform_driver(tango_ir_driver);
+
+MODULE_DESCRIPTION("SMP86xx IR decoder driver");
+MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> The tango IR decoder supports NEC, RC-5, RC-6 protocols.
> NB: Only the NEC decoder was tested.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/media/rc/Kconfig    |   5 +
>  drivers/media/rc/Makefile   |   1 +
>  drivers/media/rc/tango-ir.c | 263 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 269 insertions(+)
>  create mode 100644 drivers/media/rc/tango-ir.c
>
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index d9ce8ff55d0c..f84923289964 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -469,6 +469,11 @@ config IR_SIR
>  	   To compile this driver as a module, choose M here: the module will
>  	   be called sir-ir.
>  +config IR_TANGO
> +	tristate "Sigma Designs SMP86xx IR decoder"
> +	depends on RC_CORE
> +	depends on ARCH_TANGO || COMPILE_TEST
> +
>  config IR_ZX
>  	tristate "ZTE ZX IR remote control"
>  	depends on RC_CORE

This hunk looks damaged.

What have you changed compared to my original code?

I tested all three protocols with a few random remotes I had lying
around back when I wrote the driver, but that's quite a while ago.

You should also write a devicetree binding.

Finally, when sending patches essentially written by someone else,
please make sure to set a From: line for correct attribution.  It's not
nice to take other people's code and apparently pass it off as your own
even you've made a few small changes.

-- 
Måns Rullgård

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

On 18/09/2017 17:33, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
>> index d9ce8ff55d0c..f84923289964 100644
>> --- a/drivers/media/rc/Kconfig
>> +++ b/drivers/media/rc/Kconfig
>> @@ -469,6 +469,11 @@ config IR_SIR
>>  	   To compile this driver as a module, choose M here: the module will
>>  	   be called sir-ir.
>>  +config IR_TANGO
>> +	tristate "Sigma Designs SMP86xx IR decoder"
>> +	depends on RC_CORE
>> +	depends on ARCH_TANGO || COMPILE_TEST
>> +
>>  config IR_ZX
>>  	tristate "ZTE ZX IR remote control"
>>  	depends on RC_CORE
> 
> This hunk looks damaged.

It appears that the SMTP server has been mangling outgoing messages
for a few months. I will find a work-around.

> What have you changed compared to my original code?

o Rename tangox to tango
o Handle protocol selection (enable/disable) in the change_protocol callback,
instead of unconditionally in open/close
o Delete open/close callbacks
o Rebase driver on top of linuxtv/master
o Use ir_nec_bytes_to_scancode() in tango_ir_handle_nec()
o Use devm_rc_allocate_device() in tango_ir_probe()
o Use Use devm_rc_register_device() in tango_ir_probe()
o Rename rc->input_name to rc->device_name
o List all NEC variants for rc->allowed_protocols
o Change type of clkrate to u64
o Fix tango_ir_probe and tango_ir_remove for devm
o Move around some init calls in tango_ir_probe() for devm
o Use relaxed variants of MMIO accessors

> I tested all three protocols with a few random remotes I had lying
> around back when I wrote the driver, but that's quite a while ago.

OK, I don't think I changed anything wrt RC-5 and RC-6 handling.
It would be great if you could give the driver a quick spin to check
these two protocols. But if you don't have time, no problem.

> You should also write a devicetree binding.

Will do.

> Finally, when sending patches essentially written by someone else,
> please make sure to set a From: line for correct attribution.  It's not
> nice to take other people's code and apparently pass it off as your own
> even you've made a few small changes.

It was not my intent to "take other people's code and apparently pass it off
as my own". I clearly stated where I got the driver from, and your copyright
notice is right there, at the top of the driver. But I understand that you
also want to be credited as the author in the git log, and I will fix that
in v2. Please accept my apologies.

Regards.

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 18/09/2017 17:33, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>>> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
>>> index d9ce8ff55d0c..f84923289964 100644
>>> --- a/drivers/media/rc/Kconfig
>>> +++ b/drivers/media/rc/Kconfig
>>> @@ -469,6 +469,11 @@ config IR_SIR
>>>  	   To compile this driver as a module, choose M here: the module will
>>>  	   be called sir-ir.
>>>  +config IR_TANGO
>>> +	tristate "Sigma Designs SMP86xx IR decoder"
>>> +	depends on RC_CORE
>>> +	depends on ARCH_TANGO || COMPILE_TEST
>>> +
>>>  config IR_ZX
>>>  	tristate "ZTE ZX IR remote control"
>>>  	depends on RC_CORE
>> 
>> This hunk looks damaged.
>
> It appears that the SMTP server has been mangling outgoing messages
> for a few months. I will find a work-around.
>
>> What have you changed compared to my original code?
>
> o Rename tangox to tango
> o Handle protocol selection (enable/disable) in the change_protocol callback,
> instead of unconditionally in open/close
> o Delete open/close callbacks
> o Rebase driver on top of linuxtv/master
> o Use ir_nec_bytes_to_scancode() in tango_ir_handle_nec()
> o Use devm_rc_allocate_device() in tango_ir_probe()
> o Use Use devm_rc_register_device() in tango_ir_probe()
> o Rename rc->input_name to rc->device_name
> o List all NEC variants for rc->allowed_protocols

Did you test the NEC32 variant?  I don't have anything that produces
such codes.

> o Change type of clkrate to u64
> o Fix tango_ir_probe and tango_ir_remove for devm
> o Move around some init calls in tango_ir_probe() for devm
> o Use relaxed variants of MMIO accessors

Thanks for fixing it up.

>> I tested all three protocols with a few random remotes I had lying
>> around back when I wrote the driver, but that's quite a while ago.
>
> OK, I don't think I changed anything wrt RC-5 and RC-6 handling.
> It would be great if you could give the driver a quick spin to check
> these two protocols. But if you don't have time, no problem.

I'll try to find the time.

>> You should also write a devicetree binding.
>
> Will do.
>
>> Finally, when sending patches essentially written by someone else,
>> please make sure to set a From: line for correct attribution.  It's not
>> nice to take other people's code and apparently pass it off as your own
>> even you've made a few small changes.
>
> It was not my intent to "take other people's code and apparently pass it off
> as my own". I clearly stated where I got the driver from, and your copyright
> notice is right there, at the top of the driver. But I understand that you
> also want to be credited as the author in the git log, and I will fix that
> in v2. Please accept my apologies.

No problem.  I know you're not intentionally trying to mislead anyone.

-- 
Måns Rullgård

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

On 19/09/2017 11:48, Måns Rullgård wrote:

> Did you test the NEC32 variant?  I don't have anything that produces
> such codes.

I don't have a NEC32 IR remote control either.

IIUC, NEC32 means 16-bit address and 16-bit command.

I checked the RTL with a HW engineer. The HW block translates the IR
pulses into logical 1s and 0s according to the protocol parameters,
stuffs the logical bits into a register, and fires an IRQ when there
are 32 bits available. The block doesn't care if the bits are significant
or just checksums (that is left up to software).

Regards.

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

On 18/09/2017 17:33, Måns Rullgård wrote:

> What have you changed compared to my original code?

I forgot to mention one change you may not approve of, so we should
probably discuss it.

Your driver supported an optional DT property "linux,rc-map-name"
to override the RC_MAP_EMPTY map. Since the IR decoder supports
multiple protocols, I found it odd to specify a scancode map in
something as low-level as the device tree.

I saw only one board using that property:
$ git grep "linux,rc-map-name" arch/
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts:     linux,rc-map-name = "rc-geekbox";

So I removed support for "linux,rc-map-name" and used ir-keytable
to load a given map from user-space, depending on which RC I use.

Mans, Sean, what do you think?

Regards.

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 19/09/2017 11:48, Måns Rullgård wrote:
>
>> Did you test the NEC32 variant?  I don't have anything that produces
>> such codes.
>
> I don't have a NEC32 IR remote control either.
>
> IIUC, NEC32 means 16-bit address and 16-bit command.
>
> I checked the RTL with a HW engineer. The HW block translates the IR
> pulses into logical 1s and 0s according to the protocol parameters,
> stuffs the logical bits into a register, and fires an IRQ when there
> are 32 bits available. The block doesn't care if the bits are significant
> or just checksums (that is left up to software).

In that case I suppose it ought to just work.

-- 
Måns Rullgård

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 18/09/2017 17:33, Måns Rullgård wrote:
>
>> What have you changed compared to my original code?
>
> I forgot to mention one change you may not approve of, so we should
> probably discuss it.
>
> Your driver supported an optional DT property "linux,rc-map-name"
> to override the RC_MAP_EMPTY map. Since the IR decoder supports
> multiple protocols, I found it odd to specify a scancode map in
> something as low-level as the device tree.
>
> I saw only one board using that property:
> $ git grep "linux,rc-map-name" arch/
> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts:     linux,rc-map-name = "rc-geekbox";
>
> So I removed support for "linux,rc-map-name" and used ir-keytable
> to load a given map from user-space, depending on which RC I use.
>
> Mans, Sean, what do you think?

The property is documented as common for IR receivers although only a
few drivers seem to actually implement the feature.  Since driver
support is trivial, I see no reason to skip it.  Presumably someone
had a use for it, or it wouldn't have been added.

-- 
Måns Rullgård

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

+ Rob & Mark for the DT bindings question.

On 19/09/2017 14:21, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>> On 18/09/2017 17:33, Måns Rullgård wrote:
>>
>>> What have you changed compared to my original code?
>>
>> I forgot to mention one change you may not approve of, so we should
>> probably discuss it.
>>
>> Your driver supported an optional DT property "linux,rc-map-name"
>> to override the RC_MAP_EMPTY map. Since the IR decoder supports
>> multiple protocols, I found it odd to specify a scancode map in
>> something as low-level as the device tree.
>>
>> I saw only one board using that property:
>> $ git grep "linux,rc-map-name" arch/
>> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts:     linux,rc-map-name = "rc-geekbox";
>>
>> So I removed support for "linux,rc-map-name" and used ir-keytable
>> to load a given map from user-space, depending on which RC I use.
>>
>> Mans, Sean, what do you think?
> 
> The property is documented as common for IR receivers although only a
> few drivers seem to actually implement the feature.  Since driver
> support is trivial, I see no reason to skip it.  Presumably someone
> had a use for it, or it wouldn't have been added.

I do not dispute the usefulness of the "linux,rc-map-name" property
in general, e.g. for boards that support a single remote control.

I am arguing that the person writing the device tree has no way of
knowing which rc-map a given user will be using, because it depends
on the actual remote control being used.

Maybe I'm missing something.

Regards.

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> + Rob & Mark for the DT bindings question.
>
> On 19/09/2017 14:21, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>>> On 18/09/2017 17:33, Måns Rullgård wrote:
>>>
>>>> What have you changed compared to my original code?
>>>
>>> I forgot to mention one change you may not approve of, so we should
>>> probably discuss it.
>>>
>>> Your driver supported an optional DT property "linux,rc-map-name"
>>> to override the RC_MAP_EMPTY map. Since the IR decoder supports
>>> multiple protocols, I found it odd to specify a scancode map in
>>> something as low-level as the device tree.
>>>
>>> I saw only one board using that property:
>>> $ git grep "linux,rc-map-name" arch/
>>> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts:     linux,rc-map-name = "rc-geekbox";
>>>
>>> So I removed support for "linux,rc-map-name" and used ir-keytable
>>> to load a given map from user-space, depending on which RC I use.
>>>
>>> Mans, Sean, what do you think?
>> 
>> The property is documented as common for IR receivers although only a
>> few drivers seem to actually implement the feature.  Since driver
>> support is trivial, I see no reason to skip it.  Presumably someone
>> had a use for it, or it wouldn't have been added.
>
> I do not dispute the usefulness of the "linux,rc-map-name" property
> in general, e.g. for boards that support a single remote control.
>
> I am arguing that the person writing the device tree has no way of
> knowing which rc-map a given user will be using, because it depends
> on the actual remote control being used.

Most products (DVD players, TVs, etc) are only intended for use with the
supplied remote control.

-- 
Måns Rullgård

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Sean Young]

On Tue, Sep 19, 2017 at 02:43:17PM +0200, Marc Gonzalez wrote:
> + Rob & Mark for the DT bindings question.
> 
> On 19/09/2017 14:21, Måns Rullgård wrote:
> 
> > Marc Gonzalez writes:
> > 
> >> On 18/09/2017 17:33, Måns Rullgård wrote:
> >>
> >>> What have you changed compared to my original code?
> >>
> >> I forgot to mention one change you may not approve of, so we should
> >> probably discuss it.
> >>
> >> Your driver supported an optional DT property "linux,rc-map-name"
> >> to override the RC_MAP_EMPTY map. Since the IR decoder supports
> >> multiple protocols, I found it odd to specify a scancode map in
> >> something as low-level as the device tree.
> >>
> >> I saw only one board using that property:
> >> $ git grep "linux,rc-map-name" arch/
> >> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts:     linux,rc-map-name = "rc-geekbox";
> >>
> >> So I removed support for "linux,rc-map-name" and used ir-keytable
> >> to load a given map from user-space, depending on which RC I use.
> >>
> >> Mans, Sean, what do you think?
> > 
> > The property is documented as common for IR receivers although only a
> > few drivers seem to actually implement the feature.  Since driver
> > support is trivial, I see no reason to skip it.  Presumably someone
> > had a use for it, or it wouldn't have been added.
> 
> I do not dispute the usefulness of the "linux,rc-map-name" property
> in general, e.g. for boards that support a single remote control.
> 
> I am arguing that the person writing the device tree has no way of
> knowing which rc-map a given user will be using, because it depends
> on the actual remote control being used.
> 
> Maybe I'm missing something.

The device tree for a board can be for a specific product, which ships
with a specific remote. It makes sense to support it, so that any
product that uses the tango-ir can select the remote it ships with.


Sean