| Message ID | 20200718145918.17752-6-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: document rkisp1-common.h | expand |
On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: > The code in rkisp1-isp.c requires access to struct 'rkisp1_device' > in several places. It access it using the 'container_of' macro. > This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' > to simplify the access. What is wrong with container_of? I usually prefer moving to container_of then the other way around, since this avoid a new field in the struct, and also avoid the requirements of keeping the pointer in sync. Thanks Helen > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/rkisp1-common.h | 1 + > drivers/staging/media/rkisp1/rkisp1-isp.c | 12 +++++------- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h > index e54793d13c3d..893caa9df891 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-common.h > +++ b/drivers/staging/media/rkisp1/rkisp1-common.h > @@ -106,6 +106,7 @@ struct rkisp1_sensor_async { > */ > struct rkisp1_isp { > struct v4l2_subdev sd; > + struct rkisp1_device *rkisp1; > struct media_pad pads[RKISP1_ISP_PAD_MAX]; > struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; > const struct rkisp1_isp_mbus_info *sink_fmt; > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > index 6ec1e9816e9f..b2131aea5488 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_selection *sel) > { > - struct rkisp1_device *rkisp1 = > - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); > struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); > + struct rkisp1_device *rkisp1 = isp->rkisp1; > int ret = 0; > > if (sel->target != V4L2_SEL_TGT_CROP) > @@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = { > static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp, > struct rkisp1_sensor_async *sensor) > { > - struct rkisp1_device *rkisp1 = > - container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev); > + struct rkisp1_device *rkisp1 = isp->rkisp1; > union phy_configure_opts opts; > struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > s64 pixel_clock; > @@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor) > > static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) > { > - struct rkisp1_device *rkisp1 = > - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); > - struct rkisp1_isp *isp = &rkisp1->isp; > + struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); > + struct rkisp1_device *rkisp1 = isp->rkisp1; > struct v4l2_subdev *sensor_sd; > int ret = 0; > > @@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1, > struct v4l2_subdev *sd = &isp->sd; > int ret; > > + isp->rkisp1 = rkisp1; > v4l2_subdev_init(sd, &rkisp1_isp_ops); > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > sd->entity.ops = &rkisp1_isp_media_ops; >
Hi, On 20.07.20 21:25, Helen Koike wrote: > > > On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: >> The code in rkisp1-isp.c requires access to struct 'rkisp1_device' >> in several places. It access it using the 'container_of' macro. >> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' >> to simplify the access. > > What is wrong with container_of? I remember Laurent suggested it a while ago. I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. Thanks, Dafna > > I usually prefer moving to container_of then the other way around, since this avoid a new field > in the struct, and also avoid the requirements of keeping the pointer in sync. > > Thanks > Helen > >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/staging/media/rkisp1/rkisp1-common.h | 1 + >> drivers/staging/media/rkisp1/rkisp1-isp.c | 12 +++++------- >> 2 files changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h >> index e54793d13c3d..893caa9df891 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-common.h >> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h >> @@ -106,6 +106,7 @@ struct rkisp1_sensor_async { >> */ >> struct rkisp1_isp { >> struct v4l2_subdev sd; >> + struct rkisp1_device *rkisp1; >> struct media_pad pads[RKISP1_ISP_PAD_MAX]; >> struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; >> const struct rkisp1_isp_mbus_info *sink_fmt; >> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c >> index 6ec1e9816e9f..b2131aea5488 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c >> @@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd, >> struct v4l2_subdev_pad_config *cfg, >> struct v4l2_subdev_selection *sel) >> { >> - struct rkisp1_device *rkisp1 = >> - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); >> struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); >> + struct rkisp1_device *rkisp1 = isp->rkisp1; >> int ret = 0; >> >> if (sel->target != V4L2_SEL_TGT_CROP) >> @@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = { >> static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp, >> struct rkisp1_sensor_async *sensor) >> { >> - struct rkisp1_device *rkisp1 = >> - container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev); >> + struct rkisp1_device *rkisp1 = isp->rkisp1; >> union phy_configure_opts opts; >> struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; >> s64 pixel_clock; >> @@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor) >> >> static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) >> { >> - struct rkisp1_device *rkisp1 = >> - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); >> - struct rkisp1_isp *isp = &rkisp1->isp; >> + struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); >> + struct rkisp1_device *rkisp1 = isp->rkisp1; >> struct v4l2_subdev *sensor_sd; >> int ret = 0; >> >> @@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1, >> struct v4l2_subdev *sd = &isp->sd; >> int ret; >> >> + isp->rkisp1 = rkisp1; >> v4l2_subdev_init(sd, &rkisp1_isp_ops); >> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; >> sd->entity.ops = &rkisp1_isp_media_ops; >>
On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > Hi, > > On 20.07.20 21:25, Helen Koike wrote: > > > > > > On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: > >> The code in rkisp1-isp.c requires access to struct 'rkisp1_device' > >> in several places. It access it using the 'container_of' macro. > >> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' > >> to simplify the access. > > > > What is wrong with container_of? > > I remember Laurent suggested it a while ago. > I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead? Best regards, Tomasz
Hi, On 21.07.20 14:36, Tomasz Figa wrote: > On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld > <dafna.hirschfeld@collabora.com> wrote: >> >> Hi, >> >> On 20.07.20 21:25, Helen Koike wrote: >>> >>> >>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: >>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device' >>>> in several places. It access it using the 'container_of' macro. >>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' >>>> to simplify the access. >>> >>> What is wrong with container_of? >> >> I remember Laurent suggested it a while ago. >> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. >> > > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead? pass to where ? You mean to the function rkisp1_mipi_csi2_start ? Thanks, Dafna > > Best regards, > Tomasz >
On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > Hi, > > On 21.07.20 14:36, Tomasz Figa wrote: > > On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld > > <dafna.hirschfeld@collabora.com> wrote: > >> > >> Hi, > >> > >> On 20.07.20 21:25, Helen Koike wrote: > >>> > >>> > >>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: > >>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device' > >>>> in several places. It access it using the 'container_of' macro. > >>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' > >>>> to simplify the access. > >>> > >>> What is wrong with container_of? > >> > >> I remember Laurent suggested it a while ago. > >> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. > >> > > > > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead? > > pass to where ? You mean to the function rkisp1_mipi_csi2_start ? Yes, all around the driver, where rkisp1_isp is passed.
On 21.07.20 17:32, Tomasz Figa wrote: > On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld > <dafna.hirschfeld@collabora.com> wrote: >> >> Hi, >> >> On 21.07.20 14:36, Tomasz Figa wrote: >>> On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld >>> <dafna.hirschfeld@collabora.com> wrote: >>>> >>>> Hi, >>>> >>>> On 20.07.20 21:25, Helen Koike wrote: >>>>> >>>>> >>>>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: >>>>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device' >>>>>> in several places. It access it using the 'container_of' macro. >>>>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' >>>>>> to simplify the access. >>>>> >>>>> What is wrong with container_of? >>>> >>>> I remember Laurent suggested it a while ago. >>>> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. >>>> >>> >>> Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead? >> >> pass to where ? You mean to the function rkisp1_mipi_csi2_start ? > > Yes, all around the driver, where rkisp1_isp is passed. Most of the functions are part of subdevice callback implementation where the rkisp1_device is not needed, so I don't the see the point. Thanks, Dafna >
On Sat, Aug 01, 2020 at 06:08:06PM +0200, Dafna Hirschfeld wrote: > > > On 21.07.20 17:32, Tomasz Figa wrote: > > On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld > > <dafna.hirschfeld@collabora.com> wrote: > > > > > > Hi, > > > > > > On 21.07.20 14:36, Tomasz Figa wrote: > > > > On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld > > > > <dafna.hirschfeld@collabora.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On 20.07.20 21:25, Helen Koike wrote: > > > > > > > > > > > > > > > > > > On 7/18/20 11:59 AM, Dafna Hirschfeld wrote: > > > > > > > The code in rkisp1-isp.c requires access to struct 'rkisp1_device' > > > > > > > in several places. It access it using the 'container_of' macro. > > > > > > > This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' > > > > > > > to simplify the access. > > > > > > > > > > > > What is wrong with container_of? > > > > > > > > > > I remember Laurent suggested it a while ago. > > > > > I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device. > > > > > > > > > > > > > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead? > > > > > > pass to where ? You mean to the function rkisp1_mipi_csi2_start ? > > > > Yes, all around the driver, where rkisp1_isp is passed. > > Most of the functions are part of subdevice callback implementation > where the rkisp1_device is not needed, so I don't the see the point. Okay, so then I'd just lean towards keeping it as is. container_of is generally considered more efficient than a pointer, because it doesn't require a load operation to obtain a reference to the parent struct. Best regards, Tomasz
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h index e54793d13c3d..893caa9df891 100644 --- a/drivers/staging/media/rkisp1/rkisp1-common.h +++ b/drivers/staging/media/rkisp1/rkisp1-common.h @@ -106,6 +106,7 @@ struct rkisp1_sensor_async { */ struct rkisp1_isp { struct v4l2_subdev sd; + struct rkisp1_device *rkisp1; struct media_pad pads[RKISP1_ISP_PAD_MAX]; struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX]; const struct rkisp1_isp_mbus_info *sink_fmt; diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c index 6ec1e9816e9f..b2131aea5488 100644 --- a/drivers/staging/media/rkisp1/rkisp1-isp.c +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c @@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_selection *sel) { - struct rkisp1_device *rkisp1 = - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); + struct rkisp1_device *rkisp1 = isp->rkisp1; int ret = 0; if (sel->target != V4L2_SEL_TGT_CROP) @@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = { static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp, struct rkisp1_sensor_async *sensor) { - struct rkisp1_device *rkisp1 = - container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev); + struct rkisp1_device *rkisp1 = isp->rkisp1; union phy_configure_opts opts; struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; s64 pixel_clock; @@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor) static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable) { - struct rkisp1_device *rkisp1 = - container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev); - struct rkisp1_isp *isp = &rkisp1->isp; + struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd); + struct rkisp1_device *rkisp1 = isp->rkisp1; struct v4l2_subdev *sensor_sd; int ret = 0; @@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd = &isp->sd; int ret; + isp->rkisp1 = rkisp1; v4l2_subdev_init(sd, &rkisp1_isp_ops); sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; sd->entity.ops = &rkisp1_isp_media_ops;
The code in rkisp1-isp.c requires access to struct 'rkisp1_device' in several places. It access it using the 'container_of' macro. This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp' to simplify the access. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/rkisp1-common.h | 1 + drivers/staging/media/rkisp1/rkisp1-isp.c | 12 +++++------- 2 files changed, 6 insertions(+), 7 deletions(-)