Em Mon, 03 Nov 2014 21:54:53 +0100
Beniamino Galvani <b.galvani@xxxxxxxxx> escreveu:
> 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?
Yes, this is 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.
Ok, thanks!
>
> >
> >
> > >
> > > 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
