Re: [PATCH RFC] media: rc: OF: Add Generic bindings forremote-control

[Mark Rutland]

On Wed, Oct 09, 2013 at 01:17:30PM +0100, srinivas kandagatla wrote:
> >>>>  .../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?
> 
> I think for properties like interrupts, reg can be left undocumented
> here and let the actual device bindings document on this.

If it has no particur meaning for the class of binding, leaving it to
the individual device bindings seems fine to me.

> 
> >>>
> >>> 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? 
> 
> Mauro C. had an option that this is not a real use-case and let's not
> overdesign the API, thinking on a possible scenario that may never happen.
> 
> Do you still think that this use case should be considered in this
> discussion?

Given how simple a device we're attempting to describe, I'm not even
sure it makes sense to have a class of binding. We could leave this to
individual device bindings for the moment.

> 
> >>
> >> 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.
> > 
> > Why? If an IP block happens to have support for N connections, that
> > doesn't mean that each must be described individually. They likely share
> > a bank of registers, and depending on the device they might not even be
> > assigned consistently orgranised windows of that register bank.
> > 
> >>
> >>> What if it supports something other than
> >>> "infrared" or "uhf"? What if a device can only be wired up as
> >>> "infrared"? 
> 
> I think "infrared" and "uhf" covers all the use cases for remote controls.
> 
> If the device only supports one mode. It can be documented in device
> specific bindings. something like phy-mode of ethernet bindings.

Given there's the possibility of a device supporting one mode, I think
it would make more sense to describe the *-mode properties in the
specific bindings which require them (though they may be identical).

> >>
> >>> 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.
> > 
> > To be used by another OS, they should be defined somewhere that's not
> > subject to arbitrary changes at any time at the whim of Linux
> > developers, without dt-related review.
> > 
> > That's not to say we couldn't use strings the kernel happened to use.
> > I'm saying that the names exposed by bindings should be well-defined,
> > and should not depend on the current state of a linux-internal header
> > file.
> > 
> > I think it would be nicer to have a way of defining the keymap in dt
> > anyway, so as to handle the general case and not get into the mess of
> > having an arbitrary set of strings we need to constantly update.
> > 
> >> 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;
> > 
> > The kernel uses the strings, so it has them defined in its include path
> > somewhere. If dt bindings wish to use the strings, they should be
> > defined somewhere. That somewhere should not be a Linux-internal header
> > file.
> > 
> >>
> >> 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).
> > 
> > The name of each table is not exposed to userspace, they are not defined
> > under include/uapi. The fact that the names may be used to request other
> > data does not change the fact that the kernel has one copy, userspace
> > another. The set of strings the kernel understands *is* hard-wired.
> > 
> >>
> >> 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 is possible to handle setting up the mapping within the kernel, or to
> > actually describe the general case, something like how gpio-keys works.
> > I think that would be preferable.
> 
> Am sure we can do something on gpio-keys lines. This will be one more
> method to load the keymaps to rc system.
> 
> Drivers can decide if they want a static keymaping which is already
> compiled in kernel or a custom one which can be loaded via DT or user space.
> 
> my_keymap: keymap {
> 	rc-keymap-name = "my-keymap";
> 	rc-codes	= 	<0x12, KEY_POWER,
> 				0x01, KEY_TV,
> 				0x15, KEY_DVD>;
> 		...
>  };
> 
> my-rc-device {
> 	compatible = "my,rc-device";
> 	rc-keymap = <&my_keymap>;
> 	rx-mode  = "infrared";
> };

This may be ok, but we'll need to nail down the kaymap binding..

> 
> Mauro, Does this sound Ok to you, and correct me if I miss understood
> some thing here.
> 
> 
> So at the end the bindings doc will look something like:
> 
> == Generic device tree bindings for remote control ==
> 
> properties:
> 	- rx-mode: Can be "infrared" or "uhf". This property specifies the L1
> protocol used for receiving the remote control signals. Some devices may
> support only one of the modes, this restriction should be  documented in
> there respective device binding document.
> 
> 	- tx-mode: Can be "infrared" or "uhf". This property specifies the L1
> protocol used for transmitting remote control signals. Some devices may
> support only one of the modes, this restriction should be  documented in
> there respective device binding document.

I think it would be better to place these in the specific bindings for
the devices which need them.

> 
> Optional properties:
> 	- rc-keymap: phandle to remote control keymap.
> 
> == Remote control Keymaps ==
> 
> properties:
> 	- rc-keymap-name: 	Should be the name of the keymap.
> 	- rc-keymaps:		Should be an array of pair of scan code and actual key
> code with first cell representing rc scan code and second cell
> representing actual keycode.

Is one cell always enough for any scan code (or any actual keycode)?

As the format of the scan code will be device-specific, should this be
under the node for the device? Are we likely to have multiple rc devices
in a single system that can share a keymap?

What's the format of the actual keycode? What values are valid?

> 
> example:
> 
> my_keymap: keymap {
> 	rc-keymap-name = "my-keymap";
> 	rc-keymaps	= 	<0x12, KEY_POWER,
> 				0x01, KEY_TV,
> 				0x15, KEY_DVD>;
> 		...
>  };

Please bracket list entries individually (it makes it far easier for
humans to read arbitrary lists in dt, regardless of how consistent this
may be).

Also, commas shouldn't be between brackets, dtc will barf if they're
there.

So this should be something like:

	rc-keymaps = <0x12 KEY_POWER>,
	             <0x01 KEY_TV>,
	             <0x15 KEY_DVD>;

> 
> my-rc-device {
> 	compatible = "my,rc-device";
> 	rc-keymap = <&my_keymap>;
> 	rx-mode  = "infrared";
> };

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