| Message ID | 20190111124714.19566-2-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V7,1/2] dt-bindings: panel: Add Sitronix ST7701 panel documentation | expand |
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);
Den 11.01.2019 22.19, skrev Sam Ravnborg: > 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); This happens automatically on driver detach if devm_of_find_backlight() is used Noralf.
On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > 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. Thanks for the review. > > 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? Yes it is used in the code, please check in struct st7701_panel_desc. > > > > + 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. Delay while doing unprare is not needed I suppose. > > > + > > + 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? 120ms is the one recommended by st7701 controller delay after a sleep out command, please check it in datasheet [1], page 188. And this 80ms is specific to TS8550B panel as per BSP code this doesn't have proper documentation but the BSP code doing this extra 80ms. > > > > + > > +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. Please see above comment. [1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf
Hi Jagan. > On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > 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. > > Thanks for the review. > > > > > 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? > > Yes it is used in the code, please check in struct st7701_panel_desc. I have applied the patch and deleted num_supplies - now build errors. So num_supplies in struct st7701 is not used and should be deleted. > > > +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); Should this be MIPI_DCS_ENTER_SLEEP_MODE? Note - you have shortcuts fot the standard commands like in this case: mipi_dsi_dcs_enter_sleep_mode(st7701->dsi); Thay could be introduced in many palces, but I also like how all the commands follows a consistent way to be issued. So consider this only if this was new for you. > > > + > > > + 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. > > Delay while doing unprare is not needed I suppose. If the purpose is alone to reset the display then a single write '0' should do it I think And there is a requirement that it must be low for a minimum of 10 us which would be good to have here. I aslo found in chapter 9. (page 163 - second line) this statement: "VDDA and VDDI must be powered down with minimum 120msec." This is similar to the unprepare delay to be found in simple-panel.c So an unprepare delay seems in order here. > > > +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? > > 120ms is the one recommended by st7701 controller delay after a sleep > out command, please check it in datasheet [1], page 188. And this 80ms > is specific to TS8550B panel as per BSP code this doesn't have proper > documentation but the BSP code doing this extra 80ms. OK, thanks for the pointer to the datasheet. > [1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf Sam
On Sat, Jan 12, 2019 at 3:44 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Jagan. > > > On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > 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. > > > > Thanks for the review. > > > > > > > > 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? > > > > Yes it is used in the code, please check in struct st7701_panel_desc. > I have applied the patch and deleted num_supplies - now build errors. > So num_supplies in struct st7701 is not used and should be deleted. Sorry, now I found it thanks, will drop in next version. > > > > > +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); > > Should this be MIPI_DCS_ENTER_SLEEP_MODE? > > Note - you have shortcuts fot the standard commands like > in this case: > > mipi_dsi_dcs_enter_sleep_mode(st7701->dsi); I even tried this, but the difference wrt ST7701 can pass, same command with type MIPI_DSI_DCS_SHORT_WRITE_PARAM but the default mipi_dsi_dcs_enter_sleep_mode has length 0 so it's MIPI_DSI_DCS_SHORT_WRITE type. ie reason I have not used. > > Thay could be introduced in many palces, but I also like how all the > commands follows a consistent way to be issued. > So consider this only if this was new for you. > > > > > > + > > > > + 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. > > > > Delay while doing unprare is not needed I suppose. > > If the purpose is alone to reset the display then a single write '0' > should do it I think I even tried this just set 0, since prepare is doing a sequence, it good behavior to do the reverse during handoff. ie reason I just initiated this sequence. > And there is a requirement that it must be low for a minimum of 10 us > which would be good to have here. Sorry, I didn't get this requirement what is this for? > > I aslo found in chapter 9. (page 163 - second line) this statement: > "VDDA and VDDI must be powered down with minimum 120msec." Yes unprepare is doing the same, it exit from sleep out mode and wait for 120msec and do the reset. > > This is similar to the unprepare delay to be found in simple-panel.c > So an unprepare delay seems in order here. Look like simple-panel.c is doing delay after reset initiated and regulator disabled.
> > > > > > > + > > > > > + 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. > > > > > > Delay while doing unprare is not needed I suppose. > > > > If the purpose is alone to reset the display then a single write '0' > > should do it I think > > I even tried this just set 0, since prepare is doing a sequence, it > good behavior to do the reverse during handoff. ie reason I just > initiated this sequence. > > > And there is a requirement that it must be low for a minimum of 10 us > > which would be good to have here. > > Sorry, I didn't get this requirement what is this for? The st7701 will ignore reset pulse shorter than 10 us - see Trw in table 9 reset timing (page 54). But as we just assert reset (set it to 0), this timing constraint can be ignored. > > I aslo found in chapter 9. (page 163 - second line) this statement: > > "VDDA and VDDI must be powered down with minimum 120msec." > > Yes unprepare is doing the same, it exit from sleep out mode and wait > for 120msec and do the reset. > > > > > This is similar to the unprepare delay to be found in simple-panel.c > > So an unprepare delay seems in order here. > > Look like simple-panel.c is doing delay after reset initiated and > regulator disabled. Sam
On Sat, Jan 12, 2019 at 5:31 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > > > > > + > > > > > > + 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. > > > > > > > > Delay while doing unprare is not needed I suppose. > > > > > > If the purpose is alone to reset the display then a single write '0' > > > should do it I think > > > > I even tried this just set 0, since prepare is doing a sequence, it > > good behavior to do the reverse during handoff. ie reason I just > > initiated this sequence. > > > > > And there is a requirement that it must be low for a minimum of 10 us > > > which would be good to have here. > > > > Sorry, I didn't get this requirement what is this for? > The st7701 will ignore reset pulse shorter than 10 us - see Trw in > table 9 reset timing (page 54). > > But as we just assert reset (set it to 0), this timing constraint can be ignored. But we unaware of reset pulse duration right? it's the hardware that bring the reset assert if we set the line 0. am I correct or do we need to explicitly wait 10us after reset initiated? there is family of chip Sitronix st7789v which don't taking care of this sequence (I don't know why?) in drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of datasheet[2] [2] https://www.newhavendisplay.com/appnotes/datasheets/LCDs/ST7789V.pdf
Hi Jagan. > > But as we just assert reset (set it to 0), this timing constraint can be ignored. > > But we unaware of reset pulse duration right? it's the hardware that > bring the reset assert if we set the line 0. am I correct or do we > need to explicitly wait 10us after reset initiated? > there is family of chip Sitronix st7789v which don't taking care of > this sequence (I don't know why?) in > drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of > datasheet[2] The prepare sequence used is: 1) reset 20 ms 2) regulator enable 20 ms 3) unassert reset 20 ms 4) assert reset 30 ms 4) unassert reset 150 ms 5) SOFT RESET 5 ms 6) Exit SLEEP mode sleep_delay (120 ms) 7) COMMANDS The enable sequence is 1) Display ON 2) backlight enable The disable sequence is 1) backlight disable 2) Display OFF The unprepare sequence is 1) Exit SLEEP mode <= this should be Enter SLEEP mode (covered by previous mail) sleep_delay (120 ms) 2) assert reset 3) unassert reset 4) assert reset 5) regulator disable The implementation in simple-panel supports a long list of panels so we can assume this is a good reference implementation. Unless your combo of st7701 + panel has special requirements. Prepare sequence: ----------------- - Just set reset to 1 - no need to trigger an edge. But if you somehow think the edge is required keep the current code but drop the delays that are not required. We can assume power is OK when it is enabled, no extra waiting required. - No reason to unassert and then assert and then unassert reset. In other words: - Delay between 1 and 2 can be dropped. - step 3 and step 4 can be dropped Unprepare sequence: ------------------- Enter SLEEP mode may take 120 ms so delay between 1) and 2) is OK. No reason to do the reset dance, drop 2) and 3) Make sure reset is completed, so wait before leaving the function. See note 3. on page 54 in datasheet: "During the Resetting period, the display will be blanked (The display is entering blanking sequence, which maximum time is 120 ms, when Reset Starts in Sleep Out –mode. The display remains the blank state in Sleep In –mode.) and then return to Default condition for Hardware Reset." We have already sent an Enter SLEEP command but we have no timing constrains when panel is in SLEEP mode, So stick to the 120 ms. Just use sleep_delay. Thats my best understanding, your reading of the datasheet or your testing may prove different. Sam
Hi Sam, On Sat, Jan 12, 2019 at 9:03 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Jagan. > > > > But as we just assert reset (set it to 0), this timing constraint can be ignored. > > > > But we unaware of reset pulse duration right? it's the hardware that > > bring the reset assert if we set the line 0. am I correct or do we > > need to explicitly wait 10us after reset initiated? > > there is family of chip Sitronix st7789v which don't taking care of > > this sequence (I don't know why?) in > > drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of > > datasheet[2] > > > The prepare sequence used is: > 1) reset > 20 ms > 2) regulator enable > 20 ms > 3) unassert reset > 20 ms > 4) assert reset > 30 ms > 4) unassert reset > 150 ms > 5) SOFT RESET > 5 ms > 6) Exit SLEEP mode > sleep_delay (120 ms) > 7) COMMANDS > > The enable sequence is > 1) Display ON > 2) backlight enable > > > The disable sequence is > 1) backlight disable > 2) Display OFF > > The unprepare sequence is > 1) Exit SLEEP mode <= this should be Enter SLEEP mode (covered by previous mail) > sleep_delay (120 ms) > 2) assert reset > 3) unassert reset > 4) assert reset > 5) regulator disable > > > The implementation in simple-panel supports a long list of panels so we > can assume this is a good reference implementation. > Unless your combo of st7701 + panel has special requirements. > > Prepare sequence: > ----------------- > - Just set reset to 1 - no need to trigger an edge. > But if you somehow think the edge is required keep the current code > but drop the delays that are not required. > We can assume power is OK when it is enabled, no extra waiting required. > - No reason to unassert and then assert and then unassert reset. > > In other words: > - Delay between 1 and 2 can be dropped. > - step 3 and step 4 can be dropped > > Unprepare sequence: > ------------------- > Enter SLEEP mode may take 120 ms so delay between 1) and 2) is OK. > No reason to do the reset dance, drop 2) and 3) > Make sure reset is completed, so wait before leaving the function. > See note 3. on page 54 in datasheet: > > "During the Resetting period, the display will be blanked > (The display is entering blanking sequence, which maximum > time is 120 ms, when Reset Starts in Sleep Out –mode. > The display remains the blank state in Sleep In –mode.) and > then return to Default condition for Hardware Reset." > > We have already sent an Enter SLEEP command but we have no timing constrains > when panel is in SLEEP mode, So stick to the 120 ms. > Just use sleep_delay. So we need this delay even after reset unlike SLEEP mode delay. OK, thanks for the points will try to check all these points. When did .unprepare and .disable are actually called? I turn-off the backlight by echo 1 > /sys/class/backlight/backlight/bl_power and even power of the board I assume the video transmission stop so it would ended-up calling these, but I couldn't see prints. does it the same time uart dead or something? do you have any inputs on sanity testing of typical panels, if yes please share. Jagan.
Hi Jagan. > When did .unprepare and .disable are actually called? I turn-off the > backlight by echo 1 > /sys/class/backlight/backlight/bl_power and even > power of the board I assume the video transmission stop so it would > ended-up calling these, but I couldn't see prints. does it the same > time uart dead or something? do you have any inputs on sanity testing > of typical panels, if yes please share. Sorry, I do not rememebr the details to help you here. Maybe module unload and then manipulating the panel in /sys/ would do the trick. Sam
diff --git a/MAINTAINERS b/MAINTAINERS index 3f6db876be1f..d2928a4d2847 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4831,6 +4831,12 @@ S: Orphan / Obsolete F: drivers/gpu/drm/sis/ F: include/uapi/drm/sis_drm.h +DRM DRIVER FOR SITRONIX ST7701 PANELS +M: Jagan Teki <jagan@amarulasolutions.com> +S: Maintained +F: drivers/gpu/drm/panel/panel-sitronix-st7701.c +F: Documentation/devicetree/bindings/display/panel/sitronix,st7701.txt + DRM DRIVER FOR SITRONIX ST7586 PANELS M: David Lechner <david@lechnology.com> S: Maintained diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 3f3537719beb..d93b2ba709bc 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -196,6 +196,16 @@ config DRM_PANEL_SHARP_LS043T1LE01 Say Y here if you want to enable support for Sharp LS043T1LE01 qHD (540x960) DSI panel as found on the Qualcomm APQ8074 Dragonboard +config DRM_PANEL_SITRONIX_ST7701 + tristate "Sitronix ST7701 panel driver" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y here if you want to enable support for the Sitronix + ST7701 controller for 480X864 LCD panels with MIPI/RGB/SPI + system interfaces. + config DRM_PANEL_SITRONIX_ST7789V tristate "Sitronix ST7789V panel" depends on OF && SPI diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 4396658a7996..6a9b4cec1891 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -20,5 +20,6 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o +obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c new file mode 100644 index 000000000000..8638a5463cb6 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c @@ -0,0 +1,427 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2019, Amarula Solutions. + * Author: Jagan Teki <jagan@amarulasolutions.com> + */ + +#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/of_device.h> + +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +/* Command2 BKx selection command */ +#define DSI_CMD2BKX_SEL 0xFF + +/* Command2, BK0 commands */ +#define DSI_CMD2_BK0_PVGAMCTRL 0xB0 /* Positive Voltage Gamma Control */ +#define DSI_CMD2_BK0_NVGAMCTRL 0xB1 /* Negative Voltage Gamma Control */ +#define DSI_CMD2_BK0_LNESET 0xC0 /* Display Line setting */ +#define DSI_CMD2_BK0_PORCTRL 0xC1 /* Porch control */ +#define DSI_CMD2_BK0_INVSEL 0xC2 /* Inversion selection, Frame Rate Control */ + +/* Command2, BK1 commands */ +#define DSI_CMD2_BK1_VRHS 0xB0 /* Vop amplitude setting */ +#define DSI_CMD2_BK1_VCOM 0xB1 /* VCOM amplitude setting */ +#define DSI_CMD2_BK1_VGHSS 0xB2 /* VGH Voltage setting */ +#define DSI_CMD2_BK1_TESTCMD 0xB3 /* TEST Command Setting */ +#define DSI_CMD2_BK1_VGLS 0xB5 /* VGL Voltage setting */ +#define DSI_CMD2_BK1_PWCTLR1 0xB7 /* Power Control 1 */ +#define DSI_CMD2_BK1_PWCTLR2 0xB8 /* Power Control 2 */ +#define DSI_CMD2_BK1_SPD1 0xC1 /* Source pre_drive timing set1 */ +#define DSI_CMD2_BK1_SPD2 0xC2 /* Source EQ2 Setting */ +#define DSI_CMD2_BK1_MIPISET1 0xD0 /* MIPI Setting 1 */ + +/** + * Command2 with BK function selection. + * + * BIT[4, 0]: [CN2, BKXSEL] + * 10 = CMD2BK0, Command2 BK0 + * 11 = CMD2BK1, Command2 BK1 + * 00 = Command2 disable + */ +#define DSI_CMD2BK1_SEL 0x11 +#define DSI_CMD2BK0_SEL 0x10 +#define DSI_CMD2BKX_SEL_NONE 0x00 + +/* Command2, BK0 bytes */ +#define DSI_LINESET_LINE 0x69 +#define DSI_LINESET_LDE_EN BIT(7) +#define DSI_LINESET_LINEDELTA GENMASK(1, 0) +#define DSI_CMD2_BK0_LNESET_B1 DSI_LINESET_LINEDELTA +#define DSI_CMD2_BK0_LNESET_B0 (DSI_LINESET_LDE_EN | DSI_LINESET_LINE) +#define DSI_INVSEL_DEFAULT GENMASK(5, 4) +#define DSI_INVSEL_NLINV GENMASK(2, 0) +#define DSI_INVSEL_RTNI GENMASK(2, 1) +#define DSI_CMD2_BK0_INVSEL_B1 DSI_INVSEL_RTNI +#define DSI_CMD2_BK0_INVSEL_B0 (DSI_INVSEL_DEFAULT | DSI_INVSEL_NLINV) +#define DSI_CMD2_BK0_PORCTRL_B0(m) ((m)->vtotal - (m)->vsync_end) +#define DSI_CMD2_BK0_PORCTRL_B1(m) ((m)->vsync_start - (m)->vdisplay) + +/* Command2, BK1 bytes */ +#define DSI_CMD2_BK1_VRHA_SET 0x45 +#define DSI_CMD2_BK1_VCOM_SET 0x13 +#define DSI_CMD2_BK1_VGHSS_SET GENMASK(2, 0) +#define DSI_CMD2_BK1_TESTCMD_VAL BIT(7) +#define DSI_VGLS_DEFAULT BIT(6) +#define DSI_VGLS_SEL GENMASK(2, 0) +#define DSI_CMD2_BK1_VGLS_SET (DSI_VGLS_DEFAULT | DSI_VGLS_SEL) +#define DSI_PWCTLR1_AP BIT(7) /* Gamma OP bias, max */ +#define DSI_PWCTLR1_APIS BIT(2) /* Source OP input bias, min */ +#define DSI_PWCTLR1_APOS BIT(0) /* Source OP output bias, min */ +#define DSI_CMD2_BK1_PWCTLR1_SET (DSI_PWCTLR1_AP | DSI_PWCTLR1_APIS | \ + DSI_PWCTLR1_APOS) +#define DSI_PWCTLR2_AVDD BIT(5) /* AVDD 6.6v */ +#define DSI_PWCTLR2_AVCL 0x0 /* AVCL -4.4v */ +#define DSI_CMD2_BK1_PWCTLR2_SET (DSI_PWCTLR2_AVDD | DSI_PWCTLR2_AVCL) +#define DSI_SPD1_T2D BIT(3) +#define DSI_CMD2_BK1_SPD1_SET (GENMASK(6, 4) | DSI_SPD1_T2D) +#define DSI_CMD2_BK1_SPD2_SET DSI_CMD2_BK1_SPD1_SET +#define DSI_MIPISET1_EOT_EN BIT(3) +#define DSI_CMD2_BK1_MIPISET1_SET (BIT(7) | DSI_MIPISET1_EOT_EN) + +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; + 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); +} + +#define ST7701_DSI(st7701, seq...) \ + { \ + const u8 d[] = { seq }; \ + st7701_dsi_write(st7701, d, ARRAY_SIZE(d)); \ + } + +static void st7701_init_sequence(struct st7701 *st7701) +{ + const struct drm_display_mode *mode = st7701->desc->mode; + + ST7701_DSI(st7701, MIPI_DCS_SOFT_RESET, 0x00); + + /* We need to wait 5ms before sending new commands */ + msleep(5); + + ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00); + + msleep(st7701->sleep_delay); + + /* Command2, BK0 */ + ST7701_DSI(st7701, DSI_CMD2BKX_SEL, + 0x77, 0x01, 0x00, 0x00, DSI_CMD2BK0_SEL); + ST7701_DSI(st7701, DSI_CMD2_BK0_PVGAMCTRL, 0x00, 0x0E, 0x15, 0x0F, + 0x11, 0x08, 0x08, 0x08, 0x08, 0x23, 0x04, 0x13, 0x12, + 0x2B, 0x34, 0x1F); + ST7701_DSI(st7701, DSI_CMD2_BK0_NVGAMCTRL, 0x00, 0x0E, 0x95, 0x0F, + 0x13, 0x07, 0x09, 0x08, 0x08, 0x22, 0x04, 0x10, 0x0E, + 0x2C, 0x34, 0x1F); + ST7701_DSI(st7701, DSI_CMD2_BK0_LNESET, + DSI_CMD2_BK0_LNESET_B0, DSI_CMD2_BK0_LNESET_B1); + ST7701_DSI(st7701, DSI_CMD2_BK0_PORCTRL, + DSI_CMD2_BK0_PORCTRL_B0(mode), + DSI_CMD2_BK0_PORCTRL_B1(mode)); + ST7701_DSI(st7701, DSI_CMD2_BK0_INVSEL, + DSI_CMD2_BK0_INVSEL_B0, DSI_CMD2_BK0_INVSEL_B1); + + /* Command2, BK1 */ + ST7701_DSI(st7701, DSI_CMD2BKX_SEL, + 0x77, 0x01, 0x00, 0x00, DSI_CMD2BK1_SEL); + ST7701_DSI(st7701, DSI_CMD2_BK1_VRHS, DSI_CMD2_BK1_VRHA_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_VCOM, DSI_CMD2_BK1_VCOM_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_VGHSS, DSI_CMD2_BK1_VGHSS_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_TESTCMD, DSI_CMD2_BK1_TESTCMD_VAL); + ST7701_DSI(st7701, DSI_CMD2_BK1_VGLS, DSI_CMD2_BK1_VGLS_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_PWCTLR1, DSI_CMD2_BK1_PWCTLR1_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_PWCTLR2, DSI_CMD2_BK1_PWCTLR2_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_SPD1, DSI_CMD2_BK1_SPD1_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_SPD2, DSI_CMD2_BK1_SPD2_SET); + ST7701_DSI(st7701, DSI_CMD2_BK1_MIPISET1, DSI_CMD2_BK1_MIPISET1_SET); + + /** + * ST7701_SPEC_V1.2 is unable to provide enough information above this + * specific command sequence, so grab the same from vendor BSP driver. + */ + ST7701_DSI(st7701, 0xE0, 0x00, 0x00, 0x02); + ST7701_DSI(st7701, 0xE1, 0x0B, 0x00, 0x0D, 0x00, 0x0C, 0x00, 0x0E, + 0x00, 0x00, 0x44, 0x44); + ST7701_DSI(st7701, 0xE2, 0x33, 0x33, 0x44, 0x44, 0x64, 0x00, 0x66, + 0x00, 0x65, 0x00, 0x67, 0x00, 0x00); + ST7701_DSI(st7701, 0xE3, 0x00, 0x00, 0x33, 0x33); + ST7701_DSI(st7701, 0xE4, 0x44, 0x44); + ST7701_DSI(st7701, 0xE5, 0x0C, 0x78, 0x3C, 0xA0, 0x0E, 0x78, 0x3C, + 0xA0, 0x10, 0x78, 0x3C, 0xA0, 0x12, 0x78, 0x3C, 0xA0); + ST7701_DSI(st7701, 0xE6, 0x00, 0x00, 0x33, 0x33); + ST7701_DSI(st7701, 0xE7, 0x44, 0x44); + ST7701_DSI(st7701, 0xE8, 0x0D, 0x78, 0x3C, 0xA0, 0x0F, 0x78, 0x3C, + 0xA0, 0x11, 0x78, 0x3C, 0xA0, 0x13, 0x78, 0x3C, 0xA0); + ST7701_DSI(st7701, 0xEB, 0x02, 0x02, 0x39, 0x39, 0xEE, 0x44, 0x00); + ST7701_DSI(st7701, 0xEC, 0x00, 0x00); + ST7701_DSI(st7701, 0xED, 0xFF, 0xF1, 0x04, 0x56, 0x72, 0x3F, 0xFF, + 0xFF, 0xFF, 0xFF, 0xF3, 0x27, 0x65, 0x40, 0x1F, 0xFF); + + /* disable Command2 */ + ST7701_DSI(st7701, DSI_CMD2BKX_SEL, + 0x77, 0x01, 0x00, 0x00, DSI_CMD2BKX_SEL_NONE); +} + +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_enable(struct drm_panel *panel) +{ + struct st7701 *st7701 = panel_to_st7701(panel); + + ST7701_DSI(st7701, MIPI_DCS_SET_DISPLAY_ON, 0x00); + backlight_enable(st7701->backlight); + + return 0; +} + +static int st7701_disable(struct drm_panel *panel) +{ + struct st7701 *st7701 = panel_to_st7701(panel); + + backlight_disable(st7701->backlight); + ST7701_DSI(st7701, MIPI_DCS_SET_DISPLAY_OFF, 0x00); + + 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); + + 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); + 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 drm_panel_funcs st7701_funcs = { + .disable = st7701_disable, + .unprepare = st7701_unprepare, + .prepare = st7701_prepare, + .enable = st7701_enable, + .get_modes = st7701_get_modes, +}; + +static const struct drm_display_mode ts8550b_mode = { + .clock = 27500, + + .hdisplay = 480, + .hsync_start = 480 + 38, + .hsync_end = 480 + 38 + 12, + .htotal = 480 + 38 + 12 + 12, + + .vdisplay = 854, + .vsync_start = 854 + 4, + .vsync_end = 854 + 4 + 8, + .vtotal = 854 + 4 + 8 + 18, + + .width_mm = 69, + .height_mm = 139, + + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, +}; + +static const char * const ts8550b_supply_names[] = { + "VCC", + "IOVCC", +}; + +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 */ +}; + +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; + } + + drm_panel_init(&st7701->panel); + + /* We need to wait 120ms after a sleep out command */ + st7701->sleep_delay = 120 + desc->panel_sleep_delay; + 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; + + 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); + + return 0; +} + +static const struct of_device_id st7701_of_match[] = { + { .compatible = "techstar,ts8550b", .data = &ts8550b_desc }, + { } +}; +MODULE_DEVICE_TABLE(of, st7701_of_match); + +static struct mipi_dsi_driver st7701_dsi_driver = { + .probe = st7701_dsi_probe, + .remove = st7701_dsi_remove, + .driver = { + .name = "st7701", + .of_match_table = st7701_of_match, + }, +}; +module_mipi_dsi_driver(st7701_dsi_driver); + +MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>"); +MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver"); +MODULE_LICENSE("GPL");
ST7701 designed for small and medium sizes of TFT LCD display, is capable of supporting up to 480RGBX864 in resolution. It provides several system interfaces like MIPI/RGB/SPI. Currently added support for Techstar TS8550B which is ST7701 based 480x854, 2-lane MIPI DSI LCD panel. Driver now registering mipi_dsi device, but indeed it can extendable for RGB if any requirement trigger in future. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v7: - rebase on master Changes for v6: - use sleep delay value as per datasheet - add panel_sleep_delay variable for panel specific delay - use command sequnce display on and off instead panel functions - add proper comments on the delays - remove delays from command switch - move mode type on struct display mode - drop refresh rate value, let drm compute Changes for v5: - found the chip from vendor, so added new panel driver - here is v4: https://patchwork.kernel.org/patch/10680335/ MAINTAINERS | 6 + drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-sitronix-st7701.c | 427 ++++++++++++++++++ 4 files changed, 444 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-sitronix-st7701.c