Moi, On Tue, Oct 10, 2023 at 10:40:05AM +0300, Tomi Valkeinen wrote: > 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). Yes, that's correct. > > > 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. In all other callbacks the whence is known but not in init_cfg. Comparing with sd->active_state does this pretty cleanly, without a need to change APIs. > > > 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. In the case of the CCS driver it's not necessary to store these parameters for try states: they are driver internal only. > > 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. I dislike using driver specific flags in generic APIs. While trivial sensor drivers have no use for such functionality, I would be surprised if there would not be other similar cases. -- Terveisin, Sakari Ailus
