Re: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2848 bytes --]

Hi Sam,

Thanks for the review, I'll address the points left out.

On Sat, Jan 26, 2019 at 04:39:46PM +0100, Sam Ravnborg wrote:
> > +		return ret;
> > +	}
> > +
> > +	/* Reset */
> > +	msleep(20);
> > +	gpiod_set_value(ctx->gpios.power, 1);
> > +	msleep(20);
> > +	gpiod_set_value(ctx->gpios.reset, 1);
> > +	msleep(20);
> > +	return 0;
> > +}
> To verify the above pointer to a datasheet would be nice.

Unfortunately, it's not publicly available :/

> > +
> > +static int rb070d30_panel_unprepare(struct drm_panel *panel)
> > +{
> > +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> > +
> > +	gpiod_set_value(ctx->gpios.power, 0);
> > +	gpiod_set_value(ctx->gpios.reset, 0);
> > +	regulator_disable(ctx->supply);
> > +
> > +	return 0;
> > +}
>
> There is sometimes timing constrains after deassert reset..
> The order is not strictly reverse of prepare - is that on purpose?

You're right about the order. I couldn't find any delay after a reset
though in the datasheet.

> > +/* Default timings */
> > +static const struct drm_display_mode default_mode = {
> > +	.clock		= 51206,
> > +	.hdisplay	= 1024,
> > +	.hsync_start	= 1024 + 160,
> > +	.hsync_end	= 1024 + 160 + 80,
> > +	.htotal		= 1024 + 160 + 80 + 80,
> > +	.vdisplay	= 600,
> > +	.vsync_start	= 600 + 12,
> > +	.vsync_end	= 600 + 12 + 10,
> > +	.vtotal		= 600 + 12 + 10 + 13,
> > +	.vrefresh	= 60,
> > +};
> height and width missing here. Seems better to add them here than hiding in code below.
> Is it on purpose that no flags are specified?
> 
> > +
> > +static int rb070d30_panel_get_modes(struct drm_panel *panel)
> > +{
> > +	struct drm_connector *connector = panel->connector;
> > +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
> > +	struct drm_display_mode *mode;
> > +	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +
> > +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> > +	if (!mode) {
> > +		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> > +			default_mode.hdisplay, default_mode.vdisplay,
> > +			default_mode.vrefresh);
> Use"
> 		DRM_DEV_ERROR(&ctx->dsi->dev,
> 			      "failed to add mode" DRM_MODE_FMT,
> 			      DRM_MODE_ARG(mode));
> > +		return -EINVAL;
> > +	}
> > +
> > +	drm_mode_set_name(mode);
> > +
> > +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > +	drm_mode_probed_add(connector, mode);
> > +
> > +	panel->connector->display_info.bpc = 8;
> > +	panel->connector->display_info.width_mm = 154;
> > +	panel->connector->display_info.height_mm = 85;
> See comment on height above.
> Same goes for bpc

Sorry, I'm not sure to follow you here. bpc and height are both set?

Thanks!
Maxime


-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel