Hi Александр,
(Gmail seems to have messed up the indentation, so I apologise for that.)
First things first:
You should really split this into a few patches:
1. Make any modifications unrelated to the driver itself
2. Add / amend the documentation for the device tree bindings.
3. Add the driver.
4. Add the "base" device tree stuff (in .dtsi files)
5. Update board files to use that.
Some of these changes have to be ACKed by different people, (e.g. the
device tree people care about the device tree stuff, but not the
driver itself) and it also makes it easier to review.
That said, maybe wait a couple of days for any other feedback before
splitting and re-sending this.
Also, you really need to run checkpatch over your patches.
If you're expecting this to go upstream, you're going to need to send
it to the relevant mailing lists, _and_ linux-sunxi.
I.e. as this is an input driver, you need to CC it to the linux-input
list, and the input maintainer. The device tree stuff will have to be
CC'd to the device tree maintainer and list also.
Other nits and stuff below.
On Fri, Apr 25, 2014 at 7:38 PM, Александр Берсенев <b...@hackerdom.ru> wrote:
> This patch introduces Consumer IR(CIR) support for cubietruck. It was ported
> from sunxi linux 3.4 and supports only NEC protocol.
>
> Changes from the original driver:
> - moved kernel's Input Subsystem API
> - initialization code was rewritten
> - using devm_ functions to simplify cleanup
> - moved gpio and timers initialization to *.dts and *.dtsi files
> - using sun7i_ir_data struct to hold driver state instead of global vars
> - obtain mmio adresses dynamicly instead of using fixed adresses like
> 0xf1c200b0
>
> It was tested on 20 A20 boards. Now it only works on Cubietrucks, but I
> believe
> it is not hard to make it work on other sunxi architectures.
>
> Alexander Bersenev, Institute of Mathematics and Mechanics, Russia
>
> Signed-off-by: Alexander Bersenev <b...@hackerdom.ru>
This should be more like:
= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = =
This patch introduces Consumer IR(CIR) support for cubietruck. It
supports only NEC protocol.
It's ported from the original open source driver supplied by Allwinner.
Changes from original driver:
- .
- .
- .
Signed-off-by: ....
---
It was tested....
= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = =
Nobody other than us really care if it's only been tested on some
chipsets / boards. That said, if you've only tested it on an A20
board, you should probably restrict it to only build on that, then
other people (or you) will provide updates as it gets tested
elsewhere.
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index ebf6a2f..42e79a9 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -121,6 +121,13 @@
> };
> };
>
> + ir: ir@01c21800 {
> + pinctrl-names = "default";
"default"?
> + pinctrl-0 = <&ir_pin_cubietruck>;
> + gpios = <&pio 1 4 0>;
> + };
> +
> +
Only one blank line here.
> uart0: serial@01c28000 {
> pinctrl-names = "default";
> pinctrl-0 = <&uart0_pins_a>;
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi
> b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 15ea85e..c0202c6 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -85,6 +85,14 @@
> clock-output-names = "pll1";
> };
>
> + pll3: clk@01c20010 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-a10-pll1-clk";
> + reg = <0x01c20010 0x4>;
> + clocks = <&osc24M>;
> + clock-output-names = "pll3";
> + }
> +
> pll4: clk@01c20018 {
> #clock-cells = <0>;
> compatible = "allwinner,sun4i-a10-pll1-clk";
> @@ -302,6 +310,7 @@
> compatible = "allwinner,sun4i-a10-mod0-clk";
> reg = <0x01c200b0 0x4>;
> clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
> + clock-frequency = <3000000>;
Are you sure this is right?
> clock-output-names = "ir0";
> };
>
> @@ -591,6 +600,13 @@
> allwinner,pull = <0>;
> };
>
> + ir_pin_cubietruck: ir_pin@0 {
> + allwinner,pins = "PB4";
> + allwinner,function = "ir0";
> + allwinner,drive = <0>;
> + allwinner,pull = <0>;
> + };
> +
> uart2_pins_a: uart2@0 {
> allwinner,pins = "PI16", "PI17", "PI18", "PI19";
> allwinner,function = "uart2";
> @@ -750,6 +766,14 @@
> status = "disabled";
> };
>
> + ir: ir@01c21800 {
> + compatible = "allwinner,sun7i-a20-ir";
> + reg = <0x01c21800 0x100>;
> + interrupts = <0 5 4>;
> + clocks = <&apb0_gates 6>, <&ir0_clk>;
> + };
> +
> +
Another extra blank line.
> sid: eeprom@01c23800 {
> compatible = "allwinner,sun7i-a20-sid";
> reg = <0x01c23800 0x200>;
> @@ -931,5 +955,14 @@
> #interrupt-cells = <3>;
> interrupts = <1 9 0xf04>;
> };
> +
> + timer {
> + compatible ="arm,armv7-timer";
> + interrupts = <1 13 0x308>,
> + <1 14 0x308>,
> + <1 11 0x308>,
> + <1 10 0x308>;
> + clock-frequency = <24000000>;
> + };
Is this used?
> };
> };
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 3be8846..83a20fa 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -1036,11 +1036,17 @@ static void __init sunxi_gates_clk_setup(struct
> device_node *node,
> /* No driver claims this clock, but it should remain gated */
> ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0;
>
> +
Don't add this blank line.
> clk_data->clks[i] = clk_register_gate(NULL, clk_name,
> clk_parent, ignore,
> reg + 4 * (i/32), i % 32,
> 0, &clk_lock);
> WARN_ON(IS_ERR(clk_data->clks[i]));
> +
> + if(!IS_ERR(clk_data->clks[i])) {
> + clk_register_clkdev(clk_data->clks[i], clk_name, NULL);
> + }
What's this supposed to solve? - It should be a separate bug fix at
the very least.
> +
And another.
>
> j++;
> }
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index d8a51cd..53d778f 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -662,4 +662,10 @@ config KEYBOARD_CROS_EC
> To compile this driver as a module, choose M here: the
> module will be called cros_ec_keyb.
>
> +config IR_SUNXI
> + tristate "sunxi IR support"
> +
Add some help text. Just copy what other drivers say and edit it to suit.
Also, if this only works on sun7i, depend on that. - I think you need
some dependencies here.
> endif
> +
> +
> +
Don't add these blank lines.
> diff --git a/drivers/input/keyboard/Makefile
> b/drivers/input/keyboard/Makefile
> index f3265bd..97bc1b7 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_KEYBOARD_TNETV107X) += tnetv107x-keypad.o
> obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
> obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
> +obj-$(CONFIG_IR_SUNXI) += sunxi-ir.o
> diff --git a/drivers/input/keyboard/sunxi-ir.c
> b/drivers/input/keyboard/sunxi-ir.c
> new file mode 100644
> index 0000000..ca37a4b
> --- /dev/null
> +++ b/drivers/input/keyboard/sunxi-ir.c
> @@ -0,0 +1,576 @@
> +/*
> + * drivers/input/keyboard/sunxi-ir.c
> + *
> + * (C) Copyright 2007-2012
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
If you've modified this at all, you should add your copyright here
too. (with the correct year(s))
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
I'm not sure that this is needed. Could someone else more
knowledgeable chime in about this please?
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/keyboard.h>
> +#include <linux/ioport.h>
> +#include <asm/irq.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
All these are needed right? Also, some people prefer these to be
ordered in some form. Putting the asm/* files at the start is a good
start.
> +
> +
Only one blank line
> +#include "ir-keymap.h"
> +
> +#define MAX_KEYS 256
> +
> +#ifdef DEBUG_IR
> +#define DEBUG_IR_LEVEL0
> +#define DEBUG_IR_LEVEL1
> +#define dprintk(level, fmt, arg...) (if (debug >= level) \
> + printk(KERN_DEBUG fmt , ## arg))
> +#else
> +#undef DEBUG_IR_LEVEL0
> +#undef DEBUG_IR_LEVEL1
> +#define dprintk(level, fmt, arg...)
> +#endif
Proper debug stuff please, also use an empty function defined with
__printf() instead of an empty #define.
Also, you shouldn't need a DEBUG_IR #define. Either this should be
part of Kconfig (e.g. CONFIG_IR_SUNXI_DEBUG) or it should go under
some more generic debug config variable.
You should investigate pr_dbg() and friends at the very least.
> +
> +//
// comments are not recommended, see the coding style documentation.
> +/* Registers */
> +#define IR_REG(x) (x)
You don't need this at all.
> +
> +#define IR_CTRL_REG IR_REG(0x00) /* IR Control */
> +#define IR_RXCFG_REG IR_REG(0x10) /* Rx Config */
> +#define IR_RXDAT_REG IR_REG(0x20) /* Rx Data */
> +#define IR_RXINTE_REG IR_REG(0x2c) /* Rx Interrupt Enable */
> +#define IR_RXINTS_REG IR_REG(0x30) /* Rx Interrupt Status */
> +#define IR_SPLCFG_REG IR_REG(0x34) /* IR Sample Config */
> +
> +/* Bit Definition of IR_RXINTS_REG Register */
> +#define IR_RXINTS_RXOF (0x1 << 0) /* Rx FIFO Overflow */
> +#define IR_RXINTS_RXPE (0x1 << 1) /* Rx Packet End */
> +#define IR_RXINTS_RXDA (0x1 << 4) /* Rx FIFO Data Available */
There's a BIT() macro which will simplify this.
> +
> +#ifdef CONFIG_ARCH_SUN5I
> +#define IR_FIFO_SIZE 64 /* 64Bytes */
> +#else
> +#define IR_FIFO_SIZE 16 /* 16Bytes */
> +#endif
Add a blank line here.
> +/* Frequency of Sample Clock = 23437.5Hz, Cycle is 42.7us */
> +/* Pulse of NEC Remote >560us */
One multi-line comment, not two single line comments.
> +#define IR_RXFILT_VAL 8 /* Filter Threshold = 8*42.7 = ~341us < 500us */
> +#define IR_RXIDLE_VAL 2 /* Idle Threshold = (2+1)*128*42.7 = ~16.4ms > 9ms
> */
Is this line over 80 characters? Maybe put the comment on the next
line or explain the math above.
> +
> +#define IR_L1_MIN 80 /* 80*42.7 = ~3.4ms, Lead1(4.5ms) > IR_L1_MIN */
> +#define IR_L0_MIN 40 /* 40*42.7 = ~1.7ms, Lead0(4.5ms) Lead0R(2.25ms)>
> IR_L0_MIN */
> +#define IR_PMAX 26 /* 26*42.7 = ~1109us ~= 561*2, Pluse < IR_PMAX */
> +#define IR_DMID 26 /* 26*42.7 = ~1109us ~= 561*2, D1 > IR_DMID, D0 =<
> IR_DMID */
> +#define IR_DMAX 53 /* 53*42.7 = ~2263us ~= 561*4, D < IR_DMAX */
> +
> +#define IR_RAW_BUF_SIZE 128
> +#define IR_ERROR_CODE 0xffffffff
> +#define IR_REPEAT_CODE 0x00000000
> +#define DRV_VERSION "1.00"
Driver versions are generally not recommended.
> +
> +struct ir_raw_buffer {
> + unsigned long dcnt; /*Packet Count*/
> + unsigned char buf[IR_RAW_BUF_SIZE];
> +};
> +
> +#ifdef DEBUG_IR
> +static int debug = 8;
> +#endif
What's this for?
> +
> +
Only one line here.
> +struct sun7i_ir_data {
> + struct device *dev;
> + struct input_dev *input;
> + void __iomem *base;
> +
> + u32 ir_keycodes[MAX_KEYS];
> + unsigned long ir_code;
> + int timer_used;
> +
> + struct ir_raw_buffer ir_rawbuf;
> +
> + unsigned int ir_cnt;
> + struct timer_list *s_timer;
> +};
If this is sun7i only, why do you have sun5i stuff above? if it's not,
then this struct's name shouldn't reference sun7i.
> +
> +
Another extra line.
> +static inline void ir_reset_rawbuffer(struct sun7i_ir_data *ir)
> +{
> + ir->ir_rawbuf.dcnt = 0;
> +}
> +
> +static inline void ir_write_rawbuffer(struct sun7i_ir_data *ir, unsigned
> char data)
> +{
> + if (ir->ir_rawbuf.dcnt < IR_RAW_BUF_SIZE)
> + ir->ir_rawbuf.buf[ir->ir_rawbuf.dcnt++] = data;
> + else
> + printk("ir_write_rawbuffer: IR Rx buffer full\n");
> +}
> +
> +static inline unsigned char ir_read_rawbuffer(struct sun7i_ir_data *ir)
> +{
> + unsigned char data = 0x00;
> +
> + if (ir->ir_rawbuf.dcnt > 0)
> + data = ir->ir_rawbuf.buf[--ir->ir_rawbuf.dcnt];
> +
> + return data;
You don't need the data variable.
> +}
> +
> +static inline int ir_rawbuffer_empty(struct sun7i_ir_data *ir)
> +{
> + return (ir->ir_rawbuf.dcnt == 0);
> +}
> +
> +static inline int ir_rawbuffer_full(struct sun7i_ir_data *ir)
> +{
> + return (ir->ir_rawbuf.dcnt >= IR_RAW_BUF_SIZE);
Parentheses aren't needed here.
I'm stopping here as I don't know enough about the input subsystem /
hardware to adequately review the rest.
You really need to split this up and run checkpatch over all of it
before you resubmit.
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
