diff mbox series

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

Message ID 20230330115853.1628216-5-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Separate links and async sub-devices | expand

Commit Message

Sakari Ailus March 30, 2023, 11:58 a.m. UTC
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(-)

Comments

Jacopo Mondi April 13, 2023, 4:51 p.m. UTC | #1
Hi Sakari

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:
>  			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
>   *
> - * @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.

These two could be on a single line

>   *		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;

nit: I would maintain a blank line

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j

>  	/* v4l2-async core private: not to be used by drivers */
>  	struct list_head list;
>  	struct list_head asd_list;
> --
> 2.30.2
>
Laurent Pinchart April 25, 2023, 1:10 a.m. UTC | #2
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;
Sakari Ailus April 27, 2023, 10:36 a.m. UTC | #3
Hi Laurent,

On Tue, Apr 25, 2023 at 04:10:57AM +0300, Laurent Pinchart wrote:
> 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.

I'll add one on top.

> 
> >  			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 ?

v4l2_async_match_desc is shorter, I'll use that.

> 
> >   *
> > - * @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;
>
Sakari Ailus April 27, 2023, 10:47 a.m. UTC | #4
Hi Jacopo,

On Thu, Apr 13, 2023 at 06:51:29PM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> 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:
> >  			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
> >   *
> > - * @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.
> 
> These two could be on a single line

Will fix for v2.

> 
> >   *		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;
> 
> nit: I would maintain a blank line

I'll rework some of this still, the comment is actually wrong as none of
these fields are expected to be accessed by drivers.

> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks!

> 
> Thanks
>    j
> 
> >  	/* v4l2-async core private: not to be used by drivers */
> >  	struct list_head list;
> >  	struct list_head asd_list;
diff mbox series

Patch

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:
 			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
  *
- * @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;