On 27/09/2021 02:06, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Mon, Aug 30, 2021 at 02:00:41PM +0300, Tomi Valkeinen wrote:
The recently added v4l2_subdev_alloc_state() and
v4l2_subdev_free_state() were somewhat badly named. They allocate/free
the state that can be used for a subdev, but they don't change the
subdev itself in any way.
In the future we want to have the subdev state available easily for all
subdevs, and we want to store the state to subdev struct. Thus we will
be needing a new function which allocates the state and stores it into
the subdev struct.
This patch renames v4l2_subdev_alloc/free_state functions to
v4l2_alloc/free_subdev_state, so that we can later use
v4l2_subdev_alloc/free_state for the versions which work on the subdev
struct.
With have video_device_alloc() and media_request_alloc(), should we use
v4l2_subdev_state_alloc() and v4l2_subdev_state_free() to be consistent
?
With or without this (but preferably with ;-)),
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Note that regardless of the name, if we have both
v4l2_subdev_alloc_state() (to allocate the state and store it in the
subdev structure) and v4l2_subdev_alloc_state() (to allocate the state),
it could be confusing for driver authors. Let's discuss this further in
the patch series when the problem arises (if it does).
It is confusing. I guess to prove the point you used
"v4l2_subdev_alloc_state" twice above ;).
To me, the change you propose makes it more confusing than my version,
but I think that's up to the reader.
My thinking was that v4l2_subdev_xyz function does something to the
subdev, thus v4l2_alloc_subdev_state does something that won't affect
the subdev. With v4l2_subdev_alloc_state() and v4l2_subdev_state_alloc()
that logic doesn't hold.
Perhaps we should just use something totally else for the version that
allocates and stores the state? v4l2_subdev_setup_state or something in
that direction? Or v4l2_subdev_setup, i.e. don't even refer to "state".
You propose "v4l2_subdev_init_done" in a later reply, so I think however
we restructure (or don't) the subdev init, we should rename
v4l2_subdev_alloc_state to something else.
We can discuss that rename in the other thread. As to your proposal for
this patch, I agree with it presuming we also rename
v4l2_subdev_alloc_state.
Tomi
