Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver

From: "Noralf Trønnes" <noralf@tronnes.org>
To: Sam Ravnborg <sam@ravnborg.org>, Jagan Teki <jagan@amarulasolutions.com>
Cc: David Airlie <airlied@linux.ie>,
	linux-amarula@amarulasolutions.com, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Michael Trimarchi <michael@amarulasolutions.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver
Date: Fri, 11 Jan 2019 22:41:17 +0100	[thread overview]
Message-ID: <27708d2d-23cf-747e-7e54-7ec894a25729@tronnes.org> (raw)
In-Reply-To: <20190111211931.GA29735@ravnborg.org>

Den 11.01.2019 22.19, skrev Sam Ravnborg:
> Hi Jagan.
> 
> Gave this another more detailed read - triggered some additional comments.
> Depite the comments it looks good, and this is all more or
> less cosmetic improvements.
> 
> 	Sam
> 
>> +struct st7701_panel_desc {
>> +	const struct drm_display_mode *mode;
>> +	unsigned int lanes;
>> +	unsigned long flags;
>> +	enum mipi_dsi_pixel_format format;
>> +	const char *const *supply_names;
>> +	unsigned int num_supplies;
>> +	unsigned int panel_sleep_delay;
>> +};
>> +
>> +struct st7701 {
>> +	struct drm_panel panel;
>> +	struct mipi_dsi_device *dsi;
>> +	const struct st7701_panel_desc *desc;
>> +
>> +	struct backlight_device *backlight;
>> +	struct regulator_bulk_data *supplies;
>> +	unsigned int num_supplies;
> I cannot see that num_supplies in this struct are used?
> 
> 
>> +	struct gpio_desc *reset;
>> +	unsigned int sleep_delay;
>> +};
>> +
>> +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
>> +{
>> +	return container_of(panel, struct st7701, panel);
>> +}
>> +
>> +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
>> +				   size_t len)
>> +{
>> +	return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
>> +}
>> +
> 
> 
>> +static int st7701_prepare(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +	int ret;
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +	msleep(20);
>> +
>> +	ret = regulator_bulk_enable(st7701->desc->num_supplies,
>> +				    st7701->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +	msleep(20);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +	msleep(20);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +	msleep(30);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +	msleep(150);
>> +
>> +	st7701_init_sequence(st7701);
>> +
>> +	return 0;
>> +}
>> +
> 
>> +static int st7701_unprepare(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +
>> +	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
>> +
>> +	msleep(st7701->sleep_delay);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
> No timing constrains here? In prepare there are sleeps intermixed.
> 
>> +
>> +	regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
>> +
>> +	return 0;
>> +}
>> +
>> +static int st7701_get_modes(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +	const struct drm_display_mode *desc_mode = st7701->desc->mode;
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(panel->drm, desc_mode);
>> +	if (!mode) {
>> +		dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
>> +			desc_mode->hdisplay, desc_mode->vdisplay,
>> +			desc_mode->vrefresh);
> Use something like:
> 		DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
> 			      DRM_MODE_ARG(desc_mode));
> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	drm_mode_set_name(mode);
>> +	drm_mode_probed_add(panel->connector, mode);
>> +
>> +	panel->connector->display_info.width_mm = desc_mode->width_mm;
>> +	panel->connector->display_info.height_mm = desc_mode->height_mm;
>> +
>> +	return 1;
>> +}
>> +
> 
>> +static const struct st7701_panel_desc ts8550b_desc = {
>> +	.mode = &ts8550b_mode,
>> +	.lanes = 2,
>> +	.flags = MIPI_DSI_MODE_VIDEO,
>> +	.format = MIPI_DSI_FMT_RGB888,
>> +	.supply_names = ts8550b_supply_names,
>> +	.num_supplies = ARRAY_SIZE(ts8550b_supply_names),
>> +	.panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> In the only place this is used there is added 120 ms too.
> Looks inconsistent - do we have the same delay twice?
> 
> 
>> +
>> +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	const struct st7701_panel_desc *desc;
>> +	struct device_node *np;
>> +	struct st7701 *st7701;
>> +	int ret, i;
>> +
>> +	st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
>> +	if (!st7701)
>> +		return -ENOMEM;
>> +
>> +	desc = of_device_get_match_data(&dsi->dev);
>> +	dsi->mode_flags = desc->flags;
>> +	dsi->format = desc->format;
>> +	dsi->lanes = desc->lanes;
>> +
>> +	st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
>> +					sizeof(*st7701->supplies),
>> +					GFP_KERNEL);
>> +	if (!st7701->supplies)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < desc->num_supplies; i++)
>> +		st7701->supplies[i].supply = desc->supply_names[i];
>> +
>> +	ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
>> +				      st7701->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st7701->reset)) {
>> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
>> +		return PTR_ERR(st7701->reset);
>> +	}
>> +
>> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
>> +	if (np) {
>> +		st7701->backlight = of_find_backlight_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!st7701->backlight)
>> +			return -EPROBE_DEFER;
>> +	}
> This is a simpler variant of the above:
> 	st7701->backlight = devm_of_find_backlight(dsi->dev);
> 	if (IS_ERR(st7701->backlight))
> 		return PTR_ERR(st7701->backlight);
> 
> Suggested by Noralf in another thread for a similar driver.
> 
> 
>> +
>> +	drm_panel_init(&st7701->panel);
>> +
>> +	/* We need to wait 120ms after a sleep out command */
>> +	st7701->sleep_delay = 120 + desc->panel_sleep_delay;
> This is the other place where we add 120 to the previous 80 - in total 200 ms.
> 
>> +	st7701->panel.funcs = &st7701_funcs;
>> +	st7701->panel.dev = &dsi->dev;
>> +
>> +	ret = drm_panel_add(&st7701->panel);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mipi_dsi_set_drvdata(dsi, st7701);
>> +	st7701->dsi = dsi;
> 
>> +	st7701->desc = desc;
> This assignment could come earlier, so we do not have it unassigned when calling
> other functions. As it is now it works so you can safely ignore this comment.
> 
>> +
>> +	return mipi_dsi_attach(dsi);
>> +}
>> +
>> +static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +	drm_panel_remove(&st7701->panel);
>> +
>> +	if (st7701->backlight)
>> +		put_device(&st7701->backlight->dev);
> 
> Use:
> 	backlight_put(st7701->backlight);

This happens automatically on driver detach if devm_of_find_backlight()
is used

Noralf.