Em Sat, 30 Sep 2017 01:05:24 +0300
Sakari Ailus <sakari.ailus@xxxxxx> escreveu:
> 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@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);
>
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
