Hi,
On 15/10/2021 17:05, Jacopo Mondi wrote:
Ok, I understand this allow you to
int _set_routing(sd, state, krouting)
{
v4l2_subdev_set_routing(krouting);
}
int set_routing(sd, state, which, krouting)
{
_set_routing(sd, state, krouting);
if (which == ACTIVE)
apply_to_hw();
}
pad_ops ops = {
.set_routing = set_routing,
};
int init_cfg(sd, state)
{
routes = {
...
};
krouting routing {
.routes = routes,
.num_routes = 1,
};
_set_routing(sd, state, &routing);
}
Yes, although I would guess that the likely use of which in set_routing is
if (which == ACTIVE && priv->streaming)
return -EBUSY;
We're moving the issue with init_cfg() not being aware of the
active/try to the set_routing() operation signature (and in cascade to
all operation that operate on krouting from where 'which' had been
removed to comply with init_cfg).
Hmm, what operations? set_routing() subdev op is the only place that
needs the which, afaics. The issue with the previous series was that we
needed 'which' in places where we shouldn't. In the v9, that issue is gone.
Or do you see issues with your series caused by this change?
I was actually advocating instead for adding back which to
krouting and have all the other v4l2_subdev functions that operates on
routing and do not care about 'which' to use the raw routes and their
number
--- v4l2-subdev.h/c ---
struct krouting {
which;
num_routes;
routes;
};
int v4l2_subdev_set_routing(sd, state, routes, num_routes)
{
/* Copy routes into state */
}
---- subdevice driver ----
int _set_routing(sd, state, routes, num_routes)
{
return v4l2_subdev_set_routing(sd, state, routes,
num_routes);
}
int set_routing(sd, state, krouting)
{
_set_routing(sd, state, krouting->routes, krouting->num_routes)
if (krouting->which == ACTIVE)
apply_to_hw();
}
pad_ops ops = {
.set_routing = set_routing,
};
int init_cfg(sd, state)
{
routes routing[] = {
...
};
_set_routing(sd, state, routing, ARRAY_SIZE(routing));
}
So that
- krouting contains 'which' but is only used in the set_routing
operation
- init_cfg doesn't have to initialize a krouting just for the purpose
of pleasing the v4l2_subdev function signatures
Why do you want the routes and num_routes handled separately? Isn't it
nicer if they're grouped together in a struct?
It requires to remove krouting from all the v4l2_subdev_*route*()
functions in v4l2_subdev.c and have them work on the raw routes
('routes' and 'num_routes' could as well be grouped in one structure
like you had in v8).
If they're grouped, then... Wouldn't we just be back in the case I
described in the mail you linked, having a struct for the sole purpose
of passing parameters to set_routing?
As you asked for opinions and I failed to provide one in v8 it
wouldn't be fair to ask you to backtrack just to see how it looks
like, so here you have a patch to be applied on top of your branch for
you to take into consideration (test on my gmsl work on top of your
v9).
So it's not quite clear to me what's the issue you see here.
Is it just that you don't like passing 'which' separately to
set_routing(), but other ops get it in a struct? If that's a problem, we
can create the extra struct used solely for calling set_routing.
Or do you think it's better to have routes and num_routes used
separately, as you do in your patch? If so, why, what's the benefit?
Tomi
