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

[Sam Ravnborg <sam@ravnborg.org>]

Hi Noralf,

> >> 	    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.
> > 
> 
> I still need to verify the values to ensure that front_porch and
> sync_len are zero. Maybe I need a comment now to tell what I'm checking
> since I'm further away from the DT values:
> 
> /*
>  * Make sure width and height are set and that only back porch and
>  * pixelclock are set in the other timing values. Also check that
>  * width and height don't exceed the 16-bit value specified by MIPI DCS.
>  */
Yes, that would be nice.
> 
> >>
> >> 	/* 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.
> > 
> 
> I did the same as of_get_drm_display_mode(), don't panel drivers need
> the bus flags?

In my haste I missed the display_timing combines flags for the bus and
the mode - so yes it is needed.


> 
> >> 	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.
> > 
> 
> I'll leave this to others to sort out. I want the function to look the
> same as of_get_drm_display_mode() and it uses videomode. If videomode
> goes away both can be fixed at the same time.

When I have dig myself out of the bridge hole I am in I may take a
look at this.

	Sam