Re: [PATCH 04/18] media: v4l: async: Make V4L2 async match information a struct

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

Hi Sakari,

Thank you for the patch.

On Thu, Mar 30, 2023 at 02:58:39PM +0300, Sakari Ailus wrote:
> Make V4L2 async match information a struct, making it easier to use it
> elsewhere outside the scope of struct v4l2_async_subdev.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c  | 18 ++++++-------
>  drivers/media/v4l2-core/v4l2-fwnode.c |  2 +-
>  include/media/v4l2-async.h            | 39 ++++++++++++++++-----------
>  3 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 13fe0bdc70b6..bb78e3618ab5 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -147,7 +147,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
>  
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
> -		switch (asd->match_type) {
> +		switch (asd->match.type) {
>  		case V4L2_ASYNC_MATCH_I2C:

Renaming V4L2_ASYNC_MATCH_* to V4L2_ASYNC_MATCH_TYPE_* would be nice in
a separate patch.

>  			match = match_i2c;
>  			break;
> @@ -172,10 +172,10 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
>  static bool asd_equal(struct v4l2_async_subdev *asd_x,
>  		      struct v4l2_async_subdev *asd_y)
>  {
> -	if (asd_x->match_type != asd_y->match_type)
> +	if (asd_x->match.type != asd_y->match.type)
>  		return false;
>  
> -	switch (asd_x->match_type) {
> +	switch (asd_x->match.type) {
>  	case V4L2_ASYNC_MATCH_I2C:
>  		return asd_x->match.i2c.adapter_id ==
>  			asd_y->match.i2c.adapter_id &&
> @@ -494,7 +494,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  	if (!asd)
>  		return -EINVAL;
>  
> -	switch (asd->match_type) {
> +	switch (asd->match.type) {
>  	case V4L2_ASYNC_MATCH_I2C:
>  	case V4L2_ASYNC_MATCH_FWNODE:
>  		if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) {
> @@ -504,7 +504,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  		break;
>  	default:
>  		dev_err(dev, "Invalid match type %u on %p\n",
> -			asd->match_type, asd);
> +			asd->match.type, asd);
>  		return -EINVAL;
>  	}
>  
> @@ -630,7 +630,7 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier)
>  		return;
>  
>  	list_for_each_entry_safe(asd, tmp, &notifier->asd_list, asd_list) {
> -		switch (asd->match_type) {
> +		switch (asd->match.type) {
>  		case V4L2_ASYNC_MATCH_FWNODE:
>  			fwnode_handle_put(asd->match.fwnode);
>  			break;
> @@ -685,7 +685,7 @@ __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier,
>  	if (!asd)
>  		return ERR_PTR(-ENOMEM);
>  
> -	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	asd->match.type = V4L2_ASYNC_MATCH_FWNODE;
>  	asd->match.fwnode = fwnode_handle_get(fwnode);
>  
>  	ret = __v4l2_async_nf_add_subdev(notifier, asd);
> @@ -732,7 +732,7 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id,
>  	if (!asd)
>  		return ERR_PTR(-ENOMEM);
>  
> -	asd->match_type = V4L2_ASYNC_MATCH_I2C;
> +	asd->match.type = V4L2_ASYNC_MATCH_I2C;
>  	asd->match.i2c.adapter_id = adapter_id;
>  	asd->match.i2c.address = address;
>  
> @@ -850,7 +850,7 @@ EXPORT_SYMBOL(v4l2_async_unregister_subdev);
>  static void print_waiting_subdev(struct seq_file *s,
>  				 struct v4l2_async_subdev *asd)
>  {
> -	switch (asd->match_type) {
> +	switch (asd->match.type) {
>  	case V4L2_ASYNC_MATCH_I2C:
>  		seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id,
>  			   asd->match.i2c.address);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3d9533c1b202..e6bd63364bed 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -811,7 +811,7 @@ v4l2_async_nf_fwnode_parse_endpoint(struct device *dev,
>  	if (!asd)
>  		return -ENOMEM;
>  
> -	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	asd->match.type = V4L2_ASYNC_MATCH_FWNODE;
>  	asd->match.fwnode =
>  		fwnode_graph_get_remote_port_parent(endpoint);
>  	if (!asd->match.fwnode) {
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 25eb1d138c06..0c4cffd081c9 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -34,23 +34,38 @@ enum v4l2_async_match_type {
>  };
>  
>  /**
> - * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> + * struct v4l2_async_match - async sub-device match information

The new structure name sounds like it stores data about an actual match,
while in reality it stores information for matching subdevs with
notifier entries. How about naming the structure v4l2_async_match_info,
or v4l2_async_match_descriptor or v4l2_async_match_desc ?

>   *
> - * @match_type:	type of match that will be used
> - * @match:	union of per-bus type matching data sets
> - * @match.fwnode:
> + * @type:	type of match that will be used
> + * @fwnode:
>   *		pointer to &struct fwnode_handle to be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_FWNODE.
> - * @match.i2c:	embedded struct with I2C parameters to be matched.
> + * @i2c:	embedded struct with I2C parameters to be matched.
>   *		Both @match.i2c.adapter_id and @match.i2c.address
>   *		should be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> - * @match.i2c.adapter_id:
> + * @i2c.adapter_id:
>   *		I2C adapter ID to be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> - * @match.i2c.address:
> + * @i2c.address:
>   *		I2C address to be matched.
>   *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> + */
> +struct v4l2_async_match {
> +	enum v4l2_async_match_type type;
> +	union {
> +		struct fwnode_handle *fwnode;
> +		struct {
> +			int adapter_id;
> +			unsigned short address;
> +		} i2c;
> +	};
> +};
> +
> +/**
> + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> + *
> + * @match:	struct of match type and per-bus type matching data sets
>   * @asd_list:	used to add struct v4l2_async_subdev objects to the
>   *		master notifier @asd_list
>   * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> @@ -61,15 +76,7 @@ enum v4l2_async_match_type {
>   * v4l2_async_subdev as its first member.
>   */
>  struct v4l2_async_subdev {
> -	enum v4l2_async_match_type match_type;
> -	union {
> -		struct fwnode_handle *fwnode;
> -		struct {
> -			int adapter_id;
> -			unsigned short address;
> -		} i2c;
> -	} match;
> -
> +	struct v4l2_async_match match;
>  	/* v4l2-async core private: not to be used by drivers */
>  	struct list_head list;
>  	struct list_head asd_list;

-- 
Regards,

Laurent Pinchart