Re: [PATCH 3/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Thomas Zimmermann]

Hi

Am 01.02.22 um 15:36 schrieb Javier Martinez Canillas:
> On 2/1/22 15:05, Geert Uytterhoeven wrote:
> > Hi Javier,
> >
> > On Tue, Feb 1, 2022 at 2:02 PM Javier Martinez Canillas
> > <javierm@xxxxxxxxxx> wrote:
> > > On 2/1/22 10:33, Thomas Zimmermann wrote:
> > > > > +{
> > > > > +    u8 col_end = col_start + cols - 1;
> > > > > +    int ret;
> > > > > +
> > > > > +    if (col_start == ssd1307->col_start && col_end == ssd1307->col_end)
> > > > > +            return 0;
> > > > > +
> > > > > +    ret = ssd1307_write_cmd(ssd1307->client, SSD1307_SET_COL_RANGE);
> > > > > +    if (ret < 0)
> > > > > +            return ret;
> > > > > +
> > > > > +    ret = ssd1307_write_cmd(ssd1307->client, col_start);
> > > > > +    if (ret < 0)
> > > > > +            return ret;
> > > > > +
> > > > > +    ret = ssd1307_write_cmd(ssd1307->client, col_end);
> > > > > +    if (ret < 0)
> > > > > +            return ret;
> > > >
> > > > Can you write these cmds in one step, such as setting up an array and
> > > > sending it with ssd1307_write_array?
> > >
> > > I don't think so because the commands are different. But I'll check the
> > > ssd1306 datasheet again to confirma that's the case.
> >
> > IIRC, I tried that while working on the optimizations for ssd1307fb,
> > and it didn't work.
>
> That's what I would had expected by reading the datasheet. Thanks a
> lot for confirming my assumption.

Thanks to both of you. I was asking because I found the code to be 
repetitive and it's not clear that these 3 statements belong together.

I'd like to suggest to add a function

  ssd1307_write_cmds(client, len, const u8 *cmds)

that loops through cmds and sends the values one by one. A call would 
look like this:

  const u8 set_col_range[] = {
    SSD1307_SET_COL_RANGE,
    col_start,
    col_end
  };

  ssd1307_write_cmds(client, ARRAY_SIZE(set_col_range), set_col_range);

AND/OR

You could have functions that take a command with arguments; either as 
va_args or with one function per number of arguments. Or you could 
combine all these somehow.

Best regards
Thomas

>   
>
> Best regards,

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature