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

[Srinivas KANDAGATLA <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>]

Thanks Mark,

On 16/09/13 15:10, Mark Rutland wrote:
>> +
>> > +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.
> 
>> > +       - 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.
>> > +
>> > +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.
> 
>> > +       - 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.
> 
>> > +       - 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