Re: [PATCH v6 3/7] media: i2c: max2175: Add MAX2175 support

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Thu, 8 Jun 2017 09:42:43 +0000
Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> escreveu:

> > Subject: Re: [PATCH v6 3/7] media: i2c: max2175: Add MAX2175 support
> > 
> > Em Wed, 31 May 2017 09:44:53 +0100
> > Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> escreveu:
> >   
> > > +++ b/Documentation/media/v4l-drivers/max2175.rst
> > > @@ -0,0 +1,60 @@
> > > +Maxim Integrated MAX2175 RF to bits tuner driver
> > > +================================================
> > > +
> > > +The MAX2175 driver implements the following driver-specific controls:
> > > +
> > > +``V4L2_CID_MAX2175_I2S_ENABLE``
> > > +-------------------------------
> > > +    Enable/Disable I2S output of the tuner.
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       1 4
> > > +
> > > +    * - ``(0)``
> > > +      - I2S output is disabled.
> > > +    * - ``(1)``
> > > +      - I2S output is enabled.  
> > 
> > Hmm... There are other drivers at the subsystem that use I2S (for audio -
> > not for SDR - but I guess the issue is similar).
> > 
> > On such drivers, the bridge driver controls it directly, being sure that
> > I2S is enabled when it is expecting some data coming from the I2S bus.
> > 
> > On some drivers, there are both I2S and A/D inputs at the bridge chipset.
> > On such drivers, enabling/disabling I2S is done via VIDIOC_S_INPUT (and
> > optionally via ALSA mixer), being transparent to the user if the stream
> > comes from a tuner via I2S or from a directly connected A/D input.
> > 
> > I don't think it is a good idea to enable it via a control, as, if the
> > bridge driver is expecting data via I2S, disabling it will cause timeouts
> > at the videobuf handling.  
> 
> The MAX2175 device is exposed as a v4l2 subdev with tuner ops and can interface with an SDR device. When the tuner is configured, the I2S output is enabled by default. From an independent tuner device perspective, this default behaviour is enough and this control may not be needed/used.
> 
> However, for the use case here, the R-Car DRIF device acts as the main SDR device and the Maxim MAX2175 provides a sub-dev interface with tuner ops.
> 
> +---------------------+                +---------------------+
> |                     |-----SCK------->|CLK                  |   
> |       Master        |-----SS-------->|SYNC  DRIFn (slave)  |
> |      (MAX2175)      |-----SD0------->|D0                   |   
> |                     |-----SD1------->|D1                   |   
> +---------------------+                +---------------------+
> 
> The DRIF device design is such that it involves separate register writes to enable Rx on each of the data line. To keep both the data lines in sync it expects the master device to enable output after both the data line Rx are enabled.
> 
> This level of control is exposed as a feature in the MAX2175 using this control. When interfaced with DRIF this control is used to achieve the desired functionality. When not interfaced with DRIF, the MAX2175 default behaviour does not have to change because of DRIF and hence this I2S control may be unused. Like MAX2175, DRIF is also an independent device and can interface with a different third party tuner. 
> 
> Hence, this I2S enable/disable is exposed as a user control. The end user application (knowing both these devices) is expected to use these controls appropriately. Please let me know if I need to explain anything in further detail.


The usecase is clear. That's exactly what other drivers with I2S do,
except that, on those other drivers, they pass I2S control info via
platform_data (they're not platform drivers).

With those drivers, generic applications work as-is via the standard
video, radio or sdr devnodes, without knowing about I2S.

The main difference here is that you're requiring an specialized
application for this device to work, as a generic one won't be
aware of this device-specific control, and may end by exposing this
"internal" control to the end user. That is OK for embedded usage,
but, as soon as this is used on some non-embedded usecase (with
is likely, as there are several PC consumer products using other
chips from Maxim), we'll have problems.

I guess the solution here is to make such control visible only via the
subdev interface.

Thanks,
Mauro