Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart