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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
