Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Arun,

On 07/09/2013 02:04 PM, Arun Kumar K wrote:
> On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuil<hverkuil@xs4all.nl>  wrote:
>> On Fri May 31 2013 15:03:23 Arun Kumar K wrote:
>>> FIMC-IS uses certain sensors which are exclusively controlled
>>> from the IS firmware. This patch adds the sensor subdev for the
>>> fimc-is sensors.
>>>
>>> Signed-off-by: Arun Kumar K<arun.kk@samsung.com>
>>> Signed-off-by: Kilyeon Im<kilyeon.im@samsung.com>
>>
>> Not surprisingly I really hate the idea of sensor drivers that are tied to
>> a specific SoC, since it completely destroys the reusability of such drivers.
>>
>
> Yes agree to it.
>
>> I understand that you have little choice to do something special here, but
>> I was wondering whether there is a way of keeping things as generic as
>> possible.
>>
>> I'm just brainstorming here, but as far as I can see this driver is basically
>> a partial sensor driver: it handles the clock, the format negotiation and
>> power management. Any sensor driver needs that.
>>
>> What would be nice is if the fmic specific parts are replaced by callbacks
>> into the bridge driver using v4l2_subdev_notify().
>>
>> The platform data (or DT) can also state if this sensor is firmware controlled
>> or not. If not, then the missing bits can be implemented in the future by
>> someone who needs that.
>>
>> That way the driver itself remains independent from fimc.
>>
>> And existing sensor drivers can be adapted to be usable with fimc as well by
>> adding support for the notify callback.
>>
>> Would a scheme along those lines work?
>>
>
> Yes this should make the implementation very generic.
> Will check the feasibility of this approach.

Is I suggested earlier, you likely could do without this call back to the
FIMC-IS from within the sensor subdev. Look at your call chain right now:

  /dev/video?     media-dev-driver    sensor-subdev         FIMC-IS
     |                 |                   |                  |
     | VIDIOC_STREAMON |                   |                  |
     |---------------->#     s_stream()    |                  |
     |                 #------------------># pipeline_open()  |
     |                 |                   # ---------------->|
     |                 |                   # pipeline_start() |
     |                 |                   # ---------------->|
     |                 |                   |                  |

Couldn't you move pipeline_open(), pipeline_start() to s_stream handler
of the ISP subdev ? It is currently empty. The media device driver could
call s_stream on the ISP subdev each time it sees s_stream request on
the sensor subdev. And you wouldn't need any hacks to get the pipeline
pointer in the sensor subdev. Then it would be something like:

  /dev/video?     media-dev-driver    sensor-subdev  FIMC-IS-ISP-subdev
     |                 |                   |             |
     | VIDIOC_STREAMON |                   |             |
     |---------------->#     s_stream()    |             |
     |                 #------------------>|             |
     |                 #     s_stream()    |             |
     |                 #-------------------+------------># pipeline_open()
     |                 |                   |             # pipeline_start()
     |                 |                   |             #

I suppose pipeline_open() is better candidate for the s_power callback.
It just needs to be ensured at the media device level the subdev
operations sequences are correct.

--
Regards,
Sylwester