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@xxxxxx>");
> +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@xxxxxxxxxxxx>
Sam
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
