Hi Sam,
On Mon, Jul 08, 2019 at 09:10:08PM +0200, Sam Ravnborg wrote:
Hi Laurent.
Thanks for keeping me busy :-)
My pleasure ;-)
On Sun, Jul 07, 2019 at 09:18:57PM +0300, Laurent Pinchart wrote:
This panel is used on the Zoom2/3/3630 SDP boards.
This information may be good to have in the Kconfig help entry too.
Maybe tell in the changelog where this code originates from.
Good point, I'll do so.
--- /dev/null +++ b/drivers/gpu/drm/panel/panel-nec-nl8048hl11.c @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- NEC NL8048HL11 Panel Driver
- Copyright (C) 2019 Texas Instruments Incorporated
- Based on the omapdrm-specific panel-nec-nl8048hl11 driver
- Copyright (C) 2010 Texas Instruments Incorporated
- Author: Erik Gilling konkers@android.com
- */
No added copyright from you? (Apply to all panel drivers)
The copyright goes to TI for this specific work.
+#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/pm.h> +#include <linux/spi/spi.h>
+#include <drm/drm_connector.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h>
Good to see panel drivers that are NOT using drmP.h :-)
+struct nl8048_device {
- struct drm_panel panel;
- struct spi_device *spi;
- struct gpio_desc *reset_gpio;
+};
Naming bikeshedding. This is a nl8048_panel, not a device.
It's a device too, but I'll change this.
+static const struct drm_display_mode nl8048_mode = {
- /* NEC PIX Clock Ratings MIN:21.8MHz TYP:23.8MHz MAX:25.7MHz */
- .clock = 23800,
- .hdisplay = 800,
- .hsync_start = 800 + 6,
- .hsync_end = 800 + 6 + 1,
- .htotal = 800 + 6 + 1 + 4,
- .vdisplay = 480,
- .vsync_start = 480 + 3,
- .vsync_end = 480 + 3 + 1,
- .vtotal = 480 + 3 + 1 + 4,
- .vrefresh = 60,
- .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
- .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
Same comment as for previous patch on height/width.
Fixed.
+static int nl8048_get_modes(struct drm_panel *panel) +{
- struct drm_connector *connector = panel->connector;
- struct drm_display_mode *mode;
- mode = drm_mode_duplicate(panel->drm, &nl8048_mode);
- if (!mode)
return -ENOMEM;- drm_mode_set_name(mode);
- drm_mode_probed_add(connector, mode);
- connector->display_info.width_mm = 89;
- connector->display_info.height_mm = 53;
- connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH
| DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE| DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- return 1;
+}
This should be moved to drm_panle_helper.c (which we do not have yet. But the function is used in some many drivers it makes sense. On my TODO list. I have yet to find a good way to specify the bus_flags though.
It's a bit annoying to have different set of flags for the various structures that describe modes and display information. Cleaning all that up would be great, but will be a significant effort.
+#ifdef CONFIG_PM_SLEEP
Use __maybe_unused, and loose the #ifdef
OK.
And why does this panel need suspend/resume? The panel is supposed to be turned off in disable()
To simple solution would be to move the write to "2" to the enable and disable functions. And then this driver is alinged with the rest.
+static int nl8048_suspend(struct device *dev) +{
- struct nl8048_device *lcd = dev_get_drvdata(dev);
- nl8048_write(lcd, 2, 0x01);
- msleep(40);
This sleep puzzle me. What is the puspose? And the write is to a display that is already reset??
This has been copied from the omapdrm-specific driver, and I have no hardware to test this, so I didn't dare changing the code. I would prefer reworking the suspend/resume on top of this patch, ideally developed by someone who can test the changes.
- return 0;
+}
+static int nl8048_resume(struct device *dev) +{
- struct nl8048_device *lcd = dev_get_drvdata(dev);
- /* Reinitialize the panel. */
- spi_setup(lcd->spi);
- nl8048_write(lcd, 2, 0x00);
- nl8048_init(lcd);
- return 0;
+}
+static SIMPLE_DEV_PM_OPS(nl8048_pm_ops, nl8048_suspend, nl8048_resume); +#endif
+static int nl8048_probe(struct spi_device *spi) +{
- struct nl8048_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->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(lcd->reset_gpio)) {
dev_err(&spi->dev, "failed to parse reset gpio\n");return PTR_ERR(lcd->reset_gpio);- }
- spi->mode = SPI_MODE_0;
- spi->bits_per_word = 32;
- ret = spi_setup(spi);
- if (ret < 0) {
dev_err(&spi->dev, "failed to setup SPI: %d\n", ret);return ret;- }
- ret = nl8048_init(lcd);
- if (ret < 0)
return ret;- drm_panel_init(&lcd->panel);
- lcd->panel.dev = &lcd->spi->dev;
- lcd->panel.funcs = &nl8048_funcs;
- return drm_panel_add(&lcd->panel);
+}
+static int nl8048_remove(struct spi_device *spi) +{
- struct nl8048_device *lcd = spi_get_drvdata(spi);
- drm_panel_remove(&lcd->panel);
- nl8048_disable(&lcd->panel);
Use drm_panel_disable() - same comment as other panel driver.
Sure.
- return 0;
+}
+static const struct of_device_id nl8048_of_match[] = {
- { .compatible = "nec,nl8048hl11", },
- {},
{ /* sentinel */ },?
Done.
+};
+MODULE_DEVICE_TABLE(of, nl8048_of_match);
+static struct spi_driver nl8048_driver = {
- .probe = nl8048_probe,
- .remove = nl8048_remove,
- .driver = {
.name = "panel-nec-nl8048hl11",+#ifdef CONFIG_PM_SLEEP
.pm = &nl8048_pm_ops,+#endif
.of_match_table = nl8048_of_match,- },
+};
+module_spi_driver(nl8048_driver);
+MODULE_ALIAS("spi:nec,nl8048hl11"); +MODULE_AUTHOR("Erik Gilling konkers@android.com"); +MODULE_DESCRIPTION("NEC-NL8048HL11 Driver"); +MODULE_LICENSE("GPL");
"GPL v2"?
The suspend/resume thing needs to be sorted out before I can add my r-b on this.