Hi Jagan.
Gave this another more detailed read - triggered some additional comments.
Depite the comments it looks good, and this is all more or
less cosmetic improvements.
Sam
> +struct st7701_panel_desc {
> + const struct drm_display_mode *mode;
> + unsigned int lanes;
> + unsigned long flags;
> + enum mipi_dsi_pixel_format format;
> + const char *const *supply_names;
> + unsigned int num_supplies;
> + unsigned int panel_sleep_delay;
> +};
> +
> +struct st7701 {
> + struct drm_panel panel;
> + struct mipi_dsi_device *dsi;
> + const struct st7701_panel_desc *desc;
> +
> + struct backlight_device *backlight;
> + struct regulator_bulk_data *supplies;
> + unsigned int num_supplies;
I cannot see that num_supplies in this struct are used?
> + struct gpio_desc *reset;
> + unsigned int sleep_delay;
> +};
> +
> +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> +{
> + return container_of(panel, struct st7701, panel);
> +}
> +
> +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
> + size_t len)
> +{
> + return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
> +}
> +
> +static int st7701_prepare(struct drm_panel *panel)
> +{
> + struct st7701 *st7701 = panel_to_st7701(panel);
> + int ret;
> +
> + gpiod_set_value(st7701->reset, 0);
> + msleep(20);
> +
> + ret = regulator_bulk_enable(st7701->desc->num_supplies,
> + st7701->supplies);
> + if (ret < 0)
> + return ret;
> + msleep(20);
> +
> + gpiod_set_value(st7701->reset, 1);
> + msleep(20);
> +
> + gpiod_set_value(st7701->reset, 0);
> + msleep(30);
> +
> + gpiod_set_value(st7701->reset, 1);
> + msleep(150);
> +
> + st7701_init_sequence(st7701);
> +
> + return 0;
> +}
> +
> +static int st7701_unprepare(struct drm_panel *panel)
> +{
> + struct st7701 *st7701 = panel_to_st7701(panel);
> +
> + ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
> +
> + msleep(st7701->sleep_delay);
> +
> + gpiod_set_value(st7701->reset, 0);
> +
> + gpiod_set_value(st7701->reset, 1);
> +
> + gpiod_set_value(st7701->reset, 0);
No timing constrains here? In prepare there are sleeps intermixed.
> +
> + regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
> +
> + return 0;
> +}
> +
> +static int st7701_get_modes(struct drm_panel *panel)
> +{
> + struct st7701 *st7701 = panel_to_st7701(panel);
> + const struct drm_display_mode *desc_mode = st7701->desc->mode;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(panel->drm, desc_mode);
> + if (!mode) {
> + dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
> + desc_mode->hdisplay, desc_mode->vdisplay,
> + desc_mode->vrefresh);
Use something like:
DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
DRM_MODE_ARG(desc_mode));
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> + drm_mode_probed_add(panel->connector, mode);
> +
> + panel->connector->display_info.width_mm = desc_mode->width_mm;
> + panel->connector->display_info.height_mm = desc_mode->height_mm;
> +
> + return 1;
> +}
> +
> +static const struct st7701_panel_desc ts8550b_desc = {
> + .mode = &ts8550b_mode,
> + .lanes = 2,
> + .flags = MIPI_DSI_MODE_VIDEO,
> + .format = MIPI_DSI_FMT_RGB888,
> + .supply_names = ts8550b_supply_names,
> + .num_supplies = ARRAY_SIZE(ts8550b_supply_names),
> + .panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
In the only place this is used there is added 120 ms too.
Looks inconsistent - do we have the same delay twice?
> +
> +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> + const struct st7701_panel_desc *desc;
> + struct device_node *np;
> + struct st7701 *st7701;
> + int ret, i;
> +
> + st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
> + if (!st7701)
> + return -ENOMEM;
> +
> + desc = of_device_get_match_data(&dsi->dev);
> + dsi->mode_flags = desc->flags;
> + dsi->format = desc->format;
> + dsi->lanes = desc->lanes;
> +
> + st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
> + sizeof(*st7701->supplies),
> + GFP_KERNEL);
> + if (!st7701->supplies)
> + return -ENOMEM;
> +
> + for (i = 0; i < desc->num_supplies; i++)
> + st7701->supplies[i].supply = desc->supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
> + st7701->supplies);
> + if (ret < 0)
> + return ret;
> +
> + st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(st7701->reset)) {
> + dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> + return PTR_ERR(st7701->reset);
> + }
> +
> + np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> + if (np) {
> + st7701->backlight = of_find_backlight_by_node(np);
> + of_node_put(np);
> +
> + if (!st7701->backlight)
> + return -EPROBE_DEFER;
> + }
This is a simpler variant of the above:
st7701->backlight = devm_of_find_backlight(dsi->dev);
if (IS_ERR(st7701->backlight))
return PTR_ERR(st7701->backlight);
Suggested by Noralf in another thread for a similar driver.
> +
> + drm_panel_init(&st7701->panel);
> +
> + /* We need to wait 120ms after a sleep out command */
> + st7701->sleep_delay = 120 + desc->panel_sleep_delay;
This is the other place where we add 120 to the previous 80 - in total 200 ms.
> + st7701->panel.funcs = &st7701_funcs;
> + st7701->panel.dev = &dsi->dev;
> +
> + ret = drm_panel_add(&st7701->panel);
> + if (ret < 0)
> + return ret;
> +
> + mipi_dsi_set_drvdata(dsi, st7701);
> + st7701->dsi = dsi;
> + st7701->desc = desc;
This assignment could come earlier, so we do not have it unassigned when calling
other functions. As it is now it works so you can safely ignore this comment.
> +
> + return mipi_dsi_attach(dsi);
> +}
> +
> +static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> + struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
> +
> + mipi_dsi_detach(dsi);
> + drm_panel_remove(&st7701->panel);
> +
> + if (st7701->backlight)
> + put_device(&st7701->backlight->dev);
Use:
backlight_put(st7701->backlight);
