Re: [PATCH 19/60] drm/panel: Add driver for the LG Philips LB035Q02 panel

[Sam Ravnborg <sam@ravnborg.org>]

Hi Laurent.

Good to move omapdrm to a more standard way to do things.

> new file mode 100644
> index 000000000000..d8a8c3a3a8c5
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg-lb035q02.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LG.Philips LB035Q02 LCD Panel Driver

Looks like a typo. As far as I know LG and Philips are not the same.
But I can see this is used in several places, so I need to check up on
actual status here and driver is likely OK.
Google... this is fine. Some joint venture in 2001.

> + * Based on the omapdrm-specific panel-lg-lb035q02 driver
Will we have two drivers with the same name, or are this above already
disabled from the build?

> +	unsigned int i;
index to arrays are default "int" IIRC.
Not that it matters but noticed it.

> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(init_data); ++i) {
> +		ret = lb035q02_write(lcd, init_data[i].index,
> +				     init_data[i].value);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode lb035q02_mode = {
> +	.clock = 6500,
> +	.hdisplay = 320,
> +	.hsync_start = 320 + 20,
> +	.hsync_end = 320 + 20 + 2,
> +	.htotal = 320 + 20 + 2 + 68,
> +	.vdisplay = 240,
> +	.vsync_start = 240 + 4,
> +	.vsync_end = 240 + 4 + 2,
> +	.vtotal = 240 + 4 + 2 + 18,
> +	.vrefresh = 60,
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
We already specify all the timing details.
Consider to use display_mode to specify the width/height too.
So the panel specificatiosn are hardcoded only in one place.

> +
> +static int lb035q02_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &lb035q02_mode);
> +	if (!mode)
> +		return -ENOMEM;
> +
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(connector, mode);
> +
> +	connector->display_info.width_mm = 70;
> +	connector->display_info.height_mm = 53;
So we avoid hardcoding height/width here, but do it with the timing
above.
> +	/*
> +	 * FIXME: According to the datasheet pixel data is sampled on the
> +	 * rising edge of the clock, but the code running on the Gumstix Overo
> +	 * Palo35 indicates sampling on the negative edge. This should be
> +	 * tested on a real device.
> +	 */
> +	connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH
> +					  | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE
> +					  | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs lb035q02_funcs = {
> +	.disable = lb035q02_disable,
> +	.enable = lb035q02_enable,
> +	.get_modes = lb035q02_get_modes,
> +};
> +
> +static int lb035q02_probe(struct spi_device *spi)
> +{
> +	struct lb035q02_device *lcd;
> +	int ret;
> +
> +	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
> +	if (lcd == NULL)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, lcd);
> +	lcd->spi = spi;
> +
> +	lcd->enable_gpio = devm_gpiod_get(&spi->dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(lcd->enable_gpio)) {
> +		dev_err(&spi->dev, "failed to parse enable gpio\n");
> +		return PTR_ERR(lcd->enable_gpio);
> +	}
> +
> +	ret = lb035q02_init(lcd);
> +	if (ret < 0)
> +		return ret;
> +
> +	drm_panel_init(&lcd->panel);
> +	lcd->panel.dev = &lcd->spi->dev;
> +	lcd->panel.funcs = &lb035q02_funcs;
> +
> +	return drm_panel_add(&lcd->panel);
> +}
> +
> +static int lb035q02_remove(struct spi_device *spi)
> +{
> +	struct lb035q02_device *lcd = spi_get_drvdata(spi);
> +
> +	drm_panel_remove(&lcd->panel);
> +	lb035q02_disable(&lcd->panel);
Use drm_panel_disable() so the driver will benefit when we move more
functionality to the drm_panel_disable() function.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lb035q02_of_match[] = {
> +	{ .compatible = "lgphilips,lb035q02", },
> +	{},
Some drivers use { /* sentinel */ }, to document this is on purpose the
last entry.

> +};
> +
> +MODULE_DEVICE_TABLE(of, lb035q02_of_match);
> +
> +static struct spi_driver lb035q02_driver = {
> +	.probe		= lb035q02_probe,
> +	.remove		= lb035q02_remove,
> +	.driver		= {
> +		.name	= "panel-lg-lb035q02",
> +		.of_match_table = lb035q02_of_match,
> +	},
> +};
> +
> +module_spi_driver(lb035q02_driver);
> +
> +MODULE_ALIAS("spi:lgphilips,lb035q02");
> +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
> +MODULE_DESCRIPTION("LG.Philips LB035Q02 LCD Panel driver");
> +MODULE_LICENSE("GPL");
This should be "GPL v2" if I read https://www.kernel.org/doc/html/latest/process/license-rules.html
correct. See "MODULE_LICENSE" table.

With the above comments addressed/considered:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel