Re: [PATCH 2/2] drm/panel: Add driver for Novatek NT37703 based panels

[Esteban Urrutia <esteuwu@proton.me>]

On 8/12/26 7:55 AM, Neil Armstrong wrote:
>> +static int nt37703_get_modes(struct drm_panel *panel, struct drm_connector *connector)
>> +{
>> +	struct panel_info *pinfo = to_panel_info(panel);
>> +	int i;
>> +
>> +	for (i = 0; i < pinfo->desc->num_modes; i++) {
>> +		const struct drm_display_mode *m = &pinfo->desc->modes[i];
>> +		struct drm_display_mode *mode;
>> +
>> +		mode = drm_mode_duplicate(connector->dev, m);
>> +		if (!mode) {
>> +			dev_err(panel->dev, "Failed to add mode %ux%u@%u\n", m->hdisplay,
>> +				m->vdisplay, drm_mode_vrefresh(m));
>> +			return -ENOMEM;
>> +		}
>> +
>> +		mode->type = DRM_MODE_TYPE_DRIVER;
>> +		if (i == 0)
>> +			mode->type |= DRM_MODE_TYPE_PREFERRED;
>> +
>> +		drm_mode_set_name(mode);
>> +		drm_mode_probed_add(connector, mode);
>> +	}
>> +
>> +	connector->display_info.width_mm = pinfo->desc->width_mm;
>> +	connector->display_info.height_mm = pinfo->desc->height_mm;
>> +
>> +	return pinfo->desc->num_modes;
> 
> The only supported panel only supports a single mode, just support
> a single mode for now and drop this open coded support for multiple modes.
> 
> You can add it later if somehow you need multiple modes.


The idea is that for each new panel, instead of modifying the driver,
only these structures are added:
- (panel)_modes[]
- (panel)_init_sequence()
- struct drm_dsc_config (panel)_dsc
- (panel)_desc
And modifying the of_device_id table afterwards.

So dropping only the modes would mean getting rid of all these
altogether, otherwise the only supported mode would be hardcoded which
renders these structures useless.

[...]

>> +static int nt37703_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct panel_info *pinfo;
>> +	int ret;
>> +
>> +	pinfo = devm_drm_panel_alloc(dev, struct panel_info, panel, &nt37703_panel_funcs,
>> +				     DRM_MODE_CONNECTOR_DSI);
>> +	if (IS_ERR(pinfo))
>> +		return PTR_ERR(pinfo);
>> +
>> +	pinfo->desc = of_device_get_match_data(dev);
>> +	if (IS_ERR(pinfo->desc))
>> +		return PTR_ERR(pinfo->desc);
>> +
>> +	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(nt37703_supplies), nt37703_supplies,
>> +					    &pinfo->supplies);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
>> +
>> +	pinfo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pinfo->reset_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(pinfo->reset_gpio),
>> +				     "Failed to get reset-gpios\n");
>> +
>> +	pinfo->dsi = dsi;
>> +	mipi_dsi_set_drvdata(dsi, pinfo);
>> +
>> +	dsi->lanes = pinfo->desc->lanes;
>> +	dsi->format = pinfo->desc->format;
>> +	dsi->mode_flags = pinfo->desc->mode_flags;
>> +
>> +	pinfo->panel.prepare_prev_first = true;
>> +
>> +	pinfo->panel.backlight = nt37703_create_backlight(dsi);
>> +	if (IS_ERR(pinfo->panel.backlight))
>> +		return dev_err_probe(dev, PTR_ERR(pinfo->panel.backlight),
>> +				     "Failed to create backlight\n");
>> +
>> +	drm_panel_add(&pinfo->panel);
> 
> Use devm_ variant


Ack

>> +
>> +	/* This panel only supports DSC; unconditionally enable it */
>> +	pinfo->dsc = *(pinfo->desc->dsc);
>> +	dsi->dsc = &pinfo->dsc;
>> +
>> +	ret = mipi_dsi_attach(dsi);
> 
> Use devm_ variant

Ack


>> +	if (ret < 0) {
>> +		drm_panel_remove(&pinfo->panel);
>> +		return dev_err_probe(dev, ret, "Failed to attach to DSI host\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void nt37703_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
>> +	int ret;
>> +
>> +	ret = mipi_dsi_detach(dsi);
>> +	if (ret < 0)
>> +		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
>> +
>> +	drm_panel_remove(&pinfo->panel);
>> +}
> 
> And drop the remove()


Ack

Thanks for reviewing.
Regards,
Esteban