[PATCH 0/2] drm/ssd130x: Wire up ssd133x backlight/dimming

[PATCH 0/2] drm/ssd130x: Wire up ssd133x backlight/dimming

[Amit Barzilai]

The ssd130x backlight controller adjusts brightness by writing the panel's
contrast, but ssd130x_update_bl() writes the single global SSD13XX_CONTRAST
(0x81) command that only the SSD130x and SSD132x families expose.  SSD133x
has no such command: brightness is controlled through three per-channel
contrast registers (CONTRAST_A/B/C) that form a white-balance calibration
and must be scaled together.  As a result the backlight device registered
for an ssd133x panel had no effect.

This series adds ssd133x dimming:

  - Patch 1 extracts the ssd133x_init() contrast magic values into
    ssd133x_set_contrast(), which scales the per-channel calibration by a
    requested brightness so the white point is preserved while dimming.

  - Patch 2 makes backlight_ops.update_status a per-family choice, so
    ssd133x uses a handler that drives the three channels while SSD130x and
    SSD132x keep the existing single-register path.

Note patch 1 changes the ssd133x power-on brightness: it previously booted
at the unscaled (full) calibration and ignored the contrast field, and now
honours the shared default contrast of 127, i.e. half of MAX_CONTRAST.
This is intentional and matches ssd130x, whose contrast register also
defaults to mid-scale, so all families now power on at ~50%.

WARNING - NOT TESTED ON HARDWARE: this series is compile-tested only.  I do
not have an ssd133x panel, so the runtime behaviour -- in particular the
ssd133x contrast scaling and the new per-family backlight update -- is
unverified.  It is posted now so that dependent work (ssd1351 support) can
be based on it.  Review and testing by anyone with the hardware is welcome.

Amit Barzilai (2):
  drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init
  drm/ssd130x: Add per-family update backlight logic

 drivers/gpu/drm/solomon/ssd130x.c | 67 +++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH 1/2] drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init

[Amit Barzilai]

ssd133x_init() wrote the SSD133X_CONTRAST_A/B/C commands with magic hex
values (0x91/0x50/0x7d).  These are a per-channel white-balance
calibration: the A/B/C channels drive sub-pixels whose OLED materials
differ in luminous efficiency, so the values set the white point at full
brightness.

Extract them into ssd133x_set_contrast(), which scales each channel by a
requested brightness via ssd130x_scale_contrast(), instead of writing the
calibration unconditionally.  This makes the sequence readable, avoids
repetition, and is a prerequisite for wiring up an ssd133x backlight
controller that dims while preserving the white point.

Note this changes the ssd133x power-on brightness.  Previously the init
wrote the calibration unscaled and ignored ssd130x->contrast, so the
panel always booted at full brightness.  It now scales by the shared
default contrast of 127, i.e. half of MAX_CONTRAST (255).  This is
intentional and matches ssd130x, whose contrast register also defaults
to 127 (mid-scale), so all families now power on at ~50% and report
props.brightness = 127 / max_brightness = 255 to userspace.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 43 ++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 0940eae7a2e4..f49e5245b216 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -14,6 +14,7 @@
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/math.h>
 #include <linux/property.h>
 #include <linux/pwm.h>
 #include <linux/regulator/consumer.h>
@@ -140,6 +141,11 @@
 #define SSD133X_SET_PRECHARGE_VOLTAGE		0xbb
 #define SSD133X_SET_VCOMH_VOLTAGE		0xbe
 
+/* ssd133x A/B/C channel contrast at full brightness (white balance) */
+#define SSD133X_DEFAULT_CONTRAST_A		0x91
+#define SSD133X_DEFAULT_CONTRAST_B		0x50
+#define SSD133X_DEFAULT_CONTRAST_C		0x7d
+
 #define MAX_CONTRAST 255
 
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
@@ -582,12 +588,37 @@ static int ssd132x_init(struct ssd130x_device *ssd130x)
 	return ssd130x_run_cmd_seq(ssd130x, cmds);
 }
 
-static int ssd133x_init(struct ssd130x_device *ssd130x)
+/* Scale a channel's white-balance calibration contrast by the requested brightness */
+static u8 ssd130x_scale_contrast(u8 calibration, u32 brightness)
+{
+	return DIV_ROUND_CLOSEST(calibration * brightness, MAX_CONTRAST);
+}
+
+/*
+ * The A/B/C contrast channels drive sub-pixels whose OLED materials differ
+ * in luminous efficiency, so the per-channel values are a white-balance
+ * calibration.  Scale them by the requested brightness instead of
+ * overwriting them, to keep the white point while dimming.
+ */
+static int ssd133x_set_contrast(struct ssd130x_device *ssd130x, u32 brightness)
 {
 	const u8 cmds[] = {
-		2, SSD133X_CONTRAST_A, 0x91,
-		2, SSD133X_CONTRAST_B, 0x50,
-		2, SSD133X_CONTRAST_C, 0x7d,
+		2, SSD133X_CONTRAST_A,
+		ssd130x_scale_contrast(SSD133X_DEFAULT_CONTRAST_A, brightness),
+		2, SSD133X_CONTRAST_B,
+		ssd130x_scale_contrast(SSD133X_DEFAULT_CONTRAST_B, brightness),
+		2, SSD133X_CONTRAST_C,
+		ssd130x_scale_contrast(SSD133X_DEFAULT_CONTRAST_C, brightness),
+		0,
+	};
+
+	return ssd130x_run_cmd_seq(ssd130x, cmds);
+}
+
+static int ssd133x_init(struct ssd130x_device *ssd130x)
+{
+	int ret;
+	const u8 cmds[] = {
 		2, SSD133X_SET_MASTER_CURRENT, 0x06,
 		3, SSD133X_SET_COL_RANGE, 0x00, ssd130x->width - 1,
 		3, SSD133X_SET_ROW_RANGE, 0x00, ssd130x->height - 1,
@@ -614,6 +645,10 @@ static int ssd133x_init(struct ssd130x_device *ssd130x)
 		0,
 	};
 
+	ret = ssd133x_set_contrast(ssd130x, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
 	return ssd130x_run_cmd_seq(ssd130x, cmds);
 }
 
-- 
2.55.0

Re: [PATCH 1/2] drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The generic backlight update function corrupts the white balance on ssd133x panels by only updating contrast channel A.
--

--- Patch [1]: [PATCH 1/2] drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init ---
commit 2b92cd2c08daa2f9c52218a1668057ae50f9c448
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init
    
This patch extracts the SSD133X A/B/C channel contrast calibration into
ssd133x_set_contrast() which scales each channel by the requested brightness.

> Extract them into ssd133x_set_contrast(), which scales each channel by a
> requested brightness via ssd130x_scale_contrast(), instead of writing the
> calibration unconditionally.  This makes the sequence readable, avoids
> repetition, and is a prerequisite for wiring up an ssd133x backlight
> controller that dims while preserving the white point.

[Severity: High]
This isn't a bug introduced by this patch, but does the generic backlight
update function corrupt the white balance on ssd133x panels by only updating
contrast channel A?

The commit message notes this is a prerequisite for a proper ssd133x backlight
controller, but currently ssd130x_probe() unconditionally registers the
generic backlight controller for all devices:

drivers/gpu/drm/solomon/ssd130x.c:ssd130x_probe() {
	...
	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
					    &ssd130xfb_bl_ops, NULL);
	...
}

When the backlight is enabled or updated via sysfs, ssd130x_update_bl()
blindly writes the unscaled brightness to 0x81 (SSD13XX_CONTRAST). On ssd133x
panels, this maps to SSD133X_CONTRAST_A (Red):

drivers/gpu/drm/solomon/ssd130x.c:ssd130x_update_bl() {
	...
	ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
	if (ret < 0)
		return ret;

	ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast);
	...
}

This leaves channels B and C untouched at their initialization values,
destroying the white balance.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729053054.29374-1-amit.barzilai22@gmail.com?part=1

[PATCH 2/2] drm/ssd130x: Add per-family update backlight logic

[Amit Barzilai]

ssd130x_update_bl() runs for every SSD13xx panel, but it only works for
SSD130x and SSD132x: it writes the single global SSD13XX_CONTRAST (0x81)
command, which those two families expose.  SSD133x has no such command --
it has three per-channel contrast registers (CONTRAST_A/B/C) that must be
scaled together -- so ssd130x_update_bl() has no effect on it.

Make backlight_ops.update_status a per-family choice.  SSD130x and SSD132x
keep ssd130x_update_bl() because they share the SSD13XX_CONTRAST interface,
while SSD133x gets ssd133x_update_bl(), which drives the three channels
through ssd133x_set_contrast().

Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index f49e5245b216..0b0fc6fe3df2 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1711,8 +1711,25 @@ static int ssd130x_update_bl(struct backlight_device *bdev)
 	return 0;
 }
 
-static const struct backlight_ops ssd130xfb_bl_ops = {
-	.update_status	= ssd130x_update_bl,
+static int ssd133x_update_bl(struct backlight_device *bdev)
+{
+	struct ssd130x_device *ssd130x = bl_get_data(bdev);
+
+	ssd130x->contrast = backlight_get_brightness(bdev);
+
+	return ssd133x_set_contrast(ssd130x, ssd130x->contrast);
+}
+
+static const struct backlight_ops ssd130xfb_bl_ops[] = {
+	[SSD130X_FAMILY] = {
+		.update_status	= ssd130x_update_bl,
+	},
+	[SSD132X_FAMILY] = {
+		.update_status	= ssd130x_update_bl,
+	},
+	[SSD133X_FAMILY] = {
+		.update_status	= ssd133x_update_bl,
+	},
 };
 
 static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
@@ -1919,7 +1936,8 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 		return ERR_PTR(ret);
 
 	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
-					    &ssd130xfb_bl_ops, NULL);
+					    &ssd130xfb_bl_ops[ssd130x->device_info->family_id],
+					    NULL);
 	if (IS_ERR(bl))
 		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
 					     "Unable to register backlight device\n"));
-- 
2.55.0

Re: [PATCH 2/2] drm/ssd130x: Add per-family update backlight logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Multi-byte hardware command sequences can interleave during concurrent display and backlight updates, corrupting the panel state.
--

--- Patch [2]: [PATCH 2/2] drm/ssd130x: Add per-family update backlight logic ---
commit 3c6fc209e783670f32fed55dedcfe0a2a66ad662
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/ssd130x: Add per-family update backlight logic
    
This commit adds per-family backlight update logic to support SSD133x panels,
which require a three-channel contrast update sequence instead of a single
global contrast command.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index f49e5245b2160..0b0fc6fe3df23 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -1711,8 +1711,25 @@ static int ssd130x_update_bl(struct backlight_device *bdev)
>  	return 0;
>  }
>  
> -static const struct backlight_ops ssd130xfb_bl_ops = {
> -	.update_status	= ssd130x_update_bl,
> +static int ssd133x_update_bl(struct backlight_device *bdev)
> +{
> +	struct ssd130x_device *ssd130x = bl_get_data(bdev);
> +
> +	ssd130x->contrast = backlight_get_brightness(bdev);
> +
> +	return ssd133x_set_contrast(ssd130x, ssd130x->contrast);
> +}

[Severity: High]
This is a pre-existing issue, but does this newly added function exacerbate a
race condition with concurrent DRM atomic plane updates?

When a backlight sysfs brightness update runs concurrently with a DRM atomic
plane update, both paths invoke regmap_write() repeatedly to send multi-byte
sequences. Since regmap_write() only locks internally for a single byte, there
appears to be no driver-level mutex to serialize the entire command sequence.

The newly added ssd133x_update_bl() sends a 7-command multi-byte sequence via
ssd133x_set_contrast() and ssd130x_run_cmd_seq() without driver-level locks.
Similarly, the existing ssd130x_update_bl() function splits a 2-byte command
across two separate ssd130x_write_cmd() calls.

Could these multi-byte hardware command sequences interleave during concurrent
display and backlight updates, and potentially corrupt the panel's hardware
state machine?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729053054.29374-1-amit.barzilai22@gmail.com?part=2