[PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure

[Sakari Ailus]

Hi Mauro,

(Removing the non-list recipients.)

On Fri, Sep 29, 2017 at 06:27:13AM -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Sep 2017 15:09:21 +0300
> Sakari Ailus <sakari.ailus at iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote:
> > > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> > > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> > > match criteria requires just a device name.
> > > 
> > > So, it doesn't make sense to enclose those into structs,
> > > as the criteria can go directly into the union.
> > > 
> > > That makes easier to document it, as we don't need to document
> > > weird senseless structs.  
> > 
> > The idea is that in the union, there's a struct which is specific to the
> > match_type field. I wouldn't call it senseless.
> 
> Why a struct for each specific match_type is **needed**? It it is not
> needed, then it is senseless per definition :-) 
> 
> In the specific case of fwnode, there's already a named struct
> for fwnode_handle. The only thing is that it is declared outside
> enum v4l2_async_match_type. So, I don't see any reason to do things
> like:
> 
> 		struct {
> 			struct fwnode_handle *fwnode;
> 		} fwnode;
> 
> If you're in doubt about that, think on how would you document
> both fwnode structs. Both fwnode structs specify the match
> criteria if %V4L2_ASYNC_MATCH_FWNODE.
> 
> The same applies to this:
> 
> 		struct {
> 			const char *name;
> 		} device_name;
> 
> Both device_name and name specifies the match criteria if
> %V4L2_ASYNC_MATCH_DEVNAME.
> 
> > 
> > In the two cases there's just a single field in the containing struct. You
> > could remove the struct in that case as you do in this patch, and just use
> > the field. But I think the result is less clean and so I wouldn't make this
> > change.
> 
> It is actually cleaner without the stucts.
> 
> Without the useless struct, if one wants to match a firmware node, it
> should be doing:
> 
>  		pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> 		pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);

This code should be and will be moved out of drivers. See:

<URL:http://www.spinics.net/lists/linux-media/msg122688.html>

So there are going to be quite a bit fewer instances of it, and none should
remain in drivers. I frankly don't have a strong opinion on this; there are
arguments for and against. I just don't see a reason to change it.

It'd be nice to have Hans's and Laurent's opinion though.

> 
> And that' it. For anyone that reads the above code, even not knowing
> all details of "match", is clear that the match criteria is whatever
> of_fwnode_handle() returns.
> 
> Now, on this:
> 
>  		pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> 		pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
> 
> It sounds that something is missing, as only one field of match.fwnode
> was specified. Anyone not familiar with that non-conventional usage of
> a struct with just one struct field inside would need to seek for the
> header file declaring the struct. That would consume a lot of time for
> code reviewers for no good reason.
> 
> The same apply for devname search:
> 
> In this case:
> 		asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> 		asd->match.device_name.name = imxsd->devname;
> 
> I would be expecting something else to be filled at device_name's
> struct, for example to specify a case sensitive or case insensitive
> match criteria, to allow seeking for a device's substring, or to
> allow using other struct device fields to narrow the seek.
> 
> With this:
> 
> 		asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> 		asd->match.device_name = imxsd->devname;
> 
> It is clear that the match criteria is fully specified.
> 
> > The confusion comes possibly from the fact that the struct is named the
> > same as the field in the struct. These used to be called of and node, but
> > with the fwnode property framework the references to the fwnode are, well,
> > typically similarly called "fwnode". There's no underlying firmware
> > interface with that name, fwnode property API is just an API.
> 
> The duplicated "fwnode" name only made it more evident that we don't
> need to enclose a single match criteria field inside a struct.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi