[PATCH 20/60] drm/panel: Add driver for the NEC NL8048HL11 panel

[Laurent Pinchart]

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 at 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 at 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.

-- 
Regards,

Laurent Pinchart