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

[Amit Barzilai]

On Thu, Sep 10, 2026 at 3:00 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> >  static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
> >                             size_t len)
> > @@ -271,6 +310,17 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
> >       unsigned int i;
> >       int ret;
> >
> > +     if (ssd130x->device_info->cmd_params_are_data) {
> > +             if (!len)
> > +                     return 0;
> > +
>
>
> Can len even be 0? If that's the case then I guess that makes more sense
> to check and bail out early regardless if cmd_params_are_data is true ?
>
> For the !cmd_params_are_data case, the for loop will be a no-op anyways
> but still I think is cleaner to check as the first thing in this function.

I leaned on the no-op loop in the second branch, but I agree it would
be cleaner to check this and remove the edge case regardless if parameters are
data or not.

Addressed in v6.

> > +             ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
> > +             if (ret || len == 1)
> > +                     return ret;
> > +
>
> The len == 1 case is for commands that do not have parameters right? I
> think that adding some comments explaining this to make it clear why
> there is an early return.

Added a comment in v6.

> I'm happy with the implementation now, thanks a lot for bearing with
> me and your patience iterating over this series.
>
> Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

No problem at all, happy to work with you.
Thank you for the review.


--
Thanks,
Amit