| Message ID | 20210413180253.2575451-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Add r8a779a0 support | expand |
Hi Niklas, On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > The controls for the video device are created in different code paths > depending on if the driver is using the media graph centric model (Gen3) > or the device centric model (Gen2 and earlier). This have lead to code > duplication that can be consolidated. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > 1 file changed, 45 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index cb3025992817d625..c798dc9409e4cdcd 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > .s_ctrl = rvin_s_ctrl, > }; > > +static void rvin_free_controls(struct rvin_dev *vin) > +{ > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > +} > + > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > +{ > + int ret; > + > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > + if (ret < 0) > + return ret; > + > + /* The VIN directly deals with alpha component. */ > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > + > + if (vin->ctrl_handler.error) { > + ret = vin->ctrl_handler.error; > + rvin_free_controls(vin); > + return ret; > + } > + > + /* For the non-MC mode add controls from the subdevice. */ > + if (subdev) { > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > + subdev->ctrl_handler, NULL, true); > + if (ret < 0) { > + rvin_free_controls(vin); > + return ret; > + } > + } > + > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > + > + return 0; > +} > + > /* ----------------------------------------------------------------------------- > * Async notifier > */ > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > return ret; > > /* Add the controls */ > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > + ret = rvin_create_controls(vin, subdev); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > - NULL, true); > - if (ret < 0) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > vin->parallel.subdev = subdev; > > return 0; > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > rvin_v4l2_unregister(vin); > vin->parallel.subdev = NULL; > > - if (!vin->info->use_mc) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - vin->vdev.ctrl_handler = NULL; > - } > + if (!vin->info->use_mc) I know it was there already, but give that rvin_parallel_notify_unbind() is only registered for parallel, can this happen ? Apart this small nit: Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > + rvin_free_controls(vin); > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > if (ret) > rvin_group_put(vin); > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > + ret = rvin_create_controls(vin, NULL); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > return ret; > } > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > return 0; > > error_group_unregister: > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > if (vin->info->use_mc) { > mutex_lock(&vin->group->lock); > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > rvin_group_put(vin); > } > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > rvin_dma_unregister(vin); > > -- > 2.31.1 >
Hi again, On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > The controls for the video device are created in different code paths > depending on if the driver is using the media graph centric model (Gen3) > or the device centric model (Gen2 and earlier). This have lead to code > duplication that can be consolidated. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > 1 file changed, 45 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index cb3025992817d625..c798dc9409e4cdcd 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > .s_ctrl = rvin_s_ctrl, > }; > > +static void rvin_free_controls(struct rvin_dev *vin) > +{ > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > +} > + > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > +{ > + int ret; > + > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); Not a big deal, but 16 because we have to reserve space for the eventual subdevice controls ? > + if (ret < 0) > + return ret; > + > + /* The VIN directly deals with alpha component. */ > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > + > + if (vin->ctrl_handler.error) { > + ret = vin->ctrl_handler.error; > + rvin_free_controls(vin); > + return ret; > + } > + > + /* For the non-MC mode add controls from the subdevice. */ > + if (subdev) { > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > + subdev->ctrl_handler, NULL, true); > + if (ret < 0) { > + rvin_free_controls(vin); > + return ret; > + } > + } > + > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > + > + return 0; > +} > + > /* ----------------------------------------------------------------------------- > * Async notifier > */ > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > return ret; > > /* Add the controls */ > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > + ret = rvin_create_controls(vin, subdev); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > - NULL, true); > - if (ret < 0) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > vin->parallel.subdev = subdev; > > return 0; > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > rvin_v4l2_unregister(vin); > vin->parallel.subdev = NULL; > > - if (!vin->info->use_mc) { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - vin->vdev.ctrl_handler = NULL; > - } > + if (!vin->info->use_mc) > + rvin_free_controls(vin); > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > if (ret) > rvin_group_put(vin); > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > + ret = rvin_create_controls(vin, NULL); > if (ret < 0) > return ret; > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > - > - if (vin->ctrl_handler.error) { > - ret = vin->ctrl_handler.error; > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - return ret; > - } > - > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > - > return ret; > } > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > return 0; > > error_group_unregister: > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > if (vin->info->use_mc) { > mutex_lock(&vin->group->lock); > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > rvin_group_put(vin); > } > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > + rvin_free_controls(vin); > > rvin_dma_unregister(vin); > > -- > 2.31.1 >
Hi Jacopo, On 2021-07-06 18:04:01 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > > The controls for the video device are created in different code paths > > depending on if the driver is using the media graph centric model (Gen3) > > or the device centric model (Gen2 and earlier). This have lead to code > > duplication that can be consolidated. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > > 1 file changed, 45 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index cb3025992817d625..c798dc9409e4cdcd 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > > .s_ctrl = rvin_s_ctrl, > > }; > > > > +static void rvin_free_controls(struct rvin_dev *vin) > > +{ > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + vin->vdev.ctrl_handler = NULL; > > +} > > + > > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > > +{ > > + int ret; > > + > > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > + if (ret < 0) > > + return ret; > > + > > + /* The VIN directly deals with alpha component. */ > > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > + > > + if (vin->ctrl_handler.error) { > > + ret = vin->ctrl_handler.error; > > + rvin_free_controls(vin); > > + return ret; > > + } > > + > > + /* For the non-MC mode add controls from the subdevice. */ > > + if (subdev) { > > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > > + subdev->ctrl_handler, NULL, true); > > + if (ret < 0) { > > + rvin_free_controls(vin); > > + return ret; > > + } > > + } > > + > > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > > + > > + return 0; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Async notifier > > */ > > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > return ret; > > > > /* Add the controls */ > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > + ret = rvin_create_controls(vin, subdev); > > if (ret < 0) > > return ret; > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > - > > - if (vin->ctrl_handler.error) { > > - ret = vin->ctrl_handler.error; > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > > - NULL, true); > > - if (ret < 0) { > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > - > > vin->parallel.subdev = subdev; > > > > return 0; > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > rvin_v4l2_unregister(vin); > > vin->parallel.subdev = NULL; > > > > - if (!vin->info->use_mc) { > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - vin->vdev.ctrl_handler = NULL; > > - } > > + if (!vin->info->use_mc) > > I know it was there already, but give that rvin_parallel_notify_unbind() > is only registered for parallel, can this happen ? Yes, on Gen2 where we don't use a media-graph. > > Apart this small nit: > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Thanks > j > > > + rvin_free_controls(vin); > > } > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > > if (ret) > > rvin_group_put(vin); > > > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > > + ret = rvin_create_controls(vin, NULL); > > if (ret < 0) > > return ret; > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > - > > - if (vin->ctrl_handler.error) { > > - ret = vin->ctrl_handler.error; > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > - > > return ret; > > } > > > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > > return 0; > > > > error_group_unregister: > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + rvin_free_controls(vin); > > > > if (vin->info->use_mc) { > > mutex_lock(&vin->group->lock); > > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > > rvin_group_put(vin); > > } > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + rvin_free_controls(vin); > > > > rvin_dma_unregister(vin); > > > > -- > > 2.31.1 > >
Hello :-) On 2021-07-06 18:08:42 +0200, Jacopo Mondi wrote: > Hi again, > > On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > > The controls for the video device are created in different code paths > > depending on if the driver is using the media graph centric model (Gen3) > > or the device centric model (Gen2 and earlier). This have lead to code > > duplication that can be consolidated. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > > 1 file changed, 45 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index cb3025992817d625..c798dc9409e4cdcd 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > > .s_ctrl = rvin_s_ctrl, > > }; > > > > +static void rvin_free_controls(struct rvin_dev *vin) > > +{ > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + vin->vdev.ctrl_handler = NULL; > > +} > > + > > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > > +{ > > + int ret; > > + > > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > Not a big deal, but 16 because we have to reserve space for the > eventual subdevice controls ? Yes, again for Gen2 where the controls of the subdevice are exposed on the video device. IIRC the number 16 comes from the soc-camera ancestor for this driver. > > > + if (ret < 0) > > + return ret; > > + > > + /* The VIN directly deals with alpha component. */ > > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > + > > + if (vin->ctrl_handler.error) { > > + ret = vin->ctrl_handler.error; > > + rvin_free_controls(vin); > > + return ret; > > + } > > + > > + /* For the non-MC mode add controls from the subdevice. */ > > + if (subdev) { > > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > > + subdev->ctrl_handler, NULL, true); > > + if (ret < 0) { > > + rvin_free_controls(vin); > > + return ret; > > + } > > + } > > + > > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > > + > > + return 0; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Async notifier > > */ > > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > return ret; > > > > /* Add the controls */ > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > + ret = rvin_create_controls(vin, subdev); > > if (ret < 0) > > return ret; > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > - > > - if (vin->ctrl_handler.error) { > > - ret = vin->ctrl_handler.error; > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > > - NULL, true); > > - if (ret < 0) { > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > - > > vin->parallel.subdev = subdev; > > > > return 0; > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > rvin_v4l2_unregister(vin); > > vin->parallel.subdev = NULL; > > > > - if (!vin->info->use_mc) { > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - vin->vdev.ctrl_handler = NULL; > > - } > > + if (!vin->info->use_mc) > > + rvin_free_controls(vin); > > } > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > > if (ret) > > rvin_group_put(vin); > > > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > > + ret = rvin_create_controls(vin, NULL); > > if (ret < 0) > > return ret; > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > - > > - if (vin->ctrl_handler.error) { > > - ret = vin->ctrl_handler.error; > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - return ret; > > - } > > - > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > - > > return ret; > > } > > > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > > return 0; > > > > error_group_unregister: > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + rvin_free_controls(vin); > > > > if (vin->info->use_mc) { > > mutex_lock(&vin->group->lock); > > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > > rvin_group_put(vin); > > } > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + rvin_free_controls(vin); > > > > rvin_dma_unregister(vin); > > > > -- > > 2.31.1 > >
Hi Niklas, On Tue, Jul 06, 2021 at 06:16:04PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > On 2021-07-06 18:04:01 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote: > > > The controls for the video device are created in different code paths > > > depending on if the driver is using the media graph centric model (Gen3) > > > or the device centric model (Gen2 and earlier). This have lead to code > > > duplication that can be consolidated. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- > > > 1 file changed, 45 insertions(+), 37 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > > index cb3025992817d625..c798dc9409e4cdcd 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > > > .s_ctrl = rvin_s_ctrl, > > > }; > > > > > > +static void rvin_free_controls(struct rvin_dev *vin) > > > +{ > > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > + vin->vdev.ctrl_handler = NULL; > > > +} > > > + > > > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) > > > +{ > > > + int ret; > > > + > > > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* The VIN directly deals with alpha component. */ > > > + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > > + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > > + > > > + if (vin->ctrl_handler.error) { > > > + ret = vin->ctrl_handler.error; > > > + rvin_free_controls(vin); > > > + return ret; > > > + } > > > + > > > + /* For the non-MC mode add controls from the subdevice. */ > > > + if (subdev) { > > > + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, > > > + subdev->ctrl_handler, NULL, true); > > > + if (ret < 0) { > > > + rvin_free_controls(vin); > > > + return ret; > > > + } > > > + } > > > + > > > + vin->vdev.ctrl_handler = &vin->ctrl_handler; > > > + > > > + return 0; > > > +} > > > + > > > /* ----------------------------------------------------------------------------- > > > * Async notifier > > > */ > > > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > > return ret; > > > > > > /* Add the controls */ > > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); > > > + ret = rvin_create_controls(vin, subdev); > > > if (ret < 0) > > > return ret; > > > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > > - > > > - if (vin->ctrl_handler.error) { > > > - ret = vin->ctrl_handler.error; > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > - return ret; > > > - } > > > - > > > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, > > > - NULL, true); > > > - if (ret < 0) { > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > - return ret; > > > - } > > > - > > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > > - > > > vin->parallel.subdev = subdev; > > > > > > return 0; > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > > rvin_v4l2_unregister(vin); > > > vin->parallel.subdev = NULL; > > > > > > - if (!vin->info->use_mc) { > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > - vin->vdev.ctrl_handler = NULL; > > > - } > > > + if (!vin->info->use_mc) > > > > I know it was there already, but give that rvin_parallel_notify_unbind() > > is only registered for parallel, can this happen ? > > Yes, on Gen2 where we don't use a media-graph. > Ah correct, for gen3 the controls are freed elsewhere, right! Thanks for the clarification > > > > Apart this small nit: > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > Thanks > > j > > > > > + rvin_free_controls(vin); > > > } > > > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) > > > if (ret) > > > rvin_group_put(vin); > > > > > > - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > > > + ret = rvin_create_controls(vin, NULL); > > > if (ret < 0) > > > return ret; > > > > > > - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, > > > - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); > > > - > > > - if (vin->ctrl_handler.error) { > > > - ret = vin->ctrl_handler.error; > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > - return ret; > > > - } > > > - > > > - vin->vdev.ctrl_handler = &vin->ctrl_handler; > > > - > > > return ret; > > > } > > > > > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > > > return 0; > > > > > > error_group_unregister: > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > + rvin_free_controls(vin); > > > > > > if (vin->info->use_mc) { > > > mutex_lock(&vin->group->lock); > > > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > > > rvin_group_put(vin); > > > } > > > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > + rvin_free_controls(vin); > > > > > > rvin_dma_unregister(vin); > > > > > > -- > > > 2.31.1 > > > > > -- > Regards, > Niklas Söderlund
Hi Niklas, Jacopo, On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote: > > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > > > rvin_v4l2_unregister(vin); > > > > vin->parallel.subdev = NULL; > > > > > > > > - if (!vin->info->use_mc) { > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > > - vin->vdev.ctrl_handler = NULL; > > > > - } > > > > + if (!vin->info->use_mc) > > > > > > I know it was there already, but give that rvin_parallel_notify_unbind() > > > is only registered for parallel, can this happen ? > > > > Yes, on Gen2 where we don't use a media-graph. > > > > Ah correct, for gen3 the controls are freed elsewhere, right! > > Thanks for the clarification I already had the set in my tree but I can throw it out if you'd prefer to send v2 instead. At least I noticed only minor matters in the comments.
Hi Sakari, On 2021-07-08 16:40:40 +0300, Sakari Ailus wrote: > Hi Niklas, Jacopo, > > On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote: > > > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > > > > rvin_v4l2_unregister(vin); > > > > > vin->parallel.subdev = NULL; > > > > > > > > > > - if (!vin->info->use_mc) { > > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > > > - vin->vdev.ctrl_handler = NULL; > > > > > - } > > > > > + if (!vin->info->use_mc) > > > > > > > > I know it was there already, but give that rvin_parallel_notify_unbind() > > > > is only registered for parallel, can this happen ? > > > > > > Yes, on Gen2 where we don't use a media-graph. > > > > > > > Ah correct, for gen3 the controls are freed elsewhere, right! > > > > Thanks for the clarification > > I already had the set in my tree but I can throw it out if you'd prefer to > send v2 instead. At least I noticed only minor matters in the comments. There is one small issue in a cleanup path in 4/11 that should be fixed and it's always good to get the small things fixed. If I have a v2 out before end of day tomorrow could you refresh what you have in your tree? > > -- > Regards, > > Sakari Ailus
On Thu, Jul 08, 2021 at 04:05:43PM +0200, Niklas Söderlund wrote: > Hi Sakari, > > On 2021-07-08 16:40:40 +0300, Sakari Ailus wrote: > > Hi Niklas, Jacopo, > > > > On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote: > > > > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > > > > > rvin_v4l2_unregister(vin); > > > > > > vin->parallel.subdev = NULL; > > > > > > > > > > > > - if (!vin->info->use_mc) { > > > > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > > > > - vin->vdev.ctrl_handler = NULL; > > > > > > - } > > > > > > + if (!vin->info->use_mc) > > > > > > > > > > I know it was there already, but give that rvin_parallel_notify_unbind() > > > > > is only registered for parallel, can this happen ? > > > > > > > > Yes, on Gen2 where we don't use a media-graph. > > > > > > > > > > Ah correct, for gen3 the controls are freed elsewhere, right! > > > > > > Thanks for the clarification > > > > I already had the set in my tree but I can throw it out if you'd prefer to > > send v2 instead. At least I noticed only minor matters in the comments. > > There is one small issue in a cleanup path in 4/11 that should be fixed > and it's always good to get the small things fixed. If I have a v2 out > before end of day tomorrow could you refresh what you have in your tree? Certainly. There's no urgency --- we don't have even rc1 yet.
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index cb3025992817d625..c798dc9409e4cdcd 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = { .s_ctrl = rvin_s_ctrl, }; +static void rvin_free_controls(struct rvin_dev *vin) +{ + v4l2_ctrl_handler_free(&vin->ctrl_handler); + vin->vdev.ctrl_handler = NULL; +} + +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev) +{ + int ret; + + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); + if (ret < 0) + return ret; + + /* The VIN directly deals with alpha component. */ + v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, + V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); + + if (vin->ctrl_handler.error) { + ret = vin->ctrl_handler.error; + rvin_free_controls(vin); + return ret; + } + + /* For the non-MC mode add controls from the subdevice. */ + if (subdev) { + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, + subdev->ctrl_handler, NULL, true); + if (ret < 0) { + rvin_free_controls(vin); + return ret; + } + } + + vin->vdev.ctrl_handler = &vin->ctrl_handler; + + return 0; +} + /* ----------------------------------------------------------------------------- * Async notifier */ @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, return ret; /* Add the controls */ - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16); + ret = rvin_create_controls(vin, subdev); if (ret < 0) return ret; - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); - - if (vin->ctrl_handler.error) { - ret = vin->ctrl_handler.error; - v4l2_ctrl_handler_free(&vin->ctrl_handler); - return ret; - } - - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler, - NULL, true); - if (ret < 0) { - v4l2_ctrl_handler_free(&vin->ctrl_handler); - return ret; - } - - vin->vdev.ctrl_handler = &vin->ctrl_handler; - vin->parallel.subdev = subdev; return 0; @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) rvin_v4l2_unregister(vin); vin->parallel.subdev = NULL; - if (!vin->info->use_mc) { - v4l2_ctrl_handler_free(&vin->ctrl_handler); - vin->vdev.ctrl_handler = NULL; - } + if (!vin->info->use_mc) + rvin_free_controls(vin); } static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin) if (ret) rvin_group_put(vin); - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); + ret = rvin_create_controls(vin, NULL); if (ret < 0) return ret; - v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops, - V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255); - - if (vin->ctrl_handler.error) { - ret = vin->ctrl_handler.error; - v4l2_ctrl_handler_free(&vin->ctrl_handler); - return ret; - } - - vin->vdev.ctrl_handler = &vin->ctrl_handler; - return ret; } @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev) return 0; error_group_unregister: - v4l2_ctrl_handler_free(&vin->ctrl_handler); + rvin_free_controls(vin); if (vin->info->use_mc) { mutex_lock(&vin->group->lock); @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev) rvin_group_put(vin); } - v4l2_ctrl_handler_free(&vin->ctrl_handler); + rvin_free_controls(vin); rvin_dma_unregister(vin);
The controls for the video device are created in different code paths depending on if the driver is using the media graph centric model (Gen3) or the device centric model (Gen2 and earlier). This have lead to code duplication that can be consolidated. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++---------- 1 file changed, 45 insertions(+), 37 deletions(-)