[PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Himadri Pandya]

Use list_entry instead of container_of for lists iss_buffer and
iss_video.

Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
---
 drivers/staging/media/omap4iss/iss_video.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index f22489edb562..e8d27b7ac329 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -118,7 +118,7 @@ struct iss_buffer {
 	dma_addr_t iss_addr;
 };
 
-#define to_iss_buffer(buf)	container_of(buf, struct iss_buffer, vb)
+#define to_iss_buffer(buf)	list_entry(buf, struct iss_buffer, vb)
 
 enum iss_video_dmaqueue_flags {
 	/* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
@@ -170,7 +170,7 @@ struct iss_video {
 	const struct iss_video_operations *ops;
 };
 
-#define to_iss_video(vdev)	container_of(vdev, struct iss_video, video)
+#define to_iss_video(vdev)	list_entry(vdev, struct iss_video, video)
 
 struct iss_video_fh {
 	struct v4l2_fh vfh;
-- 
2.17.1

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Julia Lawall]

On Sun, 24 Mar 2019, Himadri Pandya wrote:

> Use list_entry instead of container_of for lists iss_buffer and
> iss_video.
>
> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> ---
>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index f22489edb562..e8d27b7ac329 100644
> --- a/drivers/staging/media/omap4iss/iss_video.h
> +++ b/drivers/staging/media/omap4iss/iss_video.h
> @@ -118,7 +118,7 @@ struct iss_buffer {
>  	dma_addr_t iss_addr;
>  };
>
> -#define to_iss_buffer(buf)	container_of(buf, struct iss_buffer, vb)
> +#define to_iss_buffer(buf)	list_entry(buf, struct iss_buffer, vb)

Actually, this macro is no longer used.  Perhaps someone else converted
the uses to use the proper list operations and forgot to drop the macro
definition.

>
>  enum iss_video_dmaqueue_flags {
>  	/* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> @@ -170,7 +170,7 @@ struct iss_video {
>  	const struct iss_video_operations *ops;
>  };
>
> -#define to_iss_video(vdev)	container_of(vdev, struct iss_video, video)
> +#define to_iss_video(vdev)	list_entry(vdev, struct iss_video, video)

This doesn't involve a list.  Nor does the previous one.  To use
list_entry, the third argument should be the name of a field that has type
list_head.

julia

>  struct iss_video_fh {
>  	struct v4l2_fh vfh;
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190324165433.95833-1-himadri18.07%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Himadri Pandya]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Sun, 24 Mar 2019, Himadri Pandya wrote:
>
> > Use list_entry instead of container_of for lists iss_buffer and
> > iss_video.
> >
> > Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> > ---
> >  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss_video.h
> b/drivers/staging/media/omap4iss/iss_video.h
> > index f22489edb562..e8d27b7ac329 100644
> > --- a/drivers/staging/media/omap4iss/iss_video.h
> > +++ b/drivers/staging/media/omap4iss/iss_video.h
> > @@ -118,7 +118,7 @@ struct iss_buffer {
> >       dma_addr_t iss_addr;
> >  };
> >
> > -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
> > +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
>
> Actually, this macro is no longer used.  Perhaps someone else converted
> the uses to use the proper list operations and forgot to drop the macro
> definition.
>

Then should I send a patch to drop these macros?


>
> >
> >  enum iss_video_dmaqueue_flags {
> >       /* Set if DMA queue becomes empty when
> ISS_PIPELINE_STREAM_CONTINUOUS */
> > @@ -170,7 +170,7 @@ struct iss_video {
> >       const struct iss_video_operations *ops;
> >  };
> >
> > -#define to_iss_video(vdev)   container_of(vdev, struct iss_video, video)
> > +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video, video)
>
> This doesn't involve a list.  Nor does the previous one.  To use
> list_entry, the third argument should be the name of a field that has type
> list_head.
>

Understood. Thank you.

- Himadri


>
> julia
>
> >  struct iss_video_fh {
> >       struct v4l2_fh vfh;
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20190324165433.95833-1-himadri18.07%40gmail.com
> .
> > For more options, visit https://groups.google.com/d/optout.
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1903241812520.2526%40hadrien
> .
> For more options, visit https://groups.google.com/d/optout.
>

[-- Attachment #2: Type: text/html, Size: 4697 bytes --]

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]



On Sun, 24 Mar 2019, Himadri Pandya wrote:

>
>
> On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
>       On Sun, 24 Mar 2019, Himadri Pandya wrote:
>
>       > Use list_entry instead of container_of for lists iss_buffer
>       and
>       > iss_video.
>       >
>       > Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
>       > ---
>       >  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
>       >  1 file changed, 2 insertions(+), 2 deletions(-)
>       >
>       > diff --git a/drivers/staging/media/omap4iss/iss_video.h
>       b/drivers/staging/media/omap4iss/iss_video.h
>       > index f22489edb562..e8d27b7ac329 100644
>       > --- a/drivers/staging/media/omap4iss/iss_video.h
>       > +++ b/drivers/staging/media/omap4iss/iss_video.h
>       > @@ -118,7 +118,7 @@ struct iss_buffer {
>       >       dma_addr_t iss_addr;
>       >  };
>       >
>       > -#define to_iss_buffer(buf)   container_of(buf, struct
>       iss_buffer, vb)
>       > +#define to_iss_buffer(buf)   list_entry(buf, struct
>       iss_buffer, vb)
>
>       Actually, this macro is no longer used.  Perhaps someone else
>       converted
>       the uses to use the proper list operations and forgot to drop
>       the macro
>       definition.
>
>
> Then should I send a patch to drop these macros?

One of the macros is used and one is not.  You can send a patch to drop
the one that is not used.  The other one is used only once.  Still it
looks more readable than a call to container_of.

julia

>  
>
>       >
>       >  enum iss_video_dmaqueue_flags {
>       >       /* Set if DMA queue becomes empty when
>       ISS_PIPELINE_STREAM_CONTINUOUS */
>       > @@ -170,7 +170,7 @@ struct iss_video {
>       >       const struct iss_video_operations *ops;
>       >  };
>       >
>       > -#define to_iss_video(vdev)   container_of(vdev, struct
>       iss_video, video)
>       > +#define to_iss_video(vdev)   list_entry(vdev, struct
>       iss_video, video)
>
>       This doesn't involve a list.  Nor does the previous one.  To use
>       list_entry, the third argument should be the name of a field
>       that has type
>       list_head.
>
>
> Understood. Thank you.
>
> - Himadri 
>  
>
>       julia
>
>       >  struct iss_video_fh {
>       >       struct v4l2_fh vfh;
>       > --
>       > 2.17.1
>       >
>       > --
>       > You received this message because you are subscribed to the
>       Google Groups "outreachy-kernel" group.
>       > To unsubscribe from this group and stop receiving emails from
>       it, send an email to
>       outreachy-kernel+unsubscribe@googlegroups.com.
>       > To post to this group, send email to
>       outreachy-kernel@googlegroups.com.
>       > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/20190324165433.95833-1-h
>       imadri18.07%40gmail.com.
>       > For more options, visit https://groups.google.com/d/optout.
>       >
>
>       --
>       You received this message because you are subscribed to the
>       Google Groups "outreachy-kernel" group.
>       To unsubscribe from this group and stop receiving emails from
>       it, send an email to
>       outreachy-kernel+unsubscribe@googlegroups.com.
>       To post to this group, send email to
>       outreachy-kernel@googlegroups.com.
>       To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.19032418
>       12520.2526%40hadrien.
>       For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CAPnhUpYSAaY0J6dRsQf2hMv
> MfUhvaSXqu%2BVgt3aQ7g_mjGX8Hw%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Laurent Pinchart]

Hello Himadri,

On Sun, Mar 24, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
> On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 24 Mar 2019, Himadri Pandya wrote:
> >> Use list_entry instead of container_of for lists iss_buffer and
> >> iss_video.
> >>
> >> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> >> ---
> >>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> >> index f22489edb562..e8d27b7ac329 100644
> >> --- a/drivers/staging/media/omap4iss/iss_video.h
> >> +++ b/drivers/staging/media/omap4iss/iss_video.h
> >> @@ -118,7 +118,7 @@ struct iss_buffer {
> >>       dma_addr_t iss_addr;
> >>  };
> >>
> >> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
> >> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
> > 
> > Actually, this macro is no longer used.  Perhaps someone else converted
> > the uses to use the proper list operations and forgot to drop the macro
> > definition.
> 
> Then should I send a patch to drop these macros?

It would be better to modify the to_iss_buffer macro to convert from a
vb2_buffer to a iss_buffer, and use it where the driver currently uses
container_of directly.

> >>  enum iss_video_dmaqueue_flags {
> >>       /* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> >> @@ -170,7 +170,7 @@ struct iss_video {
> >>       const struct iss_video_operations *ops;
> >>  };
> >>
> >> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video, video)
> >> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video, video)
> > 
> > This doesn't involve a list.  Nor does the previous one.  To use
> > list_entry, the third argument should be the name of a field that has type
> > list_head.
> 
> Understood. Thank you.
> 
> >>  struct iss_video_fh {
> >>       struct v4l2_fh vfh;

-- 
Regards,

Laurent Pinchart

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Himadri Pandya]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

On Mon, Mar 25, 2019 at 7:17 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello Himadri,
>
> On Sun, Mar 24, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
> > On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall <julia.lawall@lip6.fr>
> wrote:
> > > On Sun, 24 Mar 2019, Himadri Pandya wrote:
> > >> Use list_entry instead of container_of for lists iss_buffer and
> > >> iss_video.
> > >>
> > >> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> > >> ---
> > >>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/staging/media/omap4iss/iss_video.h
> b/drivers/staging/media/omap4iss/iss_video.h
> > >> index f22489edb562..e8d27b7ac329 100644
> > >> --- a/drivers/staging/media/omap4iss/iss_video.h
> > >> +++ b/drivers/staging/media/omap4iss/iss_video.h
> > >> @@ -118,7 +118,7 @@ struct iss_buffer {
> > >>       dma_addr_t iss_addr;
> > >>  };
> > >>
> > >> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
> > >> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
> > >
> > > Actually, this macro is no longer used.  Perhaps someone else converted
> > > the uses to use the proper list operations and forgot to drop the macro
> > > definition.
> >
> > Then should I send a patch to drop these macros?
>
> It would be better to modify the to_iss_buffer macro to convert from a
> vb2_buffer to a iss_buffer, and use it where the driver currently uses
> container_of directly.
>
>
I am sorry but I am not sure if I understand what you are suggesting
correctly.

to_iss_buffer is defined as:
#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)

And it is not used. Instead, container_of is used directly
static void iss_video_buf_cleanup(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
        ...
}

static int iss_video_buf_prepare(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
        ...
}

static void iss_video_buf_queue(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
struct iss_video *video = vfh->video;
struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
        ...
}


If I understand it correctly, these usages are converting vb2_buffer to
iss_buffer. Which could be done by using the to_iss_buffer(vbuf) as well.
Why is this macro no longer used? I also don't understand what modification
you are suggesting. Can you please elaborate?

Thank you.

- Himadri



> > >>  enum iss_video_dmaqueue_flags {
> > >>       /* Set if DMA queue becomes empty when
> ISS_PIPELINE_STREAM_CONTINUOUS */
> > >> @@ -170,7 +170,7 @@ struct iss_video {
> > >>       const struct iss_video_operations *ops;
> > >>  };
> > >>
> > >> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video,
> video)
> > >> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video,
> video)
> > >
> > > This doesn't involve a list.  Nor does the previous one.  To use
> > > list_entry, the third argument should be the name of a field that has
> type
> > > list_head.
> >
> > Understood. Thank you.
> >
> > >>  struct iss_video_fh {
> > >>       struct v4l2_fh vfh;
>
> --
> Regards,
>
> Laurent Pinchart
>

[-- Attachment #2: Type: text/html, Size: 6395 bytes --]

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Himadri Pandya]

[-- Attachment #1: Type: text/plain, Size: 4028 bytes --]

On Wed, Mar 27, 2019 at 1:13 AM Himadri Pandya <himadri18.07@gmail.com>
wrote:

>
>
> On Mon, Mar 25, 2019 at 7:17 AM Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
>
>> Hello Himadri,
>>
>> On Sun, Mar 24, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
>> > On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall <julia.lawall@lip6.fr>
>> wrote:
>> > > On Sun, 24 Mar 2019, Himadri Pandya wrote:
>> > >> Use list_entry instead of container_of for lists iss_buffer and
>> > >> iss_video.
>> > >>
>> > >> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
>> > >> ---
>> > >>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
>> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >>
>> > >> diff --git a/drivers/staging/media/omap4iss/iss_video.h
>> b/drivers/staging/media/omap4iss/iss_video.h
>> > >> index f22489edb562..e8d27b7ac329 100644
>> > >> --- a/drivers/staging/media/omap4iss/iss_video.h
>> > >> +++ b/drivers/staging/media/omap4iss/iss_video.h
>> > >> @@ -118,7 +118,7 @@ struct iss_buffer {
>> > >>       dma_addr_t iss_addr;
>> > >>  };
>> > >>
>> > >> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer,
>> vb)
>> > >> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
>> > >
>> > > Actually, this macro is no longer used.  Perhaps someone else
>> converted
>> > > the uses to use the proper list operations and forgot to drop the
>> macro
>> > > definition.
>> >
>> > Then should I send a patch to drop these macros?
>>
>> It would be better to modify the to_iss_buffer macro to convert from a
>> vb2_buffer to a iss_buffer, and use it where the driver currently uses
>> container_of directly.
>>
>>
> I am sorry but I am not sure if I understand what you are suggesting
> correctly.
>
> to_iss_buffer is defined as:
> #define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
>
> And it is not used. Instead, container_of is used directly
> static void iss_video_buf_cleanup(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
>
> static int iss_video_buf_prepare(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
>
> static void iss_video_buf_queue(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_video *video = vfh->video;
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
>
>
> If I understand it correctly, these usages are converting vb2_buffer to
> iss_buffer. Which could be done by using the to_iss_buffer(vbuf) as well.
> Why is this macro no longer used? I also don't understand what modification
> you are suggesting. Can you please elaborate?
>
> Thank you.
>
> - Himadri
>
>
>
>> > >>  enum iss_video_dmaqueue_flags {
>> > >>       /* Set if DMA queue becomes empty when
>> ISS_PIPELINE_STREAM_CONTINUOUS */
>> > >> @@ -170,7 +170,7 @@ struct iss_video {
>> > >>       const struct iss_video_operations *ops;
>> > >>  };
>> > >>
>> > >> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video,
>> video)
>> > >> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video,
>> video)
>> > >
>> > > This doesn't involve a list.  Nor does the previous one.  To use
>> > > list_entry, the third argument should be the name of a field that has
>> type
>> > > list_head.
>> >
>> > Understood. Thank you.
>> >
>> > >>  struct iss_video_fh {
>> > >>       struct v4l2_fh vfh;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>
I have been suggested to modify the macro to_iss_bufer(). But I do not
understand how should it be changed. Can anybody please explain it? I need
to understand this in order to send the next version of the patch.

Thank you.

- Himadri

[-- Attachment #2: Type: text/html, Size: 6983 bytes --]

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Laurent Pinchart]

Hello Himadri,

On Wed, Mar 27, 2019 at 01:13:27AM +0530, Himadri Pandya wrote:
> On Mon, Mar 25, 2019 at 7:17 AM Laurent Pinchart wrote:
> > On Sun, Mar 23, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
> >> On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall wrote:
> >>> On Sun, 24 Mar 2019, Himadri Pandya wrote:
> >>>> Use list_entry instead of container_of for lists iss_buffer and
> >>>> iss_video.
> >>>>
> >>>> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> >>>> ---
> >>>>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> >>>> index f22489edb562..e8d27b7ac329 100644
> >>>> --- a/drivers/staging/media/omap4iss/iss_video.h
> >>>> +++ b/drivers/staging/media/omap4iss/iss_video.h
> >>>> @@ -118,7 +118,7 @@ struct iss_buffer {
> >>>>       dma_addr_t iss_addr;
> >>>>  };
> >>>>
> >>>> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
> >>>> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
> >>>
> >>> Actually, this macro is no longer used.  Perhaps someone else converted
> >>> the uses to use the proper list operations and forgot to drop the macro
> >>> definition.
> >>
> >> Then should I send a patch to drop these macros?
> > 
> > It would be better to modify the to_iss_buffer macro to convert from a
> > vb2_buffer to a iss_buffer, and use it where the driver currently uses
> > container_of directly.
> 
> I am sorry but I am not sure if I understand what you are suggesting
> correctly. 
> 
> to_iss_buffer is defined as:
> #define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)

to_iss_buffer() casts a vb2_v4l2_buffer pointer to a iss_buffer pointer.

> And it is not used. Instead, container_of is used directly
> static void iss_video_buf_cleanup(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
> 
> static int iss_video_buf_prepare(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
> 
> static void iss_video_buf_queue(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_video *video = vfh->video;
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }

These three functions need to cast a vb2_buffer pointer to a iss_buffer
pointer. They do so by first casting to vb2_v4l2_buffer, and then to
iss_buffer with a direct use of container_of. It would be best to
combine the two casts in a single to_iss_buffer macro and use it through
the code.

> If I understand it correctly, these usages are converting vb2_buffer to
> iss_buffer. Which could be done by using the to_iss_buffer(vbuf) as well. Why
> is this macro no longer used? I also don't understand what modification you are
> suggesting. Can you please elaborate?
> 
> >>>>  enum iss_video_dmaqueue_flags {
> >>>>       /* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> >>>> @@ -170,7 +170,7 @@ struct iss_video {
> >>>>       const struct iss_video_operations *ops;
> >>>>  };
> >>>>
> >>>> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video, video)
> >>>> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video, video)
> >>>
> >>> This doesn't involve a list.  Nor does the previous one.  To use
> >>> list_entry, the third argument should be the name of a field that has
> >>> type list_head.
> >>
> >> Understood. Thank you.
> >>
> >>>>  struct iss_video_fh {
> >>>>       struct v4l2_fh vfh;

-- 
Regards,

Laurent Pinchart

Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Himadri Pandya]

[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]

On Sun, Mar 31, 2019 at 3:38 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello Himadri,
>
> On Wed, Mar 27, 2019 at 01:13:27AM +0530, Himadri Pandya wrote:
> > On Mon, Mar 25, 2019 at 7:17 AM Laurent Pinchart wrote:
> > > On Sun, Mar 23, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
> > >> On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall wrote:
> > >>> On Sun, 24 Mar 2019, Himadri Pandya wrote:
> > >>>> Use list_entry instead of container_of for lists iss_buffer and
> > >>>> iss_video.
> > >>>>
> > >>>> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> > >>>> ---
> > >>>>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> > >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/staging/media/omap4iss/iss_video.h
> b/drivers/staging/media/omap4iss/iss_video.h
> > >>>> index f22489edb562..e8d27b7ac329 100644
> > >>>> --- a/drivers/staging/media/omap4iss/iss_video.h
> > >>>> +++ b/drivers/staging/media/omap4iss/iss_video.h
> > >>>> @@ -118,7 +118,7 @@ struct iss_buffer {
> > >>>>       dma_addr_t iss_addr;
> > >>>>  };
> > >>>>
> > >>>> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer,
> vb)
> > >>>> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
> > >>>
> > >>> Actually, this macro is no longer used.  Perhaps someone else
> converted
> > >>> the uses to use the proper list operations and forgot to drop the
> macro
> > >>> definition.
> > >>
> > >> Then should I send a patch to drop these macros?
> > >
> > > It would be better to modify the to_iss_buffer macro to convert from a
> > > vb2_buffer to a iss_buffer, and use it where the driver currently uses
> > > container_of directly.
> >
> > I am sorry but I am not sure if I understand what you are suggesting
> > correctly.
> >
> > to_iss_buffer is defined as:
> > #define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
>
> to_iss_buffer() casts a vb2_v4l2_buffer pointer to a iss_buffer pointer.
>
> > And it is not used. Instead, container_of is used directly
> > static void iss_video_buf_cleanup(struct vb2_buffer *vb)
> > {
> > struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
> >         ...
> > }
> >
> > static int iss_video_buf_prepare(struct vb2_buffer *vb)
> > {
> > struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> > struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
> >         ...
> > }
> >
> > static void iss_video_buf_queue(struct vb2_buffer *vb)
> > {
> > struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> > struct iss_video *video = vfh->video;
> > struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
> >         ...
> > }
>
> These three functions need to cast a vb2_buffer pointer to a iss_buffer
> pointer. They do so by first casting to vb2_v4l2_buffer, and then to
> iss_buffer with a direct use of container_of. It would be best to
> combine the two casts in a single to_iss_buffer macro and use it through
> the code.
>

Got it. Thank you.

- Himadri


> > If I understand it correctly, these usages are converting vb2_buffer to
> > iss_buffer. Which could be done by using the to_iss_buffer(vbuf) as
> well. Why
> > is this macro no longer used? I also don't understand what modification
> you are
> > suggesting. Can you please elaborate?
> >
> > >>>>  enum iss_video_dmaqueue_flags {
> > >>>>       /* Set if DMA queue becomes empty when
> ISS_PIPELINE_STREAM_CONTINUOUS */
> > >>>> @@ -170,7 +170,7 @@ struct iss_video {
> > >>>>       const struct iss_video_operations *ops;
> > >>>>  };
> > >>>>
> > >>>> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video,
> video)
> > >>>> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video,
> video)
> > >>>
> > >>> This doesn't involve a list.  Nor does the previous one.  To use
> > >>> list_entry, the third argument should be the name of a field that has
> > >>> type list_head.
> > >>
> > >> Understood. Thank you.
> > >>
> > >>>>  struct iss_video_fh {
> > >>>>       struct v4l2_fh vfh;
>
> --
> Regards,
>
> Laurent Pinchart
>

[-- Attachment #2: Type: text/html, Size: 5774 bytes --]