Em Thu, 28 Sep 2017 15:09:21 +0300
Sakari Ailus <sakari.ailus@xxxxxx> 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);
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
