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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 19/60] drm/panel: Add driver for the LG Philips LB035Q02 panel
Date: Tue, 9 Jul 2019 03:56:49 +0300	[thread overview]
Message-ID: <20190709005649.GC4819@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20190708185129.GA4363@ravnborg.org>

Hi Sam,

On Mon, Jul 08, 2019 at 08:51:29PM +0200, Sam Ravnborg wrote:
> Hi Laurent.
> 
> Good to move omapdrm to a more standard way to do things.

I hope it will help defining the next step for the standard ;-)

> > 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?

The omapdrm-specific driver is called panel-lgphilips-lb035q02.c. I'll
update the comment.

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

Are they ? I've always advocated for unsigned indexes to use unsigned
int.

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

I didn't know drm_display_mode had width_mm and height_mm fields. I'll
fix this.

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

Will do.

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

I don't mind either way so I'll change it.

> > +};
> > +
> > +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.

According to that table, "GPL v2" is defined as "Same as “GPL”. It
exists for historic reasons.". My understanding is that "GPL v2" exists
for historical reasons and should not be used in new code.

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

Thank you.

-- 
Regards,

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