Re: [PATCH 3/4] video: add MIPI DBI framebuffer helpers

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Ahmad,

thank you for the review.

On Thu, Mar 30, 2023 at 10:43:54AM +0200, Ahmad Fatoum wrote:
> On 29.03.23 12:56, Philipp Zabel wrote:
[...]
> > +static void mipi_dbi_buf_copy(void *dst, struct fb_info *info, bool swap)
> > +{
> > +	u16 *src = (u16 *)info->screen_base;
> > +	u16 *dst16 = dst;
> > +	size_t len = info->xres * info->yres;
> > +	int i;
> > +
> > +	if (swap) {
> > +		for (i = 0; i < len; i++) {
> > +			*dst16++ = *src << 8 | *src >> 8;
> > +			src++;
> > +		}
> > +	} else {
> > +		memcpy(dst, src, len * 2);
> 
> Do we need this? Why can't we send out framebuffer directly if there is no
> swapping to be done?

We do not. If byte swapping is not necessary, we can transfer directly
from the framebuffer, as long as this driver only supports 16-bit
RGB565. I'll improve this in v2.

[...]
> > +/**
> > + * mipi_dbi_enable_flush - MIPI DBI enable helper
> > + * @dbidev: MIPI DBI device structure
> > + * @crtc_state: CRTC state
> > + * @plane_state: Plane state
> > + *
> > + * Flushes the whole framebuffer and enables the backlight. Drivers can use this
> > + * in their &drm_simple_display_pipe_funcs->enable callback.
> 
> fb_ops->fb_enable ?

Yes. The whole comment slipped through unchanged from the Linux driver.
Will fix.

[...]
> > +void mipi_dbi_fb_disable(struct fb_info *info)
> > +{
> > +	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
> > +
> > +	if (dbidev->backlight)
> > +		backlight_set_brightness(dbidev->backlight, 0);
> > +	else
> > +		mipi_dbi_blank(dbidev);
> > +
> > +	if (dbidev->regulator)
> > +		regulator_disable(dbidev->regulator);
> > +	if (dbidev->io_regulator)
> > +		regulator_disable(dbidev->io_regulator);
> 
> Calling regulator_disable on NULL pointer is a no-op.

Ah right, this is different from Linux.
I'll remove the unnecessary checks.

> > +int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev, struct fb_ops *ops,
> > +		      struct fb_videomode *mode)
> > +{
> > +	struct fb_info *info = &dbidev->info;
> > +
> > +	info->mode = mode;
> > +	info->fbops = ops;
> > +	info->dev.parent = dbidev->dev;
> > +
> > +	info->xres = mode->xres;
> > +	info->yres = mode->yres;
> > +	info->bits_per_pixel = 16;
> > +	info->line_length = info->xres * (info->bits_per_pixel >> 3);
> > +
> > +	info->screen_size = info->line_length * info->yres;
> > +	info->screen_base = kzalloc(info->screen_size, GFP_KERNEL);
> 
> dma_alloc? In case some SPI driver gets DMA support.

Ok.

> > +
> > +	info->red.length = 5;
> > +	info->red.offset = 11;
> > +	info->green.length = 6;
> > +	info->green.offset = 5;
> > +	info->blue.length = 5;
> > +	info->blue.offset = 0;
> > +
> > +	dbidev->tx_buf = kzalloc(mode->xres * mode->yres * 2, GFP_KERNEL);
> 
> Use info->screen_size here as well?

Yes. This driver doesn't support 32-bit framebuffers with conversion
to 16-bit transfers, so this can use info->screen_size as well.

regards
Philipp