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

[Sam Ravnborg]

Hi Noralf.

On Sun, Feb 20, 2022 at 07:11:14PM +0100, Noralf Trønnes wrote:
> > 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:

I like, but would like to get rid of video_mode entirely.

> 
> 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;
The accesss functions I posed below could be used here - so we avoid
typing these (trivial) calculations in many places.

> 
> 	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;
> 	}
With the display_timing => drm_display_mode I think the above is no
longer required.

> 
> 	/* 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)
> {
Not sure about the bus_flags argument here - seems misplaced.

> 	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);

This does a:
display_timing => video_mode => drm_display_display_mode

We could do a:
display_timing => drm_display_mode.

Sample implementation could look like this:
void drm_mode_from_display_timing(struct drm_display_mode *mode,
                                  const struct display_timing *dt)
{
	mode->hdisplay = dt->hactive.typ;
        mode->hsync_start = dt->hactive.typ + dt->hfront_porch.typ;
        mode->hsync_end = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ;
        mode->htotal = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ + dt->hback_porch.typ;

        mode->vdisplay = dt->vactive.typ;
        mode->vsync_start = dt->vactive.typ + dt->vfront_porch.typ;
        mode->vsync_end = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ;
        mode->vtotal = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ + dt->vback_porch.typ;

        mode->clock = dt->pixelclock.typ / 1000;

        mode->flags = 0;
        if (dt->flags & DISPLAY_FLAGS_HSYNC_HIGH)
                mode->flags |= DRM_MODE_FLAG_PHSYNC;
        else if (dt->flags & DISPLAY_FLAGS_HSYNC_LOW)
                mode->flags |= DRM_MODE_FLAG_NHSYNC;
        if (dt->flags & DISPLAY_FLAGS_VSYNC_HIGH)
                mode->flags |= DRM_MODE_FLAG_PVSYNC;
        else if (dt->flags & DISPLAY_FLAGS_VSYNC_LOW)
                mode->flags |= DRM_MODE_FLAG_NVSYNC;
        if (dt->flags & DISPLAY_FLAGS_INTERLACED)
                mode->flags |= DRM_MODE_FLAG_INTERLACE;
        if (dt->flags & DISPLAY_FLAGS_DOUBLESCAN)
                mode->flags |= DRM_MODE_FLAG_DBLSCAN;
        if (dt->flags & DISPLAY_FLAGS_DOUBLECLK)
                mode->flags |= DRM_MODE_FLAG_DBLCLK;
        drm_mode_set_name(mode);
}
EXPORT_SYMBOL_GPL(drm_mode_from_display_timing);

If we then on top of this would like easy access to the names we know we
could add a few access functions:

static inline u32 drm_mode_hactive(const struct drm_display_mode *mode)
{
        mode->hdisplay;
}

static inline u32 drm_mode_hfront_porch(const struct drm_display_mode *mode)
{
        mode->hsync_start - mode->hdisplay;
}

static inline u32 drm_mode_hback_porch(const struct drm_display_mode *mode)
{
        mode->htotal - mode->hsync_start;
}

static inline u32 drm_mode_hsync_len(const struct drm_display_mode *mode)
{
        return mode->hsync_end - mode->hsync_start;
}

static inline u32 drm_mode_vactive(const struct drm_display_mode *mode)
{
        return mode->vdisplay;
}

static inline u32 drm_mode_vfront_porch(const struct drm_display_mode *mode)
{
        return mode->vsync_start - mode->vdisplay;
}

static inline u32 drm_mode_vsync_len(const struct drm_display_mode *mode)
{
        return mode->vsync_end - mode->vsync_start;
}

static inline u32 drm_mode_vback_porch(const struct drm_display_mode *mode)
{
        return mode->vtotal - mode->vsync_end;
}


The above was something I just quickly typed. This was the easy part.
Writing kernel-.doc and fix it so it works is the time consuming part..

	Sam