Re: [PATCHv7 2/9] v4l2: video device: Add V4L2_CTRL_CLASS_FM_TX controls

[Eduardo Valentin <eduardo.valentin@nokia.com>]

On Sun, Jun 14, 2009 at 06:59:13PM +0200, ext Hans Verkuil wrote:
> On Sunday 14 June 2009 18:23:41 Trent Piepho wrote:
> > On Sun, 14 Jun 2009, Eduardo Valentin wrote:
> > > >> +/* FM Modulator class control IDs */
> > > >> +#define V4L2_CID_FM_TX_CLASS_BASE      (V4L2_CTRL_CLASS_FM_TX | 0x900)
> > > >> +#define V4L2_CID_FM_TX_CLASS                 (V4L2_CTRL_CLASS_FM_TX | 1)
> > > >> +
> > > >> +#define V4L2_CID_RDS_ENABLED                 (V4L2_CID_FM_TX_CLASS_BASE + 1)
> > > >> +#define V4L2_CID_RDS_PI                              (V4L2_CID_FM_TX_CLASS_BASE + 2)
> > > >> +#define V4L2_CID_RDS_PTY                     (V4L2_CID_FM_TX_CLASS_BASE + 3)
> > > >> +#define V4L2_CID_RDS_PS_NAME                 (V4L2_CID_FM_TX_CLASS_BASE + 4)
> > > >> +#define V4L2_CID_RDS_RADIO_TEXT                      (V4L2_CID_FM_TX_CLASS_BASE + 5)
> > > >
> > > > I think these RDS controls should be renamed to V4L2_CID_RDS_TX_. This makes
> > > > it clear that these controls relate to the RDS transmitter instead of a
> > > > receiver. I would not be surprised to see similar controls appear for an RDS
> > > > receiver in the future.
> > 
> > So there should there be different controls to set the same thing, one set
> > for tx and another for rx?
> 
> Sure. Say some RDS decoder stores the PI in a register. I can imagine that
> we add a V4L2_CID_RDS_RX_PI control for that. Whereas a V4L2_CID_RDS_TX_PI
> control will return the PI sent out by the encoder.
> 
> Currently no such controls exist (or are needed) for an RDS decoder, but I
> wouldn't be surprised at all if we need them at some point in the future.
> 
> > 
> > > >> +#define V4L2_CID_PREEMPHASIS                 (V4L2_CID_FM_TX_CLASS_BASE + 17)
> > > >> +enum v4l2_fm_tx_preemphasis {
> > > >> +     V4L2_FM_TX_PREEMPHASIS_DISABLED         = 0,
> > > >> +     V4L2_FM_TX_PREEMPHASIS_50_uS            = 1,
> > > >> +     V4L2_FM_TX_PREEMPHASIS_75_uS            = 2,
> > > >> +};
> > > >
> > > > I suggest renaming this to V4L2_CID_FM_TX_PREEMPHASIS. There is already a
> > > > similar V4L2_CID_MPEG_EMPHASIS control and others might well appear in the
> > > > future, so I think this name should be more specific to the FM_TX API.
> > 
> > The cx88 driver could get support for setting the fm preemphasis via a
> > control.  I added support via a module option, but a control would be
> > better.  You're saying it shouldn't use this fm preemphasis control?
> 
> Correct. This set the pre-emphasis when transmitting. For receiving you want
> a separate control. Although the enum should be made generic. So FM_TX can be
> removed from the enum.
> 
> Why should we have one rx and one tx control for this? Because you can have
> both receivers and transmitters in one device and you want independent control
> of the two.

Yes, agreed here. There is the possibility to have receiver and transmitter
both in the same device. So, I think it is better to have separated controls.

> 
> It is my believe that the other fm_tx controls are unambiguously transmitter
> related, so I don't think they need a TX prefix. It doesn't hurt if someone
> can double check that, though.

hmm.. I see no problem removing the fmtx prefix of the preemphasis
enum. But, if it is becoming a generic enum, better to check if its
meaning is the same of existing emphasis enum for mpeg.

> 
> Regards,
> 
> 	Hans
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Eduardo Valentin