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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
