On 02/13/2017 04:31 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Monday 13 Feb 2017 15:19:13 Hans Verkuil wrote:
>> Hi Niklas,
>>
>> One general remark: in many commit logs you mistype 'subdeivce'. Can you
>> fix that for the v2?
>>
>> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
>>> Hi,
>>>
>>> This series address issues with the R-Car Gen2 VIN driver. The most
>>> serious issue is the OPS when unbind and rebinding the i2c driver for
>>> the video source subdevice which have popped up as a blocker for other
>>> work.
>>>
>>> This series is broken out of my much larger R-Car Gen3 enablement series
>>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I
>>> plan to remove that series form patchwork and focus on these fixes first
>>> as they are blocking other development. Once the blocking issues are
>>> removed I will rebase and repost the larger Gen3 series.
>>>
>>> Patch 1-4 fix simple problems found while testing
>>>
>>> 1-2 Fix format problems when the format is (re)set.
>>> 3 Fix media pad errors
>>> 4 Fix standard enumeration problem
>>>
>>> Patch 5 adds a wrapper function to retrieve the active video source
>>> subdevice. This is strictly not needed on Gen2 which only have one
>>> possible video source per VIN instance (This will change on Gen3). But
>>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as
>>> not risk breaking things by removing this wrapper in this series and
>>> then readding it in a later Gen3 series I have chosen to keep the patch.
>>> Please let me know if I should drop it and rewrite patch 6-11 (if
>>> possible I would like to avoid that).
>>>
>>> Patch 6-8 deals with video source subdevice pad index handling by moving
>>> the information from struct rvin_dev to struct rvin_graph_entity and
>>> moving the pad index probing to the struct v4l2_async_notifier complete
>>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
>>>
>>> Patch 9 use the pad information when calling enum_mbus_code.
>>>
>>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
>>
>> This will not help: you can unbind a subdev at any time, including when
>> it is in use.
>>
>> But why do you need this at all? You can also set suppress_bind_attrs in
>> the adv7180 driver to prevent the bind/unbind files from appearing.
>>
>> It really makes no sense for subdevs. In fact, all subdevs should set this
>> flag since in the current implementation this is completely impossible to
>> implement safely.
>>
>> I suggest you drop the patches relating to this and instead set the suppress
>> flag.
>
> The adv7180 is connected to an I2C controller that can be unbound. Setting
> the
> suppress_bind_attrs flag in the driver thus won't prevent the device from
> being unbound. suppress_bind_attrs is not a good solution for I2C drivers.
Then just drop these patches, since those don't solve anything either. Without
some of the things we discussed during the refcounting meeting (i.e. a locking
scheme before calling into subdev ops) anything you do will at best just give
you an illusion that you're safe.
Regards,
Hans
