Message-ID: <5255495A.6040704@st.com>
Date:	Wed, 9 Oct 2013 13:17:30 +0100
From:	srinivas kandagatla <srinivas.kandagatla@...com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Mauro Carvalho Chehab <m.chehab@...sung.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

>>>>  .../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.

>>>
>>> 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?

>>
>> 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.

The possibility of device supporting something other than
these("infrared" and "uhf")  L1 protocols is very little, unless a new
remote control PHY protocol comes up. I would like to know if there is a
new one you are already aware of.

>>
>> 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 would say that it is still one device, one which happens to have two
> outputs. Just because we want two dev nodes does not mean the dt has to
> be structured as two devices.
> 
>>
>>> 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";
};

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.

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.

example:

my_keymap: keymap {
	rc-keymap-name = "my-keymap";
	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,
srini

> 
>>
>> 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.
> 
> While it's true that some description is necessary, I do nto believe
> this is the right one.
> 
> Thanks,
> Mark.
> 
> 

--
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