* [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha @ 2019-07-04 1:58 Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 1:58 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund Hi, This small series adds support for two new pixel formats for the rcar-vin driver; V4L2_PIX_FMT_ARGB555 and V4L2_PIX_FMT_ABGR32. Both formats have an alpha component so a new standard control is also added to control its value, V4L2_CID_ALPHA_COMPONENT. The series is based on the latest media-tree and is tested on both Renesas Gen2 and Gen3 hardware without any regressions found. Patch 1/4 fixes a badly named register name, 2/4 adds the new control while 3/4 adds the two new pixel formats. Patch 4/4 is a clean up now that Gen2 and Gen3 always have controls and v4l2_ctrl_handler_setup() should be called for both cases. * Changes since v2 - Protect the writing of the alpha value when streaming with the spin lock to make sure the streaming state is stable. - Add patch 4/4 to call v4l2_ctrl_handler_setup() for the media controller centric Gen3 mode of the driver. Niklas Söderlund (4): rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB rcar-vin: Add control for alpha component rcar-vin: Add support for RGB formats with alpha component rcar-vin: Always setup controls when opening video device drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++- drivers/media/platform/rcar-vin/rcar-dma.c | 44 ++++++++++++++++- drivers/media/platform/rcar-vin/rcar-v4l2.c | 38 +++++++++------ drivers/media/platform/rcar-vin/rcar-vin.h | 5 ++ 4 files changed, 121 insertions(+), 19 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB 2019-07-04 1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund @ 2019-07-04 1:58 ` Niklas Söderlund 2019-07-04 14:13 ` Kieran Bingham 2019-07-04 1:58 ` [PATCH v3 2/4] rcar-vin: Add control for alpha component Niklas Söderlund ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 1:58 UTC (permalink / raw) To: Laurent Pinchart, linux-media Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht The value have nothing to do with ARGB1555, it controls if the alpha component should be filled in for ARGB1555 or ARGB888. Rename it to reflect this. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/platform/rcar-vin/rcar-dma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 91ab064404a185af..2d146ecf93d66ad5 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -114,7 +114,7 @@ #define VNDMR_EXRGB (1 << 8) #define VNDMR_BPSM (1 << 4) #define VNDMR_DTMD_YCSEP (1 << 1) -#define VNDMR_DTMD_ARGB1555 (1 << 0) +#define VNDMR_DTMD_ARGB (1 << 0) /* Video n Data Mode Register 2 bits */ #define VNDMR2_VPS (1 << 30) @@ -721,7 +721,7 @@ static int rvin_setup(struct rvin_dev *vin) output_is_yuv = true; break; case V4L2_PIX_FMT_XRGB555: - dmr = VNDMR_DTMD_ARGB1555; + dmr = VNDMR_DTMD_ARGB; break; case V4L2_PIX_FMT_RGB565: dmr = 0; -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB 2019-07-04 1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund @ 2019-07-04 14:13 ` Kieran Bingham 0 siblings, 0 replies; 12+ messages in thread From: Kieran Bingham @ 2019-07-04 14:13 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart, linux-media Cc: linux-renesas-soc, Ulrich Hecht Hi Niklas, On 04/07/2019 02:58, Niklas Söderlund wrote: > The value have nothing to do with ARGB1555, it controls if the alpha > component should be filled in for ARGB1555 or ARGB888. Rename it to > reflect this. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 91ab064404a185af..2d146ecf93d66ad5 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -114,7 +114,7 @@ > #define VNDMR_EXRGB (1 << 8) > #define VNDMR_BPSM (1 << 4) > #define VNDMR_DTMD_YCSEP (1 << 1) > -#define VNDMR_DTMD_ARGB1555 (1 << 0) > +#define VNDMR_DTMD_ARGB (1 << 0) > > /* Video n Data Mode Register 2 bits */ > #define VNDMR2_VPS (1 << 30) > @@ -721,7 +721,7 @@ static int rvin_setup(struct rvin_dev *vin) > output_is_yuv = true; > break; > case V4L2_PIX_FMT_XRGB555: > - dmr = VNDMR_DTMD_ARGB1555; > + dmr = VNDMR_DTMD_ARGB; > break; > case V4L2_PIX_FMT_RGB565: > dmr = 0; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/4] rcar-vin: Add control for alpha component 2019-07-04 1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund @ 2019-07-04 1:58 ` Niklas Söderlund 2019-07-04 15:15 ` Kieran Bingham 2019-07-04 1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund 3 siblings, 1 reply; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 1:58 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund In preparation to adding support for RGB pixel formats with an alpha component add a control to allow the user to control which alpha value should be used. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++- drivers/media/platform/rcar-vin/rcar-dma.c | 5 ++ drivers/media/platform/rcar-vin/rcar-vin.h | 5 ++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 64f9cf790445d14e..ee6e6cb39c749675 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin) kref_put(&group->refcount, rvin_group_release); } +/* ----------------------------------------------------------------------------- + * Controls + */ + +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl) +{ + struct rvin_dev *vin = + container_of(ctrl->handler, struct rvin_dev, ctrl_handler); + + switch (ctrl->id) { + case V4L2_CID_ALPHA_COMPONENT: + rvin_set_alpha(vin, ctrl->val); + break; + } + + return 0; +} + +static const struct v4l2_ctrl_ops rvin_ctrl_ops = { + .s_ctrl = rvin_s_ctrl, +}; + /* ----------------------------------------------------------------------------- * Async notifier */ @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, 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) { @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin) if (ret) rvin_group_put(vin); + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); + 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; } @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev) vin->dev = &pdev->dev; vin->info = of_device_get_match_data(&pdev->dev); + vin->alpha = 0xff; /* * Special care is needed on r8a7795 ES1.x since it @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev) return 0; error_group_unregister: + v4l2_ctrl_handler_free(&vin->ctrl_handler); + if (vin->info->use_mc) { mutex_lock(&vin->group->lock); if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev) } mutex_unlock(&vin->group->lock); rvin_group_put(vin); - } else { - v4l2_ctrl_handler_free(&vin->ctrl_handler); } + v4l2_ctrl_handler_free(&vin->ctrl_handler); + rvin_dma_unregister(vin); return 0; diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 2d146ecf93d66ad5..4e991cce5fb56a90 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) return 0; } + +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) +{ + vin->alpha = alpha; +} diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h index 0b13b34d03e3dce4..365dfde06ec25add 100644 --- a/drivers/media/platform/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/rcar-vin/rcar-vin.h @@ -178,6 +178,8 @@ struct rvin_info { * @compose: active composing * @source: active size of the video source * @std: active video standard of the video source + * + * @alpha: Alpha component to fill in for supported pixel formats */ struct rvin_dev { struct device *dev; @@ -215,6 +217,8 @@ struct rvin_dev { struct v4l2_rect compose; struct v4l2_rect source; v4l2_std_id std; + + unsigned int alpha; }; #define vin_to_source(vin) ((vin)->parallel->subdev) @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat); void rvin_crop_scale_comp(struct rvin_dev *vin); int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel); +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha); #endif -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] rcar-vin: Add control for alpha component 2019-07-04 1:58 ` [PATCH v3 2/4] rcar-vin: Add control for alpha component Niklas Söderlund @ 2019-07-04 15:15 ` Kieran Bingham 2019-07-04 15:35 ` Niklas Söderlund 0 siblings, 1 reply; 12+ messages in thread From: Kieran Bingham @ 2019-07-04 15:15 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc Hi Niklas, On 04/07/2019 02:58, Niklas Söderlund wrote: > In preparation to adding support for RGB pixel formats with an alpha > component add a control to allow the user to control which alpha value > should be used. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Comment/Question below, but: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++- > drivers/media/platform/rcar-vin/rcar-dma.c | 5 ++ > drivers/media/platform/rcar-vin/rcar-vin.h | 5 ++ > 3 files changed, 61 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 64f9cf790445d14e..ee6e6cb39c749675 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin) > kref_put(&group->refcount, rvin_group_release); > } > > +/* ----------------------------------------------------------------------------- > + * Controls > + */ > + > +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct rvin_dev *vin = > + container_of(ctrl->handler, struct rvin_dev, ctrl_handler); > + > + switch (ctrl->id) { > + case V4L2_CID_ALPHA_COMPONENT: > + rvin_set_alpha(vin, ctrl->val); > + break; > + } > + > + return 0; > +} > + > +static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > + .s_ctrl = rvin_s_ctrl, > +}; > + > /* ----------------------------------------------------------------------------- > * Async notifier > */ > @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > 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) { > @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin) > if (ret) > rvin_group_put(vin); > > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > + 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; There's quite a few lines duplicated between rvin_mc_init() and rvin_parallel_subdevice_attach() to instantiate the controls. Could that code be shared in a single function, which would make it easier to extend with new controls? Perhaps no other controls are expected though. Anyway, that's not so crucial - so RB tag added above. > + > return ret; > } > > @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > > vin->dev = &pdev->dev; > vin->info = of_device_get_match_data(&pdev->dev); > + vin->alpha = 0xff; > > /* > * Special care is needed on r8a7795 ES1.x since it > @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev) > return 0; > > error_group_unregister: > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + > if (vin->info->use_mc) { > mutex_lock(&vin->group->lock); > if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { > @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev) > } > mutex_unlock(&vin->group->lock); > rvin_group_put(vin); > - } else { > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > } > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + > rvin_dma_unregister(vin); > > return 0; > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 2d146ecf93d66ad5..4e991cce5fb56a90 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > return 0; > } > + > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > +{ > + vin->alpha = alpha; > +} > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h > index 0b13b34d03e3dce4..365dfde06ec25add 100644 > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > @@ -178,6 +178,8 @@ struct rvin_info { > * @compose: active composing > * @source: active size of the video source > * @std: active video standard of the video source > + * > + * @alpha: Alpha component to fill in for supported pixel formats > */ > struct rvin_dev { > struct device *dev; > @@ -215,6 +217,8 @@ struct rvin_dev { > struct v4l2_rect compose; > struct v4l2_rect source; > v4l2_std_id std; > + > + unsigned int alpha; > }; > > #define vin_to_source(vin) ((vin)->parallel->subdev) > @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat); > void rvin_crop_scale_comp(struct rvin_dev *vin); > > int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel); > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha); > > #endif > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] rcar-vin: Add control for alpha component 2019-07-04 15:15 ` Kieran Bingham @ 2019-07-04 15:35 ` Niklas Söderlund 0 siblings, 0 replies; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 15:35 UTC (permalink / raw) To: Kieran Bingham; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc Hi Kieran, Thanks for your feedback. On 2019-07-04 16:15:54 +0100, Kieran Bingham wrote: > Hi Niklas, > > On 04/07/2019 02:58, Niklas Söderlund wrote: > > In preparation to adding support for RGB pixel formats with an alpha > > component add a control to allow the user to control which alpha value > > should be used. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Comment/Question below, but: > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 53 ++++++++++++++++++++- > > drivers/media/platform/rcar-vin/rcar-dma.c | 5 ++ > > drivers/media/platform/rcar-vin/rcar-vin.h | 5 ++ > > 3 files changed, 61 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index 64f9cf790445d14e..ee6e6cb39c749675 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -389,6 +389,28 @@ static void rvin_group_put(struct rvin_dev *vin) > > kref_put(&group->refcount, rvin_group_release); > > } > > > > +/* ----------------------------------------------------------------------------- > > + * Controls > > + */ > > + > > +static int rvin_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct rvin_dev *vin = > > + container_of(ctrl->handler, struct rvin_dev, ctrl_handler); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_ALPHA_COMPONENT: > > + rvin_set_alpha(vin, ctrl->val); > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops rvin_ctrl_ops = { > > + .s_ctrl = rvin_s_ctrl, > > +}; > > + > > /* ----------------------------------------------------------------------------- > > * Async notifier > > */ > > @@ -478,6 +500,15 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > 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) { > > @@ -870,6 +901,21 @@ static int rvin_mc_init(struct rvin_dev *vin) > > if (ret) > > rvin_group_put(vin); > > > > + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1); > > + 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; > > There's quite a few lines duplicated between rvin_mc_init() and > rvin_parallel_subdevice_attach() to instantiate the controls. Could that > code be shared in a single function, which would make it easier to > extend with new controls? Yes there is, and I have deliberately keep them so. Reason for this is that I'm trying to work up the courage to split this driver in two, one which is pure video device centric (Gen2 only) and one that is media device centric (Gen2 and Gen3). Then marking the Gen2 only driver as deprecated and in time delete it completely making rcar-vin media device centric only for both Gen2 and Gen3. Another option is of course to skip the splitting step and sometime in the future delete the video device centric mode directly. In both cases it will be easier to do if the two modes init code are split to make it easier to move or delete. So I would prefer to keep the code as it is for now. > > Perhaps no other controls are expected though. No other controls are expected for Gen3. > > Anyway, that's not so crucial - so RB tag added above. Thanks. > > > > > + > > return ret; > > } > > > > @@ -1245,6 +1291,7 @@ static int rcar_vin_probe(struct platform_device *pdev) > > > > vin->dev = &pdev->dev; > > vin->info = of_device_get_match_data(&pdev->dev); > > + vin->alpha = 0xff; > > > > /* > > * Special care is needed on r8a7795 ES1.x since it > > @@ -1288,6 +1335,8 @@ static int rcar_vin_probe(struct platform_device *pdev) > > return 0; > > > > error_group_unregister: > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + > > if (vin->info->use_mc) { > > mutex_lock(&vin->group->lock); > > if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { > > @@ -1323,10 +1372,10 @@ static int rcar_vin_remove(struct platform_device *pdev) > > } > > mutex_unlock(&vin->group->lock); > > rvin_group_put(vin); > > - } else { > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > } > > > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + > > rvin_dma_unregister(vin); > > > > return 0; > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > index 2d146ecf93d66ad5..4e991cce5fb56a90 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -1343,3 +1343,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > > > return 0; > > } > > + > > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > > +{ > > + vin->alpha = alpha; > > +} > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h > > index 0b13b34d03e3dce4..365dfde06ec25add 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > > @@ -178,6 +178,8 @@ struct rvin_info { > > * @compose: active composing > > * @source: active size of the video source > > * @std: active video standard of the video source > > + * > > + * @alpha: Alpha component to fill in for supported pixel formats > > */ > > struct rvin_dev { > > struct device *dev; > > @@ -215,6 +217,8 @@ struct rvin_dev { > > struct v4l2_rect compose; > > struct v4l2_rect source; > > v4l2_std_id std; > > + > > + unsigned int alpha; > > }; > > > > #define vin_to_source(vin) ((vin)->parallel->subdev) > > @@ -266,5 +270,6 @@ const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat); > > void rvin_crop_scale_comp(struct rvin_dev *vin); > > > > int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel); > > +void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha); > > > > #endif > > > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/4] rcar-vin: Add support for RGB formats with alpha component 2019-07-04 1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 2/4] rcar-vin: Add control for alpha component Niklas Söderlund @ 2019-07-04 1:58 ` Niklas Söderlund 2019-07-04 12:14 ` Laurent Pinchart 2019-07-04 15:50 ` Kieran Bingham 2019-07-04 1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund 3 siblings, 2 replies; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 1:58 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and allow the alpha component to be changed while streaming using the V4L2_CID_ALPHA_COMPONENT control. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-dma.c | 35 +++++++++++++++++++++ drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 +++++ 2 files changed, 43 insertions(+) diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 4e991cce5fb56a90..620976d173585694 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -111,8 +111,11 @@ #define VNIE_EFE (1 << 1) /* Video n Data Mode Register bits */ +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) +#define VNDMR_A8BIT_MASK (0xff << 24) #define VNDMR_EXRGB (1 << 8) #define VNDMR_BPSM (1 << 4) +#define VNDMR_ABIT (1 << 2) #define VNDMR_DTMD_YCSEP (1 << 1) #define VNDMR_DTMD_ARGB (1 << 0) @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) /* Note: not supported on M1 */ dmr = VNDMR_EXRGB; break; + case V4L2_PIX_FMT_ARGB555: + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; + break; + case V4L2_PIX_FMT_ABGR32: + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; + break; default: vin_err(vin, "Invalid pixelformat (0x%x)\n", vin->format.pixelformat); @@ -1346,5 +1355,31 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) { + unsigned long flags; + u32 dmr; + + spin_lock_irqsave(&vin->qlock, flags); + vin->alpha = alpha; + + if (vin->state == STOPPED) + goto out; + + switch (vin->format.pixelformat) { + case V4L2_PIX_FMT_ARGB555: + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; + if (vin->alpha) + dmr |= VNDMR_ABIT; + break; + case V4L2_PIX_FMT_ABGR32: + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; + dmr |= VNDMR_A8BIT(vin->alpha); + break; + default: + goto out; + } + + rvin_write(vin, dmr, VNDMR_REG); +out: + spin_unlock_irqrestore(&vin->qlock, flags); } diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 0936bcd98df1f75d..f8b6ec4408b2f5fa 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { .fourcc = V4L2_PIX_FMT_XBGR32, .bpp = 4, }, + { + .fourcc = V4L2_PIX_FMT_ARGB555, + .bpp = 2, + }, + { + .fourcc = V4L2_PIX_FMT_ABGR32, + .bpp = 4, + }, }; const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat) -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/4] rcar-vin: Add support for RGB formats with alpha component 2019-07-04 1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund @ 2019-07-04 12:14 ` Laurent Pinchart 2019-07-04 15:50 ` Kieran Bingham 1 sibling, 0 replies; 12+ messages in thread From: Laurent Pinchart @ 2019-07-04 12:14 UTC (permalink / raw) To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc Hi Niklas, Thank you for the patch. On Thu, Jul 04, 2019 at 03:58:16AM +0200, Niklas Söderlund wrote: > The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and > V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and > allow the alpha component to be changed while streaming using the > V4L2_CID_ALPHA_COMPONENT control. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 35 +++++++++++++++++++++ > drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 +++++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 4e991cce5fb56a90..620976d173585694 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -111,8 +111,11 @@ > #define VNIE_EFE (1 << 1) > > /* Video n Data Mode Register bits */ > +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) > +#define VNDMR_A8BIT_MASK (0xff << 24) > #define VNDMR_EXRGB (1 << 8) > #define VNDMR_BPSM (1 << 4) > +#define VNDMR_ABIT (1 << 2) > #define VNDMR_DTMD_YCSEP (1 << 1) > #define VNDMR_DTMD_ARGB (1 << 0) > > @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) > /* Note: not supported on M1 */ > dmr = VNDMR_EXRGB; > break; > + case V4L2_PIX_FMT_ARGB555: > + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; > + break; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > vin->format.pixelformat); > @@ -1346,5 +1355,31 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > { > + unsigned long flags; > + u32 dmr; > + > + spin_lock_irqsave(&vin->qlock, flags); > + > vin->alpha = alpha; > + > + if (vin->state == STOPPED) > + goto out; > + > + switch (vin->format.pixelformat) { > + case V4L2_PIX_FMT_ARGB555: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; > + if (vin->alpha) > + dmr |= VNDMR_ABIT; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; > + dmr |= VNDMR_A8BIT(vin->alpha); > + break; > + default: > + goto out; > + } > + > + rvin_write(vin, dmr, VNDMR_REG); > +out: > + spin_unlock_irqrestore(&vin->qlock, flags); > } > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index 0936bcd98df1f75d..f8b6ec4408b2f5fa 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { > .fourcc = V4L2_PIX_FMT_XBGR32, > .bpp = 4, > }, > + { > + .fourcc = V4L2_PIX_FMT_ARGB555, > + .bpp = 2, > + }, > + { > + .fourcc = V4L2_PIX_FMT_ABGR32, > + .bpp = 4, > + }, > }; > > const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat) > -- > 2.21.0 > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/4] rcar-vin: Add support for RGB formats with alpha component 2019-07-04 1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund 2019-07-04 12:14 ` Laurent Pinchart @ 2019-07-04 15:50 ` Kieran Bingham 1 sibling, 0 replies; 12+ messages in thread From: Kieran Bingham @ 2019-07-04 15:50 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc Hi Niklas, On 04/07/2019 02:58, Niklas Söderlund wrote: > The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and > V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and > allow the alpha component to be changed while streaming using the > V4L2_CID_ALPHA_COMPONENT control. Hrm, I expected there to be more ARGB formats supported, but the Gen3 datasheet does seem to indicate only the two. > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 35 +++++++++++++++++++++ > drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 +++++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 4e991cce5fb56a90..620976d173585694 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -111,8 +111,11 @@ > #define VNIE_EFE (1 << 1) > > /* Video n Data Mode Register bits */ > +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) > +#define VNDMR_A8BIT_MASK (0xff << 24) > #define VNDMR_EXRGB (1 << 8) > #define VNDMR_BPSM (1 << 4) > +#define VNDMR_ABIT (1 << 2) > #define VNDMR_DTMD_YCSEP (1 << 1) > #define VNDMR_DTMD_ARGB (1 << 0) > > @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) > /* Note: not supported on M1 */ > dmr = VNDMR_EXRGB; > break; > + case V4L2_PIX_FMT_ARGB555: > + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; > + break; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > vin->format.pixelformat); > @@ -1346,5 +1355,31 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > { > + unsigned long flags; > + u32 dmr; > + > + spin_lock_irqsave(&vin->qlock, flags); > + > vin->alpha = alpha; > + > + if (vin->state == STOPPED) > + goto out; > + > + switch (vin->format.pixelformat) { > + case V4L2_PIX_FMT_ARGB555: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; > + if (vin->alpha) > + dmr |= VNDMR_ABIT; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; > + dmr |= VNDMR_A8BIT(vin->alpha); > + break; > + default: > + goto out; > + } > + > + rvin_write(vin, dmr, VNDMR_REG); > +out: > + spin_unlock_irqrestore(&vin->qlock, flags); > } > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index 0936bcd98df1f75d..f8b6ec4408b2f5fa 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { > .fourcc = V4L2_PIX_FMT_XBGR32, > .bpp = 4, > }, > + { > + .fourcc = V4L2_PIX_FMT_ARGB555, > + .bpp = 2, > + }, > + { > + .fourcc = V4L2_PIX_FMT_ABGR32, > + .bpp = 4, > + }, > }; > > const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device 2019-07-04 1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund ` (2 preceding siblings ...) 2019-07-04 1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund @ 2019-07-04 1:58 ` Niklas Söderlund 2019-07-04 12:16 ` Laurent Pinchart 2019-07-04 15:55 ` Kieran Bingham 3 siblings, 2 replies; 12+ messages in thread From: Niklas Söderlund @ 2019-07-04 1:58 UTC (permalink / raw) To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund Now that both Gen2 (device centric) and Gen3 (media device centric) modes of this driver have controls it make sens to call v4l2_ctrl_handler_setup() unconditionally when opening the video device. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) if (ret) goto err_unlock; - if (vin->info->use_mc) { + if (vin->info->use_mc) ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1); - if (ret < 0) - goto err_open; - } else { - if (v4l2_fh_is_singular_file(file)) { - ret = rvin_power_parallel(vin, true); - if (ret < 0) - goto err_open; + else if (v4l2_fh_is_singular_file(file)) + ret = rvin_power_parallel(vin, true); + + if (ret < 0) + goto err_open; + + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); + if (ret) + goto err_power; - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); - if (ret) - goto err_parallel; - } - } mutex_unlock(&vin->lock); return 0; -err_parallel: - rvin_power_parallel(vin, false); +err_power: + if (vin->info->use_mc) + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); + else if (v4l2_fh_is_singular_file(file)) + rvin_power_parallel(vin, false); err_open: v4l2_fh_release(file); err_unlock: -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device 2019-07-04 1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund @ 2019-07-04 12:16 ` Laurent Pinchart 2019-07-04 15:55 ` Kieran Bingham 1 sibling, 0 replies; 12+ messages in thread From: Laurent Pinchart @ 2019-07-04 12:16 UTC (permalink / raw) To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc Hi Niklas, Thank you for the patch. On Thu, Jul 04, 2019 at 03:58:17AM +0200, Niklas Söderlund wrote: > Now that both Gen2 (device centric) and Gen3 (media device centric) > modes of this driver have controls it make sens to call s/sens/sense/ > v4l2_ctrl_handler_setup() unconditionally when opening the video device. Not only does it make sense, but it's required by 3/4. I think you should explain why in the commit message. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) > if (ret) > goto err_unlock; > > - if (vin->info->use_mc) { > + if (vin->info->use_mc) > ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1); > - if (ret < 0) > - goto err_open; > - } else { > - if (v4l2_fh_is_singular_file(file)) { > - ret = rvin_power_parallel(vin, true); > - if (ret < 0) > - goto err_open; > + else if (v4l2_fh_is_singular_file(file)) > + ret = rvin_power_parallel(vin, true); > + > + if (ret < 0) > + goto err_open; > + > + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > + if (ret) > + goto err_power; > > - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > - if (ret) > - goto err_parallel; > - } > - } > mutex_unlock(&vin->lock); > > return 0; > -err_parallel: > - rvin_power_parallel(vin, false); > +err_power: > + if (vin->info->use_mc) > + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); > + else if (v4l2_fh_is_singular_file(file)) > + rvin_power_parallel(vin, false); > err_open: > v4l2_fh_release(file); > err_unlock: -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device 2019-07-04 1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund 2019-07-04 12:16 ` Laurent Pinchart @ 2019-07-04 15:55 ` Kieran Bingham 1 sibling, 0 replies; 12+ messages in thread From: Kieran Bingham @ 2019-07-04 15:55 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc Hi Niklas, On 04/07/2019 02:58, Niklas Söderlund wrote: > Now that both Gen2 (device centric) and Gen3 (media device centric) s/Gen2 (device centric)/Gen2 (video device centric)/ ? (only if you feel it helps clarify the distinction). > modes of this driver have controls it make sens to call s/sens/sense/ > v4l2_ctrl_handler_setup() unconditionally when opening the video device. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) > if (ret) > goto err_unlock; > > - if (vin->info->use_mc) { > + if (vin->info->use_mc) > ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1); > - if (ret < 0) > - goto err_open; > - } else { > - if (v4l2_fh_is_singular_file(file)) { > - ret = rvin_power_parallel(vin, true); > - if (ret < 0) > - goto err_open; > + else if (v4l2_fh_is_singular_file(file)) > + ret = rvin_power_parallel(vin, true); > + > + if (ret < 0) > + goto err_open; > + > + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > + if (ret) > + goto err_power; > > - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > - if (ret) > - goto err_parallel; > - } > - } > mutex_unlock(&vin->lock); > > return 0; It was already here before this patch, but a new line would be nice here to separate the error paths... Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > -err_parallel: > - rvin_power_parallel(vin, false); > +err_power: > + if (vin->info->use_mc) > + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); > + else if (v4l2_fh_is_singular_file(file)) > + rvin_power_parallel(vin, false); > err_open: > v4l2_fh_release(file); > err_unlock: > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-07-04 15:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-04 1:58 [PATCH v3 0/4] rcar-vin: Add support for RGB formats with alpha Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 1/4] rcar-vin: Rename VNDMR_DTMD_ARGB1555 to VNDMR_DTMD_ARGB Niklas Söderlund 2019-07-04 14:13 ` Kieran Bingham 2019-07-04 1:58 ` [PATCH v3 2/4] rcar-vin: Add control for alpha component Niklas Söderlund 2019-07-04 15:15 ` Kieran Bingham 2019-07-04 15:35 ` Niklas Söderlund 2019-07-04 1:58 ` [PATCH v3 3/4] rcar-vin: Add support for RGB formats with " Niklas Söderlund 2019-07-04 12:14 ` Laurent Pinchart 2019-07-04 15:50 ` Kieran Bingham 2019-07-04 1:58 ` [PATCH v3 4/4] rcar-vin: Always setup controls when opening video device Niklas Söderlund 2019-07-04 12:16 ` Laurent Pinchart 2019-07-04 15:55 ` Kieran Bingham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).