Hi Hans,
On Wed, Feb 21, 2024 at 12:48:51PM +0100, Hans Verkuil wrote:
> On 21/02/2024 12:40, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote:
> >> On 21/02/2024 11:53, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote:
> >>>> On 20/12/2023 11:37, Sakari Ailus wrote:
> >>>>> Release all the resources when the media device is related, moving away
> >>>
> >>> s/related/released/
> >>>
> >>>>> form the struct v4l2_device used for that purpose.
> >>>>
> >>>> form -> from
> >>>
> >>> Yes.
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------
> >>>>> 1 file changed, 9 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> >>>>> index af127476e920..3e59f8c256c7 100644
> >>>>> --- a/drivers/media/test-drivers/vimc/vimc-core.c
> >>>>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> >>>>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> >>>>> +static void vimc_mdev_release(struct media_device *mdev)
> >>>>> {
> >>>>> struct vimc_device *vimc =
> >>>>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev);
> >>>>> + container_of_const(mdev, struct vimc_device, mdev);
> >>>>
> >>>> Why this change?
> >>>
> >>> I changed the line already. There's no reason to continue using
> >>> container_of() instead of container_of_const() that takes const-ness into
> >>> account, too.
> >>
> >> But neither vimc nor mdev can be const anyway, that would make no sense
> >> here.
> >
> > Neither is const, true. Yet container_of_const() is preferred over
>
> Says who?
container_of() documentation comes with a big, fat warning on this issue.
I can post a patch to add an explicit recommentation, too.
>
> It makes sense in generic defines that use it, e.g.:
>
> drivers/base/firmware_loader/sysfs.h:#define to_fw_sysfs(__dev) container_of_const(__dev, struct fw_sysfs, dev)
>
> That way it can handle both const and non-const __dev pointers.
>
> In cases where this doesn't come into play I think there is no need to
> make code changes. Perhaps when writing new code it might make sense to
> use it, but changing it in existing code, esp. as part of a patch that
> deals with something else entirely, seems just unnecessary churn.
>
> I won't block this, but I recommend just dropping this change in this patch.
--
Regards,
Sakari Ailus
