diff mbox

IR driver support for tango platforms

Message ID 6076a18d-c5ba-cb83-ac36-8eda965c7eb8@free.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Mason Sept. 11, 2017, 2:37 p.m. UTC 
Hello Sean,

After a long hiatus, I can now work again on the IR driver support
for tango platforms. I'm still using this driver:

https://github.com/mansr/linux-tangox/blob/master/drivers/media/rc/tangox-ir.c

There are two nits I'd like to discuss.

A) When I hold a key on the RC, ir-keytable reports scancode = 0x00
instead of the scancode for the repeated key.

# ir-keytable -t -v
[   70.561432] show_protocols: allowed - 0x4204, enabled - 0x0
Found device /sys/class/rc/rc0/
Input sysfs node is /sys/class/rc/rc0/input0/
Event sysfs node is /sys/class/rc/rc0/input0/event0/
Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
/sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
/sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
/sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
Parsing uevent /sys/class/rc/rc0/uevent
/sys/class/rc/rc0/uevent uevent NAME=rc-empty
input device is /dev/input/event0
/sys/class/rc/rc0/protocols protocol rc-5 (disabled)
/sys/class/rc/rc0/protocols protocol nec (disabled)
/sys/class/rc/rc0/protocols protocol rc-6 (disabled)
Opening /dev/input/event0
Input Protocol version: 0x00010001
Testing events. Please, press CTRL-C to abort.
[  227.977324] rc_keydown: keycode=0
227.980533: event type EV_MSC(0x04): scancode = 0x4cb41
227.980533: event type EV_SYN(0x00).
228.031069: event type EV_MSC(0x04): scancode = 0x00
228.031069: event type EV_SYN(0x00).
228.138834: event type EV_MSC(0x04): scancode = 0x00
228.138834: event type EV_SYN(0x00).
228.246603: event type EV_MSC(0x04): scancode = 0x00
228.246603: event type EV_SYN(0x00).
228.354373: event type EV_MSC(0x04): scancode = 0x00
228.354373: event type EV_SYN(0x00).
228.462143: event type EV_MSC(0x04): scancode = 0x00
228.462143: event type EV_SYN(0x00).
228.569913: event type EV_MSC(0x04): scancode = 0x00
228.569913: event type EV_SYN(0x00).

This behavior is caused by ir_do_keydown() not recording the keypress
when keycode == KEY_RESERVED

If I apply the following patch, then the repeated scancode works
as I would expect.



# ir-keytable -t -v
[   68.192161] show_protocols: allowed - 0x4204, enabled - 0x0
Found device /sys/class/rc/rc0/
Input sysfs node is /sys/class/rc/rc0/input0/
Event sysfs node is /sys/class/rc/rc0/input0/event0/
Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
/sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
/sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
/sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
Parsing uevent /sys/class/rc/rc0/uevent
/sys/class/rc/rc0/uevent uevent NAME=rc-empty
input device is /dev/input/event0
/sys/class/rc/rc0/protocols protocol rc-5 (disabled)
/sys/class/rc/rc0/protocols protocol nec (disabled)
/sys/class/rc/rc0/protocols protocol rc-6 (disabled)
Opening /dev/input/event0
Input Protocol version: 0x00010001
Testing events. Please, press CTRL-C to abort.
[   92.739308] rc_keydown: keycode=0
[   92.742650] tango-ir: key down event, key 0x00f0, protocol 0x0009, scancode 0x0004cb41
92.749621: event type EV_MSC(0x04): scancode = 0x4cb41
92.749621: event type EV_SYN(0x00).
92.792201: event type EV_MSC(0x04): scancode = 0x4cb41
92.792201: event type EV_SYN(0x00).
92.899966: event type EV_MSC(0x04): scancode = 0x4cb41
92.899966: event type EV_SYN(0x00).
93.007734: event type EV_MSC(0x04): scancode = 0x4cb41
93.007734: event type EV_SYN(0x00).
93.115501: event type EV_MSC(0x04): scancode = 0x4cb41
93.115501: event type EV_SYN(0x00).
93.223269: event type EV_MSC(0x04): scancode = 0x4cb41
93.223269: event type EV_SYN(0x00).
93.331039: event type EV_MSC(0x04): scancode = 0x4cb41
93.331039: event type EV_SYN(0x00).
[   93.600995] keyup key 0x00f0


I'm confused. Does this mean a keymap is mandatory?
I thought it was possible to handle the "scancode to keycode"
stepin user-space?


B) Currently, the driver doesn't seem to allow selective protocol
enabling/disabling. It just silently enables all protocols at init.

It would seem useful to add support for that, so that the HW
doesn't fire spurious RC5 interrupts for NEC events.

What do you think?

Regards.


Archives of previous threads:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg114854.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg115316.html

Comments

Sean Young Sept. 11, 2017, 9:12 p.m. UTC | #1 
Hi Mason,

On Mon, Sep 11, 2017 at 04:37:42PM +0200, Mason wrote:
> Hello Sean,
> 
> After a long hiatus, I can now work again on the IR driver support
> for tango platforms. I'm still using this driver:
> 
> https://github.com/mansr/linux-tangox/blob/master/drivers/media/rc/tangox-ir.c
> 
> There are two nits I'd like to discuss.
> 
> A) When I hold a key on the RC, ir-keytable reports scancode = 0x00
> instead of the scancode for the repeated key.
> 
> # ir-keytable -t -v
> [   70.561432] show_protocols: allowed - 0x4204, enabled - 0x0
> Found device /sys/class/rc/rc0/
> Input sysfs node is /sys/class/rc/rc0/input0/
> Event sysfs node is /sys/class/rc/rc0/input0/event0/
> Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
> /sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
> /sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
> /sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
> Parsing uevent /sys/class/rc/rc0/uevent
> /sys/class/rc/rc0/uevent uevent NAME=rc-empty
> input device is /dev/input/event0
> /sys/class/rc/rc0/protocols protocol rc-5 (disabled)
> /sys/class/rc/rc0/protocols protocol nec (disabled)
> /sys/class/rc/rc0/protocols protocol rc-6 (disabled)
> Opening /dev/input/event0
> Input Protocol version: 0x00010001
> Testing events. Please, press CTRL-C to abort.
> [  227.977324] rc_keydown: keycode=0
> 227.980533: event type EV_MSC(0x04): scancode = 0x4cb41
> 227.980533: event type EV_SYN(0x00).
> 228.031069: event type EV_MSC(0x04): scancode = 0x00
> 228.031069: event type EV_SYN(0x00).
> 228.138834: event type EV_MSC(0x04): scancode = 0x00
> 228.138834: event type EV_SYN(0x00).
> 228.246603: event type EV_MSC(0x04): scancode = 0x00
> 228.246603: event type EV_SYN(0x00).
> 228.354373: event type EV_MSC(0x04): scancode = 0x00
> 228.354373: event type EV_SYN(0x00).
> 228.462143: event type EV_MSC(0x04): scancode = 0x00
> 228.462143: event type EV_SYN(0x00).
> 228.569913: event type EV_MSC(0x04): scancode = 0x00
> 228.569913: event type EV_SYN(0x00).
> 
> This behavior is caused by ir_do_keydown() not recording the keypress
> when keycode == KEY_RESERVED

That's interesting. I think happens. First, scancode 0x4cb1 is received
using extended nec; the scancode is reported via rc_keydown() by the
driver; no keycode is matched. so dev->last_scancode is not set.

Next a nec repeat is received; this is reported via rc_repeat(). This
function reports the last_scancode. Which is set to 0, since it was
never set to anything.

Note that this behaviour changed since commit 265a2988d202 ("media:
rc-core: consistent use of rc_repeat()"). Since then, the scancode is only
reported on rc_repeat() if the scancode translated to a key.

> If I apply the following patch, then the repeated scancode works
> as I would expect.
> 
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -687,6 +687,10 @@ void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 togg
>         unsigned long flags;
>         u32 keycode = rc_g_keycode_from_table(dev, scancode);
> +       printk("%s: keycode=%x\n", __func__, keycode);
> +       if (keycode == KEY_RESERVED)
> +               keycode = KEY_UNKNOWN;
> +
>         spin_lock_irqsave(&dev->keylock, flags);
>         ir_do_keydown(dev, protocol, scancode, keycode, toggle);
> 
> 
> # ir-keytable -t -v
> [   68.192161] show_protocols: allowed - 0x4204, enabled - 0x0
> Found device /sys/class/rc/rc0/
> Input sysfs node is /sys/class/rc/rc0/input0/
> Event sysfs node is /sys/class/rc/rc0/input0/event0/
> Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
> /sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
> /sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
> /sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
> Parsing uevent /sys/class/rc/rc0/uevent
> /sys/class/rc/rc0/uevent uevent NAME=rc-empty
> input device is /dev/input/event0
> /sys/class/rc/rc0/protocols protocol rc-5 (disabled)
> /sys/class/rc/rc0/protocols protocol nec (disabled)
> /sys/class/rc/rc0/protocols protocol rc-6 (disabled)
> Opening /dev/input/event0
> Input Protocol version: 0x00010001
> Testing events. Please, press CTRL-C to abort.
> [   92.739308] rc_keydown: keycode=0
> [   92.742650] tango-ir: key down event, key 0x00f0, protocol 0x0009, scancode 0x0004cb41
> 92.749621: event type EV_MSC(0x04): scancode = 0x4cb41
> 92.749621: event type EV_SYN(0x00).
> 92.792201: event type EV_MSC(0x04): scancode = 0x4cb41
> 92.792201: event type EV_SYN(0x00).
> 92.899966: event type EV_MSC(0x04): scancode = 0x4cb41
> 92.899966: event type EV_SYN(0x00).
> 93.007734: event type EV_MSC(0x04): scancode = 0x4cb41
> 93.007734: event type EV_SYN(0x00).
> 93.115501: event type EV_MSC(0x04): scancode = 0x4cb41
> 93.115501: event type EV_SYN(0x00).
> 93.223269: event type EV_MSC(0x04): scancode = 0x4cb41
> 93.223269: event type EV_SYN(0x00).
> 93.331039: event type EV_MSC(0x04): scancode = 0x4cb41
> 93.331039: event type EV_SYN(0x00).
> [   93.600995] keyup key 0x00f0
> 
> 
> I'm confused. Does this mean a keymap is mandatory?
> I thought it was possible to handle the "scancode to keycode"
> stepin user-space?

The handling could be better here, but for nec repeats, yes a matching
keycode is required here.


> B) Currently, the driver doesn't seem to allow selective protocol
> enabling/disabling. It just silently enables all protocols at init.
> 
> It would seem useful to add support for that, so that the HW
> doesn't fire spurious RC5 interrupts for NEC events.
> 
> What do you think?

That could be useful. In order to that, have to implement the 
rc_dev->change_protocol function; in that function, you have to tell
the hardware to not generate interrupts for protocols which aren't
disabled. So, in order to implement that you'll need to know how
to do that. Is there a datasheet available?

Thanks,

Sean
Mason Sept. 12, 2017, 3:56 p.m. UTC | #2 
On 11/09/2017 23:12, Sean Young wrote:

> On Sep 11, 2017 at 04:37, Mason wrote:
>
>> I'm confused. Does this mean a keymap is mandatory?
>> I thought it was possible to handle the "scancode to keycode"
>> step in user-space?
> 
> The handling could be better here, but for nec repeats, yes a matching
> keycode is required here.
> 
> 
>> B) Currently, the driver doesn't seem to allow selective protocol
>> enabling/disabling. It just silently enables all protocols at init.
>>
>> It would seem useful to add support for that, so that the HW
>> doesn't fire spurious RC5 interrupts for NEC events.
>>
>> What do you think?
> 
> That could be useful. In order to that, have to implement the
> rc_dev->change_protocol function; in that function, you have to tell
> the hardware to not generate interrupts for protocols which aren't
> disabled. So, in order to implement that you'll need to know how
> to do that. Is there a datasheet available?

I have access to some documentation, but I was told I cannot share
it publically :-(

I've made some progress. I'd like to run the code by you (and Mans,
the original author), to make sure it is in an acceptable state.
I tested the driver using ir-keytable and a NEC remote control.
(RC-5 and RC-6 are untested, for now.)

# ir-keytable -p nec
Protocols changed to nec

# ir-keytable -c
Old keytable cleared

# ir-keytable -w sigma.rc
Wrote 35 keycode(s) to driver

# ir-keytable -r
scancode 0x4cb01 = KEY_RIGHT (0x6a)
scancode 0x4cb02 = KEY_OK (0x160)
scancode 0x4cb03 = KEY_2 (0x03)
scancode 0x4cb06 = KEY_UP (0x67)
scancode 0x4cb07 = KEY_5 (0x06)
scancode 0x4cb0a = KEY_DOWN (0x6c)
scancode 0x4cb0f = KEY_SETUP (0x8d)
scancode 0x4cb16 = KEY_ANGLE (0x173)
scancode 0x4cb17 = KEY_8 (0x09)
scancode 0x4cb19 = KEY_ZOOM (0x174)
scancode 0x4cb1a = KEY_AUDIO (0x188)
scancode 0x4cb1b = KEY_0 (0x0b)
scancode 0x4cb1d = KEY_BLUE (0x191)
scancode 0x4cb1e = KEY_GREEN (0x18f)
scancode 0x4cb41 = KEY_1 (0x02)
scancode 0x4cb42 = KEY_3 (0x04)
scancode 0x4cb43 = KEY_LEFT (0x69)
scancode 0x4cb44 = KEY_EJECTCD (0xa1)
scancode 0x4cb45 = KEY_4 (0x05)
scancode 0x4cb46 = KEY_6 (0x07)
scancode 0x4cb47 = KEY_BACK (0x9e)
scancode 0x4cb4a = KEY_POWER (0x74)
scancode 0x4cb4b = KEY_INFO (0x166)
scancode 0x4cb4c = KEY_STOP (0x80)
scancode 0x4cb4f = KEY_TITLE (0x171)
scancode 0x4cb50 = KEY_PLAY (0xcf)
scancode 0x4cb51 = KEY_MUTE (0x71)
scancode 0x4cb53 = KEY_MENU (0x8b)
scancode 0x4cb54 = KEY_PAUSE (0x77)
scancode 0x4cb55 = KEY_7 (0x08)
scancode 0x4cb56 = KEY_9 (0x0a)
scancode 0x4cb58 = KEY_SUBTITLE (0x172)
scancode 0x4cb59 = KEY_DELETE (0x6f)
scancode 0x4cb5c = KEY_YELLOW (0x190)
scancode 0x4cb5f = KEY_RED (0x18e)
Enabled protocols: nec

# ir-keytable -t -v
Found device /sys/class/rc/rc0/
Input sysfs node is /sys/class/rc/rc0/input0/
Event sysfs node is /sys/class/rc/rc0/input0/event0/
Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
/sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
/sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
/sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
Parsing uevent /sys/class/rc/rc0/uevent
/sys/class/rc/rc0/uevent uevent NAME=rc-empty
/sys/class/rc/rc0/uevent uevent DRV_NAME=tango-ir
input device is /dev/input/event0
/sys/class/rc/rc0/protocols protocol rc-5 (disabled)
/sys/class/rc/rc0/protocols protocol nec (enabled)
/sys/class/rc/rc0/protocols protocol rc-6 (disabled)
Opening /dev/input/event0
Input Protocol version: 0x00010001
Testing events. Please, press CTRL-C to abort.
122.327942: event type EV_MSC(0x04): scancode = 0x4cb41
122.327942: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
122.327942: event type EV_SYN(0x00).
122.381794: event type EV_MSC(0x04): scancode = 0x4cb41
122.381794: event type EV_SYN(0x00).
122.489565: event type EV_MSC(0x04): scancode = 0x4cb41
122.489565: event type EV_SYN(0x00).
122.597335: event type EV_MSC(0x04): scancode = 0x4cb41
122.597335: event type EV_SYN(0x00).
122.705110: event type EV_MSC(0x04): scancode = 0x4cb41
122.705110: event type EV_SYN(0x00).
122.812883: event type EV_MSC(0x04): scancode = 0x4cb41
122.812883: event type EV_SYN(0x00).
122.853335: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
122.853335: event type EV_SYN(0x00).
122.920659: event type EV_MSC(0x04): scancode = 0x4cb41
122.920659: event type EV_SYN(0x00).
122.983335: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
122.983335: event type EV_SYN(0x00).
123.028428: event type EV_MSC(0x04): scancode = 0x4cb41
123.028428: event type EV_SYN(0x00).
123.113334: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
123.113334: event type EV_SYN(0x00).
123.243333: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
123.243333: event type EV_SYN(0x00).
123.280000: event type EV_KEY(0x01) key_up: KEY_1(0x0002)
123.280000: event type EV_SYN(0x00).
134.743698: event type EV_MSC(0x04): scancode = 0x4cb50
134.743698: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
134.743698: event type EV_SYN(0x00).
134.797577: event type EV_MSC(0x04): scancode = 0x4cb50
134.797577: event type EV_SYN(0x00).
134.905349: event type EV_MSC(0x04): scancode = 0x4cb50
134.905349: event type EV_SYN(0x00).
135.013123: event type EV_MSC(0x04): scancode = 0x4cb50
135.013123: event type EV_SYN(0x00).
135.120896: event type EV_MSC(0x04): scancode = 0x4cb50
135.120896: event type EV_SYN(0x00).
135.228670: event type EV_MSC(0x04): scancode = 0x4cb50
135.228670: event type EV_SYN(0x00).
135.253332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.253332: event type EV_SYN(0x00).
135.336443: event type EV_MSC(0x04): scancode = 0x4cb50
135.336443: event type EV_SYN(0x00).
135.383332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.383332: event type EV_SYN(0x00).
135.444215: event type EV_MSC(0x04): scancode = 0x4cb50
135.444215: event type EV_SYN(0x00).
135.513332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.513332: event type EV_SYN(0x00).
135.551989: event type EV_MSC(0x04): scancode = 0x4cb50
135.551989: event type EV_SYN(0x00).
135.643332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.643332: event type EV_SYN(0x00).
135.659762: event type EV_MSC(0x04): scancode = 0x4cb50
135.659762: event type EV_SYN(0x00).
135.767537: event type EV_MSC(0x04): scancode = 0x4cb50
135.767537: event type EV_SYN(0x00).
135.773331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.773331: event type EV_SYN(0x00).
135.875309: event type EV_MSC(0x04): scancode = 0x4cb50
135.875309: event type EV_SYN(0x00).
135.903331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
135.903331: event type EV_SYN(0x00).
135.983083: event type EV_MSC(0x04): scancode = 0x4cb50
135.983083: event type EV_SYN(0x00).
136.033332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
136.033332: event type EV_SYN(0x00).
136.090857: event type EV_MSC(0x04): scancode = 0x4cb50
136.090857: event type EV_SYN(0x00).
136.163331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
136.163331: event type EV_SYN(0x00).
136.198628: event type EV_MSC(0x04): scancode = 0x4cb50
136.198628: event type EV_SYN(0x00).
136.293332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
136.293332: event type EV_SYN(0x00).
136.423332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
136.423332: event type EV_SYN(0x00).
136.453333: event type EV_KEY(0x01) key_up: KEY_PLAY(0x00cf)
136.453333: event type EV_SYN(0x00).


Everything seems to work as expected... Right?


The code for the driver is below:
If it looks good, I'll make a formal submission.


/*
  * 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)
{
	unsigned int data;
	unsigned int addr;
	unsigned int naddr;
	unsigned int key;
	unsigned int nkey;

	data = readl(ir->rc5_base + IR_NEC_DATA);

	if (!data) {
		rc_repeat(ir->rc);
		return;
	}

	addr = data & 0xff;
	naddr = data >> 8 & 0xff;
	key = data >> 16 & 0xff;
	nkey = data >> 24 & 0xff;

	if ((addr ^ naddr) != 255)
		addr = addr << 8 | naddr;

	if ((key ^ nkey) != 255)
		return;

	rc_keydown(ir->rc, RC_TYPE_NEC, RC_SCANCODE_NEC(addr, key), 0);
}

static void tango_ir_handle_rc5(struct tango_ir *ir)
{
	unsigned int data;
	unsigned int field;
	unsigned int toggle;
	unsigned int addr;
	unsigned int cmd;

	data = readl(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;

	rc_keydown(ir->rc, RC_TYPE_RC5, RC_SCANCODE_RC5(addr, cmd), toggle);
}

static void tango_ir_handle_rc6(struct tango_ir *ir)
{
	unsigned int data0, data1;
	unsigned int toggle;
	unsigned int mode;
	unsigned int addr;
	unsigned int cmd;

	data0 = readl(ir->rc6_base + RC6_DATA0);
	data1 = readl(ir->rc6_base + RC6_DATA1);

	mode = data0 >> 1 & 7;

	if (mode != 0)
		return;

	toggle = data0 & 1;
	addr = data0 >> 16;
	cmd = data1;

	rc_keydown(ir->rc, RC_TYPE_RC6_0, RC_SCANCODE_RC6_0(addr, cmd), 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(ir->rc5_base + IR_INT);
	writel(rc5_stat, ir->rc5_base + IR_INT);

	rc6_stat = readl(ir->rc6_base + RC6_CTRL);
	writel(rc6_stat, ir->rc6_base + RC6_CTRL);

	if (!(rc5_stat & 3) && !(rc6_stat & BIT(31)))
		return IRQ_NONE;

	if (rc5_stat & 1)
		tango_ir_handle_rc5(ir);

	if (rc5_stat & 2)
		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_BIT_NEC)
		rc5_ctrl = 0;

	if (*rc_type & RC_BIT_RC5)
		rc5_ctrl |= ENABLE_RC5;

	if (*rc_type & RC_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 = &pdev->dev;
	struct rc_dev *rc;
	struct tango_ir *ir;
	struct resource *rc5_res;
	struct resource *rc6_res;
	unsigned long clkrate;
	u64 clkdiv;
	int irq;
	int 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);

	err = clk_prepare_enable(ir->clk);
	if (err)
		return err;

	clkrate = clk_get_rate(ir->clk);

	rc = rc_allocate_device();
	if (!rc) {
		err = -ENOMEM;
		goto err_clk;
	}

	rc->dev.parent = dev;
	rc->input_name = "tango-ir";
	rc->input_phys = "tango-ir/input0";
	rc->driver_name = "tango-ir";
	rc->map_name = RC_MAP_EMPTY;
	rc->driver_type = RC_DRIVER_SCANCODE;
	rc->allowed_protocols = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;
	rc->change_protocol = tango_change_protocol;

	rc->priv = ir;
	ir->rc = rc;

	clkdiv = (u64)clkrate * NEC_TIME_BASE;
	do_div(clkdiv, 1000000);

	writel(31 << 24 | 12 << 16 | clkdiv, ir->rc5_base + IR_NEC_CTRL);

	clkdiv = (u64)clkrate * RC5_TIME_BASE;
	do_div(clkdiv, 1000000);

	writel(0x110, ir->rc5_base + IR_CTRL);
	writel(clkdiv, ir->rc5_base + IR_RC5_CLK_DIV);
	writel(0x3, ir->rc5_base + IR_INT);

	clkdiv = (u64)clkrate * RC6_TIME_BASE;
	do_div(clkdiv, RC6_CARRIER);

	writel(0xc0000000, ir->rc6_base + RC6_CTRL);
	writel((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_rc;

	dev_info(dev, "SMP86xx IR decoder at 0x%x/0x%x IRQ %d\n",
		 rc5_res->start, rc6_res->start, irq);

	err = rc_register_device(rc);
	if (err)
		goto err_rc;

	platform_set_drvdata(pdev, ir);

	return 0;

err_rc:
	rc_free_device(rc);
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);

	rc_unregister_device(ir->rc);
	rc_free_device(ir->rc);
	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");
Sean Young Sept. 12, 2017, 6:19 p.m. UTC | #3 
On Tue, Sep 12, 2017 at 05:56:11PM +0200, Mason wrote:
> On 11/09/2017 23:12, Sean Young wrote:
> 
> > On Sep 11, 2017 at 04:37, Mason wrote:
> > 
> > > I'm confused. Does this mean a keymap is mandatory?
> > > I thought it was possible to handle the "scancode to keycode"
> > > step in user-space?
> > 
> > The handling could be better here, but for nec repeats, yes a matching
> > keycode is required here.
> > 
> > 
> > > B) Currently, the driver doesn't seem to allow selective protocol
> > > enabling/disabling. It just silently enables all protocols at init.
> > > 
> > > It would seem useful to add support for that, so that the HW
> > > doesn't fire spurious RC5 interrupts for NEC events.
> > > 
> > > What do you think?
> > 
> > That could be useful. In order to that, have to implement the
> > rc_dev->change_protocol function; in that function, you have to tell
> > the hardware to not generate interrupts for protocols which aren't
> > disabled. So, in order to implement that you'll need to know how
> > to do that. Is there a datasheet available?
> 
> I have access to some documentation, but I was told I cannot share
> it publically :-(
> 
> I've made some progress. I'd like to run the code by you (and Mans,
> the original author), to make sure it is in an acceptable state.
> I tested the driver using ir-keytable and a NEC remote control.
> (RC-5 and RC-6 are untested, for now.)

It looks great, thanks! I have made some minor points below.
 
> # ir-keytable -p nec
> Protocols changed to nec
> 
> # ir-keytable -c
> Old keytable cleared
> 
> # ir-keytable -w sigma.rc
> Wrote 35 keycode(s) to driver
> 
> # ir-keytable -r
> scancode 0x4cb01 = KEY_RIGHT (0x6a)
> scancode 0x4cb02 = KEY_OK (0x160)
> scancode 0x4cb03 = KEY_2 (0x03)
> scancode 0x4cb06 = KEY_UP (0x67)
> scancode 0x4cb07 = KEY_5 (0x06)
> scancode 0x4cb0a = KEY_DOWN (0x6c)
> scancode 0x4cb0f = KEY_SETUP (0x8d)
> scancode 0x4cb16 = KEY_ANGLE (0x173)
> scancode 0x4cb17 = KEY_8 (0x09)
> scancode 0x4cb19 = KEY_ZOOM (0x174)
> scancode 0x4cb1a = KEY_AUDIO (0x188)
> scancode 0x4cb1b = KEY_0 (0x0b)
> scancode 0x4cb1d = KEY_BLUE (0x191)
> scancode 0x4cb1e = KEY_GREEN (0x18f)
> scancode 0x4cb41 = KEY_1 (0x02)
> scancode 0x4cb42 = KEY_3 (0x04)
> scancode 0x4cb43 = KEY_LEFT (0x69)
> scancode 0x4cb44 = KEY_EJECTCD (0xa1)
> scancode 0x4cb45 = KEY_4 (0x05)
> scancode 0x4cb46 = KEY_6 (0x07)
> scancode 0x4cb47 = KEY_BACK (0x9e)
> scancode 0x4cb4a = KEY_POWER (0x74)
> scancode 0x4cb4b = KEY_INFO (0x166)
> scancode 0x4cb4c = KEY_STOP (0x80)
> scancode 0x4cb4f = KEY_TITLE (0x171)
> scancode 0x4cb50 = KEY_PLAY (0xcf)
> scancode 0x4cb51 = KEY_MUTE (0x71)
> scancode 0x4cb53 = KEY_MENU (0x8b)
> scancode 0x4cb54 = KEY_PAUSE (0x77)
> scancode 0x4cb55 = KEY_7 (0x08)
> scancode 0x4cb56 = KEY_9 (0x0a)
> scancode 0x4cb58 = KEY_SUBTITLE (0x172)
> scancode 0x4cb59 = KEY_DELETE (0x6f)
> scancode 0x4cb5c = KEY_YELLOW (0x190)
> scancode 0x4cb5f = KEY_RED (0x18e)
> Enabled protocols: nec
> 
> # ir-keytable -t -v
> Found device /sys/class/rc/rc0/
> Input sysfs node is /sys/class/rc/rc0/input0/
> Event sysfs node is /sys/class/rc/rc0/input0/event0/
> Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
> /sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
> /sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
> /sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
> Parsing uevent /sys/class/rc/rc0/uevent
> /sys/class/rc/rc0/uevent uevent NAME=rc-empty
> /sys/class/rc/rc0/uevent uevent DRV_NAME=tango-ir
> input device is /dev/input/event0
> /sys/class/rc/rc0/protocols protocol rc-5 (disabled)
> /sys/class/rc/rc0/protocols protocol nec (enabled)
> /sys/class/rc/rc0/protocols protocol rc-6 (disabled)
> Opening /dev/input/event0
> Input Protocol version: 0x00010001
> Testing events. Please, press CTRL-C to abort.
> 122.327942: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.327942: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
> 122.327942: event type EV_SYN(0x00).
> 122.381794: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.381794: event type EV_SYN(0x00).
> 122.489565: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.489565: event type EV_SYN(0x00).
> 122.597335: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.597335: event type EV_SYN(0x00).
> 122.705110: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.705110: event type EV_SYN(0x00).
> 122.812883: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.812883: event type EV_SYN(0x00).
> 122.853335: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
> 122.853335: event type EV_SYN(0x00).
> 122.920659: event type EV_MSC(0x04): scancode = 0x4cb41
> 122.920659: event type EV_SYN(0x00).
> 122.983335: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
> 122.983335: event type EV_SYN(0x00).
> 123.028428: event type EV_MSC(0x04): scancode = 0x4cb41
> 123.028428: event type EV_SYN(0x00).
> 123.113334: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
> 123.113334: event type EV_SYN(0x00).
> 123.243333: event type EV_KEY(0x01) key_down: KEY_1(0x0002)
> 123.243333: event type EV_SYN(0x00).
> 123.280000: event type EV_KEY(0x01) key_up: KEY_1(0x0002)
> 123.280000: event type EV_SYN(0x00).
> 134.743698: event type EV_MSC(0x04): scancode = 0x4cb50
> 134.743698: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 134.743698: event type EV_SYN(0x00).
> 134.797577: event type EV_MSC(0x04): scancode = 0x4cb50
> 134.797577: event type EV_SYN(0x00).
> 134.905349: event type EV_MSC(0x04): scancode = 0x4cb50
> 134.905349: event type EV_SYN(0x00).
> 135.013123: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.013123: event type EV_SYN(0x00).
> 135.120896: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.120896: event type EV_SYN(0x00).
> 135.228670: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.228670: event type EV_SYN(0x00).
> 135.253332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.253332: event type EV_SYN(0x00).
> 135.336443: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.336443: event type EV_SYN(0x00).
> 135.383332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.383332: event type EV_SYN(0x00).
> 135.444215: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.444215: event type EV_SYN(0x00).
> 135.513332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.513332: event type EV_SYN(0x00).
> 135.551989: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.551989: event type EV_SYN(0x00).
> 135.643332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.643332: event type EV_SYN(0x00).
> 135.659762: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.659762: event type EV_SYN(0x00).
> 135.767537: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.767537: event type EV_SYN(0x00).
> 135.773331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.773331: event type EV_SYN(0x00).
> 135.875309: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.875309: event type EV_SYN(0x00).
> 135.903331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 135.903331: event type EV_SYN(0x00).
> 135.983083: event type EV_MSC(0x04): scancode = 0x4cb50
> 135.983083: event type EV_SYN(0x00).
> 136.033332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 136.033332: event type EV_SYN(0x00).
> 136.090857: event type EV_MSC(0x04): scancode = 0x4cb50
> 136.090857: event type EV_SYN(0x00).
> 136.163331: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 136.163331: event type EV_SYN(0x00).
> 136.198628: event type EV_MSC(0x04): scancode = 0x4cb50
> 136.198628: event type EV_SYN(0x00).
> 136.293332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 136.293332: event type EV_SYN(0x00).
> 136.423332: event type EV_KEY(0x01) key_down: KEY_PLAY(0x00cf)
> 136.423332: event type EV_SYN(0x00).
> 136.453333: event type EV_KEY(0x01) key_up: KEY_PLAY(0x00cf)
> 136.453333: event type EV_SYN(0x00).
> 
> 
> Everything seems to work as expected... Right?

As far as I can see, yet.
> 
> The code for the driver is below:
> If it looks good, I'll make a formal submission.
> 
> 
> /*
>  * 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)
> {
> 	unsigned int data;
> 	unsigned int addr;
> 	unsigned int naddr;
> 	unsigned int key;
> 	unsigned int nkey;
> 
> 	data = readl(ir->rc5_base + IR_NEC_DATA);
> 
> 	if (!data) {
> 		rc_repeat(ir->rc);
> 		return;
> 	}
> 
> 	addr = data & 0xff;
> 	naddr = data >> 8 & 0xff;
> 	key = data >> 16 & 0xff;
> 	nkey = data >> 24 & 0xff;
> 
> 	if ((addr ^ naddr) != 255)
> 		addr = addr << 8 | naddr;
> 
> 	if ((key ^ nkey) != 255)
> 		return;

Please use ir_nec_bytes_to_scancode() rather than handcoding this.
> 
> 	rc_keydown(ir->rc, RC_TYPE_NEC, RC_SCANCODE_NEC(addr, key), 0);
> }
> 
> static void tango_ir_handle_rc5(struct tango_ir *ir)
> {
> 	unsigned int data;
> 	unsigned int field;
> 	unsigned int toggle;
> 	unsigned int addr;
> 	unsigned int cmd;
> 
> 	data = readl(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;
> 
> 	rc_keydown(ir->rc, RC_TYPE_RC5, RC_SCANCODE_RC5(addr, cmd), toggle);
> }
> 
> static void tango_ir_handle_rc6(struct tango_ir *ir)
> {
> 	unsigned int data0, data1;
> 	unsigned int toggle;
> 	unsigned int mode;
> 	unsigned int addr;
> 	unsigned int cmd;
> 
> 	data0 = readl(ir->rc6_base + RC6_DATA0);
> 	data1 = readl(ir->rc6_base + RC6_DATA1);
> 
> 	mode = data0 >> 1 & 7;
> 
> 	if (mode != 0)
> 		return;
> 
> 	toggle = data0 & 1;
> 	addr = data0 >> 16;
> 	cmd = data1;
> 
> 	rc_keydown(ir->rc, RC_TYPE_RC6_0, RC_SCANCODE_RC6_0(addr, cmd), 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(ir->rc5_base + IR_INT);
> 	writel(rc5_stat, ir->rc5_base + IR_INT);
> 
> 	rc6_stat = readl(ir->rc6_base + RC6_CTRL);
> 	writel(rc6_stat, ir->rc6_base + RC6_CTRL);
> 
> 	if (!(rc5_stat & 3) && !(rc6_stat & BIT(31)))
> 		return IRQ_NONE;
> 
> 	if (rc5_stat & 1)
> 		tango_ir_handle_rc5(ir);
> 
> 	if (rc5_stat & 2)
> 		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_BIT_NEC)
> 		rc5_ctrl = 0;
> 
> 	if (*rc_type & RC_BIT_RC5)
> 		rc5_ctrl |= ENABLE_RC5;
> 
> 	if (*rc_type & RC_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 = &pdev->dev;
> 	struct rc_dev *rc;
> 	struct tango_ir *ir;
> 	struct resource *rc5_res;
> 	struct resource *rc6_res;
> 	unsigned long clkrate;
> 	u64 clkdiv;
> 	int irq;
> 	int 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);
> 
> 	err = clk_prepare_enable(ir->clk);
> 	if (err)
> 		return err;
> 
> 	clkrate = clk_get_rate(ir->clk);
> 
> 	rc = rc_allocate_device();

You could use devm_rc_allocate_device(RC_DRIVER_SCANCODE) here. Your
code is based on an old linux version, it won't compile against the
latest kernel.

> 	if (!rc) {
> 		err = -ENOMEM;
> 		goto err_clk;
> 	}
> 
> 	rc->dev.parent = dev;
> 	rc->input_name = "tango-ir";

This field has been renamed to device_name, please base your patch 
on linuxtv/master. If the device has a better name, that would be
nice rather than copying the driver name.

> 	rc->input_phys = "tango-ir/input0";
> 	rc->driver_name = "tango-ir";
> 	rc->map_name = RC_MAP_EMPTY;
> 	rc->driver_type = RC_DRIVER_SCANCODE;
> 	rc->allowed_protocols = RC_BIT_RC5 | RC_BIT_NEC | RC_BIT_RC6_0;

Since the driver can generated 24 bits NEC scancodes, it also supports
RC_BIT_NECX, and probably RC_BIT_NEC32 too.

> 	rc->change_protocol = tango_change_protocol;
> 
> 	rc->priv = ir;
> 	ir->rc = rc;
> 
> 	clkdiv = (u64)clkrate * NEC_TIME_BASE;

You could change the type of clkrate to u64 rather than casting it
each time it is used. Casts are to be avoided if it all possible.

> 	do_div(clkdiv, 1000000);
> 
> 	writel(31 << 24 | 12 << 16 | clkdiv, ir->rc5_base + IR_NEC_CTRL);
> 
> 	clkdiv = (u64)clkrate * RC5_TIME_BASE;
> 	do_div(clkdiv, 1000000);
> 
> 	writel(0x110, ir->rc5_base + IR_CTRL);
> 	writel(clkdiv, ir->rc5_base + IR_RC5_CLK_DIV);
> 	writel(0x3, ir->rc5_base + IR_INT);
> 
> 	clkdiv = (u64)clkrate * RC6_TIME_BASE;
> 	do_div(clkdiv, RC6_CARRIER);
> 
> 	writel(0xc0000000, ir->rc6_base + RC6_CTRL);
> 	writel((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_rc;
> 
> 	dev_info(dev, "SMP86xx IR decoder at 0x%x/0x%x IRQ %d\n",
> 		 rc5_res->start, rc6_res->start, irq);
> 
> 	err = rc_register_device(rc);

Use devm_rc_register_device()

> 	if (err)
> 		goto err_rc;
> 
> 	platform_set_drvdata(pdev, ir);
> 
> 	return 0;
> 
> err_rc:
> 	rc_free_device(rc);
> 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);
> 
> 	rc_unregister_device(ir->rc);
> 	rc_free_device(ir->rc);

rc_unregister_device() already calls rc_free_device(), so this is a double
free. However if you use devm_rc_allocate_device() and
devm_rc_register_device(), both these calls are no longer needed.

> 	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");
Mason Sept. 13, 2017, 2:03 p.m. UTC | #4 
On 12/09/2017 20:19, Sean Young wrote:

> It looks great, thanks! I have made some minor points below.

Thanks for having reviewed the driver! :-)

I have now fixed all the points you mentioned.

Changes from v1 to v2:

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 (not sure what value to use here)
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

TODO: test RC-5 and RC-6 (I need to locate proper remote)


/*
  * 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->device_name = "tango-ir"; /* not sure what else to use here??? */
	rc->input_phys = "tango-ir/input0";
	rc->driver_name = "tango-ir";
	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");
Sean Young Sept. 13, 2017, 2:57 p.m. UTC | #5 
Hi,

On Wed, Sep 13, 2017 at 04:03:43PM +0200, Mason wrote:
> On 12/09/2017 20:19, Sean Young wrote:
> 
> > It looks great, thanks! I have made some minor points below.
> 
> Thanks for having reviewed the driver! :-)
> 
> I have now fixed all the points you mentioned.
> 
> Changes from v1 to v2:
> 
> 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 (not sure what value to use here)
> 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
> 
> TODO: test RC-5 and RC-6 (I need to locate proper remote)

You could get a IR transmitter (e.g. raspberry pi + IR led + resistor) or
some of the mceusb devices, and then you can use the ir-ctl tool to
test all the different protocols, including extended rc5 and the other
rc6 variants.

But I don't think we need to block merging because these protocols haven't
been tested. It would be nice though.

> 
> 
> /*
>  * 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->device_name = "tango-ir"; /* not sure what else to use here??? */
> 	rc->input_phys = "tango-ir/input0";
> 	rc->driver_name = "tango-ir";
> 	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");

Looks great, thanks.


Sean
Mason Sept. 13, 2017, 3:34 p.m. UTC | #6 
On 13/09/2017 16:57, Sean Young wrote:

> On Sep 13, 2017 at 16:03, Mason wrote:
>
>> Changes from v1 to v2:
>>
>> 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 (not sure what value to use here)
>> 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
>>
>> TODO: test RC-5 and RC-6 (I need to locate proper remote)
> 
> You could get a IR transmitter (e.g. raspberry pi + IR led + resistor) or
> some of the mceusb devices, and then you can use the ir-ctl tool to
> test all the different protocols, including extended rc5 and the other
> rc6 variants.

Thanks for the suggestions.

I do have a box full of remote controls, and I'm hoping some of
them are RC-5 and RC-6. (Someone told me there is a Sony decoder
in the chip, but I have found no documentation whatsoever regarding
that feature!)

There is an IR transmitter on the board, but I have no driver for
it, only a custom test app. So that doesn't help me for ir-ctl...
I don't know how much time a driver would require.

> But I don't think we need to block merging because these protocols haven't
> been tested. It would be nice though.

I'll try my best to test the driver thoroughly. And then I'll
send a formal patch.

Regards.
diff mbox

Patch

--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -687,6 +687,10 @@  void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 togg
         unsigned long flags;
         u32 keycode = rc_g_keycode_from_table(dev, scancode);
  
+       printk("%s: keycode=%x\n", __func__, keycode);
+       if (keycode == KEY_RESERVED)
+               keycode = KEY_UNKNOWN;
+
         spin_lock_irqsave(&dev->keylock, flags);
         ir_do_keydown(dev, protocol, scancode, keycode, toggle);