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