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
>