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

["Noralf Trønnes" <noralf@tronnes.org>]

Den 20.02.2022 20.57, skrev 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.
> 

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.
 */

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

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

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

I don't think I'll do these either, it's more work than I can invest in
this.

Noralf.

> 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