| Message ID | 20180426165346.494-8-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kieran, Thank you for the patch. On Thursday, 26 April 2018 19:53:36 EEST Kieran Bingham wrote: > The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output. > > This platform is unusual in that the VGA is connected to DU3 leaving DU2 > unpopulated. This is reflected by the channel_mask accordingly. I'd write s/VGA/DPAD/g (or s/VGA/RGB/g) as the DPAD output can be used for other purposes than VGA. > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index d6ebc628fc22..4d195ff8c569 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > @@ -246,6 +246,34 @@ static const struct rcar_du_device_info > rcar_du_r8a7796_info = { .dpll_ch = BIT(1), > }; > > +static const struct rcar_du_device_info rcar_du_r8a77965_info = { > + .gen = 3, > + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK > + | RCAR_DU_FEATURE_EXT_CTRL_REGS > + | RCAR_DU_FEATURE_VSP1_SOURCE, > + .channel_mask = BIT(0) | BIT(1) | BIT(3), Depending on what you think of my suggestions for patch 05/17, you might want to reverse the bit order here. > + .routes = { > + /* > + * R8A77965 has one RGB output, one LVDS output and one HDMI > + * output. > + */ > + [RCAR_DU_OUTPUT_DPAD0] = { > + .possible_crtcs = BIT(2), > + .port = 0, > + }, > + [RCAR_DU_OUTPUT_HDMI0] = { > + .possible_crtcs = BIT(1), > + .port = 1, > + }, > + [RCAR_DU_OUTPUT_LVDS0] = { > + .possible_crtcs = BIT(0), I wonder whether it wouldn't be easier to read if we replaced possible_crtcs with possible_channels, as this structure describes the hardware and had its num_crtcs field replaced with a channel_mask. This would require converting the possible_channels field to a possible_crtcs field in rcar_du_modeset_init(), and I think that no change would be needed in rcar_du_group_setup_defr8() (but please double check). On the other hand, no code would be simplified, and rcar_du_modeset_init() would gain some additional complexity, so it might not be worth it. Either way this patch looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + .port = 2, > + }, > + }, > + .num_lvds = 1, > + .dpll_ch = BIT(1), > +}; > + > static const struct rcar_du_device_info rcar_du_r8a77970_info = { > .gen = 3, > .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK > @@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = { > { .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info }, > { .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info }, > { .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info }, > + { .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info }, > { .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info }, > { } > };
On 26/04/18 21:43, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thursday, 26 April 2018 19:53:36 EEST Kieran Bingham wrote: >> The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output. >> >> This platform is unusual in that the VGA is connected to DU3 leaving DU2 >> unpopulated. This is reflected by the channel_mask accordingly. > > I'd write s/VGA/DPAD/g (or s/VGA/RGB/g) as the DPAD output can be used for > other purposes than VGA. > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index d6ebc628fc22..4d195ff8c569 >> 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> @@ -246,6 +246,34 @@ static const struct rcar_du_device_info >> rcar_du_r8a7796_info = { .dpll_ch = BIT(1), >> }; >> >> +static const struct rcar_du_device_info rcar_du_r8a77965_info = { >> + .gen = 3, >> + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK >> + | RCAR_DU_FEATURE_EXT_CTRL_REGS >> + | RCAR_DU_FEATURE_VSP1_SOURCE, >> + .channel_mask = BIT(0) | BIT(1) | BIT(3), > > Depending on what you think of my suggestions for patch 05/17, you might want > to reverse the bit order here. Done. > >> + .routes = { >> + /* >> + * R8A77965 has one RGB output, one LVDS output and one HDMI >> + * output. >> + */ >> + [RCAR_DU_OUTPUT_DPAD0] = { >> + .possible_crtcs = BIT(2), >> + .port = 0, >> + }, >> + [RCAR_DU_OUTPUT_HDMI0] = { >> + .possible_crtcs = BIT(1), >> + .port = 1, >> + }, >> + [RCAR_DU_OUTPUT_LVDS0] = { >> + .possible_crtcs = BIT(0), > > I wonder whether it wouldn't be easier to read if we replaced possible_crtcs > with possible_channels, as this structure describes the hardware and had its > num_crtcs field replaced with a channel_mask. This would require converting > the possible_channels field to a possible_crtcs field in > rcar_du_modeset_init(), and I think that no change would be needed in > rcar_du_group_setup_defr8() (but please double check). On the other hand, no > code would be simplified, and rcar_du_modeset_init() would gain some > additional complexity, so it might not be worth it. I think we can leave this for now and consider it later if worth while. > > Either way this patch looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, collected. -- Kieran > >> + .port = 2, >> + }, >> + }, >> + .num_lvds = 1, >> + .dpll_ch = BIT(1), >> +}; >> + >> static const struct rcar_du_device_info rcar_du_r8a77970_info = { >> .gen = 3, >> .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK >> @@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = { >> { .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info }, >> { .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info }, >> { .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info }, >> + { .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info }, >> { .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info }, >> { } >> }; >
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index d6ebc628fc22..4d195ff8c569 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -246,6 +246,34 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = { .dpll_ch = BIT(1), }; +static const struct rcar_du_device_info rcar_du_r8a77965_info = { + .gen = 3, + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK + | RCAR_DU_FEATURE_EXT_CTRL_REGS + | RCAR_DU_FEATURE_VSP1_SOURCE, + .channel_mask = BIT(0) | BIT(1) | BIT(3), + .routes = { + /* + * R8A77965 has one RGB output, one LVDS output and one HDMI + * output. + */ + [RCAR_DU_OUTPUT_DPAD0] = { + .possible_crtcs = BIT(2), + .port = 0, + }, + [RCAR_DU_OUTPUT_HDMI0] = { + .possible_crtcs = BIT(1), + .port = 1, + }, + [RCAR_DU_OUTPUT_LVDS0] = { + .possible_crtcs = BIT(0), + .port = 2, + }, + }, + .num_lvds = 1, + .dpll_ch = BIT(1), +}; + static const struct rcar_du_device_info rcar_du_r8a77970_info = { .gen = 3, .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK @@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = { { .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info }, { .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info }, { .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info }, + { .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info }, { .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info }, { } };
The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output. This platform is unusual in that the VGA is connected to DU3 leaving DU2 unpopulated. This is reflected by the channel_mask accordingly. Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)