On 10/10/2023 10:23, Sakari Ailus wrote:
Moi,
On Tue, Oct 10, 2023 at 09:52:48AM +0300, Tomi Valkeinen wrote:
On 10/10/2023 01:09, Sakari Ailus wrote:
Hi folks,
This set includes a helper for working with V4L2 sub-device active state as
well as a fix for the CCS driver sub-device state patch.
since v1:
- There were other drivers using __v4l2_subdev_state_alloc(). Ouch. Change
those users as well. Note that this function should not be used in
drivers, hence API niceness is not a high priority.
Sakari Ailus (4):
media: v4l: subdev: Set sub-device active state earlier
media: v4l: subdev: Add a helper to tell if a sub-device state is
active
media: ccs: Rework initialising sub-device state
media: ccs: Fix a (harmless) lockdep warning
drivers/media/i2c/ccs/ccs-core.c | 64 ++++++++++++-------
.../platform/renesas/rcar-vin/rcar-v4l2.c | 3 +-
.../media/platform/renesas/vsp1/vsp1_entity.c | 3 +-
drivers/media/v4l2-core/v4l2-subdev.c | 14 ++--
drivers/staging/media/tegra-video/vi.c | 2 +-
include/media/v4l2-subdev.h | 11 +++-
6 files changed, 66 insertions(+), 31 deletions(-)
I'm not familiar with the CCS driver, and you don't explain much why you are
doing this, so it's a bit unclear to me. But I don't like it.
The idea with the subdev state was that the driver doesn't need to know
whether it's active-state or try-state. It should behave the same way in
both cases. This series goes against that.
Can you explain a bit what the issue is and what you are doing in this
series?
The driver maintains internal state and that needs to be updated when the
configuration (including what's in sub-device state) changes. Generally the
driver knows (as for the whence field) which state it's dealing with but
that is not the case for init_cfg.
So you need to set the subdev state to sd->active_state earlier (in
patch 1) so that the driver can use v4l2_subdev_state_whence()? You
don't really explain that in the patch descriptions.
In other words, if init_cfg() were to get a whence-parameter, this
wouldn't be needed? (I don't want to do that, just trying to understand
what's going on).
Alternatively I could split the internal workings of the driver into active
and try states but I prefer to improve the framework and make the driver
simpler.
I agree with the goal, but I don't think this is improving the framework.
If we decide that we need to know if a state is an active-state or a
try-state, I think it's better to add a whence-field into the state
itself. But I'd rather not.
Deducing the internal configuration solely based on sub-device state is not
really feasible.
Am I correct in that what you really need is a way to sub-class the
subdev state, so that you can have all that internal state in the subdev
state, and thus all the code in the driver can work in "whence-agnostic"
manner? That's something we've been thinking about for a long time.
But the driver needs to be fixed now, not at some point in the future,
of course. If the sub-classing would solve the issue (i.e. we have a
plan in mind), I'd rather just hack this in the driver, instead of
extending the framework, which might easily lead to other drivers going
the wrong way too.
How about a private flag, set before calling
v4l2_subdev_init_finalize(), and unset after the call. ccs_init_cfg()
can look at that flag an if it's set, it's initializing an active state.
Tomi
