Em Thu, 28 Sep 2017 15:09:21 +0300
Sakari Ailus [off-list ref] escreveu:

Hi Mauro,

On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote:

quoted

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

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.

Thanks,
Mauro