Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

> Den 20.02.2022 11.04, skrev Sam Ravnborg:
> > Hi Noralf,
> >
> >>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev,
struct drm_display_mode *mode)
> >>> +{
> >>> +	struct device *dev = dbidev->drm.dev;
> >>> +	u32 width_mm = 0, height_mm = 0;
> >>> +	struct display_timing timing;
> >>> +	struct videomode vm;
> >>> +	int ret;
> >>> +
> >>> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
dev->of_node, ret);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	videomode_from_timing(&timing, &vm);
> >>> +
> >>> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> >>> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> >>> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> >>> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> >>> +	    vm.flags) {
> >>> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> >>> +		return -EINVAL;
> >>> +	}
> >> We should have a helper that implements this. Maybe the display_timing
> >> => display_mode helper could do it.
> >
> > It would be nice with a drm_display_timing_to_mode() but that can come
> > later - the comment above should not be understood that I consider it
> > mandatory for this driver.
> >
>
> I did consider adding an of_get_drm_panel_mode() fashioned after
> of_get_drm_display_mode() but I didn't find any other driver that would
> actually be able to use it and I would have to do some substraction to
> get back the {h,v}front_porch values that I need and the optional pixel
> clock calculation becomes more complex acting from a drm_display_mode so
> I decided against it.
>
> Looking at it now, what I could do is add a function like what
> of_get_videomode() does for "display-timings":
>
> /**
>  * of_get_panel_videomode - get the panel-timing videomode from devicetree
>  * @np: devicenode containing the panel-timing subnode
>  * @vm: returns the videomode
>  *
>  * Returns:
>  * Zero on success, negative error code on failure.
>  **/
> int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
> {
> 	struct display_timing timing;
> 	int ret;
>
> 	ret = of_get_display_timing(np, "panel-timing", &timing);
> 	if (ret)
> 		return ret;
>
> 	videomode_from_timing(&timing, vm);
>
> 	return 0;
> }
>
> This could also be used by panel-lvds and 2 fbdev drivers, the other
> panel-timing users need/use the display_timing itself, some for bounds
> checking.

Scratch that, since videomode is to be avoided I tried adding a
drm_display_mode function and it didn't complicate matter as I though it
would so I'll do that instead:

static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
drm_display_mode *mode)
{
	struct device *dev = dbidev->drm.dev;
	u32 width_mm = 0, height_mm = 0;
	u16 hback_porch, vback_porch;
	struct videomode vm;
	int ret;

	ret = of_get_drm_panel_display_mode(dev->of_node, mode, NULL);
	if (ret) {
		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
dev->of_node, ret);
		return ret;
	}

	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;

	hback_porch = mode->htotal - mode->hsync_end;
	vback_porch = mode->vtotal - mode->vsync_end;

	if (mode->hsync_end > mode->hdisplay || (hback_porch + mode->hdisplay)
> 0xffff ||
	    mode->vsync_end > mode->vdisplay || (vback_porch + mode->vdisplay)
> 0xffff ||
	    mode->flags) {
		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
		return -EINVAL;
	}

	/* The driver doesn't use the pixel clock but it is mandatory so fake
one if not set */
	if (!mode->pixelclock)
		mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;

	dbidev->top_offset = vback_porch;
	dbidev->left_offset = hback_porch;

	return 0;
}


int of_get_drm_panel_display_mode(struct device_node *np,
				  struct drm_display_mode *dmode, u32 *bus_flags)
{
	u32 width_mm = 0, height_mm = 0;
	struct display_timing timing;
	struct videomode vm;
	int ret;

	ret = of_get_display_timing(np, "panel-timing", &timing);
	if (ret)
		return ret;

	videomode_from_timing(&timing, vm);

	memset(dmode, 0, sizeof(*dmode));
	drm_display_mode_from_videomode(&vm, dmode);
	if (bus_flags)
		drm_bus_flags_from_videomode(&vm, bus_flags);

	ret = of_property_read_u32(np, "width-mm", &width_mm);
	if (ret && ret != -EINVAL)
		return ret;

	ret = of_property_read_u32(np, "height-mm", &height_mm);
	if (ret && ret != -EINVAL)
		return ret;

	mode->width_mm = width_mm;
	mode->height_mm = height_mm;

	drm_mode_debug_printmodeline(dmode);

	return 0;
}
EXPORT_SYMBOL_GPL(of_get_drm_display_mode);

Noralf.