Re: [PATCH v7 3/3] media: rc: Add tango keymap

[Måns Rullgård]

Sean Young <s...@mess.org> writes:

> Hi Marc,
>
> Looks great, just some minor nitpicks.
>
> On Thu, Oct 05, 2017 at 04:54:11PM +0200, Marc Gonzalez wrote:
>> Add a keymap for the Sigma Designs Vantage (dev board) remote control.
>> 
>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>> ---
>>  drivers/media/rc/keymaps/Makefile   |  1 +
>>  drivers/media/rc/keymaps/rc-tango.c | 84 
>> +
>>  drivers/media/rc/tango-ir.c |  2 +-
>>  include/media/rc-map.h  |  1 +
>>  4 files changed, 87 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/media/rc/keymaps/rc-tango.c
>> 
>> diff --git a/drivers/media/rc/keymaps/Makefile 
>> b/drivers/media/rc/keymaps/Makefile
>> index af6496d709fb..3c1e31321e21 100644
>> --- a/drivers/media/rc/keymaps/Makefile
>> +++ b/drivers/media/rc/keymaps/Makefile
>> @@ -88,6 +88,7 @@ obj-$(CONFIG_RC_MAP) += rc-adstech-dvb-t-pci.o \
>>  rc-reddo.o \
>>  rc-snapstream-firefly.o \
>>  rc-streamzap.o \
>> +rc-tango.o \
>>  rc-tbs-nec.o \
>>  rc-technisat-ts35.o \
>>  rc-technisat-usb2.o \
>> diff --git a/drivers/media/rc/keymaps/rc-tango.c 
>> b/drivers/media/rc/keymaps/rc-tango.c
>> new file mode 100644
>> index ..c76651695959
>> --- /dev/null
>> +++ b/drivers/media/rc/keymaps/rc-tango.c
>> @@ -0,0 +1,84 @@
>
> I think you need a header with copyright statement and GPL version(s).

Is this even copyrightable?

-- 
Måns Rullgård

Re: [PATCH v6 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:

> On 26/09/2017 15:02, Måns Rullgård wrote:
>
>> I could continue nit-picking, but I think this is good enough.
>> Thanks for being patient.
>> 
>> Signed-off-by: Mans Rullgard <m...@mansr.com>
>
> Smirk.
>
> If you feel like making a final round of changes on top of this patch,
> then go for it. It's your code, after all.
>
> (Too bad there is no devm variant of clk_prepare_enable.)
>
> Does it make sense to submit a keymap for the Sigma remote control
> used with Vantage boards? And define that as the default keymap?

Maybe.  The real product I have with a tango3 chip uses a remote control
that is quite similar the Sigma one.  Having something that works out of
the box with the dev board is also nice.

-- 
Måns Rullgård

Re: [PATCH v6 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

> + do_div(clkdiv, RC6_CARRIER);
> +
> + writel_relaxed(ACK_RC6_INT, ir->rc6_base + RC6_CTRL);
> + writel_relaxed((clkdiv >> 2) << 18 | clkdiv, ir->rc6_base + RC6_CLKDIV);
> +
> +     err = devm_request_irq(dev, irq, tango_ir_irq, IRQF_SHARED,
> +dev_name(dev), ir);
> + if (err)
> + goto err_clk;
> +
> + err = devm_rc_register_device(dev, rc);
> + if (err)
> + goto err_clk;
> +
> + platform_set_drvdata(pdev, ir);
> + return 0;
> +
> +err_clk:
> + clk_disable_unprepare(ir->clk);
> + return err;
> +}
> +
> +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   = DRIVER_NAME,
> + .of_match_table = tango_ir_dt_ids,
> + },
> +};
> +module_platform_driver(tango_ir_driver);
> +
> +MODULE_DESCRIPTION("SMP86xx IR decoder driver");
> +MODULE_AUTHOR("Mans Rullgard <m...@mansr.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.11.0

-- 
Måns Rullgård

Re: [PATCH v5 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:

> * Delete two writes clearing interrupts in probe (cleared is reset value)

Noo.  You can't know what state the hardware is in when this code
runs.  Drivers should *always* fully initialise the hardware to a known
state.  It is especially important to clear any pending interrupts since
otherwise they'll fire the moment the irq is unmasked and can cause all
sorts of mayhem.

-- 
Måns Rullgård

Re: [PATCH v4 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:

> On 25/09/2017 16:08, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>> Why did you put this way early now?  Registering the device should be
>> the last thing you do (LIKE I DID IT, DAMMIT).  Otherwise something might
>> try to use it before it is fully configured.
>> 
>>> +   err = clk_prepare_enable(ir->clk);
>>> +   if (err)
>>> +   return err;
>> 
>> Why did you move this call later?  Seriously, why do you constantly move
>> things around seemingly at random?
>
> This mistake was present in v1, v2, v3.

Is that supposed to be an excuse?

> I got into this mess because I (incorrectly) tried to do all the
> devm inits before clk_prepare_enable().
>
> Why do we need clk_prepare_enable() and why would that function
> fail? The clock is a crystal oscillator which cannot be disabled
> or powered down, and which is the input for every system PLL.

You can't know that.  It happens to be the case for all systems you know
about today, but the driver has to work with any clock configuration.

>>> +   writel_relaxed(0xc000, ir->rc6_base + RC6_CTRL);
>> 
>> Since you've added somewhat descriptive macros for some things, why did
>> you skip this magic number?
>
> This write is supposed to clear interrupts, but there are none
> to clear at this point. I'll remove it.

Wrong answer.

-- 
Måns Rullgård

Re: [PATCH v4 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

_base + IR_NEC_CTRL);
> +
> + clkdiv = clkrate * RC5_TIME_BASE;
> + do_div(clkdiv, 100);
> +
> + writel_relaxed(DISABLE_NEC, 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(0xc000, ir->rc6_base + RC6_CTRL);

Since you've added somewhat descriptive macros for some things, why did
you skip this magic number?

> + writel_relaxed((clkdiv >> 2) << 18 | clkdiv, ir->rc6_base + RC6_CLKDIV);
> +
> + platform_set_drvdata(pdev, ir);
> +
> + name = dev_name(dev);
> + err = devm_request_irq(dev, irq, tango_ir_irq, IRQF_SHARED, name, ir);
> + if (err)
> + clk_disable_unprepare(ir->clk);
> +
> + return err;
> +}
> +
> +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   = DRIVER_NAME,
> + .of_match_table = tango_ir_dt_ids,
> + },
> +};
> +module_platform_driver(tango_ir_driver);
> +
> +MODULE_DESCRIPTION("SMP86xx IR decoder driver");
> +MODULE_AUTHOR("Mans Rullgard <m...@mansr.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.11.0

-- 
Måns Rullgård

Re: [PATCH v3 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:

> On 21/09/2017 17:46, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>>> From: Mans Rullgard <m...@mansr.com>
>>>
>>> The tango HW IR decoder supports NEC, RC-5, RC-6 protocols.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>> 
>> Have you been able to test all the protocols?  Universal remotes usually
>> support something or other with each of them.
>
> I found the Great Pile of Remotes locked away in a drawer.
> Played "What kind of batteries do you eat?" for about an hour.
> And found several NEC remotes, one RC-5, and one RC-6.
> Repeats seem to be handled differently than for NEC.
> I'll take a closer look.

That's not surprising, seeing as the way repeats are transmitted differs
between protocols.

If you're new to IR remote controls, this site has some good
information:
http://www.sbprojects.com/knowledge/ir/index.php

-- 
Måns Rullgård

Re: [PATCH v3 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:

> On 21/09/2017 17:57, Sean Young wrote:
>
>> On Thu, Sep 21, 2017 at 04:49:53PM +0200, Marc Gonzalez wrote:
>> 
>>> The tango HW IR decoder supports NEC, RC-5, RC-6 protocols.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>> 
>> Missing signed-off-by.
>
> I am aware of that. Hopefully, at some point, Mans will add his.

I will once I'm happy with it.  Shouldn't be far off.

-- 
Måns Rullgård

Re: [PATCH v3 2/2] media: rc: Add driver for tango HW IR decoder

[Måns Rullgård]

CODE_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(rc5_ctrl, ir->rc5_base + IR_CTRL);
> + writel_relaxed(rc6_ctrl, ir->rc6_base + RC6_CTRL);
> +
> + return 0;
> +}
> +
> +static int tango_ir_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >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;
> +
> + of_property_read_string(dev->of_node, "linux,rc-map-name", 
> >map_name);
> +
> + 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;

You shouldn't enable the irq until after you've configured the device.
Otherwise you have no idea what state it's in, and it might start firing
unexpectedly.

My original code did this properly.  Why did you move it?

> + err = clk_prepare_enable(ir->clk);
> + if (err)
> + return err;
> +
> + clkrate = clk_get_rate(ir->clk);
> +
> + clkdiv = clkrate * NEC_TIME_BASE;
> + do_div(clkdiv, 100);
> +
> + writel_relaxed(31 << 24 | 12 << 16 | clkdiv, ir->rc5_base + 
> IR_NEC_CTRL);
> +
> + clkdiv = clkrate * RC5_TIME_BASE;
> + do_div(clkdiv, 100);
> +
> + writel_relaxed(0x110, ir->rc5_base + IR_CTRL);

Since you've defined DISABLE_NEC above, I think you should use it here too.

> + 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(0xc000, 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 <m...@mansr.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.11.0

-- 
Måns Rullgård

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

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@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

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@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

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@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_gonza...@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

[Måns Rullgård]

Marc Gonzalez <marc_gonza...@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_gonza...@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