linux-kernel - Re: [PATCH 1/3] media: rc: add driver for Amlogic Meson IR remote receiver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 3 Nov 2014 21:54:53 +0100
From:	Beniamino Galvani <b.galvani@...il.com>
To:	Mauro Carvalho Chehab <m.chehab@...sung.com>
Cc:	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	Carlo Caione <carlo@...one.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Jerry Cao <jerry.cao@...ogic.com>,
	Victor Wan <victor.wan@...ogic.com>
Subject: Re: [PATCH 1/3] media: rc: add driver for Amlogic Meson IR remote
 receiver

On Mon, Nov 03, 2014 at 11:14:10AM -0200, Mauro Carvalho Chehab wrote:
> Em Sun, 12 Oct 2014 22:01:53 +0200
> Beniamino Galvani <b.galvani@...il.com> escreveu:
> 
> > Amlogic Meson SoCs include a infrared remote control receiver that can
> > operate in two modes: in "NEC" mode the hardware can decode frames
> > using the NEC IR protocol, while in "general" mode the receiver simply
> > reports the duration of pulses and spaces for software decoding.
> > 
> > This is a driver for the IR receiver that uses software decoding of
> > received frames.
> 
> There are a few checkpatch warnings there:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #71: 
> new file mode 100644
> 
> WARNING: Missing a blank line after declarations
> #151: FILE: drivers/media/rc/meson-ir.c:76:
> +	u32 duration;
> +	DEFINE_IR_RAW_EVENT(rawir);

Here the macro is actually a variable definition and so it makes sense
to group it with the other definitions without blank lines. I checked
other rc drivers and many of them have a similar pattern. Could we
consider the warning as a false positive?

> 
> WARNING: DT compatible string "amlogic,meson6-ir" appears un-documented -- check ./Documentation/devicetree/bindings/
> #272: FILE: drivers/media/rc/meson-ir.c:197:
> +	{ .compatible = "amlogic,meson6-ir" },
> 
> total: 0 errors, 3 warnings, 238 lines checked
> 
> patches/lmml_26418_1_3_media_rc_add_driver_for_amlogic_meson_ir_remote_receiver.patch has style problems, please review.
> 
> I'm seeing that the DT patches are there, after this one. The best
> would be to add them before in the series.
> 
> Please add also an entry at the MAINTAINERS file.

I'll reorder the patches and add the maintainer entry.

> 
> 
> > 
> > Signed-off-by: Beniamino Galvani <b.galvani@...il.com>
> > ---
> >  drivers/media/rc/Kconfig    |  11 +++
> >  drivers/media/rc/Makefile   |   1 +
> >  drivers/media/rc/meson-ir.c | 214 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 226 insertions(+)
> >  create mode 100644 drivers/media/rc/meson-ir.c
> > 
> > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> > index 8ce0810..2d742e2 100644
> > --- a/drivers/media/rc/Kconfig
> > +++ b/drivers/media/rc/Kconfig
> > @@ -223,6 +223,17 @@ config IR_FINTEK
> >  	   To compile this driver as a module, choose M here: the
> >  	   module will be called fintek-cir.
> >  
> > +config IR_MESON
> > +	tristate "Amlogic Meson IR remote receiver"
> > +	depends on RC_CORE
> > +	depends on ARCH_MESON
> 
> Please add COMPILE_TEST too, as we want to be able to compile it on
> x86 and other archs, in order to check if the driver builds fine and
> to enable the static analyzers to look into this code.

Ok.

[...]

> > +
> > +	ir->rc->priv = ir;
> > +	ir->rc->input_name = DRIVER_NAME;
> > +	ir->rc->input_phys = DRIVER_NAME "/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;
> 
> I don't like very much the idea of filling it like that. From where those
> numbers came? Could you add a define for them somewhere?

I've seen that other drivers as gpio-ir-recv and sunxi-cir assign
those numbers to the fields of input_id but I couldn't find a
documentation of the meaning. If the assignments are not needed I will
drop them in the next version.

Beniamino
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/