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@xxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxx>
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);
