Re: [PATCH v3] media: st-rc: Add ST remote control driver

Thanks Mark,

On 16/09/13 15:10, Mark Rutland wrote:

quoted

+

quoted

+Required properties:
+       - compatible: should be "st,comms-irb".

This should probably say "should contain" rather than "should be". There
may be future vairants of this device, which will also have a more
specific compatible string.

Ok, will change it to the suggest.

quoted

quoted

+       - reg: base physical address of the controller and length of memory
+       mapped  region.
+       - interrupts: interrupt number to the cpu. The interrupt specifier
+       format depends on the interrupt controller parent.

I don't like the phrase "interrupt number to the cpu". We already have
the term interrupt-specifier to precisely define this. How about:

- interrupts: interrupt-specifier for the sole interrupt generated by
              the device.

TBH, I did copy them from one of the existing bindings docs.
I will change it.

quoted

quoted

+
+Optional properties:
+       - rx-mode: can be "infrared" or "uhf".
+       - tx-mode: should be "infrared".

Are these required to use rx/tx?

Yes, these are required for driver to be in rx/tx mode.

One of them can be optional depending on the board setup.
So, Is it ok to move such properties to required properties section?

If you don't have a tx-mode or rx-mode, I assume you can't do
anything...

Yes, driver errs out.

quoted

quoted

+       - pinctrl-names, pinctrl-0: the pincontrol settings to configure
+       muxing properly for IRB pins.

If we're expecting names, the names we expect should be defined.

quoted

quoted

+       - clocks : phandle of clock.

This is not just a phandle. This is a phandle + clock-specifier pair.

Yep, will change it.

Thanks,
srini

Cheers,

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html