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

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Sep 18, 2013 at 11:11:29AM +0100, Srinivas KANDAGATLA wrote:
> 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.

There's a lot of inconsistency with existing docs, it would be nice to
use consistent, correct terminology going forward :)

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

I'd probably just make a note on each stating what they mean (i.e. that
rx-mode should be present iff the rx pins are wired up).

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

Cheers.

Mark.