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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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@xxxxxxxxxxxxxxx>
> > ---
> >  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;
> 

-- 
Regards,

Sakari Ailus
[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux