Message-id: <20130927104719.6637368f@samsung.com>
Date:	Fri, 27 Sep 2013 10:47:19 -0300
From:	Mauro Carvalho Chehab <m.chehab@...sung.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Srinivas KANDAGATLA <srinivas.kandagatla@...com>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Pawel Moll <Pawel.Moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] media: rc: OF: Add Generic bindings for remote-control

Em Fri, 27 Sep 2013 12:34:58 +0100
Mark Rutland <mark.rutland@....com> escreveu:

> On Fri, Sep 27, 2013 at 10:33:11AM +0100, Srinivas KANDAGATLA wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla@...com>
> > 
> > This patch attempts to collate generic bindings which can be used by
> > the remote control hardwares. Currently the list is not long as there
> > are only 2 drivers which are device tree'd.
> > 
> > Mainly this patch tries to document few bindings used by ST IRB driver
> > which can be generic as well. This document also add fews common
> > bindings used by most of the drivers like, interrupts, regs, clocks and
> > pinctrls.
> > 
> > This document can also be holding place to describe generic bindings
> > used in remote controls devices.
> > 
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...com>
> > ---
> > Hi All, 
> > Following Stephen Warren's suggestions at https://lkml.org/lkml/2013/9/24/452
> > this patch is an attempt to document such generic bindings in a common
> > document.
> > 
> > This document currently collates all the generic bindings used with
> > remote-controls and act as holding place to describe generic bindings for
> > remote controls.
> > 
> > Comments?
> > 
> > Thanks,
> > srini
> > 
> >  .../devicetree/bindings/media/remote-control.txt   |   31 ++++++++++++++++++++
> >  1 files changed, 31 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/remote-control.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/remote-control.txt b/Documentation/devicetree/bindings/media/remote-control.txt
> > new file mode 100644
> > index 0000000..901ea56
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/remote-control.txt
> > @@ -0,0 +1,31 @@
> > +Generic device tree bindings for remote control.
> > +
> > +properties:
> > +	- compatible: Can contain any remote control driver compatible string.
> > +	  example: "st-comms-irb, "gpio-ir-receiver".
> 
> This is more generic than remote control, could this not just be left
> for the specific binding to describe?
> 
> > +	- reg: Base physical address of the controller and length of memory
> > +	  mapped region.
> 
> What if it's on a bus that isn't memory mapped (e.g. i2c, SPI)?
> 
> > +	- interrupts: Interrupt-specifier for the sole interrupt generated by
> > +	  the device. The interrupt specifier format depends on the
> > +	  interrupt controller parent. Iff the device supports interrupts.
> 
> What if it has multiple interrupts, and has interrupts-names?
> 
> It might be better to only describe the properties that relate
> specifically to remote controls, rather than listing all of the generic
> properties that device tree bidnings may have. That would match the
> style of the clock bindings.
> 
> > +	- rx-mode: Can be "infrared" or "uhf". rx-mode should be present iff
> > +	  the rx pins are wired up.
> 
> I'm unsure on this. What if the device has multiple receivers that can
> be independently configured? 

Well, if a given remote controller hardware has more than one independent
receiver (or transmitter), each one should have its own devnode.
Likely, two entries at DT.

> What if it supports something other than
> "infrared" or "uhf"? What if a device can only be wired up as
> "infrared"? 

I would say that a hardware that has both infrared and uhf has actually
two different devices. So, it should be mapped as two separate devnodes.

> I'm not sure how generic these are, though we should certainly encourage
> bindings that can be described this way to be described in the same way.
> 
> > +	- tx-mode: Can be "infrared" or "uhf". tx-mode should be present iff
> > +	  the tx pins are wired up.
> 
> I have similar concerns here to those for the rx-mode property.
> 
> > +
> > +Optional properties:
> > +	- linux,rc-map-name: Linux specific remote control map name. Refer to
> > +	  include/media/rc-map.h for full list of maps.
> 
> We shouldn't refer to Linux specifics (i.e. headers) in general in
> bindings. While it's possible to relax that a bit for linux,*
> properties, I'd prefer to explicitly list elements in the binding. That
> prevents the ABI from changing under our feet by someone altering what
> looks like a completely internal header file.

Well, the remote controller keymaps at include/media/rc-map.h is just a
bunch of string names, defined as macro to avoid duplicating those names
everywhere, to avoid typos and to help some userspace parsing logic to get
all in just one single place. I don't see why the same names couldn't be 
used on any other OS using DT.

The logic behind include/media/rc-map.h, is that those names are used
by:

1) kernelspace: in order to locate a keytable with the same name, that
would be loaded when the device is initialized;

2) userspace: to seek for a keytable with that name, allowing to
dynamically load the keymap table on userspace, instead of hardwiring
them on Kernelspace (or replacing the kernel's one by an user-customized
one).

So, I would simply call it as "keymap-name", keep pointing it to 
include/media/rc-map.h.

That's said, this is actually a mandatory requirement, as without it,
the RC core will not be able to load a keytable, and the userspace tool
won't load the proper keymap, being confused on what to do.

It should be noticed that, from time to time, manufacturers change the
remote control unit, as those devices are generally manufactured by a
third part. So, they change, for example, when they get a new BID to
provide IRs for a cheaper cost, or when they need/want to provide a "deluxe"
remote, a simplified "thin" one and/or when they need to provide customized
remotes to some Cable company, for example.

So, it makes sense for it to be mandatory, as only with this information it
is possible to load the keymap that matches that specific IR unit model.

In other words, I would add this as a mandatory property, with a text
similar to:

	- keymap-name: Name of the keyboard scancode table used to match 
	  the remote controller unit model (or model set). Please add it at
	  include/media/rc-map.h for newer models.

It could be interesting to also warn there that, when the remotes are
made exclusively to a given hardware vendor, and the entries are not
incompatible, the same keymap name could be used by more than one remote
controller unit.

> > +	- pinctrl-names, pinctrl-0: The pincontrol settings to configure muxing
> > +	  properly for the device pins.
> > +	- clocks : phandle with clock-specifier pair for the device specified
> > +	  in compatible.
> 
> While devices may have these, they're also more general than remote
> control devices. I'm not sure that they need to be listed here when they
> need to be described fully in any binding that needs them anyway,
> especially as this gives an impression that they are valid for bindings
> that don't need them.
> 
> I think what we actually need to document is the process of creating a
> binding in such a way as to encourage uniformity. Something like the
> following steps:
> 
> 1. Look to see if a binding already exists. If so, use it.
> 
> 2. Is there a binding for a compatible device? If so, use/extend it.
> 
> 3. Is there a binding for a similar (but incompatible) device? Use it as
>    a template, possibly factor out portions into a class binding if
>    those portions are truly general.
> 
> 4. Is there a binding for the class of device? If so, build around that,
>    possibly extending it.
> 
> 5. If there's nothing relevant, create a binding aiming for as much
>    commonality as possible with other devices of that class that may
>    have bindings later.
> 
> Cheers,
> Mark.
> 
> > +
> > +example:
> > +
> > +	rc: rc@...18000 {
> > +		compatible	= "st,comms-irb";
> > +		reg		= <0xfe518000 0x234>;
> > +		interrupts	= <0 203 0>;
> > +		rx-mode		= "infrared";
> > +	};
> > -- 
> > 1.7.6.5
> > 
> > 


-- 

Cheers,
Mauro
--
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/

Powered by blists - more mailing lists