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

2014-05-03 Thread Maxime Ripard
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


signature.asc
Description: Digital signature


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

2014-04-30 Thread Mauro Carvalho Chehab
Em Tue, 29 Apr 2014 18:14:54 -0700
Maxime Ripard  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  
> > Signed-off-by: Alexsey Shestacov   
> > 
> > diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt 
> > b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> > new file mode 100644
> > index 000..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,--", 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>;
> 

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

2014-04-30 Thread Александр Берсенев
Thanks for comments.

The IR controllers in A10 and A20 are very similar but not same. The only 
difference I found is a FIFO size.
I'll try to use devm_clk_get to get the time, but now it's not possible to 
add gate clock by name. I've just sent a patch to this mailing list to add 
this possibility.
Also setting clock-frequency via DT is not implenented yet for sunxi 
clocks. I can try to add this functionality too.

The rest are fixed and will be available in the next version of the patch. 
I'll try to split in on smaller patches.

Best,
Alexander Bersenev, Institute of Mathematics and Mechanics

среда, 30 апреля 2014 г., 3:51:31 UTC+6 пользователь Александр Берсенев 
написал:
>
> 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. 
>
> --- 
>
> 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  
> Signed-off-by: Alexsey Shestacov   
>
> diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt 
> b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> new file mode 100644
> index 000..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".
> +   - clocks: First clock should contain SoC gate for IR clock.
> + Second should contain IR feed clock itself.
> +   - 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>;
> + 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>;
> + 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>;
> + status = "disabled";
> + };
> +
> +   ir1: ir@01c21c00 {
> + compatible = "allwinner,sunxi-ir";
> + clocks = <&apb0_gates 7>, <&ir1_clk>;
> + interrupts = <0 6 4>;
> + reg = <0x01C21c00 0x40>;
> + 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 wi

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

2014-04-29 Thread Maxime Ripard
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  
> Signed-off-by: Alexsey Shestacov   
> 
> diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt 
> b/Documentation/devicetree/bindings/media/sunxi-ir.txt
> new file mode 100644
> index 000..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,--", 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/medi