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@xxxxxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> Comment/Question below, but:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
>
>
> > ---
> > 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
