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
