Hi Jitao.
Driver looks good, one detail.
> +
> +struct panel_desc {
> + const struct drm_display_mode *modes;
> + unsigned int bpc;
> +
> + /**
> + * @width: width (in millimeters) of the panel's active display area
> + * @height: height (in millimeters) of the panel's active display area
> + */
> + struct {
> + unsigned int width;
> + unsigned int height;
> + } size;
Maybe name these width_mm and height_mm.
Then they have the same name as where they are copied to,
and it is explicit documented that it is in mm.
The extra indirection with a struct is not needed in display_mode,
maybe drop it here too?
> +
> + unsigned long mode_flags;
> + enum mipi_dsi_pixel_format format;
> + const struct panel_init_cmd *init_cmds;
> + unsigned int lanes;
> +};
> +
...
> +static int boe_panel_unprepare(struct drm_panel *panel)
> +{
> + struct boe_panel *boe = to_boe_panel(panel);
> + int ret;
> +
> + if (!boe->prepared)
> + return 0;
> +
> + ret = boe_panel_off(boe);
> + if (ret < 0) {
> + dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> + return ret;
> + }
> +
> + msleep(150);
> + if (boe->enable_gpio)
> + gpiod_set_value(boe->enable_gpio, 0);
Everywhere boe->enable_gpio is used it is checked like above.
Bot boe->enable_gpio in a mandatory property so it must be present.
The driver error out in probe if not present, so this check seems
redundandt?
Everything else looks really good.
With the above fixed / considered:
Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
Sam
