[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

[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 1/2] drm/ssd130x: Scale ssd133x per-channel contrast by brightness on init

[Javier Martinez Canillas]

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

Thanks for your patch.

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

Looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Javier Martinez Canillas]

Amit Barzilai <amit.barzilai22@gmail.com> writes:

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

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Javier Martinez Canillas]

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

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

I've reviewed the patches and are quite straightforward so I think is safe
to merge them. I'll wait a few days in case someone else have comments but
feel free to post your ssd1351 patches if you have them already, just note
in the cover letter that depend on this patch-set.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat