Re: [PATCH v2 3/4] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sun, Jun 28, 2026 at 06:43:12PM +0300, Amit Barzilai wrote:
> On Tue, 23 Jun 2026 12:37:31 +0300, Andy Shevchenko wrote:

...

> >> +	const u8 cmds[] = {
> >
> > Why not static?
> 
> This array can't be made static. It is initialised with runtime values
> (ssd130x->width - 1 and ssd130x->height - 1), so it is not a compile-time
> constant and a static/file-scope definition wouldn't compile.
> The other ssd13xx_init() functions are non-static for exactly the same
> reason.

Ah, I see. Thanks for pointing out.

> >> +		4, SSD135X_SET_CONTRAST, 0xc8, 0x80, 0xc8,
> >> +		2, SSD135X_SET_CONTRAST_MASTER, 0x0f,
> >> +		2, SSD135X_SET_PRECHARGE2, 0x01,
> >> +		1, SSD135X_SET_DISPLAY_NORMAL,
> >> +		2, SSD13XX_SET_SEG_REMAP, remap,
> >
> >> +		0,
> >
> > No trailing comma for the terminator entry.
> 
> Removing it in v3. The other init arrays in drm-misc-next still carry the
> terminator comma, but that's pre-existing code outside this series -- I've left
> it alone to avoid unrelated churn. Happy to send a separate cleanup if you'd
> prefer.

You can issue a separate cleanup patch for those.

> >> +	};

...

> >> +	/*
> >> +	 * ssd130x_power_on() issues a short reset pulse, but the SSD1351 is not
> >> +	 * ready to accept commands immediately afterwards. Give the controller
> >> +	 * time to settle before sending the init sequence.
> >> +	 */
> >
> > Any reference to the datasheet?
> 
> It's not a datasheet figure. fb_ssd1351 doesn't do it in init_display() either;
> it inherits it from the shared fbtft_reset() helper, which deasserts reset and
> then does msleep(120) before any command is sent. The 120 ms is a generic fbtft
> blanket value, not an SSD1351 number -- the SSD1351 datasheet's reset timing is
> microsecond-scale.
> 
> I removed the msleep() and retested this on the hardware. The panel still 
> initialises reliably.
> I'll drop the msleep() in v3.

Yeah, I truly believe that this long delay is for the parallel type of IO, where
data and control signals are usually connected to a quite low speed GPIOs.

But I suggest to leave some comment in the code.

-- 
With Best Regards,
Andy Shevchenko