| Message ID | 20230422205012.2464933-9-sre@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Inanbo T28CP45TN89 panel support | expand |
Hi Sebastian, Thanks for the v2 of your series. Looks great! One nitpick though: you seem to wrap the patch message lines at ~50 characters sometimes, which is awfully short. Another comment below: On 4/22/23 22:50, Sebastian Reichel wrote: > Avoid hard-coding the default_mode and supply it from match data. One > additional layer of abstraction has been introduced, which will be > needed for specifying other panel information (e.g. bus flags) in the > next steps. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > --- > .../gpu/drm/panel/panel-sitronix-st7789v.c | 24 ++++++++++++++----- > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c > index a6d6155ef45c..29c2a91f8299 100644 > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c > @@ -108,8 +108,13 @@ > return val; \ > } while (0) > > +struct st7789_panel_info { > + const struct drm_display_mode *mode; > +}; > + > struct st7789v { > struct drm_panel panel; > + const struct st7789_panel_info *info; > struct spi_device *spi; > struct gpio_desc *reset; > struct regulator *power; > @@ -160,16 +165,21 @@ static const struct drm_display_mode default_mode = { > .vtotal = 320 + 8 + 4 + 4, > }; > > +struct st7789_panel_info default_panel = { > + .mode = &default_mode, > +}; Shouldn't this be "static const struct st7789_panel_info default_panel"? (Same holds for "struct st7789_panel_info t28cp45tn89_panel" in patch 13/13.) With the comments above addressed, feel free to add my Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net> to the whole v3 of your series. Thanks and best regards, Michael > + > static int st7789v_get_modes(struct drm_panel *panel, > struct drm_connector *connector) > { > + struct st7789v *ctx = panel_to_st7789v(panel); > struct drm_display_mode *mode; > > - mode = drm_mode_duplicate(connector->dev, &default_mode); > + mode = drm_mode_duplicate(connector->dev, ctx->info->mode); > if (!mode) { > - dev_err(panel->dev, "failed to add mode %ux%ux@%u\n", > - default_mode.hdisplay, default_mode.vdisplay, > - drm_mode_vrefresh(&default_mode)); > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", > + ctx->info->mode->hdisplay, ctx->info->mode->vdisplay, > + drm_mode_vrefresh(ctx->info->mode)); > return -ENOMEM; > } > > @@ -359,6 +369,8 @@ static int st7789v_probe(struct spi_device *spi) > spi_set_drvdata(spi, ctx); > ctx->spi = spi; > > + ctx->info = device_get_match_data(&spi->dev); > + > drm_panel_init(&ctx->panel, dev, &st7789v_drm_funcs, > DRM_MODE_CONNECTOR_DPI); > > @@ -389,13 +401,13 @@ static void st7789v_remove(struct spi_device *spi) > } > > static const struct spi_device_id st7789v_spi_id[] = { > - { "st7789v" }, > + { "st7789v", (unsigned long) &default_panel }, > { } > }; > MODULE_DEVICE_TABLE(spi, st7789v_spi_id); > > static const struct of_device_id st7789v_of_match[] = { > - { .compatible = "sitronix,st7789v" }, > + { .compatible = "sitronix,st7789v", .data = &default_panel }, > { } > }; > MODULE_DEVICE_TABLE(of, st7789v_of_match);
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c index a6d6155ef45c..29c2a91f8299 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c @@ -108,8 +108,13 @@ return val; \ } while (0) +struct st7789_panel_info { + const struct drm_display_mode *mode; +}; + struct st7789v { struct drm_panel panel; + const struct st7789_panel_info *info; struct spi_device *spi; struct gpio_desc *reset; struct regulator *power; @@ -160,16 +165,21 @@ static const struct drm_display_mode default_mode = { .vtotal = 320 + 8 + 4 + 4, }; +struct st7789_panel_info default_panel = { + .mode = &default_mode, +}; + static int st7789v_get_modes(struct drm_panel *panel, struct drm_connector *connector) { + struct st7789v *ctx = panel_to_st7789v(panel); struct drm_display_mode *mode; - mode = drm_mode_duplicate(connector->dev, &default_mode); + mode = drm_mode_duplicate(connector->dev, ctx->info->mode); if (!mode) { - dev_err(panel->dev, "failed to add mode %ux%ux@%u\n", - default_mode.hdisplay, default_mode.vdisplay, - drm_mode_vrefresh(&default_mode)); + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", + ctx->info->mode->hdisplay, ctx->info->mode->vdisplay, + drm_mode_vrefresh(ctx->info->mode)); return -ENOMEM; } @@ -359,6 +369,8 @@ static int st7789v_probe(struct spi_device *spi) spi_set_drvdata(spi, ctx); ctx->spi = spi; + ctx->info = device_get_match_data(&spi->dev); + drm_panel_init(&ctx->panel, dev, &st7789v_drm_funcs, DRM_MODE_CONNECTOR_DPI); @@ -389,13 +401,13 @@ static void st7789v_remove(struct spi_device *spi) } static const struct spi_device_id st7789v_spi_id[] = { - { "st7789v" }, + { "st7789v", (unsigned long) &default_panel }, { } }; MODULE_DEVICE_TABLE(spi, st7789v_spi_id); static const struct of_device_id st7789v_of_match[] = { - { .compatible = "sitronix,st7789v" }, + { .compatible = "sitronix,st7789v", .data = &default_panel }, { } }; MODULE_DEVICE_TABLE(of, st7789v_of_match);
Avoid hard-coding the default_mode and supply it from match data. One additional layer of abstraction has been introduced, which will be needed for specifying other panel information (e.g. bus flags) in the next steps. Signed-off-by: Sebastian Reichel <sre@kernel.org> --- .../gpu/drm/panel/panel-sitronix-st7789v.c | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-)