Re: [PATCH v6 00/17] media: imx: Switch to subdev notifiers

2018-07-24 Thread Sakari Ailus
Hi Steve,

On Mon, Jul 09, 2018 at 03:39:00PM -0700, Steve Longerbeam wrote:
> This patchset converts the imx-media driver and its dependent
> subdevs to use subdev notifiers.
> 
> There are a couple shortcomings in v4l2-core that prevented
> subdev notifiers from working correctly in imx-media:
> 
> 1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
>endpoint that is not connected to a remote device as an error.
>But in the case of the video-mux subdev, this is not an error,
>it is OK if some of the mux inputs have no connection. Also,
>Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
>states that the 'remote-endpoint' property is optional. So the first
>patch is a small modification to ignore empty endpoints in
>v4l2_async_notifier_fwnode_parse_endpoint() and allow
>__v4l2_async_notifier_parse_fwnode_endpoints() to continue to
>parse the remaining port endpoints of the device.
> 
> 2. In the imx-media graph, multiple subdevs will encounter the same
>upstream subdev (such as the imx6-mipi-csi2 receiver), and so
>v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
>multiple times. This is treated as an error by
>v4l2_async_notifier_register() later.
> 
>To get around this problem, add an v4l2_async_notifier_add_subdev()
>which first verifies the provided asd does not already exist in the
>given notifier asd list or in other registered notifiers. If the asd
>exists, the function returns -EEXIST and it's up to the caller to
>decide if that is an error (in imx-media case it is never an error).
> 
>Patches 2-5 deal with adding that support.
> 
> 3. Patch 6 adds v4l2_async_register_fwnode_subdev(), which is a
>convenience function for parsing a subdev's fwnode port endpoints
>for connected remote subdevs, registering a subdev notifier, and
>then registering the sub-device itself.
> 
> 4. Patches 7-14 update the subdev drivers to register a subdev notifier
>with endpoint parsing, and the changes to imx-media to support that.
> 
> 5. Finally, the last 3 patches endeavor to completely remove support for
>the notifier->subdevs[] array in platform drivers and v4l2 core. All
>platform drivers are modified to make use of
>v4l2_async_notifier_add_subdev() and its related convenience functions
>to add asd's to the notifier @asd_list, and any allocation or reference
>to the notifier->subdevs[] array removed. After that large patch,
>notifier->subdevs[] array is stripped from v4l2-async and v4l2-subdev
>docs are updated to reflect the new method of adding asd's to notifiers.
> 
> 
> Signed-off-by: Steve Longerbeam 
> 
> Patches 07-14 (video-mux and the imx patches) are
> Reviewed-by: Philipp Zabel 
> 
> Patches 01-14 are
> Tested-by: Philipp Zabel 
> on i.MX6 with Toshiba TC358743 connected via MIPI CSI-2.
> 
> History:
> 
> v6:
> - Export v4l2_async_notifier_init(), which must be called by all
>   drivers before the first call to v4l2_async_notifier_add_subdev().
>   Suggested by Sakari Ailus.
> - Removed @num_subdevs from struct v4l2_async_notifier, and the macro
>   V4L2_MAX_SUBDEVS. Sugested by Sakari.
> - Fixed a double device node put in vpif_capture.c. Reported by Sakari.
> - Fixed wrong printk format qualifiers in xilinx-vipp.c. Reported by
>   Dan Carpenter.

The patches seem good to me. Still, the changes are non-trivial and to
allow more time for them to stay in the media tree only I'd like to
postpone applying them until we're past the next merge window.

I'll add Philipp's Tested-by: and Reviewed-by: tags then.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi


[PATCH v6 00/17] media: imx: Switch to subdev notifiers

2018-07-09 Thread Steve Longerbeam
This patchset converts the imx-media driver and its dependent
subdevs to use subdev notifiers.

There are a couple shortcomings in v4l2-core that prevented
subdev notifiers from working correctly in imx-media:

1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
   endpoint that is not connected to a remote device as an error.
   But in the case of the video-mux subdev, this is not an error,
   it is OK if some of the mux inputs have no connection. Also,
   Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
   states that the 'remote-endpoint' property is optional. So the first
   patch is a small modification to ignore empty endpoints in
   v4l2_async_notifier_fwnode_parse_endpoint() and allow
   __v4l2_async_notifier_parse_fwnode_endpoints() to continue to
   parse the remaining port endpoints of the device.

2. In the imx-media graph, multiple subdevs will encounter the same
   upstream subdev (such as the imx6-mipi-csi2 receiver), and so
   v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
   multiple times. This is treated as an error by
   v4l2_async_notifier_register() later.

   To get around this problem, add an v4l2_async_notifier_add_subdev()
   which first verifies the provided asd does not already exist in the
   given notifier asd list or in other registered notifiers. If the asd
   exists, the function returns -EEXIST and it's up to the caller to
   decide if that is an error (in imx-media case it is never an error).

   Patches 2-5 deal with adding that support.

3. Patch 6 adds v4l2_async_register_fwnode_subdev(), which is a
   convenience function for parsing a subdev's fwnode port endpoints
   for connected remote subdevs, registering a subdev notifier, and
   then registering the sub-device itself.

4. Patches 7-14 update the subdev drivers to register a subdev notifier
   with endpoint parsing, and the changes to imx-media to support that.

5. Finally, the last 3 patches endeavor to completely remove support for
   the notifier->subdevs[] array in platform drivers and v4l2 core. All
   platform drivers are modified to make use of
   v4l2_async_notifier_add_subdev() and its related convenience functions
   to add asd's to the notifier @asd_list, and any allocation or reference
   to the notifier->subdevs[] array removed. After that large patch,
   notifier->subdevs[] array is stripped from v4l2-async and v4l2-subdev
   docs are updated to reflect the new method of adding asd's to notifiers.


Signed-off-by: Steve Longerbeam 

Patches 07-14 (video-mux and the imx patches) are
Reviewed-by: Philipp Zabel 

Patches 01-14 are
Tested-by: Philipp Zabel 
on i.MX6 with Toshiba TC358743 connected via MIPI CSI-2.

History:

v6:
- Export v4l2_async_notifier_init(), which must be called by all
  drivers before the first call to v4l2_async_notifier_add_subdev().
  Suggested by Sakari Ailus.
- Removed @num_subdevs from struct v4l2_async_notifier, and the macro
  V4L2_MAX_SUBDEVS. Sugested by Sakari.
- Fixed a double device node put in vpif_capture.c. Reported by Sakari.
- Fixed wrong printk format qualifiers in xilinx-vipp.c. Reported by
  Dan Carpenter.

v5:
- see point 5 above.

v4:
- small non-functional code cleanup in video-mux.c.
- strip TODO for comparing custom asd's for equivalence.
- add three more convenience functions: v4l2_async_notifier_add_fwnode_subdev,
  v4l2_async_notifier_add_i2c_subdev, v4l2_async_notifier_add_devname_subdev.
- strip support in v4l2_async_register_fwnode_subdev for sub-devices
  that register from port nodes.

v3:
- code optimization in asd_equal(), and remove unneeded braces,
  suggested by Sakari Ailus.
- add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
- fix an error-out path in v4l2_async_register_fwnode_subdev() that
  forgot to put device.

v2:
- don't pass an empty endpoint to the parse_endpoint callback, 
  v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
  and returns success.
- Fix a couple compile warnings and errors seen in i386 and sh archs.


Steve Longerbeam (17):
  media: v4l2-fwnode: ignore endpoints that have no remote port parent
  media: v4l2: async: Allow searching for asd of any type
  media: v4l2: async: Add v4l2_async_notifier_add_subdev
  media: v4l2: async: Add convenience functions to allocate and add
asd's
  media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev
  media: v4l2-fwnode: Add a convenience function for registering subdevs
with notifiers
  media: platform: video-mux: Register a subdev notifier
  media: imx: csi: Register a subdev notifier
  media: imx: mipi csi-2: Register a subdev notifier
  media: staging/imx: of: Remove recursive graph walk
  media: staging/imx: Loop through all registered subdevs for media
links
  media: staging/imx: Rename root notifier
  media: staging/imx: Switch to v4l2_async_notifier_add_*_subdev
  media: staging/imx: TODO: Remove one assumption about OF graph parsing
  media: platform: Switch to v4l2