Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a9ec6c19-0f1d-25eb-2841-352b24ff36e2@wolfvision.net>
Date:   Fri, 5 May 2023 14:33:38 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2 08/13] drm/panel: sitronix-st7789v: avoid hardcoding
 mode info
Content-Language: en-US
To:     Sebastian Reichel <sre@kernel.org>
Cc:     Gerald Loacker <gerald.loacker@wolfvision.net>,
        Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org
References: <20230422205012.2464933-1-sre@kernel.org>
 <20230422205012.2464933-9-sre@kernel.org>
From:   Michael Riesch <michael.riesch@wolfvision.net>
In-Reply-To: <20230422205012.2464933-9-sre@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NXBPekE3NDQyTEZIcnlsUHZxdUtkcExYQzVpU2VFRlNWSm9scklIaFh5V1gw?=
 =?utf-8?B?UmtZNjZ0cVFFNTBpWmpCVXVyV1ZDOG5mVnBHN0Z2c0JxTzBWWjZZZWVxU0Rm?=
 =?utf-8?B?dy9YVXJyM0NGRXFVOUpSQUZ4dVBYR3ZrS2V4NDRYT3RwTlBZZlpMT01rWHZw?=
 =?utf-8?B?T2JocUQ1U1hVUFQrM3FSUU81UTJyYitxZnkwRkhwbDFKWEFRVTFCNFVhbTll?=
 =?utf-8?B?YWg3UDJxbkNPZUE3KzRsM2pVbGhXUk9VRjFKR2UraFVKWjhyS2pabDRiaGcy?=
 =?utf-8?B?cFhQQkJuM082YWdyNStmMC9IV0UvYWV3WkZSdE1qc3ZtTXViZDJrQnIzVE00?=
 =?utf-8?B?OFpvUWl0aTNjL3o3OFNaUTdPZERGemVGTGxNMlV3emNxbUM1b29YOGcyZ0NR?=
 =?utf-8?B?M3l6WTlTOEZLTnlNakxjSFlPMEhkYlRjdEF4YTlweUw4TTM4OU9Qa3RUenpM?=
 =?utf-8?B?SXZJZWJybFY2cU04d0loVzQ0MTBZeVY2VWZObzB3YlFQNjhHeHltWjUrcFF0?=
 =?utf-8?B?c21TR1JwUDB0aFgxUENWczhlcjUzM2s1by82bW4xQ1RCOHNQSFduTzBUa1h4?=
 =?utf-8?B?QUxOazE4djNxOFV6VURJQ05RTzZ4MUNvNGJycXZLcFJVVDJlcVpwODhSTmhs?=
 =?utf-8?B?Q05Ob2RCRHFOelhrcVZOa21wQklTeHpLc1I1aU5PZ05PQVppdVBYcHp3OEVB?=
 =?utf-8?B?MEF1Nkk3RTNnUmZINHVXR1p1RVRTRzNjbTFvMG9oWUdEMGd4aWUzT2ltWXBX?=
 =?utf-8?B?TWxIbGtIRzZXM0FOamk4dUVZVkljRUxrSkVlY3hZSVdOU0hBZHQ5cGlFS004?=
 =?utf-8?B?UTZ6RkJpSnIxVHlGcXJFbW42SmhPMUxyekwxODZhai9lcWxRQWIvTDdFZWxW?=
 =?utf-8?B?M0dkdVdQRlJ1M2wxZjVhMzlrRHVYc2U4bjROY1ErZTE1blV5YTNlUWUySTVF?=
 =?utf-8?B?dS95eUZBQWw2SW8zSE5jUjNZYVc4SkQwSVNEUWNPSVZtdi8xdUdvMFVQN2VL?=
 =?utf-8?B?YnFGUXFwek1ncGdnRHFjajRtYkpqN01WOHZSSmhpcnoreFY1dXBYSzFRTVln?=
 =?utf-8?B?algwd3pQclZjQTlpa3M5SjJ3czNna0w2R3VSVW5NczhheUl1VlJRRHRmSUJN?=
 =?utf-8?B?WDM4TTgxOGd3OGtuYU9aWVNNVWV4bVQzU0VYS0tuRWNlWC9SbURhUDEzRXR5?=
 =?utf-8?B?ZC9HMDdLSjdCUmJKQ3JNeUF6NzJaUHkxR2VhaDlDdEdJWlNydU1WdHIzSW5y?=
 =?utf-8?B?cGRZWGhNSDJxMzRoa3k0aFg5d1FLdlVOT1NHTVkrSm5LQzh2RXhNUm9Ta3VE?=
 =?utf-8?B?VlAycnNnbnZzNG10S05HZTNHWFpHUEVQUWZUMEkyRjd1cHJkSC9wdEZTb3BM?=
 =?utf-8?B?ek90T0IwSnppaE44bU1KTzZEOHZTRENDdGZ1aVJqVTJ0R2tEcWMxZk1kT2pj?=
 =?utf-8?B?QnY4V2ZWY21BV1ExL1poNHJuZDA3d1VSZFVId1FPM3IxV1dRWG5tS0Yyd0NB?=
 =?utf-8?B?YVZmN0xtUE4wRjJoVHhaL2svRk1reUpzUjFyTytIdStBRkp6ZS9uZXhqNFcy?=
 =?utf-8?B?ZUUwVHp3ejBOcm1SdzM4ZWJmUlQwTDc1M2doajAyN0hZYUltSVBaQks1R1I5?=
 =?utf-8?B?WVROelZucTJsZENZY1BsWUg5d0ttSllta0RWazVkbDhDd2F6YmZKYndBMWN3?=
 =?utf-8?B?NHUvbGFsNEM4bkZ5N3QrM2VXQlIvUGlTU2RPUG5PRlpiQlJFNmZ3ekFPcDIr?=
 =?utf-8?B?anhWY1R5MTIwcWZsMWd0RVoxeGxPQ0xXcUlWcFBGd1VMMUxmMTBTOTFGQUpC?=
 =?utf-8?B?QnlTWXBIWFJwN3c0clB3VkUrMmFPWTdOVERYRm92ai9qM2twSkFabi9qMnBF?=
 =?utf-8?B?UGxIcmRMSXNKcUo5NUFiNTVEUUhJNFU4MHJ3bjlQNVRmTC9NV1VDL05NS0tK?=
 =?utf-8?B?YVBnYS9iOGk5L3dQUXRKUU90RExzYUJ2RmJSZDJITGNET2xiNUtaL0wxdy91?=
 =?utf-8?B?T1MvSDQrZVpHR1BpcUxUOVQ0U1AyQ2tMZnpSelMxZHB1QmZqOU5YbzhJQklm?=
 =?utf-8?B?MHcxV3E4a09Bd0hLNzh1dmw1VXBzVS9zVFNPQ2FJUHpqZXlvWnFBQTZTcXEy?=
 =?utf-8?B?UjdMeExEOUlVamRFUjJOMEwwbWJOQkRzOEJieVlCSnR3YnhtWUVwcnF3N0lT?=
 =?utf-8?B?M2hBRTB1cjg3dXUzN09IT3dITXVuZ0Zvak5JWkZlSDNaV0QvdXZJazE1dklN?=
 =?utf-8?B?b2E5MWdaZDlSYStqUHVzSzV0S05nPT0=?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 32ed4fe3-9bdd-4a47-c583-08db4d64f4d8
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9155.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2023 12:33:40.4386
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: e94ec9da-9183-471e-83b3-51baa8eb804f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z+RWGqu/FaAgatHusc4KBEtqDN23lOFUskYbnS0ls9bCBd9VgqfUrCpBviJj5USeaj7HzIkkaE+u9keF8gjx0pAAqI+0j/1Ymxp4Eb4usvc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAWPR08MB9568
X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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);