Re: [PATCH v4 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Amit Barzilai]

On Tue, Aug 18, 2026 at 11:55 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> > +static int ssd135x_init(struct ssd130x_device *ssd130x)
> > +{
> > +     u8 remap = SSD135X_SET_REMAP_65K | SSD135X_SET_REMAP_COM_SPLIT |
> > +                SSD135X_SET_REMAP_COLOR_BGR | SSD135X_SET_REMAP_COM_SCAN;
>
> Same comment about const.

Will do, same as in 2/6.

> > +     int ret;
>
> Why not placing it after cmds?

No particular reason. Moved below cmds[] in v5.

> > +static void ssd135x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
> > +{
> > +     const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
> > +     unsigned int pitch;
>
> > +     if (!fi)
> > +             return;
>
> It's less maintainable than
>
>         const struct drm_format_info *fi;
>         unsigned int pitch;
>
>         fi = drm_format_info(DRM_FORMAT_RGB565);
>         if (!fi)
>                 return;
> ...
> > +static int ssd135x_fb_blit_rect(struct drm_framebuffer *fb,
> > +                             const struct iosys_map *vmap,
> > +                             struct drm_rect *rect, u8 *data_array,
> > +                             struct drm_format_conv_state *fmtcnv_state)
> > +{
> > +     struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> > +     const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
> > +     unsigned int dst_pitch;
> > +     struct iosys_map dst;
> > +
> > +     if (!fi)
> > +             return -EINVAL;
>
> Ditto. The problem is that the current style is tempting for subtle mistakes
> such as defining more variables that may use fi in between.

Noted, I'll move the fi assignment just before the guard in both functions.

> > +static void ssd135x_primary_plane_atomic_disable(struct drm_plane *plane,
> > +                                              struct drm_atomic_commit *state)
> > +{
> > +     struct drm_device *drm = plane->dev;
> > +     struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> > +     struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> > +     struct drm_crtc_state *crtc_state;
> > +     struct ssd130x_crtc_state *ssd130x_crtc_state;
> > +     int idx;
> > +
> > +     if (!plane_state->crtc)
> > +             return;
>
> In the similar way here.

I'll move plane_state to just before this guard as well.

-- 
Thanks,
Amit