Re: [PATCH v3] sunxi: Add support for consumer infrared devices

Re: [PATCH v3] sunxi: Add support for consumer infrared devices

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 16416 bytes --]

Hi,

Thanks for contributing this patch.

It seems like you're missing a few mailing lists / maintainers
though. You should use the get_maintainer.pl script, and Cc every
maintainer and mailing lists in there.

On Tue, Apr 29, 2014 at 02:51:31PM -0700, Александр Берсенев wrote:
> This patch introduces Consumer IR(CIR) support for sunxi boards.
> 
> This is based on Alexsey Shestacov's work based on the original driver 
> supplied by Allwinner. 

Your Signed-off-by should be here so that it stays in the commit log,
and not discarded.

Note that you can use git commit -s to make sure it's at the right
place.

> --- 
> 
> Changes since version 1: 
>  - Fix timer memory leaks 
>  - Fix race condition when driver unloads while interrupt handler is active
>  - Support Cubieboard 2(need testing)
> 
>  Changes since version 2:
>  - More reliable keydown events
>  - Documentation fixes
>  - Rename registers accurding to A20 user manual
>  - Remove some includes, order includes alphabetically
>  - Use BIT macro
>  - Typo fixes
> 
> Signed-off-by: Alexander Bersenev <bay@hackerdom.ru> 
> Signed-off-by: Alexsey Shestacov <wingrime@linux-sunxi.org>  
> 
> diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt 
> b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> new file mode 100644
> index 0000000..0d416f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> @@ -0,0 +1,21 @@
> +Device-Tree bindings for SUNXI IR controller found in sunXi SoC family
> +
> +Required properties:
> +       - compatible: Should be "allwinner,sunxi-ir".

We prefer to use "allwinner,<family>-<soc>-<device>", with the soc and
family being the one where it was first introduced. If this controller
is the same in A10 and A20, it should be "allwinner,sun4i-a10-ir", if
it is a new controller in the A20, "allwinner,sun7i-a20-ir".

> +       - clocks: First clock should contain SoC gate for IR clock.
> +                 Second should contain IR feed clock itself.

Whenever there's several clocks, using clock-names is to be
preferred. That way, you don't have to request any order, which is a
lot less error prone.

> +       - 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:
> +
> +       ir0: ir@01c21800 {
> +            compatible = "allwinner,sunxi-ir";
> +            clocks = <&apb0_gates 6>, <&ir0_clk>;
> +            interrupts = <0 5 1>;
> +            reg = <0x01C21800 0x40>;
> +            linux,rc-map-name = "rc-rc6-mce";
> +       };
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts 
> b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> index feeff64..01b519c 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> @@ -164,6 +164,13 @@
>   reg = <1>;
>   };
>   };
> +
> + ir0: ir@01c21800 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&ir0_pins_a>;
> + gpios = <&pio 1 4 0>;

You don't seem to be using that gpios property anywhere.

Plus, your indentation seems completely wrong. Please run
checkpatch.pl on your patches before running it, and make sure there's
no errors or warning.

> + status = "okay";
> + };
>   };
>  
>   leds {
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts 
> b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index e288562..683090f 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -232,6 +232,13 @@
>   reg = <1>;
>   };
>   };
> +
> + ir0: ir@01c21800 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&ir0_pins_a>;
> + gpios = <&pio 1 4 0>;

Same here.

> + status = "okay";
> + };
>   };
>  
>   leds {
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi 
> b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 0ae2b77..4597731 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -724,6 +724,19 @@
>   allwinner,drive = <2>;
>   allwinner,pull = <0>;
>   };
> +
> + ir0_pins_a: ir0@0 {
> +    allwinner,pins = "PB3","PB4";
> +    allwinner,function = "ir0";
> +    allwinner,drive = <0>;
> +    allwinner,pull = <0>;
> + };
> + ir1_pins_a: ir1@0 {
> +    allwinner,pins = "PB22","PB23";
> +    allwinner,function = "ir1";
> +    allwinner,drive = <0>;
> +    allwinner,pull = <0>;
> + };
>   };
>  
>   timer@01c20c00 {
> @@ -937,5 +950,21 @@
>   #interrupt-cells = <3>;
>   interrupts = <1 9 0xf04>;
>   };
> +
> +       ir0: ir@01c21800 {
> +     compatible = "allwinner,sunxi-ir";
> + clocks = <&apb0_gates 6>, <&ir0_clk>;
> + interrupts = <0 5 4>;
> + reg = <0x01C21800 0x40>;

Please use lower-case for the address here.

> + status = "disabled";
> + };
> +
> +       ir1: ir@01c21c00 {
> +     compatible = "allwinner,sunxi-ir";
> + clocks = <&apb0_gates 7>, <&ir1_clk>;
> + interrupts = <0 6 4>;
> + reg = <0x01C21c00 0x40>;

... or at least be consistent.

> + status = "disabled";
> + };
>   };
>  };
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 8fbd377..9427fad 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -343,4 +343,14 @@ config RC_ST
>  
>   If you're not sure, select N here.
>  
> +config IR_SUNXI
> +    tristate "SUNXI IR remote control"
> +    depends on RC_CORE
> +    depends on ARCH_SUNXI
> +    ---help---
> +      Say Y if you want to use sunXi internal IR Controller
> +
> +      To compile this driver as a module, choose M here: the module will
> +      be called sunxi-ir.
> +
>  endif #RC_DEVICES
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index f8b54ff..93cdbe9 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o
>  obj-$(CONFIG_IR_IGUANA) += iguanair.o
>  obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
>  obj-$(CONFIG_RC_ST) += st_rc.o
> +obj-$(CONFIG_IR_SUNXI) += sunxi-ir.o
>  obj-$(CONFIG_IR_IMG) += img-ir/
> diff --git a/drivers/media/rc/sunxi-ir.c b/drivers/media/rc/sunxi-ir.c
> new file mode 100644
> index 0000000..9b5639e
> --- /dev/null
> +++ b/drivers/media/rc/sunxi-ir.c
> @@ -0,0 +1,314 @@
> +/*
> + * Driver for Allwinner sunXi IR controller
> + *
> + * Copyright (C) 2014 Alexsey Shestacov <wingrime@linux-sunxi.org>
> + *
> + * Based on sun5i-ir.c:
> + * Copyright (C) 2007-2012 Daniel Wang
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.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 <media/rc-core.h>
> +
> +#define SUNXI_IR_DEV "sunxi-ir"
> +
> +/* Registers */
> +/* IR Control */
> +#define SUNXI_IR_CTL_REG      0x00
> +/* Rx Config */
> +#define SUNXI_IR_RXCTL_REG    0x10
> +/* Rx Data */
> +#define SUNXI_IR_RXFIFO_REG   0x20
> +/* Rx Interrupt Enable */
> +#define SUNXI_IR_RXINT_REG    0x2C
> +/* Rx Interrupt Status */
> +#define SUNXI_IR_RXSTA_REG    0x30
> +/* IR Sample Config */
> +#define SUNXI_IR_CIR_REG      0x34
> +
> +/* Bit Definition of IR_RXINTS_REG Register */
> +#define SUNXI_IR_RXINTS_RXOF   BIT(0) /* Rx FIFO Overflow */
> +#define SUNXI_IR_RXINTS_RXPE   BIT(1) /* Rx Packet End */
> +#define SUNXI_IR_RXINTS_RXDA   BIT(4) /* Rx FIFO Data Available */
> +/* Hardware supported fifo size */
> +#define SUNXI_IR_FIFO_SIZE 16
> +/* How much messages in fifo triggers IRQ */
> +#define SUNXI_IR_FIFO_TRIG 8
> +/* Required frequency for IR0 or IR1 clock in CIR mode */
> +#define SUNXI_IR_BASE_CLK 8000000

Would it make sense to have different clock frequencies? If so, that
should probably be in the DT.

> +/* Frequency after IR internal divider  */
> +#define SUNXI_IR_CLK  (SUNXI_IR_BASE_CLK / 64)
> +/* Sample period in ns */
> +#define SUNXI_IR_SAMPLE (1000000000ul / SUNXI_IR_CLK)
> +/* Filter threshold in samples  */
> +#define SUNXI_IR_RXFILT                1
> +/* Idle Threshold in samples */
> +#define SUNXI_IR_RXIDLE                20
> +/* Time after which device stops sending data in ms */
> +#define SUNXI_IR_TIMEOUT               120
> +
> +struct sunxi_ir {
> + spinlock_t      ir_lock;
> + struct rc_dev   *rc;
> + void __iomem    *base;
> + int             irq;
> + struct clk      *clk;
> + struct clk      *apb_clk;
> + const char      *map_name;
> +};
> +
> +static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
> +{
> + unsigned long status;
> + unsigned long flags;
> + unsigned char dt;
> + unsigned int cnt, rc;
> + struct sunxi_ir *ir = dev_id;
> + DEFINE_IR_RAW_EVENT(rawir);
> +
> + spin_lock_irqsave(&ir->ir_lock, flags);

You don't need to use irqsave, you're already running with interrupts
disabled.

> +
> + status = readl(ir->base + SUNXI_IR_RXSTA_REG);
> +
> + /* clean all pending statuses */
> + writel(status | 0xff, ir->base + SUNXI_IR_RXSTA_REG);
> +
> + if (status & SUNXI_IR_RXINTS_RXDA) {
> + /* How much messages in fifo */
> + rc  = ((status>>8) & 0x3f);
> + /* Sanity check */
> + rc = rc > SUNXI_IR_FIFO_SIZE ? SUNXI_IR_FIFO_SIZE : rc;
> + /* if we have data */
> + for (cnt = 0; cnt < rc; cnt++) {
> + /* for each bit in fifo */
> + dt = (unsigned char)readl(ir->base +
> +  SUNXI_IR_RXFIFO_REG);

You can use readb then.

> + rawir.pulse = (dt & 0x80) != 0;
> + rawir.duration = (dt & 0x7f)*SUNXI_IR_SAMPLE;
> + ir_raw_event_store_with_filter(ir->rc, &rawir);
> + }
> + }
> +
> + if (status & SUNXI_IR_RXINTS_RXOF)
> + ir_raw_event_reset(ir->rc);
> + else if (status & SUNXI_IR_RXINTS_RXPE) {
> + ir_raw_event_set_idle(ir->rc, true);
> + ir_raw_event_handle(ir->rc);
> + }
> +
> + spin_unlock_irqrestore(&ir->ir_lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +static int sunxi_ir_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> +
> + unsigned long tmp = 0;
> +
> + struct device *dev = &pdev->dev;
> + struct device_node *dn = dev->of_node;
> + struct resource *res;
> + struct sunxi_ir *ir;
> +
> + ir = devm_kzalloc(dev, sizeof(struct sunxi_ir), GFP_KERNEL);
> + if (!ir)
> + return -ENOMEM;
> +
> + /* Clock */
> + ir->apb_clk = of_clk_get(dn, 0);
> + if (IS_ERR(ir->apb_clk)) {
> + dev_err(dev, "failed to get a apb clock.\n");
> + return PTR_ERR(ir->apb_clk);
> + }
> + ir->clk = of_clk_get(dn, 1);
> + if (IS_ERR(ir->clk)) {
> + dev_err(dev, "failed to get a ir clock.\n");
> + ret = PTR_ERR(ir->clk);
> + goto exit_clkput_apb_clk;
> + }

You can use devm_clk_get.

> + ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
> + if (ret) {
> + dev_err(dev, "set ir base clock failed!\n");
> + goto exit_clkput_clk;
> + }
> +
> + if (clk_prepare_enable(ir->apb_clk)) {
> + dev_err(dev, "try to enable apb_ir_clk failed\n");
> + ret = -EINVAL;
> + goto exit_clkput_clk;
> + }
> +
> + if (clk_prepare_enable(ir->clk)) {
> + dev_err(dev, "try to enable ir_clk failed\n");
> + ret = -EINVAL;
> + goto exit_clkdisable_apb_clk;
> + }
> +
> + /* IO */
> + 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 = -ENOMEM;
> + goto exit_clkdisable_clk;
> + }
> +
> + /* IRQ */
> + ir->irq = platform_get_irq(pdev, 0);
> + if (ir->irq < 0) {
> + dev_err(dev, "no irq resource\n");
> + ret = -EINVAL;
> + goto exit_clkdisable_clk;
> + }
> +
> + ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
> + if (ret) {
> + dev_err(dev, "failed request irq\n");
> + ret = -EINVAL;
> + goto exit_clkdisable_clk;
> + }
> +
> + ir->rc = rc_allocate_device();
> +
> + if (!ir->rc) {
> + ret = -ENOMEM;
> + goto exit_clkdisable_clk;
> + }
> +
> + ir->rc->priv = ir;
> + ir->rc->input_name = SUNXI_IR_DEV;
> + ir->rc->input_phys = "sunxi-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 = 0x0100;
> + 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;
> + rc_set_allowed_protocols(ir->rc, RC_BIT_ALL);
> + ir->rc->rx_resolution = SUNXI_IR_SAMPLE;
> + ir->rc->timeout = MS_TO_NS(SUNXI_IR_TIMEOUT);
> + ir->rc->driver_name = SUNXI_IR_DEV;
> +
> + ret = rc_register_device(ir->rc);
> + if (ret) {
> + dev_err(dev, "can't register rc device\n");
> + ret = -EINVAL;
> + goto exit_free_dev;
> + }
> +
> + platform_set_drvdata(pdev, ir);
> +
> + /* Enable CIR Mode */
> + writel(0x3<<4, ir->base+SUNXI_IR_CTL_REG);
> +
> + /* Config IR Sample Register */
> + /* Fsample = clk */
> + tmp = 0;
> + /* Set Filter Threshold */
> + tmp |= (SUNXI_IR_RXFILT & 0x3f) << 2;
> + /* Set Idle Threshold */
> + tmp |= (SUNXI_IR_RXIDLE & 0xff) << 8;
> + writel(tmp, ir->base + SUNXI_IR_CIR_REG);
> +
> + /* Invert Input Signal */
> + writel(0x1<<2, ir->base + SUNXI_IR_RXCTL_REG);
> +
> + /* Clear All Rx Interrupt Status */
> + writel(0xff, ir->base + SUNXI_IR_RXSTA_REG);
> +
> + /* Enable IRQ on data, overflow, packed end */
> + tmp = (0x1<<4)|0x3;
> +
> + /* Rx FIFO Threshold = 4 */
> + tmp |= (SUNXI_IR_FIFO_TRIG - 1) << 8;
> +
> + writel(tmp, ir->base + SUNXI_IR_RXINT_REG);
> +
> + /* Enable IR Module */
> + tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> +
> + writel(tmp|0x3, ir->base + SUNXI_IR_CTL_REG);
> +
> + dev_info(dev, "initialized sunXi IR driver\n");
> + return 0;
> +
> +exit_free_dev:
> + rc_free_device(ir->rc);
> +exit_clkdisable_clk:
> + clk_disable_unprepare(ir->clk);
> +exit_clkdisable_apb_clk:
> + clk_disable_unprepare(ir->apb_clk);
> +exit_clkput_clk:
> + clk_put(ir->clk);
> +exit_clkput_apb_clk:
> + clk_put(ir->apb_clk);
> +
> + return ret;
> +}
> +
> +static int sunxi_ir_remove(struct platform_device *pdev)
> +{
> + unsigned long flags;
> + struct sunxi_ir *ir = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(ir->clk);
> + clk_disable_unprepare(ir->apb_clk);
> +
> + clk_put(ir->clk);
> + clk_put(ir->apb_clk);
> +
> + spin_lock_irqsave(&ir->ir_lock, flags);
> + /* disable IR IRQ */
> + writel(0, ir->base + SUNXI_IR_RXINT_REG);
> + /* clear All Rx Interrupt Status */
> + writel(0xff, ir->base + SUNXI_IR_RXSTA_REG);
> + /* disable IR */
> + writel(0, ir->base + SUNXI_IR_CTL_REG);
> + spin_unlock_irqrestore(&ir->ir_lock, flags);
> +
> + rc_unregister_device(ir->rc);
> + return 0;
> +}
> +
> +static const struct of_device_id sunxi_ir_match[] = {
> + { .compatible = "allwinner,sunxi-ir", },
> + {},
> +};
> +
> +static struct platform_driver sunxi_ir_driver = {
> + .probe          = sunxi_ir_probe,
> + .remove         = sunxi_ir_remove,
> + .driver = {
> + .name = SUNXI_IR_DEV,
> + .owner = THIS_MODULE,
> + .of_match_table = sunxi_ir_match,
> + },
> +};
> +
> +module_platform_driver(sunxi_ir_driver);
> +
> +
> +MODULE_DESCRIPTION("Allwinner sunXi IR controller driver");
> +MODULE_AUTHOR("Alexsey Shestacov <wingrime@linux-sunxi.org>");
> +MODULE_LICENSE("GPL");
> 

Overall, I'd like this to be split into several patches:
  - One for the bindings documentation
  - One for the driver
  - One that adds the controller to the DT
  - One that adds the pin muxing options
  - and finally one that enables the IR receiver on the boards.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] sunxi: Add support for consumer infrared devices

[Mauro Carvalho Chehab]

[-- Attachment #1: Type: text/plain, Size: 18176 bytes --]

Em Tue, 29 Apr 2014 18:14:54 -0700
Maxime Ripard <maxime.ripard@free-electrons.com> escreveu:

> Hi,
> 
> Thanks for contributing this patch.
> 
> It seems like you're missing a few mailing lists / maintainers
> though. You should use the get_maintainer.pl script, and Cc every
> maintainer and mailing lists in there.
> 
> On Tue, Apr 29, 2014 at 02:51:31PM -0700, Александр Берсенев wrote:
> > This patch introduces Consumer IR(CIR) support for sunxi boards.
> > 
> > This is based on Alexsey Shestacov's work based on the original driver 
> > supplied by Allwinner. 
> 
> Your Signed-off-by should be here so that it stays in the commit log,
> and not discarded.
> 
> Note that you can use git commit -s to make sure it's at the right
> place.
> 
> > --- 
> > 
> > Changes since version 1: 
> >  - Fix timer memory leaks 
> >  - Fix race condition when driver unloads while interrupt handler is active
> >  - Support Cubieboard 2(need testing)
> > 
> >  Changes since version 2:
> >  - More reliable keydown events
> >  - Documentation fixes
> >  - Rename registers accurding to A20 user manual
> >  - Remove some includes, order includes alphabetically
> >  - Use BIT macro
> >  - Typo fixes
> > 
> > Signed-off-by: Alexander Bersenev <bay@hackerdom.ru> 
> > Signed-off-by: Alexsey Shestacov <wingrime@linux-sunxi.org>  
> > 
> > diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt 
> > b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> > new file mode 100644
> > index 0000000..0d416f4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> > @@ -0,0 +1,21 @@
> > +Device-Tree bindings for SUNXI IR controller found in sunXi SoC family
> > +
> > +Required properties:
> > +       - compatible: Should be "allwinner,sunxi-ir".
> 
> We prefer to use "allwinner,<family>-<soc>-<device>", with the soc and
> family being the one where it was first introduced. If this controller
> is the same in A10 and A20, it should be "allwinner,sun4i-a10-ir", if
> it is a new controller in the A20, "allwinner,sun7i-a20-ir".
> 
> > +       - clocks: First clock should contain SoC gate for IR clock.
> > +                 Second should contain IR feed clock itself.
> 
> Whenever there's several clocks, using clock-names is to be
> preferred. That way, you don't have to request any order, which is a
> lot less error prone.
> 
> > +       - 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:
> > +
> > +       ir0: ir@01c21800 {
> > +            compatible = "allwinner,sunxi-ir";
> > +            clocks = <&apb0_gates 6>, <&ir0_clk>;
> > +            interrupts = <0 5 1>;
> > +            reg = <0x01C21800 0x40>;
> > +            linux,rc-map-name = "rc-rc6-mce";
> > +       };
> > diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts 
> > b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> > index feeff64..01b519c 100644
> > --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> > +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> > @@ -164,6 +164,13 @@
> >   reg = <1>;
> >   };
> >   };
> > +
> > + ir0: ir@01c21800 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ir0_pins_a>;
> > + gpios = <&pio 1 4 0>;
> 
> You don't seem to be using that gpios property anywhere.
> 
> Plus, your indentation seems completely wrong. Please run
> checkpatch.pl on your patches before running it, and make sure there's
> no errors or warning.
> 
> > + status = "okay";
> > + };
> >   };
> >  
> >   leds {
> > diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts 
> > b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> > index e288562..683090f 100644
> > --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> > +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> > @@ -232,6 +232,13 @@
> >   reg = <1>;
> >   };
> >   };
> > +
> > + ir0: ir@01c21800 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ir0_pins_a>;
> > + gpios = <&pio 1 4 0>;
> 
> Same here.
> 
> > + status = "okay";
> > + };
> >   };
> >  
> >   leds {
> > diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi 
> > b/arch/arm/boot/dts/sun7i-a20.dtsi
> > index 0ae2b77..4597731 100644
> > --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> > +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> > @@ -724,6 +724,19 @@
> >   allwinner,drive = <2>;
> >   allwinner,pull = <0>;
> >   };
> > +
> > + ir0_pins_a: ir0@0 {
> > +    allwinner,pins = "PB3","PB4";
> > +    allwinner,function = "ir0";
> > +    allwinner,drive = <0>;
> > +    allwinner,pull = <0>;
> > + };
> > + ir1_pins_a: ir1@0 {
> > +    allwinner,pins = "PB22","PB23";
> > +    allwinner,function = "ir1";
> > +    allwinner,drive = <0>;
> > +    allwinner,pull = <0>;
> > + };
> >   };
> >  
> >   timer@01c20c00 {
> > @@ -937,5 +950,21 @@
> >   #interrupt-cells = <3>;
> >   interrupts = <1 9 0xf04>;
> >   };
> > +
> > +       ir0: ir@01c21800 {
> > +     compatible = "allwinner,sunxi-ir";
> > + clocks = <&apb0_gates 6>, <&ir0_clk>;
> > + interrupts = <0 5 4>;
> > + reg = <0x01C21800 0x40>;
> 
> Please use lower-case for the address here.
> 
> > + status = "disabled";
> > + };
> > +
> > +       ir1: ir@01c21c00 {
> > +     compatible = "allwinner,sunxi-ir";
> > + clocks = <&apb0_gates 7>, <&ir1_clk>;
> > + interrupts = <0 6 4>;
> > + reg = <0x01C21c00 0x40>;
> 
> ... or at least be consistent.
> 
> > + status = "disabled";
> > + };
> >   };
> >  };

Please split the DT stuff on separate patches. DT stuff require a DT
maintainer SOB or ack, while the media stuff are reviewed by V4L2 people,
and finally reviewed by me and merged via my tree.

> > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> > index 8fbd377..9427fad 100644
> > --- a/drivers/media/rc/Kconfig
> > +++ b/drivers/media/rc/Kconfig
> > @@ -343,4 +343,14 @@ config RC_ST
> >  
> >   If you're not sure, select N here.
> >  
> > +config IR_SUNXI
> > +    tristate "SUNXI IR remote control"
> > +    depends on RC_CORE
> > +    depends on ARCH_SUNXI
> > +    ---help---
> > +      Say Y if you want to use sunXi internal IR Controller
> > +
> > +      To compile this driver as a module, choose M here: the module will
> > +      be called sunxi-ir.
> > +
> >  endif #RC_DEVICES
> > diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> > index f8b54ff..93cdbe9 100644
> > --- a/drivers/media/rc/Makefile
> > +++ b/drivers/media/rc/Makefile
> > @@ -32,4 +32,5 @@ obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o
> >  obj-$(CONFIG_IR_IGUANA) += iguanair.o
> >  obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
> >  obj-$(CONFIG_RC_ST) += st_rc.o
> > +obj-$(CONFIG_IR_SUNXI) += sunxi-ir.o
> >  obj-$(CONFIG_IR_IMG) += img-ir/
> > diff --git a/drivers/media/rc/sunxi-ir.c b/drivers/media/rc/sunxi-ir.c
> > new file mode 100644
> > index 0000000..9b5639e
> > --- /dev/null
> > +++ b/drivers/media/rc/sunxi-ir.c
> > @@ -0,0 +1,314 @@
> > +/*
> > + * Driver for Allwinner sunXi IR controller
> > + *
> > + * Copyright (C) 2014 Alexsey Shestacov <wingrime@linux-sunxi.org>
> > + *
> > + * Based on sun5i-ir.c:
> > + * Copyright (C) 2007-2012 Daniel Wang
> > + * Allwinner Technology Co., Ltd. <www.allwinnertech.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 <media/rc-core.h>
> > +
> > +#define SUNXI_IR_DEV "sunxi-ir"
> > +
> > +/* Registers */
> > +/* IR Control */
> > +#define SUNXI_IR_CTL_REG      0x00
> > +/* Rx Config */
> > +#define SUNXI_IR_RXCTL_REG    0x10
> > +/* Rx Data */
> > +#define SUNXI_IR_RXFIFO_REG   0x20
> > +/* Rx Interrupt Enable */
> > +#define SUNXI_IR_RXINT_REG    0x2C
> > +/* Rx Interrupt Status */
> > +#define SUNXI_IR_RXSTA_REG    0x30
> > +/* IR Sample Config */
> > +#define SUNXI_IR_CIR_REG      0x34
> > +
> > +/* Bit Definition of IR_RXINTS_REG Register */
> > +#define SUNXI_IR_RXINTS_RXOF   BIT(0) /* Rx FIFO Overflow */
> > +#define SUNXI_IR_RXINTS_RXPE   BIT(1) /* Rx Packet End */
> > +#define SUNXI_IR_RXINTS_RXDA   BIT(4) /* Rx FIFO Data Available */
> > +/* Hardware supported fifo size */
> > +#define SUNXI_IR_FIFO_SIZE 16
> > +/* How much messages in fifo triggers IRQ */
> > +#define SUNXI_IR_FIFO_TRIG 8
> > +/* Required frequency for IR0 or IR1 clock in CIR mode */
> > +#define SUNXI_IR_BASE_CLK 8000000
> 
> Would it make sense to have different clock frequencies? If so, that
> should probably be in the DT.
> 
> > +/* Frequency after IR internal divider  */
> > +#define SUNXI_IR_CLK  (SUNXI_IR_BASE_CLK / 64)
> > +/* Sample period in ns */
> > +#define SUNXI_IR_SAMPLE (1000000000ul / SUNXI_IR_CLK)
> > +/* Filter threshold in samples  */
> > +#define SUNXI_IR_RXFILT                1
> > +/* Idle Threshold in samples */
> > +#define SUNXI_IR_RXIDLE                20
> > +/* Time after which device stops sending data in ms */
> > +#define SUNXI_IR_TIMEOUT               120
> > +
> > +struct sunxi_ir {
> > + spinlock_t      ir_lock;
> > + struct rc_dev   *rc;
> > + void __iomem    *base;
> > + int             irq;
> > + struct clk      *clk;
> > + struct clk      *apb_clk;
> > + const char      *map_name;
> > +};
> > +
> > +static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
> > +{
> > + unsigned long status;
> > + unsigned long flags;
> > + unsigned char dt;
> > + unsigned int cnt, rc;
> > + struct sunxi_ir *ir = dev_id;
> > + DEFINE_IR_RAW_EVENT(rawir);
> > +
> > + spin_lock_irqsave(&ir->ir_lock, flags);
> 
> You don't need to use irqsave, you're already running with interrupts
> disabled.
> 
> > +
> > + status = readl(ir->base + SUNXI_IR_RXSTA_REG);
> > +
> > + /* clean all pending statuses */
> > + writel(status | 0xff, ir->base + SUNXI_IR_RXSTA_REG);
> > +
> > + if (status & SUNXI_IR_RXINTS_RXDA) {
> > + /* How much messages in fifo */
> > + rc  = ((status>>8) & 0x3f);

On most places, you're using spaces for the >> operator. You should be
consistent about that here too: please add spaces before/after >>.

> > + /* Sanity check */
> > + rc = rc > SUNXI_IR_FIFO_SIZE ? SUNXI_IR_FIFO_SIZE : rc;
> > + /* if we have data */
> > + for (cnt = 0; cnt < rc; cnt++) {
> > + /* for each bit in fifo */
> > + dt = (unsigned char)readl(ir->base +
> > +  SUNXI_IR_RXFIFO_REG);
> 
> You can use readb then.
> 
> > + rawir.pulse = (dt & 0x80) != 0;
> > + rawir.duration = (dt & 0x7f)*SUNXI_IR_SAMPLE;
> > + ir_raw_event_store_with_filter(ir->rc, &rawir);
> > + }
> > + }

What happened with indentation? It is hard to analyze a badly
indented patch like that...

> > +
> > + if (status & SUNXI_IR_RXINTS_RXOF)
> > + ir_raw_event_reset(ir->rc);
> > + else if (status & SUNXI_IR_RXINTS_RXPE) {
> > + ir_raw_event_set_idle(ir->rc, true);
> > + ir_raw_event_handle(ir->rc);
> > + }
> > +
> > + spin_unlock_irqrestore(&ir->ir_lock, flags);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +
> > +static int sunxi_ir_probe(struct platform_device *pdev)
> > +{
> > + int ret = 0;
> > +
> > + unsigned long tmp = 0;
> > +
> > + struct device *dev = &pdev->dev;
> > + struct device_node *dn = dev->of_node;
> > + struct resource *res;
> > + struct sunxi_ir *ir;
> > +
> > + ir = devm_kzalloc(dev, sizeof(struct sunxi_ir), GFP_KERNEL);
> > + if (!ir)
> > + return -ENOMEM;
> > +
> > + /* Clock */
> > + ir->apb_clk = of_clk_get(dn, 0);
> > + if (IS_ERR(ir->apb_clk)) {
> > + dev_err(dev, "failed to get a apb clock.\n");
> > + return PTR_ERR(ir->apb_clk);
> > + }
> > + ir->clk = of_clk_get(dn, 1);
> > + if (IS_ERR(ir->clk)) {
> > + dev_err(dev, "failed to get a ir clock.\n");
> > + ret = PTR_ERR(ir->clk);
> > + goto exit_clkput_apb_clk;
> > + }
> 
> You can use devm_clk_get.
> 
> > + ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
> > + if (ret) {
> > + dev_err(dev, "set ir base clock failed!\n");
> > + goto exit_clkput_clk;
> > + }
> > +
> > + if (clk_prepare_enable(ir->apb_clk)) {
> > + dev_err(dev, "try to enable apb_ir_clk failed\n");
> > + ret = -EINVAL;
> > + goto exit_clkput_clk;
> > + }
> > +
> > + if (clk_prepare_enable(ir->clk)) {
> > + dev_err(dev, "try to enable ir_clk failed\n");
> > + ret = -EINVAL;
> > + goto exit_clkdisable_apb_clk;
> > + }
> > +
> > + /* IO */
> > + 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 = -ENOMEM;
> > + goto exit_clkdisable_clk;
> > + }
> > +
> > + /* IRQ */
> > + ir->irq = platform_get_irq(pdev, 0);
> > + if (ir->irq < 0) {
> > + dev_err(dev, "no irq resource\n");
> > + ret = -EINVAL;
> > + goto exit_clkdisable_clk;
> > + }
> > +
> > + ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
> > + if (ret) {
> > + dev_err(dev, "failed request irq\n");
> > + ret = -EINVAL;
> > + goto exit_clkdisable_clk;
> > + }
> > +
> > + ir->rc = rc_allocate_device();
> > +
> > + if (!ir->rc) {
> > + ret = -ENOMEM;
> > + goto exit_clkdisable_clk;
> > + }
> > +
> > + ir->rc->priv = ir;
> > + ir->rc->input_name = SUNXI_IR_DEV;
> > + ir->rc->input_phys = "sunxi-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 = 0x0100;
> > + 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;
> > + rc_set_allowed_protocols(ir->rc, RC_BIT_ALL);
> > + ir->rc->rx_resolution = SUNXI_IR_SAMPLE;
> > + ir->rc->timeout = MS_TO_NS(SUNXI_IR_TIMEOUT);
> > + ir->rc->driver_name = SUNXI_IR_DEV;
> > +
> > + ret = rc_register_device(ir->rc);
> > + if (ret) {
> > + dev_err(dev, "can't register rc device\n");
> > + ret = -EINVAL;
> > + goto exit_free_dev;
> > + }
> > +
> > + platform_set_drvdata(pdev, ir);
> > +
> > + /* Enable CIR Mode */
> > + writel(0x3<<4, ir->base+SUNXI_IR_CTL_REG);

Code Style: please add the needed whitespaces before/after operators.

> > +
> > + /* Config IR Sample Register */
> > + /* Fsample = clk */
> > + tmp = 0;
> > + /* Set Filter Threshold */
> > + tmp |= (SUNXI_IR_RXFILT & 0x3f) << 2;
> > + /* Set Idle Threshold */
> > + tmp |= (SUNXI_IR_RXIDLE & 0xff) << 8;
> > + writel(tmp, ir->base + SUNXI_IR_CIR_REG);
> > +
> > + /* Invert Input Signal */
> > + writel(0x1<<2, ir->base + SUNXI_IR_RXCTL_REG);

Coding style: Please use space to separate the operators.

> > +
> > + /* Clear All Rx Interrupt Status */
> > + writel(0xff, ir->base + SUNXI_IR_RXSTA_REG);
> > +
> > + /* Enable IRQ on data, overflow, packed end */
> > + tmp = (0x1<<4)|0x3;

Coding style: Please use space to separate the operators.

> > +
> > + /* Rx FIFO Threshold = 4 */
> > + tmp |= (SUNXI_IR_FIFO_TRIG - 1) << 8;
> > +
> > + writel(tmp, ir->base + SUNXI_IR_RXINT_REG);
> > +
> > + /* Enable IR Module */
> > + tmp = readl(ir->base + SUNXI_IR_CTL_REG);
> > +
> > + writel(tmp|0x3, ir->base + SUNXI_IR_CTL_REG);
> > +
> > + dev_info(dev, "initialized sunXi IR driver\n");
> > + return 0;
> > +
> > +exit_free_dev:
> > + rc_free_device(ir->rc);
> > +exit_clkdisable_clk:
> > + clk_disable_unprepare(ir->clk);
> > +exit_clkdisable_apb_clk:
> > + clk_disable_unprepare(ir->apb_clk);
> > +exit_clkput_clk:
> > + clk_put(ir->clk);
> > +exit_clkput_apb_clk:
> > + clk_put(ir->apb_clk);
> > +
> > + return ret;
> > +}
> > +
> > +static int sunxi_ir_remove(struct platform_device *pdev)
> > +{
> > + unsigned long flags;
> > + struct sunxi_ir *ir = platform_get_drvdata(pdev);
> > +
> > + clk_disable_unprepare(ir->clk);
> > + clk_disable_unprepare(ir->apb_clk);
> > +
> > + clk_put(ir->clk);
> > + clk_put(ir->apb_clk);
> > +
> > + spin_lock_irqsave(&ir->ir_lock, flags);
> > + /* disable IR IRQ */
> > + writel(0, ir->base + SUNXI_IR_RXINT_REG);
> > + /* clear All Rx Interrupt Status */
> > + writel(0xff, ir->base + SUNXI_IR_RXSTA_REG);
> > + /* disable IR */
> > + writel(0, ir->base + SUNXI_IR_CTL_REG);
> > + spin_unlock_irqrestore(&ir->ir_lock, flags);
> > +
> > + rc_unregister_device(ir->rc);
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sunxi_ir_match[] = {
> > + { .compatible = "allwinner,sunxi-ir", },
> > + {},
> > +};
> > +
> > +static struct platform_driver sunxi_ir_driver = {
> > + .probe          = sunxi_ir_probe,
> > + .remove         = sunxi_ir_remove,
> > + .driver = {
> > + .name = SUNXI_IR_DEV,
> > + .owner = THIS_MODULE,
> > + .of_match_table = sunxi_ir_match,
> > + },
> > +};
> > +
> > +module_platform_driver(sunxi_ir_driver);
> > +
> > +
> > +MODULE_DESCRIPTION("Allwinner sunXi IR controller driver");
> > +MODULE_AUTHOR("Alexsey Shestacov <wingrime@linux-sunxi.org>");
> > +MODULE_LICENSE("GPL");
> > 
> 
> Overall, I'd like this to be split into several patches:
>   - One for the bindings documentation
>   - One for the driver
>   - One that adds the controller to the DT
>   - One that adds the pin muxing options
>   - and finally one that enables the IR receiver on the boards.
> 
> Thanks!
> Maxime
> 


-- 

Cheers,
Mauro

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] sunxi: Add support for consumer infrared devices

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Wed, Apr 30, 2014 at 02:06:27AM -0700, Александр Берсенев wrote:
> Also setting clock-frequency via DT is not implenented yet for sunxi 
> clocks. I can try to add this functionality too.

The clock frequency should be on the device node, not the clock
one. On such cases, it's up to the device to call clk_set_rate and not
to expect the clock to run at the proper frequency.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]