Em Sat, 30 Sep 2017 01:05:24 +0300
Sakari Ailus [off-list ref] escreveu:

Hi Mauro,

(Removing the non-list recipients.)

On Fri, Sep 29, 2017 at 06:27:13AM -0300, Mauro Carvalho Chehab wrote:

quoted

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

quoted

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.
  

quoted

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.

There are still a few occurrences on drivers. Just rebased it.
I'll post it in a few, inside a new patch series.

Simplifying the name of the match rules makes easier to understand
what's going on.


Thanks,
Mauro