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

[Amit Barzilai <amit.barzilai22@gmail.com>]

Firstly, I'd like to thank you for the review.

On Tue, 23 Jun 2026 12:37:31 +0300, Andy Shevchenko wrote:
> While it's not a problem _in this case_, the rule of thumb is always to have a
> trailing comma for non-terminator entry.

This was a repeating comment in the review of this patch.
I will be sending a v3 for this series, I will fix this point in
each location you mentioned in your reviews. This also covers the plane-helper
and encoder-helper arrays and the SSD133X_FAMILY enum entry. The terminator
entries (NR_SSD130X_VARIANTS and the trailing 0 in the command arrays) will be
left without a trailing comma.

>>  /*
>>   * Helper to write data (SSD13XX_DATA) to the device.
>>   */
>> -static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, int count)
>> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 *values, int count)
>>  {
>>  	return regmap_bulk_write(ssd130x->regmap, SSD13XX_DATA, values, count);
>>  }
>
> Stray change. If needed, either explain in the commit message or create
> a separate patch (depending on the dependencies).

The SSD135X branch I add in ssd130x_write_cmds() passes its const u8 *cmd buffer
to ssd130x_write_data(), which took a non-const u8 *. Rather than cast away const
at the call site, I propagated it into ssd130x_write_data(). I will explain this
in the commit message in v3.

>>  	unsigned int i;
>>  	int ret;
>>  
>> +	/*
>> +	 * The SSD135X family latches command parameters with D/C# HIGH (i.e.
>> +	 * clocked in as data), unlike the other families where the opcode and
>> +	 * all of its parameters are sent as commands (D/C# LOW). Send the
>> +	 * opcode as a command and any following parameter bytes as data.
>> +	 */
>> +	if (ssd130x->device_info->family_id == SSD135X_FAMILY) {
>> +		if (len == 0)
>> +			return 0;
>> +		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
>> +		if (ret || len == 1)
>> +			return ret;
>> +
>> +		return ssd130x_write_data(ssd130x, cmd + 1, len - 1);
>> +	}
>
>>  	for (i = 0; i < len; i++) {
>
> This loop seems for the len, so it will be the same for both devices as far as
> I can see the context. I can't find this piece in the original driver, perhaps
> it's some dependency?

Correct, it's a dependency. The for loop is unchanged context, not added by this
patch. ssd130x_write_cmds() was introduced in commit 208211646fb3 ("drm/solomon: add
ssd130x_run_cmd_seq() for batch command execution"). It is in drm-misc-next,
but not yet in the mainline.
That loop is the existing command path used by every family except SSD135X:
it clocks each byte out as a command (D/C# LOW). This patch only adds the SSD135X
branch above it, which returns early, so for SSD135X the loop is never reached.

The v3 cover letter will explicitly state that this series is based on drm-misc-next,
hopefully avoiding any future confusion.

>> +/*
>> + * Variadic wrapper around ssd130x_write_cmds(). The first variadic argument is
>> + * the command opcode and the following ones are its options/parameters.
>> + */
>> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
>> +			     /* u8 cmd, u8 option, ... */...)
>> +{
>> +	u8 buf[8];
>> +	va_list ap;
>> +	int i;
>> +
>> +	if (count > ARRAY_SIZE(buf))
>> +		return -EINVAL;
>> +
>> +	va_start(ap, count);
>
>> +	for (i = 0; i < count; i++)
>
> Can be
> 
> 	for (int i = 0; i < count; i++)

Fixed in v3.


>> +	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.

>> +		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.

>> +	};
>> +
>> +	/*
>> +	 * 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.

>> +		int ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
>> +
>> +		if (ret < 0)
>> +			return ret;
>
> This style is discouraged as it's harder to maintain. Better to split
> assignment and definition
> 
> 		int ret;
> 
> 		ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
> 		if (ret < 0)
> 			return ret;

Noted. Will fix for v3.



-- 
Thanks,
Amit