[PATCH 0/2] media: rc: add support for IR receiver on MT7623 SoC

[PATCH 0/2] media: rc: add support for IR receiver on MT7623 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This patchset introduces Consumer IR (CIR) support for MT7623 SoC 
or other similar SoC and implements raw mode for more compatibility
with different protocols. The driver simply reports the duration of 
pulses and spaces to rc core logic to decode.

Sean Wang (2):
  Documentation: devicetree: Add document bindings for mtk-cir
  media: rc: add driver for IR remote receiver on MT7623 SoC

 .../devicetree/bindings/media/mtk-cir.txt          |  23 ++
 drivers/media/rc/Kconfig                           |  10 +
 drivers/media/rc/Makefile                          |   1 +
 drivers/media/rc/mtk-cir.c                         | 319 +++++++++++++++++++++
 4 files changed, 353 insertions(+)
 create mode 100644 linux-4.8.rc1_p0/Documentation/devicetree/bindings/media/mtk-cir.txt
 create mode 100644 linux-4.8.rc1_p0/drivers/media/rc/mtk-cir.c

-- 
1.9.1

[PATCH 1/2] Documentation: devicetree: Add document bindings for mtk-cir

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This patch adds documentation for devicetree bindings for
Mediatek IR controller.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/media/mtk-cir.txt          | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 linux-4.8.rc1_p0/Documentation/devicetree/bindings/media/mtk-cir.txt

diff --git a/Documentation/devicetree/bindings/media/mtk-cir.txt b/Documentation/devicetree/bindings/media/mtk-cir.txt
new file mode 100644
index 0000000..bbedd71
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mtk-cir.txt
@@ -0,0 +1,23 @@
+Device-Tree bindings for Mediatek IR controller found in Mediatek SoC family
+
+Required properties:
+- compatible	    : "mediatek,mt7623-ir"
+- clocks	    : list of clock specifiers, corresponding to
+		      entries in clock-names property;
+- clock-names	    : should contain "clk" entries;
+- interrupts	    : should contain IR IRQ number;
+- reg		    : should contain IO map address for IR.
+
+Optional properties:
+- linux,rc-map-name : Remote control map name.
+
+Example:
+
+cir: cir@0x10013000 {
+	compatible = "mediatek,mt7623-ir";
+	reg = <0 0x10013000 0 0x1000>;
+	interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_LOW>;
+	clocks = <&infracfg CLK_INFRA_IRRX>;
+	clock-names = "clk";
+	linux,rc-map-name = "rc-rc6-mce";
+};
-- 
1.9.1

[PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This patch adds driver for IR controller on
Mediatek MT7623 SoC. Currently testing successfully
on NEC and SONY remote controller only but it should
work on others (lirc, rc-5 and rc-6).

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/media/rc/Kconfig   |  10 ++
 drivers/media/rc/Makefile  |   1 +
 drivers/media/rc/mtk-cir.c | 319 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 linux-4.8.rc1_p0/drivers/media/rc/mtk-cir.c

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 370e16e..626c500 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -389,4 +389,14 @@ config IR_SUNXI
 	   To compile this driver as a module, choose M here: the module will
 	   be called sunxi-ir.
 
+config IR_MTK
+	tristate "Mediatek IR remote control"
+	depends on RC_CORE
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	---help---
+	   Say Y if you want to use Mediatek internal IR Controller
+
+	   To compile this driver as a module, choose M here: the module will
+	   be called mtk-cir.
+
 endif #RC_DEVICES
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 379a5c0..505908d 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
 obj-$(CONFIG_RC_ST) += st_rc.o
 obj-$(CONFIG_IR_SUNXI) += sunxi-cir.o
 obj-$(CONFIG_IR_IMG) += img-ir/
+obj-$(CONFIG_IR_MTK) += mtk-cir.o
diff --git a/drivers/media/rc/mtk-cir.c b/drivers/media/rc/mtk-cir.c
new file mode 100644
index 0000000..4fa4cab
--- /dev/null
+++ b/drivers/media/rc/mtk-cir.c
@@ -0,0 +1,319 @@
+/*
+ * Driver for Mediatek MT7623 IR Receiver Controller
+ *
+ * Copyright (C) 2017 Sean Wang <sean.wang@mediatek.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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+#include <media/rc-core.h>
+
+#define MTK_IR_DEV "mtk-ir"
+
+/* Register to enable PWM and IR */
+#define MTK_CONFIG_HIGH_REG       0x0c
+/* Enable IR pulse width detection */
+#define MTK_PWM_EN		  BIT(13)
+/* Enable IR hardware function */
+#define MTK_IR_EN		  BIT(0)
+
+/* Register to setting sample period */
+#define MTK_CONFIG_LOW_REG        0x10
+/* Field to set sample period */
+#define CHK_PERIOD		  0xC00
+#define MTK_CHK_PERIOD            (((CHK_PERIOD) << 8) & (GENMASK(20, 8)))
+#define MTK_CHK_PERIOD_MASK	  (GENMASK(20, 8))
+
+/* Register to clear state of state machine */
+#define MTK_IRCLR_REG             0x20
+/* Bit to restart IR receiving */
+#define MTK_IRCLR		  BIT(0)
+
+/* Register containing pulse width data */
+#define MTK_CHKDATA_REG(i)        (0x88 + 4 * i)
+
+/* Register to enable IR interrupt */
+#define MTK_IRINT_EN_REG          0xcc
+/* Bit to enable interrupt */
+#define MTK_IRINT_EN		  BIT(0)
+
+/* Register to ack IR interrupt */
+#define MTK_IRINT_CLR_REG         0xd0
+/* Bit to clear interrupt status */
+#define MTK_IRINT_CLR		  BIT(0)
+
+/* Number of registers to record the pulse width */
+#define MTK_CHKDATA_SZ		  17
+/* Required frequency */
+#define MTK_IR_BASE_CLK		  273000000
+/* Frequency after IR internal divider */
+#define MTK_IR_CLK		  (MTK_IR_BASE_CLK / 4)
+/* Sample period in ns */
+#define MTK_IR_SAMPLE		  (((1000000000ul / MTK_IR_CLK) * CHK_PERIOD))
+/* Indicate the end of IR data*/
+#define MTK_IR_END(v)		  (v == 0xff)
+
+/* struct mtk_ir -	This is the main datasructure for holding the state
+ *			of the driver
+ * @dev:		The device pointer
+ * @ir_lock:		Make sure that synchronization between remove and ISR
+ * @rc:			The rc instrance
+ * @base:		The mapped register i/o base
+ * @irq:		The IRQ that we are using
+ * @clk:		The clock that we are using
+ * @map_name:		The name for keymap lookup
+ */
+struct mtk_ir {
+	struct device   *dev;
+	/*Protect concurrency between driver removal and ISR*/
+	spinlock_t      ir_lock;
+	struct rc_dev   *rc;
+	void __iomem    *base;
+	int             irq;
+	struct clk      *clk;
+	const char      *map_name;
+};
+
+static void mtk_w32_mask(struct mtk_ir *ir, u32 val, u32 mask, unsigned int reg)
+{
+	u32 tmp;
+
+	tmp = __raw_readl(ir->base + reg);
+	tmp = (tmp & ~mask) | val;
+	__raw_writel(tmp, ir->base + reg);
+}
+
+static void mtk_w32(struct mtk_ir *ir, u32 val, unsigned int reg)
+{
+	__raw_writel(val, ir->base + reg);
+}
+
+static u32 mtk_r32(struct mtk_ir *ir, unsigned int reg)
+{
+	return __raw_readl(ir->base + reg);
+}
+
+static inline void mtk_irq_disable(struct mtk_ir *ir, u32 mask)
+{
+	u32 val;
+
+	val = mtk_r32(ir, MTK_IRINT_EN_REG);
+	mtk_w32(ir, val & ~mask, MTK_IRINT_EN_REG);
+}
+
+static inline void mtk_irq_enable(struct mtk_ir *ir, u32 mask)
+{
+	u32 val;
+
+	val = mtk_r32(ir, MTK_IRINT_EN_REG);
+	mtk_w32(ir, val | mask, MTK_IRINT_EN_REG);
+}
+
+static irqreturn_t mtk_ir_irq(int irqno, void *dev_id)
+{
+	struct mtk_ir *ir = dev_id;
+	u8  wid = 0;
+	u32 i, j, val;
+	DEFINE_IR_RAW_EVENT(rawir);
+
+	spin_lock(&ir->ir_lock);
+
+	mtk_irq_disable(ir, MTK_IRINT_EN);
+
+	/* Reset decoder state machine */
+	ir_raw_event_reset(ir->rc);
+
+	/* First message must be pulse */
+	rawir.pulse = false;
+
+	/* Handle pulse and space until end of message */
+	for (i = 0 ; i < MTK_CHKDATA_SZ ; i++) {
+		val = mtk_r32(ir, MTK_CHKDATA_REG(i));
+		dev_dbg(ir->dev, "@reg%d=0x%08x\n", i, val);
+
+		for (j = 0 ; j < 4 ; j++) {
+			wid = (val & (0xff << j * 8)) >> j * 8;
+			rawir.pulse = !rawir.pulse;
+			rawir.duration = wid * (MTK_IR_SAMPLE + 1);
+			ir_raw_event_store_with_filter(ir->rc, &rawir);
+
+			if (MTK_IR_END(wid))
+				goto end_msg;
+		}
+	}
+end_msg:
+	/* Restart the next receive */
+	mtk_w32_mask(ir, 0x1, MTK_IRCLR, MTK_IRCLR_REG);
+
+	ir_raw_event_set_idle(ir->rc, true);
+	ir_raw_event_handle(ir->rc);
+
+	/* Clear interrupt status */
+	mtk_w32_mask(ir, 0x1, MTK_IRINT_CLR, MTK_IRINT_CLR_REG);
+
+	/* Enable interrupt */
+	mtk_irq_enable(ir, MTK_IRINT_EN);
+
+	spin_unlock(&ir->ir_lock);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_ir_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dn = dev->of_node;
+	struct resource *res;
+	struct mtk_ir *ir;
+	u32 val;
+	int ret = 0;
+
+	ir = devm_kzalloc(dev, sizeof(struct mtk_ir), GFP_KERNEL);
+	if (!ir)
+		return -ENOMEM;
+
+	spin_lock_init(&ir->ir_lock);
+
+	ir->dev = dev;
+
+	if (!of_device_is_compatible(dn, "mediatek,mt7623-ir"))
+		return -ENODEV;
+
+	ir->clk = devm_clk_get(dev, "clk");
+	if (IS_ERR(ir->clk)) {
+		dev_err(dev, "failed to get a ir clock.\n");
+		return PTR_ERR(ir->clk);
+	}
+
+	if (clk_prepare_enable(ir->clk)) {
+		dev_err(dev, "try to enable ir_clk failed\n");
+		ret = -EINVAL;
+		goto exit_clkdisable_clk;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ir->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ir->base)) {
+		dev_err(dev, "failed to map registers\n");
+		ret = PTR_ERR(ir->base);
+		goto exit_clkdisable_clk;
+	}
+
+	ir->rc = rc_allocate_device();
+	if (!ir->rc) {
+		dev_err(dev, "failed to allocate device\n");
+		ret = -ENOMEM;
+		goto exit_clkdisable_clk;
+	}
+
+	ir->rc->priv = ir;
+	ir->rc->input_name = MTK_IR_DEV;
+	ir->rc->input_phys = "mtk-ir/input0";
+	ir->rc->input_id.bustype = BUS_HOST;
+	ir->rc->input_id.vendor = 0x0001;
+	ir->rc->input_id.product = 0x0001;
+	ir->rc->input_id.version = 0x0001;
+	ir->map_name = of_get_property(dn, "linux,rc-map-name", NULL);
+	ir->rc->map_name = ir->map_name ?: RC_MAP_EMPTY;
+	ir->rc->dev.parent = dev;
+	ir->rc->driver_type = RC_DRIVER_IR_RAW;
+	ir->rc->driver_name = MTK_IR_DEV;
+	ir->rc->allowed_protocols = RC_BIT_ALL;
+	ir->rc->rx_resolution = MTK_IR_SAMPLE;
+
+	ret = rc_register_device(ir->rc);
+	if (ret) {
+		dev_err(dev, "failed to register rc device\n");
+		goto exit_free_dev;
+	}
+
+	platform_set_drvdata(pdev, ir);
+
+	ir->irq = platform_get_irq(pdev, 0);
+	if (ir->irq < 0) {
+		dev_err(dev, "no irq resource\n");
+		ret = ir->irq;
+		goto exit_free_dev;
+	}
+
+	ret = devm_request_irq(dev, ir->irq, mtk_ir_irq, 0, MTK_IR_DEV, ir);
+	if (ret) {
+		dev_err(dev, "failed request irq\n");
+		goto exit_free_dev;
+	}
+
+	mtk_irq_disable(ir, MTK_IRINT_EN);
+
+	val = mtk_r32(ir, MTK_CONFIG_HIGH_REG);
+	val |= MTK_PWM_EN | MTK_IR_EN;
+	mtk_w32(ir, val, MTK_CONFIG_HIGH_REG);
+
+	/* Setting sample period */
+	mtk_w32_mask(ir, MTK_CHK_PERIOD, MTK_CHK_PERIOD_MASK,
+		     MTK_CONFIG_LOW_REG);
+
+	mtk_irq_enable(ir, MTK_IRINT_EN);
+
+	dev_info(dev, "initialized MT7623 IR driver\n");
+	return 0;
+
+exit_free_dev:
+	rc_free_device(ir->rc);
+exit_clkdisable_clk:
+	clk_disable_unprepare(ir->clk);
+
+	return ret;
+}
+
+static int mtk_ir_remove(struct platform_device *pdev)
+{
+	unsigned long flags;
+
+	struct mtk_ir *ir = platform_get_drvdata(pdev);
+
+	spin_lock_irqsave(&ir->ir_lock, flags);
+
+	mtk_irq_disable(ir, MTK_IRINT_EN);
+
+	clk_disable_unprepare(ir->clk);
+
+	spin_unlock_irqrestore(&ir->ir_lock, flags);
+
+	rc_unregister_device(ir->rc);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_ir_match[] = {
+	{ .compatible = "mediatek,mt7623-ir" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_ir_match);
+
+static struct platform_driver mtk_ir_driver = {
+	.probe          = mtk_ir_probe,
+	.remove         = mtk_ir_remove,
+	.driver = {
+		.name = MTK_IR_DEV,
+		.of_match_table = mtk_ir_match,
+	},
+};
+
+module_platform_driver(mtk_ir_driver);
+
+MODULE_DESCRIPTION("Mediatek MT7623 IR Receiver Controller Driver");
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Young]

Hi Sean,

Some review comments.

On Fri, Jan 06, 2017 at 12:06:24AM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> This patch adds driver for IR controller on
> Mediatek MT7623 SoC. Currently testing successfully
> on NEC and SONY remote controller only but it should
> work on others (lirc, rc-5 and rc-6).
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/media/rc/Kconfig   |  10 ++
>  drivers/media/rc/Makefile  |   1 +
>  drivers/media/rc/mtk-cir.c | 319 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 330 insertions(+)
>  create mode 100644 linux-4.8.rc1_p0/drivers/media/rc/mtk-cir.c
> 
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 370e16e..626c500 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -389,4 +389,14 @@ config IR_SUNXI
>  	   To compile this driver as a module, choose M here: the module will
>  	   be called sunxi-ir.
>  
> +config IR_MTK
> +	tristate "Mediatek IR remote control"
> +	depends on RC_CORE
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	---help---
> +	   Say Y if you want to use Mediatek internal IR Controller
> +
> +	   To compile this driver as a module, choose M here: the module will
> +	   be called mtk-cir.
> +
>  endif #RC_DEVICES
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index 379a5c0..505908d 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
>  obj-$(CONFIG_RC_ST) += st_rc.o
>  obj-$(CONFIG_IR_SUNXI) += sunxi-cir.o
>  obj-$(CONFIG_IR_IMG) += img-ir/
> +obj-$(CONFIG_IR_MTK) += mtk-cir.o
> diff --git a/drivers/media/rc/mtk-cir.c b/drivers/media/rc/mtk-cir.c
> new file mode 100644
> index 0000000..4fa4cab
> --- /dev/null
> +++ b/drivers/media/rc/mtk-cir.c
> @@ -0,0 +1,319 @@
> +/*
> + * Driver for Mediatek MT7623 IR Receiver Controller
> + *
> + * Copyright (C) 2017 Sean Wang <sean.wang@mediatek.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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/reset.h>
> +#include <media/rc-core.h>
> +
> +#define MTK_IR_DEV "mtk-ir"

KBUILD_MODNAME could be used instead. Currently the module is called
mtk-cir but the rc device will have driver name mtk-ir.

> +
> +/* Register to enable PWM and IR */
> +#define MTK_CONFIG_HIGH_REG       0x0c
> +/* Enable IR pulse width detection */
> +#define MTK_PWM_EN		  BIT(13)
> +/* Enable IR hardware function */
> +#define MTK_IR_EN		  BIT(0)
> +
> +/* Register to setting sample period */
> +#define MTK_CONFIG_LOW_REG        0x10
> +/* Field to set sample period */
> +#define CHK_PERIOD		  0xC00
> +#define MTK_CHK_PERIOD            (((CHK_PERIOD) << 8) & (GENMASK(20, 8)))
> +#define MTK_CHK_PERIOD_MASK	  (GENMASK(20, 8))
> +
> +/* Register to clear state of state machine */
> +#define MTK_IRCLR_REG             0x20
> +/* Bit to restart IR receiving */
> +#define MTK_IRCLR		  BIT(0)
> +
> +/* Register containing pulse width data */
> +#define MTK_CHKDATA_REG(i)        (0x88 + 4 * i)
> +
> +/* Register to enable IR interrupt */
> +#define MTK_IRINT_EN_REG          0xcc
> +/* Bit to enable interrupt */
> +#define MTK_IRINT_EN		  BIT(0)
> +
> +/* Register to ack IR interrupt */
> +#define MTK_IRINT_CLR_REG         0xd0
> +/* Bit to clear interrupt status */
> +#define MTK_IRINT_CLR		  BIT(0)
> +
> +/* Number of registers to record the pulse width */
> +#define MTK_CHKDATA_SZ		  17
> +/* Required frequency */
> +#define MTK_IR_BASE_CLK		  273000000
> +/* Frequency after IR internal divider */
> +#define MTK_IR_CLK		  (MTK_IR_BASE_CLK / 4)
> +/* Sample period in ns */
> +#define MTK_IR_SAMPLE		  (((1000000000ul / MTK_IR_CLK) * CHK_PERIOD))
> +/* Indicate the end of IR data*/
> +#define MTK_IR_END(v)		  (v == 0xff)
> +
> +/* struct mtk_ir -	This is the main datasructure for holding the state
> + *			of the driver
> + * @dev:		The device pointer
> + * @ir_lock:		Make sure that synchronization between remove and ISR
> + * @rc:			The rc instrance
> + * @base:		The mapped register i/o base
> + * @irq:		The IRQ that we are using
> + * @clk:		The clock that we are using
> + * @map_name:		The name for keymap lookup
> + */
> +struct mtk_ir {
> +	struct device   *dev;
> +	/*Protect concurrency between driver removal and ISR*/
> +	spinlock_t      ir_lock;
> +	struct rc_dev   *rc;
> +	void __iomem    *base;
> +	int             irq;
> +	struct clk      *clk;
> +	const char      *map_name;

irq and map_name don't need to be stored here, they're only used in 
mtk_ir_probe.

> +};
> +
> +static void mtk_w32_mask(struct mtk_ir *ir, u32 val, u32 mask, unsigned int reg)
> +{
> +	u32 tmp;
> +
> +	tmp = __raw_readl(ir->base + reg);
> +	tmp = (tmp & ~mask) | val;
> +	__raw_writel(tmp, ir->base + reg);
> +}
> +
> +static void mtk_w32(struct mtk_ir *ir, u32 val, unsigned int reg)
> +{
> +	__raw_writel(val, ir->base + reg);
> +}
> +
> +static u32 mtk_r32(struct mtk_ir *ir, unsigned int reg)
> +{
> +	return __raw_readl(ir->base + reg);
> +}
> +
> +static inline void mtk_irq_disable(struct mtk_ir *ir, u32 mask)
> +{
> +	u32 val;
> +
> +	val = mtk_r32(ir, MTK_IRINT_EN_REG);
> +	mtk_w32(ir, val & ~mask, MTK_IRINT_EN_REG);
> +}
> +
> +static inline void mtk_irq_enable(struct mtk_ir *ir, u32 mask)
> +{
> +	u32 val;
> +
> +	val = mtk_r32(ir, MTK_IRINT_EN_REG);
> +	mtk_w32(ir, val | mask, MTK_IRINT_EN_REG);
> +}
> +
> +static irqreturn_t mtk_ir_irq(int irqno, void *dev_id)
> +{
> +	struct mtk_ir *ir = dev_id;
> +	u8  wid = 0;
> +	u32 i, j, val;
> +	DEFINE_IR_RAW_EVENT(rawir);
> +
> +	spin_lock(&ir->ir_lock);
> +
> +	mtk_irq_disable(ir, MTK_IRINT_EN);
> +
> +	/* Reset decoder state machine */
> +	ir_raw_event_reset(ir->rc);
> +
> +	/* First message must be pulse */
> +	rawir.pulse = false;
> +
> +	/* Handle pulse and space until end of message */
> +	for (i = 0 ; i < MTK_CHKDATA_SZ ; i++) {
> +		val = mtk_r32(ir, MTK_CHKDATA_REG(i));
> +		dev_dbg(ir->dev, "@reg%d=0x%08x\n", i, val);
> +
> +		for (j = 0 ; j < 4 ; j++) {
> +			wid = (val & (0xff << j * 8)) >> j * 8;
> +			rawir.pulse = !rawir.pulse;
> +			rawir.duration = wid * (MTK_IR_SAMPLE + 1);
> +			ir_raw_event_store_with_filter(ir->rc, &rawir);
> +
> +			if (MTK_IR_END(wid))
> +				goto end_msg;
> +		}
> +	}

If I read this correctly, there is a maximum of 17 * 4 = 68 edges per
IR message. The rc6 mce key 0 (scancode 0x800f0400) is 69 edges, so that
won't work.

> +end_msg:
> +	/* Restart the next receive */
> +	mtk_w32_mask(ir, 0x1, MTK_IRCLR, MTK_IRCLR_REG);
> +
> +	ir_raw_event_set_idle(ir->rc, true);
> +	ir_raw_event_handle(ir->rc);
> +
> +	/* Clear interrupt status */
> +	mtk_w32_mask(ir, 0x1, MTK_IRINT_CLR, MTK_IRINT_CLR_REG);
> +
> +	/* Enable interrupt */
> +	mtk_irq_enable(ir, MTK_IRINT_EN);
> +
> +	spin_unlock(&ir->ir_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_ir_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *dn = dev->of_node;
> +	struct resource *res;
> +	struct mtk_ir *ir;
> +	u32 val;
> +	int ret = 0;
> +
> +	ir = devm_kzalloc(dev, sizeof(struct mtk_ir), GFP_KERNEL);
> +	if (!ir)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&ir->ir_lock);
> +
> +	ir->dev = dev;
> +
> +	if (!of_device_is_compatible(dn, "mediatek,mt7623-ir"))
> +		return -ENODEV;
> +
> +	ir->clk = devm_clk_get(dev, "clk");
> +	if (IS_ERR(ir->clk)) {
> +		dev_err(dev, "failed to get a ir clock.\n");
> +		return PTR_ERR(ir->clk);
> +	}
> +
> +	if (clk_prepare_enable(ir->clk)) {
> +		dev_err(dev, "try to enable ir_clk failed\n");
> +		ret = -EINVAL;
> +		goto exit_clkdisable_clk;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ir->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(ir->base)) {
> +		dev_err(dev, "failed to map registers\n");
> +		ret = PTR_ERR(ir->base);
> +		goto exit_clkdisable_clk;
> +	}
> +
> +	ir->rc = rc_allocate_device();
> +	if (!ir->rc) {
> +		dev_err(dev, "failed to allocate device\n");
> +		ret = -ENOMEM;
> +		goto exit_clkdisable_clk;
> +	}
> +
> +	ir->rc->priv = ir;
> +	ir->rc->input_name = MTK_IR_DEV;
> +	ir->rc->input_phys = "mtk-ir/input0";
> +	ir->rc->input_id.bustype = BUS_HOST;
> +	ir->rc->input_id.vendor = 0x0001;
> +	ir->rc->input_id.product = 0x0001;
> +	ir->rc->input_id.version = 0x0001;
> +	ir->map_name = of_get_property(dn, "linux,rc-map-name", NULL);
> +	ir->rc->map_name = ir->map_name ?: RC_MAP_EMPTY;
> +	ir->rc->dev.parent = dev;
> +	ir->rc->driver_type = RC_DRIVER_IR_RAW;
> +	ir->rc->driver_name = MTK_IR_DEV;
> +	ir->rc->allowed_protocols = RC_BIT_ALL;
> +	ir->rc->rx_resolution = MTK_IR_SAMPLE;
> +
> +	ret = rc_register_device(ir->rc);
> +	if (ret) {
> +		dev_err(dev, "failed to register rc device\n");
> +		goto exit_free_dev;
> +	}
> +
> +	platform_set_drvdata(pdev, ir);
> +
> +	ir->irq = platform_get_irq(pdev, 0);
> +	if (ir->irq < 0) {
> +		dev_err(dev, "no irq resource\n");
> +		ret = ir->irq;

>From here on onwards the errors paths should call rc_unregister_device(),
and no longer call rc_free_device(). Note that current master has
devm_rc_allocate_device() and devm_rc_register_device() which would
simplify this code.

> +		goto exit_free_dev;
> +	}
> +
> +	ret = devm_request_irq(dev, ir->irq, mtk_ir_irq, 0, MTK_IR_DEV, ir);
> +	if (ret) {
> +		dev_err(dev, "failed request irq\n");
> +		goto exit_free_dev;
> +	}
> +
> +	mtk_irq_disable(ir, MTK_IRINT_EN);
> +
> +	val = mtk_r32(ir, MTK_CONFIG_HIGH_REG);
> +	val |= MTK_PWM_EN | MTK_IR_EN;
> +	mtk_w32(ir, val, MTK_CONFIG_HIGH_REG);
> +
> +	/* Setting sample period */
> +	mtk_w32_mask(ir, MTK_CHK_PERIOD, MTK_CHK_PERIOD_MASK,
> +		     MTK_CONFIG_LOW_REG);
> +
> +	mtk_irq_enable(ir, MTK_IRINT_EN);
> +
> +	dev_info(dev, "initialized MT7623 IR driver\n");
> +	return 0;
> +
> +exit_free_dev:
> +	rc_free_device(ir->rc);
> +exit_clkdisable_clk:
> +	clk_disable_unprepare(ir->clk);
> +
> +	return ret;
> +}
> +
> +static int mtk_ir_remove(struct platform_device *pdev)
> +{
> +	unsigned long flags;
> +
> +	struct mtk_ir *ir = platform_get_drvdata(pdev);
> +
> +	spin_lock_irqsave(&ir->ir_lock, flags);
> +
> +	mtk_irq_disable(ir, MTK_IRINT_EN);
> +
> +	clk_disable_unprepare(ir->clk);
> +
> +	spin_unlock_irqrestore(&ir->ir_lock, flags);

I'm not convinced the ir_lock is helping to prevent any race condition. An
irq might still already have occurred which will now try to use ir->rc 
which is freed. You can remove the spinlock completely and call 
sychronize_irq() after disabling the mtk interrupt. That way you're sure
the remove is safe to complete.

> +
> +	rc_unregister_device(ir->rc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_ir_match[] = {
> +	{ .compatible = "mediatek,mt7623-ir" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_ir_match);
> +
> +static struct platform_driver mtk_ir_driver = {
> +	.probe          = mtk_ir_probe,
> +	.remove         = mtk_ir_remove,
> +	.driver = {
> +		.name = MTK_IR_DEV,
> +		.of_match_table = mtk_ir_match,
> +	},
> +};
> +
> +module_platform_driver(mtk_ir_driver);
> +
> +MODULE_DESCRIPTION("Mediatek MT7623 IR Receiver Controller Driver");
> +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Andi Shyti]

Hi Sean,

> +	ir->rc = rc_allocate_device();

Yes, you should use devm_rc_allocate_device(...)

Besides, standing to this patch which is not in yet:

https://lkml.org/lkml/2016/12/18/39

rc_allocate_device should provide the driver type during
allocation, so it should be:

	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);

and this line can be removed:

> +	ir->rc->driver_type = RC_DRIVER_IR_RAW;

I don't know when Mauro will take the patch above.

Andi

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Wang]

Hi Sean,

Thanks for your effort for code reviewing. I add comments inline. 

On Thu, 2017-01-05 at 17:12 +0000, Sean Young wrote:
> Hi Sean,
> 
> Some review comments.
> 
> On Fri, Jan 06, 2017 at 12:06:24AM +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > This patch adds driver for IR controller on
> > Mediatek MT7623 SoC. Currently testing successfully
> > on NEC and SONY remote controller only but it should
> > work on others (lirc, rc-5 and rc-6).
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/media/rc/Kconfig   |  10 ++
> >  drivers/media/rc/Makefile  |   1 +
> >  drivers/media/rc/mtk-cir.c | 319 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 330 insertions(+)
> >  create mode 100644 linux-4.8.rc1_p0/drivers/media/rc/mtk-cir.c
> > 
> > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> > index 370e16e..626c500 100644
> > --- a/drivers/media/rc/Kconfig
> > +++ b/drivers/media/rc/Kconfig
> > @@ -389,4 +389,14 @@ config IR_SUNXI
> >  	   To compile this driver as a module, choose M here: the module will
> >  	   be called sunxi-ir.
> >  
> > +config IR_MTK
> > +	tristate "Mediatek IR remote control"
> > +	depends on RC_CORE
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	---help---
> > +	   Say Y if you want to use Mediatek internal IR Controller
> > +
> > +	   To compile this driver as a module, choose M here: the module will
> > +	   be called mtk-cir.
> > +
> >  endif #RC_DEVICES
> > diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> > index 379a5c0..505908d 100644
> > --- a/drivers/media/rc/Makefile
> > +++ b/drivers/media/rc/Makefile
> > @@ -37,3 +37,4 @@ obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
> >  obj-$(CONFIG_RC_ST) += st_rc.o
> >  obj-$(CONFIG_IR_SUNXI) += sunxi-cir.o
> >  obj-$(CONFIG_IR_IMG) += img-ir/
> > +obj-$(CONFIG_IR_MTK) += mtk-cir.o
> > diff --git a/drivers/media/rc/mtk-cir.c b/drivers/media/rc/mtk-cir.c
> > new file mode 100644
> > index 0000000..4fa4cab
> > --- /dev/null
> > +++ b/drivers/media/rc/mtk-cir.c
> > @@ -0,0 +1,319 @@
> > +/*
> > + * Driver for Mediatek MT7623 IR Receiver Controller
> > + *
> > + * Copyright (C) 2017 Sean Wang <sean.wang@mediatek.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.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/reset.h>
> > +#include <media/rc-core.h>
> > +
> > +#define MTK_IR_DEV "mtk-ir"
> 
> KBUILD_MODNAME could be used instead. Currently the module is called
> mtk-cir but the rc device will have driver name mtk-ir.


okay. i will use this instead of this insistent coding.

> > +
> > +/* Register to enable PWM and IR */
> > +#define MTK_CONFIG_HIGH_REG       0x0c
> > +/* Enable IR pulse width detection */
> > +#define MTK_PWM_EN		  BIT(13)
> > +/* Enable IR hardware function */
> > +#define MTK_IR_EN		  BIT(0)
> > +
> > +/* Register to setting sample period */
> > +#define MTK_CONFIG_LOW_REG        0x10
> > +/* Field to set sample period */
> > +#define CHK_PERIOD		  0xC00
> > +#define MTK_CHK_PERIOD            (((CHK_PERIOD) << 8) & (GENMASK(20, 8)))
> > +#define MTK_CHK_PERIOD_MASK	  (GENMASK(20, 8))
> > +
> > +/* Register to clear state of state machine */
> > +#define MTK_IRCLR_REG             0x20
> > +/* Bit to restart IR receiving */
> > +#define MTK_IRCLR		  BIT(0)
> > +
> > +/* Register containing pulse width data */
> > +#define MTK_CHKDATA_REG(i)        (0x88 + 4 * i)
> > +
> > +/* Register to enable IR interrupt */
> > +#define MTK_IRINT_EN_REG          0xcc
> > +/* Bit to enable interrupt */
> > +#define MTK_IRINT_EN		  BIT(0)
> > +
> > +/* Register to ack IR interrupt */
> > +#define MTK_IRINT_CLR_REG         0xd0
> > +/* Bit to clear interrupt status */
> > +#define MTK_IRINT_CLR		  BIT(0)
> > +
> > +/* Number of registers to record the pulse width */
> > +#define MTK_CHKDATA_SZ		  17
> > +/* Required frequency */
> > +#define MTK_IR_BASE_CLK		  273000000
> > +/* Frequency after IR internal divider */
> > +#define MTK_IR_CLK		  (MTK_IR_BASE_CLK / 4)
> > +/* Sample period in ns */
> > +#define MTK_IR_SAMPLE		  (((1000000000ul / MTK_IR_CLK) * CHK_PERIOD))
> > +/* Indicate the end of IR data*/
> > +#define MTK_IR_END(v)		  (v == 0xff)
> > +
> > +/* struct mtk_ir -	This is the main datasructure for holding the state
> > + *			of the driver
> > + * @dev:		The device pointer
> > + * @ir_lock:		Make sure that synchronization between remove and ISR
> > + * @rc:			The rc instrance
> > + * @base:		The mapped register i/o base
> > + * @irq:		The IRQ that we are using
> > + * @clk:		The clock that we are using
> > + * @map_name:		The name for keymap lookup
> > + */
> > +struct mtk_ir {
> > +	struct device   *dev;
> > +	/*Protect concurrency between driver removal and ISR*/
> > +	spinlock_t      ir_lock;
> > +	struct rc_dev   *rc;
> > +	void __iomem    *base;
> > +	int             irq;
> > +	struct clk      *clk;
> > +	const char      *map_name;
> 
> irq and map_name don't need to be stored here, they're only used in 
> mtk_ir_probe.
> 

I will remove map_name , but keep irq for synchronize_irq call needs.

> > +};
> > +
> > +static void mtk_w32_mask(struct mtk_ir *ir, u32 val, u32 mask, unsigned int reg)
> > +{
> > +	u32 tmp;
> > +
> > +	tmp = __raw_readl(ir->base + reg);
> > +	tmp = (tmp & ~mask) | val;
> > +	__raw_writel(tmp, ir->base + reg);
> > +}
> > +
> > +static void mtk_w32(struct mtk_ir *ir, u32 val, unsigned int reg)
> > +{
> > +	__raw_writel(val, ir->base + reg);
> > +}
> > +
> > +static u32 mtk_r32(struct mtk_ir *ir, unsigned int reg)
> > +{
> > +	return __raw_readl(ir->base + reg);
> > +}
> > +
> > +static inline void mtk_irq_disable(struct mtk_ir *ir, u32 mask)
> > +{
> > +	u32 val;
> > +
> > +	val = mtk_r32(ir, MTK_IRINT_EN_REG);
> > +	mtk_w32(ir, val & ~mask, MTK_IRINT_EN_REG);
> > +}
> > +
> > +static inline void mtk_irq_enable(struct mtk_ir *ir, u32 mask)
> > +{
> > +	u32 val;
> > +
> > +	val = mtk_r32(ir, MTK_IRINT_EN_REG);
> > +	mtk_w32(ir, val | mask, MTK_IRINT_EN_REG);
> > +}
> > +
> > +static irqreturn_t mtk_ir_irq(int irqno, void *dev_id)
> > +{
> > +	struct mtk_ir *ir = dev_id;
> > +	u8  wid = 0;
> > +	u32 i, j, val;
> > +	DEFINE_IR_RAW_EVENT(rawir);
> > +
> > +	spin_lock(&ir->ir_lock);
> > +
> > +	mtk_irq_disable(ir, MTK_IRINT_EN);
> > +
> > +	/* Reset decoder state machine */
> > +	ir_raw_event_reset(ir->rc);
> > +
> > +	/* First message must be pulse */
> > +	rawir.pulse = false;
> > +
> > +	/* Handle pulse and space until end of message */
> > +	for (i = 0 ; i < MTK_CHKDATA_SZ ; i++) {
> > +		val = mtk_r32(ir, MTK_CHKDATA_REG(i));
> > +		dev_dbg(ir->dev, "@reg%d=0x%08x\n", i, val);
> > +
> > +		for (j = 0 ; j < 4 ; j++) {
> > +			wid = (val & (0xff << j * 8)) >> j * 8;
> > +			rawir.pulse = !rawir.pulse;
> > +			rawir.duration = wid * (MTK_IR_SAMPLE + 1);
> > +			ir_raw_event_store_with_filter(ir->rc, &rawir);
> > +
> > +			if (MTK_IR_END(wid))
> > +				goto end_msg;
> > +		}
> > +	}
> 
> If I read this correctly, there is a maximum of 17 * 4 = 68 edges per
> IR message. The rc6 mce key 0 (scancode 0x800f0400) is 69 edges, so that
> won't work.
> 
Uh, this is related to hardware limitation. Maximum number hardware
holds indeed is only 68 edges as you said :( 

For the case, I will try change the logic into that the whole message 
is dropped if no end of message is seen within 68 counts to avoid
wasting CPU for decoding. 
	
> > +end_msg:
> > +	/* Restart the next receive */
> > +	mtk_w32_mask(ir, 0x1, MTK_IRCLR, MTK_IRCLR_REG);
> > +
> > +	ir_raw_event_set_idle(ir->rc, true);
> > +	ir_raw_event_handle(ir->rc);
> > +
> > +	/* Clear interrupt status */
> > +	mtk_w32_mask(ir, 0x1, MTK_IRINT_CLR, MTK_IRINT_CLR_REG);
> > +
> > +	/* Enable interrupt */
> > +	mtk_irq_enable(ir, MTK_IRINT_EN);
> > +
> > +	spin_unlock(&ir->ir_lock);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_ir_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *dn = dev->of_node;
> > +	struct resource *res;
> > +	struct mtk_ir *ir;
> > +	u32 val;
> > +	int ret = 0;
> > +
> > +	ir = devm_kzalloc(dev, sizeof(struct mtk_ir), GFP_KERNEL);
> > +	if (!ir)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&ir->ir_lock);
> > +
> > +	ir->dev = dev;
> > +
> > +	if (!of_device_is_compatible(dn, "mediatek,mt7623-ir"))
> > +		return -ENODEV;
> > +
> > +	ir->clk = devm_clk_get(dev, "clk");
> > +	if (IS_ERR(ir->clk)) {
> > +		dev_err(dev, "failed to get a ir clock.\n");
> > +		return PTR_ERR(ir->clk);
> > +	}
> > +
> > +	if (clk_prepare_enable(ir->clk)) {
> > +		dev_err(dev, "try to enable ir_clk failed\n");
> > +		ret = -EINVAL;
> > +		goto exit_clkdisable_clk;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ir->base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(ir->base)) {
> > +		dev_err(dev, "failed to map registers\n");
> > +		ret = PTR_ERR(ir->base);
> > +		goto exit_clkdisable_clk;
> > +	}
> > +
> > +	ir->rc = rc_allocate_device();
> > +	if (!ir->rc) {
> > +		dev_err(dev, "failed to allocate device\n");
> > +		ret = -ENOMEM;
> > +		goto exit_clkdisable_clk;
> > +	}
> > +
> > +	ir->rc->priv = ir;
> > +	ir->rc->input_name = MTK_IR_DEV;
> > +	ir->rc->input_phys = "mtk-ir/input0";
> > +	ir->rc->input_id.bustype = BUS_HOST;
> > +	ir->rc->input_id.vendor = 0x0001;
> > +	ir->rc->input_id.product = 0x0001;
> > +	ir->rc->input_id.version = 0x0001;
> > +	ir->map_name = of_get_property(dn, "linux,rc-map-name", NULL);
> > +	ir->rc->map_name = ir->map_name ?: RC_MAP_EMPTY;
> > +	ir->rc->dev.parent = dev;
> > +	ir->rc->driver_type = RC_DRIVER_IR_RAW;
> > +	ir->rc->driver_name = MTK_IR_DEV;
> > +	ir->rc->allowed_protocols = RC_BIT_ALL;
> > +	ir->rc->rx_resolution = MTK_IR_SAMPLE;
> > +
> > +	ret = rc_register_device(ir->rc);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register rc device\n");
> > +		goto exit_free_dev;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, ir);
> > +
> > +	ir->irq = platform_get_irq(pdev, 0);
> > +	if (ir->irq < 0) {
> > +		dev_err(dev, "no irq resource\n");
> > +		ret = ir->irq;
> 
> From here on onwards the errors paths should call rc_unregister_device(),
> and no longer call rc_free_device(). Note that current master has
> devm_rc_allocate_device() and devm_rc_register_device() which would
> simplify this code.


okay, i will use devm_rc_register_device to have simplified code.


> > +		goto exit_free_dev;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, ir->irq, mtk_ir_irq, 0, MTK_IR_DEV, ir);
> > +	if (ret) {
> > +		dev_err(dev, "failed request irq\n");
> > +		goto exit_free_dev;
> > +	}
> > +
> > +	mtk_irq_disable(ir, MTK_IRINT_EN);
> > +
> > +	val = mtk_r32(ir, MTK_CONFIG_HIGH_REG);
> > +	val |= MTK_PWM_EN | MTK_IR_EN;
> > +	mtk_w32(ir, val, MTK_CONFIG_HIGH_REG);
> > +
> > +	/* Setting sample period */
> > +	mtk_w32_mask(ir, MTK_CHK_PERIOD, MTK_CHK_PERIOD_MASK,
> > +		     MTK_CONFIG_LOW_REG);
> > +
> > +	mtk_irq_enable(ir, MTK_IRINT_EN);
> > +
> > +	dev_info(dev, "initialized MT7623 IR driver\n");
> > +	return 0;
> > +
> > +exit_free_dev:
> > +	rc_free_device(ir->rc);
> > +exit_clkdisable_clk:
> > +	clk_disable_unprepare(ir->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_ir_remove(struct platform_device *pdev)
> > +{
> > +	unsigned long flags;
> > +
> > +	struct mtk_ir *ir = platform_get_drvdata(pdev);
> > +
> > +	spin_lock_irqsave(&ir->ir_lock, flags);
> > +
> > +	mtk_irq_disable(ir, MTK_IRINT_EN);
> > +
> > +	clk_disable_unprepare(ir->clk);
> > +
> > +	spin_unlock_irqrestore(&ir->ir_lock, flags);
> 
> I'm not convinced the ir_lock is helping to prevent any race condition. An
> irq might still already have occurred which will now try to use ir->rc 
> which is freed. You can remove the spinlock completely and call 
> sychronize_irq() after disabling the mtk interrupt. That way you're sure
> the remove is safe to complete.

Okay, it's great suggestion. I will use sychronize_irq instead of the my
bad one :) 

> > +
> > +	rc_unregister_device(ir->rc);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mtk_ir_match[] = {
> > +	{ .compatible = "mediatek,mt7623-ir" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_ir_match);
> > +
> > +static struct platform_driver mtk_ir_driver = {
> > +	.probe          = mtk_ir_probe,
> > +	.remove         = mtk_ir_remove,
> > +	.driver = {
> > +		.name = MTK_IR_DEV,
> > +		.of_match_table = mtk_ir_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(mtk_ir_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek MT7623 IR Receiver Controller Driver");
> > +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 1.9.1
> > 

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Wang]

Hi Andi,

Thank for your reminder. I will refine the code based on your work.
to have elegant code and easy error handling.

Sean

On Fri, 2017-01-06 at 12:43 +0900, Andi Shyti wrote:
> Hi Sean,
> 
> > +	ir->rc = rc_allocate_device();
> 
> Yes, you should use devm_rc_allocate_device(...)
> 
> Besides, standing to this patch which is not in yet:
> 
> https://lkml.org/lkml/2016/12/18/39
> 
> rc_allocate_device should provide the driver type during
> allocation, so it should be:
> 
> 	ir->rc = rc_allocate_device(RC_DRIVER_IR_RAW);
> 
> and this line can be removed:
> 
> > +	ir->rc->driver_type = RC_DRIVER_IR_RAW;
> 
> I don't know when Mauro will take the patch above.
> 
> Andi

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Young]

Hi Sean,

On Fri, Jan 06, 2017 at 03:31:25PM +0800, Sean Wang wrote:
> On Thu, 2017-01-05 at 17:12 +0000, Sean Young wrote:
> > On Fri, Jan 06, 2017 at 12:06:24AM +0800, sean.wang@mediatek.com wrote:
> > > +	/* Handle pulse and space until end of message */
> > > +	for (i = 0 ; i < MTK_CHKDATA_SZ ; i++) {
> > > +		val = mtk_r32(ir, MTK_CHKDATA_REG(i));
> > > +		dev_dbg(ir->dev, "@reg%d=0x%08x\n", i, val);
> > > +
> > > +		for (j = 0 ; j < 4 ; j++) {
> > > +			wid = (val & (0xff << j * 8)) >> j * 8;
> > > +			rawir.pulse = !rawir.pulse;
> > > +			rawir.duration = wid * (MTK_IR_SAMPLE + 1);
> > > +			ir_raw_event_store_with_filter(ir->rc, &rawir);
> > > +
> > > +			if (MTK_IR_END(wid))
> > > +				goto end_msg;
> > > +		}
> > > +	}
> > 
> > If I read this correctly, there is a maximum of 17 * 4 = 68 edges per
> > IR message. The rc6 mce key 0 (scancode 0x800f0400) is 69 edges, so that
> > won't work.
> > 
> Uh, this is related to hardware limitation. Maximum number hardware
> holds indeed is only 68 edges as you said :( 
> 
> For the case, I will try change the logic into that the whole message 
> is dropped if no end of message is seen within 68 counts to avoid
> wasting CPU for decoding. 

I'm not sure it is worthwhile dropping the IR in that case. The processing
is minimal and it might be possible that we have just enough IR to decode
a scancode even if the trailing end of message is missing. Note that
the call to ir_raw_event_set_idle() will generate an timeout IR event, so
there will always be an end of message marker.

All I wanted to do was point out a limitation in case there is a
workaround; if there is not then we might as well make do with the IR
we do have.

Thanks
Sean

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Wang]

On Sun, 2017-01-08 at 21:16 +0000, Sean Young wrote:
> Hi Sean,
> 
> On Fri, Jan 06, 2017 at 03:31:25PM +0800, Sean Wang wrote:
> > On Thu, 2017-01-05 at 17:12 +0000, Sean Young wrote:
> > > On Fri, Jan 06, 2017 at 12:06:24AM +0800, sean.wang@mediatek.com wrote:
> > > > +	/* Handle pulse and space until end of message */
> > > > +	for (i = 0 ; i < MTK_CHKDATA_SZ ; i++) {
> > > > +		val = mtk_r32(ir, MTK_CHKDATA_REG(i));
> > > > +		dev_dbg(ir->dev, "@reg%d=0x%08x\n", i, val);
> > > > +
> > > > +		for (j = 0 ; j < 4 ; j++) {
> > > > +			wid = (val & (0xff << j * 8)) >> j * 8;
> > > > +			rawir.pulse = !rawir.pulse;
> > > > +			rawir.duration = wid * (MTK_IR_SAMPLE + 1);
> > > > +			ir_raw_event_store_with_filter(ir->rc, &rawir);
> > > > +
> > > > +			if (MTK_IR_END(wid))
> > > > +				goto end_msg;
> > > > +		}
> > > > +	}
> > > 
> > > If I read this correctly, there is a maximum of 17 * 4 = 68 edges per
> > > IR message. The rc6 mce key 0 (scancode 0x800f0400) is 69 edges, so that
> > > won't work.
> > > 
> > Uh, this is related to hardware limitation. Maximum number hardware
> > holds indeed is only 68 edges as you said :( 
> > 
> > For the case, I will try change the logic into that the whole message 
> > is dropped if no end of message is seen within 68 counts to avoid
> > wasting CPU for decoding. 
> 
> I'm not sure it is worthwhile dropping the IR in that case. The processing
> is minimal and it might be possible that we have just enough IR to decode
> a scancode even if the trailing end of message is missing. Note that
> the call to ir_raw_event_set_idle() will generate an timeout IR event, so
> there will always be an end of message marker.


1)
I agree with you :) The original logic I made already as you pointed out
is sent incomplete IR message to let ir-raw try to decode as possible.

2)
I had another question. I found multiple and same IR messages being
received when using SONY remote controller. Should driver needs to
report each message or only one of these to the upper layer ?


> All I wanted to do was point out a limitation in case there is a
> workaround; if there is not then we might as well make do with the IR
> we do have.

I also will leave some words about limitation we had in the comments.

> Thanks
> Sean

Re: [PATCH 2/2] media: rc: add driver for IR remote receiver on MT7623 SoC

[Sean Young]

On Mon, Jan 09, 2017 at 11:13:21AM +0800, Sean Wang wrote:
> I had another question. I found multiple and same IR messages being
> received when using SONY remote controller. Should driver needs to
> report each message or only one of these to the upper layer ?

In general the driver shouldn't try to change any IR message, this should
be done in rc-core if necessary.

rc-core should handle this correctly. If the same key is received twice
within IR_KEYPRESS_TIMEOUT (250ms) then it not reported to the input
layer.

Thanks
Sean

Re: [PATCH 1/2] Documentation: devicetree: Add document bindings for mtk-cir

[Rob Herring]

On Fri, Jan 06, 2017 at 12:06:23AM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> This patch adds documentation for devicetree bindings for
> Mediatek IR controller.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../devicetree/bindings/media/mtk-cir.txt          | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 linux-4.8.rc1_p0/Documentation/devicetree/bindings/media/mtk-cir.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/mtk-cir.txt b/Documentation/devicetree/bindings/media/mtk-cir.txt
> new file mode 100644
> index 0000000..bbedd71
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mtk-cir.txt
> @@ -0,0 +1,23 @@
> +Device-Tree bindings for Mediatek IR controller found in Mediatek SoC family
> +
> +Required properties:
> +- compatible	    : "mediatek,mt7623-ir"
> +- clocks	    : list of clock specifiers, corresponding to
> +		      entries in clock-names property;
> +- clock-names	    : should contain "clk" entries;
> +- interrupts	    : should contain IR IRQ number;
> +- reg		    : should contain IO map address for IR.
> +
> +Optional properties:
> +- linux,rc-map-name : Remote control map name.

Would 'label' be appropriate here instead? If not, this needs to be 
documented in a common location and explained better.

> +
> +Example:
> +
> +cir: cir@0x10013000 {

Drop the '0x'.

> +	compatible = "mediatek,mt7623-ir";
> +	reg = <0 0x10013000 0 0x1000>;
> +	interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_LOW>;
> +	clocks = <&infracfg CLK_INFRA_IRRX>;
> +	clock-names = "clk";
> +	linux,rc-map-name = "rc-rc6-mce";
> +};
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] Documentation: devicetree: Add document bindings for mtk-cir

[Sean Wang]

Hi Rob,

thanks for your effort for reviewing. I added comments inline.

On Mon, 2017-01-09 at 12:32 -0600, Rob Herring wrote:
> On Fri, Jan 06, 2017 at 12:06:23AM +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > This patch adds documentation for devicetree bindings for
> > Mediatek IR controller.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../devicetree/bindings/media/mtk-cir.txt          | 23 ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 linux-4.8.rc1_p0/Documentation/devicetree/bindings/media/mtk-cir.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mtk-cir.txt b/Documentation/devicetree/bindings/media/mtk-cir.txt
> > new file mode 100644
> > index 0000000..bbedd71
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mtk-cir.txt
> > @@ -0,0 +1,23 @@
> > +Device-Tree bindings for Mediatek IR controller found in Mediatek SoC family
> > +
> > +Required properties:
> > +- compatible	    : "mediatek,mt7623-ir"
> > +- clocks	    : list of clock specifiers, corresponding to
> > +		      entries in clock-names property;
> > +- clock-names	    : should contain "clk" entries;
> > +- interrupts	    : should contain IR IRQ number;
> > +- reg		    : should contain IO map address for IR.
> > +
> > +Optional properties:
> > +- linux,rc-map-name : Remote control map name.
> 
> Would 'label' be appropriate here instead? If not, this needs to be 
> documented in a common location and explained better.
> 
I checked with how the way applied in other IR drivers is and found that
most IR driver also use the same label to identify the scan/key table
they prefer to use such as gpio-ir-recv, ir-hix5hd2, meson-ir and
sunxi-cir or use hard coding inside the driver. So I thought it should
be appropriate here currently.

> > +
> > +Example:
> > +
> > +cir: cir@0x10013000 {
> 
> Drop the '0x'.
> 

okay, I will.

> > +	compatible = "mediatek,mt7623-ir";
> > +	reg = <0 0x10013000 0 0x1000>;
> > +	interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_LOW>;
> > +	clocks = <&infracfg CLK_INFRA_IRRX>;
> > +	clock-names = "clk";
> > +	linux,rc-map-name = "rc-rc6-mce";
> > +};
> > -- 
> > 1.9.1
> > 

Re: [PATCH 1/2] Documentation: devicetree: Add document bindings for mtk-cir

[Rob Herring]

On Mon, Jan 9, 2017 at 8:35 PM, Sean Wang <sean.wang@mediatek.com> wrote:
> Hi Rob,
>
> thanks for your effort for reviewing. I added comments inline.
>
> On Mon, 2017-01-09 at 12:32 -0600, Rob Herring wrote:
>> On Fri, Jan 06, 2017 at 12:06:23AM +0800, sean.wang@mediatek.com wrote:
>> > From: Sean Wang <sean.wang@mediatek.com>
>> >
>> > This patch adds documentation for devicetree bindings for
>> > Mediatek IR controller.
>> >
>> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> > ---
>> >  .../devicetree/bindings/media/mtk-cir.txt          | 23 ++++++++++++++++++++++
>> >  1 file changed, 23 insertions(+)
>> >  create mode 100644 linux-4.8.rc1_p0/Documentation/devicetree/bindings/media/mtk-cir.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/mtk-cir.txt b/Documentation/devicetree/bindings/media/mtk-cir.txt
>> > new file mode 100644
>> > index 0000000..bbedd71
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/media/mtk-cir.txt
>> > @@ -0,0 +1,23 @@
>> > +Device-Tree bindings for Mediatek IR controller found in Mediatek SoC family
>> > +
>> > +Required properties:
>> > +- compatible           : "mediatek,mt7623-ir"
>> > +- clocks       : list of clock specifiers, corresponding to
>> > +                 entries in clock-names property;
>> > +- clock-names          : should contain "clk" entries;
>> > +- interrupts           : should contain IR IRQ number;
>> > +- reg                  : should contain IO map address for IR.
>> > +
>> > +Optional properties:
>> > +- linux,rc-map-name : Remote control map name.
>>
>> Would 'label' be appropriate here instead? If not, this needs to be
>> documented in a common location and explained better.
>>
> I checked with how the way applied in other IR drivers is and found that
> most IR driver also use the same label to identify the scan/key table
> they prefer to use such as gpio-ir-recv, ir-hix5hd2, meson-ir and
> sunxi-cir or use hard coding inside the driver. So I thought it should
> be appropriate here currently.

Maybe so, but anything with linux prefix gets extra scrutiny and I'm
not sure that happened on the previous cases. If label has the same
meaning, then we should start using that and deprecate this property.
In any case, a property used by multiple bindings needs to be
documented in a common place. The explanation of the property is bad
too. It just spells out RC with no explanation. I'm sure you just
copy-n-pasted it from the others, but that doesn't make it okay.

Rob