diff mbox series

[v3,3/7] drm: Add driver for Solomon SSD130X OLED displays

Message ID 20220209090314.2511959-4-javierm@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series drm: Add driver for Solomon SSD130X OLED displays | expand

Commit Message

Javier Martinez Canillas Feb. 9, 2022, 9:03 a.m. UTC
This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.

It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.

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

Changes in v3:
- Move driver from tiny sub-dir to drivers/gpu/drm/solomon (Sam Ravnborg)
- Split driver in a bus agnostic core and bus specific (Andy Shevchenko)
- Use regmap to access the chip registers (Andy Shevchenko)
- Remove unnecessary blank lines (Andy Shevchenko)
- Remove unneeded inline specifier in functions (Andy Shevchenko)
- Add a comment about always returning a single mode (Andy Shevchenko)
- Change write command logic to use do while loop (Andy Shevchenko)
- Use "firmware description" instead of "device tree" (Andy Shevchenko)
- Use return foo() instead of returning the return value (Andy Shevchenko)
- Don't split lines longer than 80 chars if makes less readable (Andy Shevchenko)
- Remove redundant else statements in .mode_valid callback (Andy Shevchenko)
- Rename powero{n,ff}() functions to power_o{n,ff)() (Andy Shevchenko)
- Use dev_err_probe() to prevent spam logs on probe deferral (Andy Shevchenko)
- Remove ',' after sentinel terminator in array (Andy Shevchenko)
- Fix a bug when doing partial updates (Geert Uytterhoeven)

Changes in v2:
- Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type.
- Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg)
- Don't clear the screen and turn on display on probe (Sam Ravnborg)
- Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg)
- Use dev managed version of devm_backlight_device_register() (Sam Ravnborg)
- Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg)
- Drop the .get_brightness callback since isn't needed  (Sam Ravnborg)
- Rename driver to ssd130x since supports a display family (Thomas Zimmermann)
- Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann)
- Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann)
- Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann)
- Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann)
- Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann)
- Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann)
- Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Reorganize code in probe to make it more legible (Thomas Zimmermann)
- ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann)
- Move regulator/pwm init logic to display pipe enable callback.

 drivers/gpu/drm/Kconfig           |   2 +
 drivers/gpu/drm/Makefile          |   1 +
 drivers/gpu/drm/solomon/Kconfig   |  12 +
 drivers/gpu/drm/solomon/Makefile  |   1 +
 drivers/gpu/drm/solomon/ssd130x.c | 823 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.h |  76 +++
 6 files changed, 915 insertions(+)
 create mode 100644 drivers/gpu/drm/solomon/Kconfig
 create mode 100644 drivers/gpu/drm/solomon/Makefile
 create mode 100644 drivers/gpu/drm/solomon/ssd130x.c
 create mode 100644 drivers/gpu/drm/solomon/ssd130x.h

Comments

Mark Brown Feb. 9, 2022, 1:43 p.m. UTC | #1
On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:

> +	if (ssd130x->vbat_reg) {
> +		ret = regulator_enable(ssd130x->vbat_reg);
> +		if (ret) {
> +			dev_err(dev, "Failed to enable VBAT: %d\n", ret);
> +			return ret;
> +		}
> +	}

Unless the device supports power being physically omitted regulator
usage should not be optional, it's just more code and a recipie for poor
error handling.
Javier Martinez Canillas Feb. 9, 2022, 2:17 p.m. UTC | #2
Hello Mark,

Thanks for your feedback.

On 2/9/22 14:43, Mark Brown wrote:
> On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:
> 
>> +	if (ssd130x->vbat_reg) {
>> +		ret = regulator_enable(ssd130x->vbat_reg);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to enable VBAT: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
> 
> Unless the device supports power being physically omitted regulator
> usage should not be optional, it's just more code and a recipie for poor
> error handling.

The device has a VCC pin but in most cases this is just connected to a
power provided by the board in its pinout header. For example, I've it
connected to a rpi4 3.3v pin.

I guess in that case what we should do then is to just have a regulator
fixed as the vbat-supply in the Device Tree, that's regulator-always-on.

The old ssd1307fb fbdev driver also had this as optional and I wanted to
keep the new driver as backward compatible. But I understand now that is
not describing the hardware properly by making this regulator optional.

Best regards,
Mark Brown Feb. 9, 2022, 2:22 p.m. UTC | #3
On Wed, Feb 09, 2022 at 03:17:06PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 14:43, Mark Brown wrote:

> > Unless the device supports power being physically omitted regulator
> > usage should not be optional, it's just more code and a recipie for poor
> > error handling.

> The device has a VCC pin but in most cases this is just connected to a
> power provided by the board in its pinout header. For example, I've it
> connected to a rpi4 3.3v pin.

That sounds like a very common configuration.

> I guess in that case what we should do then is to just have a regulator
> fixed as the vbat-supply in the Device Tree, that's regulator-always-on.

Generally I'd suggest labelling things with whatever the supply is
called in the board's schematics/documentation, that tends to make
things clearer and easier to follow.

> The old ssd1307fb fbdev driver also had this as optional and I wanted to
> keep the new driver as backward compatible. But I understand now that is
> not describing the hardware properly by making this regulator optional.

It is depressingly common to see broken code here, unfortunately
graphics drivers seem like one of the most common offendors.
Javier Martinez Canillas Feb. 9, 2022, 2:50 p.m. UTC | #4
On 2/9/22 15:22, Mark Brown wrote:
> On Wed, Feb 09, 2022 at 03:17:06PM +0100, Javier Martinez Canillas wrote:
>> On 2/9/22 14:43, Mark Brown wrote:
> 
>>> Unless the device supports power being physically omitted regulator
>>> usage should not be optional, it's just more code and a recipie for poor
>>> error handling.
> 
>> The device has a VCC pin but in most cases this is just connected to a
>> power provided by the board in its pinout header. For example, I've it
>> connected to a rpi4 3.3v pin.
> 
> That sounds like a very common configuration.
>

Yep.
 
>> I guess in that case what we should do then is to just have a regulator
>> fixed as the vbat-supply in the Device Tree, that's regulator-always-on.
> 
> Generally I'd suggest labelling things with whatever the supply is
> called in the board's schematics/documentation, that tends to make
> things clearer and easier to follow.
> 

The display controller datasheet and schematics mention VBAT as the power
supply but the documentation says that it's just connected to VCC and the
label in the display says VCC.

But I understand why the Device Tree binding and fbdev driver used VBAT
since that's what the documentation mentions.

>> The old ssd1307fb fbdev driver also had this as optional and I wanted to
>> keep the new driver as backward compatible. But I understand now that is
>> not describing the hardware properly by making this regulator optional.
> 
> It is depressingly common to see broken code here, unfortunately
> graphics drivers seem like one of the most common offendors.

I'll include a patch for the existing DT binding and mark the vbat-supply
property as required. Probably we won't be able to change the fbdev driver
without causing regressions, and I'm not interested in that driver anyways.

Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Mark Brown Feb. 9, 2022, 3:03 p.m. UTC | #5
On Wed, Feb 09, 2022 at 03:50:13PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 15:22, Mark Brown wrote:
> > On Wed, Feb 09, 2022 at 03:17:06PM +0100, Javier Martinez Canillas wrote:

> >> I guess in that case what we should do then is to just have a regulator
> >> fixed as the vbat-supply in the Device Tree, that's regulator-always-on.

> > Generally I'd suggest labelling things with whatever the supply is
> > called in the board's schematics/documentation, that tends to make
> > things clearer and easier to follow.

> The display controller datasheet and schematics mention VBAT as the power
> supply but the documentation says that it's just connected to VCC and the
> label in the display says VCC.

> But I understand why the Device Tree binding and fbdev driver used VBAT
> since that's what the documentation mentions.

What is "the documentation" in this context and how is that distinct
from the datasheet for the display controller?  In general the consumer
driver should be using the name from the datasheet and the regulator
itself should get a regulator-name reflecting the name in the schematic.

> > It is depressingly common to see broken code here, unfortunately
> > graphics drivers seem like one of the most common offendors.

> I'll include a patch for the existing DT binding and mark the vbat-supply
> property as required. Probably we won't be able to change the fbdev driver
> without causing regressions, and I'm not interested in that driver anyways.

There should be little danger of causing regressions given that a dummy
regualtor will be provided when one is missing.
Andy Shevchenko Feb. 9, 2022, 3:12 p.m. UTC | #6
On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:
> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
> OLED display controllers.
> 
> It's only the core part of the driver and a bus specific driver is needed
> for each transport interface supported by the display controllers.

Thank you for the update, my comments below.

...

>  source "drivers/gpu/drm/sprd/Kconfig"
>  
> +source "drivers/gpu/drm/solomon/Kconfig"

'o' before 'p' ?

...

>  obj-$(CONFIG_DRM_SPRD) += sprd/
> +obj-y			+= solomon/

Ditto ?

...

> +/*
> + * DRM driver for Solomon SSD130X OLED displays

Solomon SSD130x (with lower letter it's easy to read and realize that it's
not a model name).

> + * Copyright 2022 Red Hat Inc.
> + * Authors: Javier Martinez Canillas <javierm@redhat.com>
> + *
> + * Based on drivers/video/fbdev/ssd1307fb.c
> + * Copyright 2012 Free Electrons
> + */

> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regulator/consumer.h>

...

> +#define DRIVER_NAME	"ssd130x"
> +#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
> +#define DRIVER_DATE	"20220131"
> +#define DRIVER_MAJOR	1
> +#define DRIVER_MINOR	0

Not sure it has a value when being defined. Only one string is reused and even
if hard coded twice linker will optimize it.

...

> +/*
> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> + * is the command to write and the following are the command options.
> + */
> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> +				    /* u8 cmd, u8 option, ... */...)
> +{
> +	va_list ap;
> +	u8 value;
> +	int ret;
> +
> +	va_start(ap, count);
> +
> +	do {
> +		value = va_arg(ap, int);
> +		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
> +		if (ret)
> +			goto out_end;
> +	} while (--count);
> +
> +out_end:
> +	va_end(ap);
> +
> +	return ret;

Can bulk operation be used in the callers instead?

I have noticed that all of the callers are using
- 1 -- makes no sense at all, can be replaced with regmap_write()
- 2
- 3

Can be helpers for two and three arguments, with use of bulk call.

What do you think?

> +}

...

> +static void ssd130x_reset(struct ssd130x_device *ssd130x)
> +{
> +	/* Reset the screen */
> +	gpiod_set_value_cansleep(ssd130x->reset, 1);
> +	udelay(4);
> +	gpiod_set_value_cansleep(ssd130x->reset, 0);
> +	udelay(4);

I don't remember if reset pin is mandatory. fbtft does

	if (!gpiod->reset)
		return;

	...do reset...

> +}

...

> +	if (ssd130x->reset)

A-ha, why not in the callee?

> +		ssd130x_reset(ssd130x);

...

> +	/* Set COM direction */
> +	com_invdir = 0xc0 | ssd130x->com_invdir << 3;

Can 0xc0 and 3 be GENMASK()'ed and defined?

...

> +	/* Set clock frequency */
> +	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;

GENMASK() ?

...

> +		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
> +			    (ssd130x->low_power ? 5 : 0));

With if's it will look better.

		u32 mode = 0;

		if (ssd130x->area_color_enable)
			mode |= 0x30;
		if (ssd130x->low_power)
			mode |= 5;

...

> +	/* Turn on the DC-DC Charge Pump */
> +	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);

Ditto.

...

> +		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {

i++ should work as well.

> +			u8 val = ssd130x->lookup_table[i];
> +
> +			if (val < 31 || val > 63)
> +				dev_warn(ssd130x->dev,
> +					 "lookup table index %d value out of range 31 <= %d <= 63\n",
> +					 i, val);
> +			ret = ssd130x_write_cmd(ssd130x, 1, val);
> +			if (ret < 0)
> +				return ret;
> +		}

...

> +	u8 *buf = NULL;

> +

Redundant blank line, not sure if checkpatch catches this.

> +	struct drm_rect fullscreen = {
> +		.x1 = 0,
> +		.x2 = ssd130x->width,
> +		.y1 = 0,
> +		.y2 = ssd130x->height,
> +	};

...

> +power_off:

out_power_off: ?

...

> +		ret = PTR_ERR(ssd130x->vbat_reg);
> +		if (ret == -ENODEV)
> +			ssd130x->vbat_reg = NULL;
> +		else
> +			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");

Can it be

		ret = PTR_ERR(ssd130x->vbat_reg);
		if (ret != -ENODEV)
			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");

		ssd130x->vbat_reg = NULL;

?

...

> +	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
> +				     struct ssd130x_device, drm);
> +	if (IS_ERR(ssd130x)) {

> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
> +		return ssd130x;

return dev_err_probe() ?

> +	}

...

> +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> +					    &ssd130xfb_bl_ops, NULL);
> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
> +		return ERR_PTR(ret);

Ditto.

> +	}

...

> +	ret = drm_dev_register(drm, 0);
> +	if (ret) {
> +		dev_err(dev, "DRM device register failed: %d\n", ret);
> +		return ERR_PTR(ret);

Ditto.

> +	}

...

I have feelings that half of my comments were ignored...
Maybe I missed the discussion(s).
Javier Martinez Canillas Feb. 9, 2022, 3:17 p.m. UTC | #7
On 2/9/22 16:03, Mark Brown wrote:
> On Wed, Feb 09, 2022 at 03:50:13PM +0100, Javier Martinez Canillas wrote:

[snip]

> 
>> But I understand why the Device Tree binding and fbdev driver used VBAT
>> since that's what the documentation mentions.
> 
> What is "the documentation" in this context and how is that distinct
> from the datasheet for the display controller?  In general the consumer
> driver should be using the name from the datasheet and the regulator
> itself should get a regulator-name reflecting the name in the schematic.
>

For "documentation" I meant the datasheet that mentions VBAT but I got
what you mean and will also propose a change to the binding to rename
the property to vcc-supply instead to match the pin name in the device.

>>> It is depressingly common to see broken code here, unfortunately
>>> graphics drivers seem like one of the most common offendors.
> 
>> I'll include a patch for the existing DT binding and mark the vbat-supply
>> property as required. Probably we won't be able to change the fbdev driver
>> without causing regressions, and I'm not interested in that driver anyways.
> 
> There should be little danger of causing regressions given that a dummy
> regualtor will be provided when one is missing.

Right, I forgot that a dummy regulator is provided in that case. Perfect.

Best regards,
Javier Martinez Canillas Feb. 9, 2022, 3:54 p.m. UTC | #8
Hello Andy,

Thanks for your feedback.

On 2/9/22 16:12, Andy Shevchenko wrote:
> On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:
>> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
>> OLED display controllers.
>>
>> It's only the core part of the driver and a bus specific driver is needed
>> for each transport interface supported by the display controllers.
> 
> Thank you for the update, my comments below.
> 
> ...
> 
>>  source "drivers/gpu/drm/sprd/Kconfig"
>>  
>> +source "drivers/gpu/drm/solomon/Kconfig"
>
> 'o' before 'p' ?
>

The Kconfig was not sorted alphabetically so I just added it at
the end. Same for the Makefile. But I will change it in v4 just
to not keep adding unsorted entries.

[snip]

>> +/*
>> + * DRM driver for Solomon SSD130X OLED displays
> 
> Solomon SSD130x (with lower letter it's easy to read and realize that it's
> not a model name).
>

Ok.
 
>> + * Copyright 2022 Red Hat Inc.
>> + * Authors: Javier Martinez Canillas <javierm@redhat.com>
>> + *
>> + * Based on drivers/video/fbdev/ssd1307fb.c
>> + * Copyright 2012 Free Electrons
>> + */
> 
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/property.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regulator/consumer.h>
> 
> ...
> 
>> +#define DRIVER_NAME	"ssd130x"
>> +#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
>> +#define DRIVER_DATE	"20220131"
>> +#define DRIVER_MAJOR	1
>> +#define DRIVER_MINOR	0
> 
> Not sure it has a value when being defined. Only one string is reused and even
> if hard coded twice linker will optimize it.
>

I like to have all this at the beginning, it makes easier to read IMO.

[snip]

>> +
>> +	do {
>> +		value = va_arg(ap, int);
>> +		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
>> +		if (ret)
>> +			goto out_end;
>> +	} while (--count);
>> +
>> +out_end:
>> +	va_end(ap);
>> +
>> +	return ret;
> 
> Can bulk operation be used in the callers instead?
>

I'm using bulk writes for the data but not for the commands. Because I
tried to do that before and didn't work. But I'll give it a try again
now that I switched to regmap. Maybe it was some mistake on my end.
 
> I have noticed that all of the callers are using
> - 1 -- makes no sense at all, can be replaced with regmap_write()

Yes, I just used for consistency. That way all the places sending a
command would use the same function call.

> - 2
> - 3
> 
> Can be helpers for two and three arguments, with use of bulk call.
> 
> What do you think?
> 

Agreed, as mentioned I'll give it a try to sending all the data as a
bulk write with regmap.

>> +}
> 
> ...
> 
>> +static void ssd130x_reset(struct ssd130x_device *ssd130x)
>> +{
>> +	/* Reset the screen */
>> +	gpiod_set_value_cansleep(ssd130x->reset, 1);
>> +	udelay(4);
>> +	gpiod_set_value_cansleep(ssd130x->reset, 0);
>> +	udelay(4);
> 
> I don't remember if reset pin is mandatory. fbtft does
> 
> 	if (!gpiod->reset)
> 		return;
> 
> 	...do reset...
> 
>> +}
> 
> ...
> 
>> +	if (ssd130x->reset)
> 
> A-ha, why not in the callee?
>

I think is easier to read when doing it in the caller, specially since there
is only a single call. Than calling it unconditionally and making it a no-op
if there isn't a reset GPIO.

>> +		ssd130x_reset(ssd130x);
> 
> ...
> 
>> +	/* Set COM direction */
>> +	com_invdir = 0xc0 | ssd130x->com_invdir << 3;
> 
> Can 0xc0 and 3 be GENMASK()'ed and defined?
>

Ok.
 
> ...
> 
>> +	/* Set clock frequency */
>> +	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;
> 
> GENMASK() ?
>

Ok.
 
> ...
> 
>> +		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
>> +			    (ssd130x->low_power ? 5 : 0));
> 
> With if's it will look better.
> 
> 		u32 mode = 0;
> 
> 		if (ssd130x->area_color_enable)
> 			mode |= 0x30;
> 		if (ssd130x->low_power)
> 			mode |= 5;
>

Sure.
 
> ...
> 
>> +	/* Turn on the DC-DC Charge Pump */
>> +	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);
> 
> Ditto.
>

Ok.
 
> ...
> 
>> +		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {
> 
> i++ should work as well.
>

Yeah.
 
>> +			u8 val = ssd130x->lookup_table[i];
>> +
>> +			if (val < 31 || val > 63)
>> +				dev_warn(ssd130x->dev,
>> +					 "lookup table index %d value out of range 31 <= %d <= 63\n",
>> +					 i, val);
>> +			ret = ssd130x_write_cmd(ssd130x, 1, val);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
> 
> ...
> 
>> +	u8 *buf = NULL;
>
>> +
>
> Redundant blank line, not sure if checkpatch catches this.
>

Agreed.
 
>> +	struct drm_rect fullscreen = {
>> +		.x1 = 0,
>> +		.x2 = ssd130x->width,
>> +		.y1 = 0,
>> +		.y2 = ssd130x->height,
>> +	};
> 
> ...
> 
>> +power_off:
> 
> out_power_off: ?
>

Ok.
 
> ...
> 
>> +		ret = PTR_ERR(ssd130x->vbat_reg);
>> +		if (ret == -ENODEV)
>> +			ssd130x->vbat_reg = NULL;
>> +		else
>> +			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");
> 
> Can it be
> 
> 		ret = PTR_ERR(ssd130x->vbat_reg);
> 		if (ret != -ENODEV)
> 			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");
> 
> 		ssd130x->vbat_reg = NULL;
>
> ?
> 

Mark mentioned that the regulator shouldn't really be optional.
So half of this part is going away.

> ...
> 
>> +	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
>> +				     struct ssd130x_device, drm);
>> +	if (IS_ERR(ssd130x)) {
> 
>> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
>> +		return ssd130x;
> 
> return dev_err_probe() ?
>

No, because this isn't a resource provided by other driver. If this
failed is mostly due a memory allocation error.
 
>> +	}
> 
> ...
> 
>> +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
>> +					    &ssd130xfb_bl_ops, NULL);
>> +	if (IS_ERR(bl)) {
>> +		ret = PTR_ERR(bl);
>> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
>> +		return ERR_PTR(ret);
> 
> Ditto.
>

Same. This is an error and not a reason to defer the probe.

>> +	}
> 
> ...
> 
>> +	ret = drm_dev_register(drm, 0);
>> +	if (ret) {
>> +		dev_err(dev, "DRM device register failed: %d\n", ret);
>> +		return ERR_PTR(ret);
> 
> Ditto.
>

And same.
 
>> +	}
> 
> ...
> 
> I have feelings that half of my comments were ignored...
> Maybe I missed the discussion(s).
> 

I assure you that no comments from you or anyone were ignored.

I may had missed something but if if I did was a mistake and
not intentionally. I keep a changelog for each revision in
the patches with the name of the reviewer so people can check
and compare.

If something that you mentioned is not there, I apologize and
please point me out so I can address it in v4.

Best regards,
Andy Shevchenko Feb. 9, 2022, 4:08 p.m. UTC | #9
On Wed, Feb 09, 2022 at 04:54:01PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 16:12, Andy Shevchenko wrote:
> > On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:

...

> >> +	do {
> >> +		value = va_arg(ap, int);
> >> +		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
> >> +		if (ret)
> >> +			goto out_end;
> >> +	} while (--count);

> > 
> > Can bulk operation be used in the callers instead?
> >
> 
> I'm using bulk writes for the data but not for the commands. Because I
> tried to do that before and didn't work. But I'll give it a try again
> now that I switched to regmap. Maybe it was some mistake on my end.
>  
> > I have noticed that all of the callers are using
> > - 1 -- makes no sense at all, can be replaced with regmap_write()
> 
> Yes, I just used for consistency. That way all the places sending a
> command would use the same function call.
> 
> > - 2
> > - 3
> > 
> > Can be helpers for two and three arguments, with use of bulk call.
> > 
> > What do you think?
> > 
> 
> Agreed, as mentioned I'll give it a try to sending all the data as a
> bulk write with regmap.

Ah, it might be that it should be noinc bulk op. Need to be checked anyway.

...

> >> +static void ssd130x_reset(struct ssd130x_device *ssd130x)
> >> +{
> >> +	/* Reset the screen */
> >> +	gpiod_set_value_cansleep(ssd130x->reset, 1);
> >> +	udelay(4);
> >> +	gpiod_set_value_cansleep(ssd130x->reset, 0);
> >> +	udelay(4);
> > 
> > I don't remember if reset pin is mandatory. fbtft does
> > 
> > 	if (!gpiod->reset)
> > 		return;
> > 
> > 	...do reset...
> > 
> >> +}
> > 
> > ...
> > 
> >> +	if (ssd130x->reset)
> > 
> > A-ha, why not in the callee?
> >
> 
> I think is easier to read when doing it in the caller, specially since there
> is only a single call. Than calling it unconditionally and making it a no-op
> if there isn't a reset GPIO.

It doesn't matter where the check is and checking that in the callee seems
better as it relies on the reset functionality. Caller in such case shouldn't
even think if it's supported or not, not their business.

Last, but not least, if you think of power management, you probably want to
assert reset there as well, means additional checks?

> >> +		ssd130x_reset(ssd130x);

...

> >> +	if (IS_ERR(ssd130x)) {
> > 
> >> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
> >> +		return ssd130x;
> > 
> > return dev_err_probe() ?
> >
> 
> No, because this isn't a resource provided by other driver. If this
> failed is mostly due a memory allocation error.

Is it a problem? dev_err_probe() got update in the documentation explaining
that's fine to call even in such cases. The outcome is less amount of LOCs.

> >> +	}

...

> >> +	if (IS_ERR(bl)) {
> >> +		ret = PTR_ERR(bl);
> >> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
> >> +		return ERR_PTR(ret);
> > 
> > Ditto.
> 
> Same. This is an error and not a reason to defer the probe.

Ditto.

> >> +	}

...

> >> +	ret = drm_dev_register(drm, 0);
> >> +	if (ret) {
> >> +		dev_err(dev, "DRM device register failed: %d\n", ret);
> >> +		return ERR_PTR(ret);
> > 
> > Ditto.
> 
> And same.

Ditto.

> >> +	}

...

> > I have feelings that half of my comments were ignored...
> > Maybe I missed the discussion(s).
> 
> I assure you that no comments from you or anyone were ignored.
> 
> I may had missed something but if if I did was a mistake and
> not intentionally. I keep a changelog for each revision in
> the patches with the name of the reviewer so people can check
> and compare.
> 
> If something that you mentioned is not there, I apologize and
> please point me out so I can address it in v4.

It's just a feeling, because I repeating that dev_err_probe() a lot :-)
Nevertheless, now I see at least your point why you went that way.
But see my comments on it.
Javier Martinez Canillas Feb. 9, 2022, 4:26 p.m. UTC | #10
On 2/9/22 17:08, Andy Shevchenko wrote:

[snip]

>> Agreed, as mentioned I'll give it a try to sending all the data as a
>> bulk write with regmap.
> 
> Ah, it might be that it should be noinc bulk op. Need to be checked anyway.
>

Yeah, I'll give it a try for v4. Let's see how it goes.

[snip]

>>>
>>> A-ha, why not in the callee?
>>>
>>
>> I think is easier to read when doing it in the caller, specially since there
>> is only a single call. Than calling it unconditionally and making it a no-op
>> if there isn't a reset GPIO.
> 
> It doesn't matter where the check is and checking that in the callee seems
> better as it relies on the reset functionality. Caller in such case shouldn't
> even think if it's supported or not, not their business.
>

Ok, no strong opinions really so I will change it.
 
> Last, but not least, if you think of power management, you probably want to
> assert reset there as well, means additional checks?
> 
>>>> +		ssd130x_reset(ssd130x);
> 
> ...
> 
>>>> +	if (IS_ERR(ssd130x)) {
>>>
>>>> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
>>>> +		return ssd130x;
>>>
>>> return dev_err_probe() ?
>>>
>>
>> No, because this isn't a resource provided by other driver. If this
>> failed is mostly due a memory allocation error.
> 
> Is it a problem? dev_err_probe() got update in the documentation explaining
> that's fine to call even in such cases. The outcome is less amount of LOCs.
>

Thanks for pointing out. In my mind that was a way to denote in the code that
a probe deferral was possible and that the message should be debug but now I
went and read the comment as you suggested:

 * Note that it is deemed acceptable to use this function for error
 * prints during probe even if the @err is known to never be -EPROBE_DEFER.
 * The benefit compared to a normal dev_err() is the standardized format
 * of the error code and the fact that the error code is returned

So you are correct and using it is preferred even when no probe defer error
will be returned. I'll do that here and in the other places you mentioned.

[snip] 

>>> I have feelings that half of my comments were ignored...
>>> Maybe I missed the discussion(s).
>>
>> I assure you that no comments from you or anyone were ignored.
>>
>> I may had missed something but if if I did was a mistake and
>> not intentionally. I keep a changelog for each revision in
>> the patches with the name of the reviewer so people can check
>> and compare.
>>
>> If something that you mentioned is not there, I apologize and
>> please point me out so I can address it in v4.
> 
> It's just a feeling, because I repeating that dev_err_probe() a lot :-)
> Nevertheless, now I see at least your point why you went that way.
> But see my comments on it.

And I did use dev_err_probe() in the places that could cause a probe
deferral so it wasn't (completely) ignored.

On that topic, I even typed a SPI driver because of your feedback :)

Best regards,
Andy Shevchenko Feb. 9, 2022, 4:44 p.m. UTC | #11
On Wed, Feb 09, 2022 at 05:26:00PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 17:08, Andy Shevchenko wrote:

...

> On that topic, I even typed a SPI driver because of your feedback :)

Much appreciated, makes me much easier to test.

Thank you for doing all this!
Javier Martinez Canillas Feb. 11, 2022, 8:54 a.m. UTC | #12
On 2/9/22 17:26, Javier Martinez Canillas wrote:
> On 2/9/22 17:08, Andy Shevchenko wrote:
> 
> [snip]
> 
>>> Agreed, as mentioned I'll give it a try to sending all the data as a
>>> bulk write with regmap.
>>
>> Ah, it might be that it should be noinc bulk op. Need to be checked anyway.
>>
> 
> Yeah, I'll give it a try for v4. Let's see how it goes.
> 

I tried to do bulk writes for the command, but the problem is that the
command stream has to be as follow (i.e: SSD130X_SET_COL_RANGE command):

SSD130X_COMMAND
SSD130X_SET_COL_RANGE
SSD130X_COMMAND
col_start
SSD130X_COMMAND
col_end

That is, a SSD130X_COMMAND has to be writtn for each command and command
option. This means that you need to either construct a command stream in
the ssd130x_write_cmd() function or pass multiple SSD130X_COMMAND as
variadic arguments.

Both cases lead to a less elegant implementation than just having a count
parameter in ssd130x_write_cmd() and doing multiple writes as it is done
in the current implementation.

After all, these are only for commands that are done once to setup the
device. The SSD130X_DATA writes that are to update the display pixels are
already done in bulk, which are more important to do it efficiently.

Best regards,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dfdd3ec5f793..c423c920c027 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -405,6 +405,8 @@  source "drivers/gpu/drm/gud/Kconfig"
 
 source "drivers/gpu/drm/sprd/Kconfig"
 
+source "drivers/gpu/drm/solomon/Kconfig"
+
 config DRM_HYPERV
 	tristate "DRM Support for Hyper-V synthetic video device"
 	depends on DRM && PCI && MMU && HYPERV
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 8675c2af7ae1..a07b777e778a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -133,3 +133,4 @@  obj-y			+= xlnx/
 obj-y			+= gud/
 obj-$(CONFIG_DRM_HYPERV) += hyperv/
 obj-$(CONFIG_DRM_SPRD) += sprd/
+obj-y			+= solomon/
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
new file mode 100644
index 000000000000..c969c358a4a7
--- /dev/null
+++ b/drivers/gpu/drm/solomon/Kconfig
@@ -0,0 +1,12 @@ 
+config DRM_SSD130X
+	tristate "DRM support for Solomon SSD130X OLED displays"
+	depends on DRM
+	select BACKLIGHT_CLASS_DEVICE
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM driver for the SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
+	  OLED controllers. This is only for the core driver, a driver for
+	  the appropriate bus transport in your chip also must be selected.
+
+	  If M is selected the module will be called ssd130x.
diff --git a/drivers/gpu/drm/solomon/Makefile b/drivers/gpu/drm/solomon/Makefile
new file mode 100644
index 000000000000..f685addb19fe
--- /dev/null
+++ b/drivers/gpu/drm/solomon/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_DRM_SSD130X)	+= ssd130x.o
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
new file mode 100644
index 000000000000..79943f2e73a2
--- /dev/null
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -0,0 +1,823 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Solomon SSD130X OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ * Authors: Javier Martinez Canillas <javierm@redhat.com>
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_probe_helper.h>
+
+#include "ssd130x.h"
+
+#define DRIVER_NAME	"ssd130x"
+#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
+#define DRIVER_DATE	"20220131"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+#define SSD130X_DATA				0x40
+#define SSD130X_COMMAND				0x80
+
+#define SSD130X_SET_ADDRESS_MODE		0x20
+#define SSD130X_SET_ADDRESS_MODE_HORIZONTAL	(0x00)
+#define SSD130X_SET_ADDRESS_MODE_VERTICAL	(0x01)
+#define SSD130X_SET_ADDRESS_MODE_PAGE		(0x02)
+#define SSD130X_SET_COL_RANGE			0x21
+#define SSD130X_SET_PAGE_RANGE			0x22
+#define SSD130X_CONTRAST			0x81
+#define SSD130X_SET_LOOKUP_TABLE		0x91
+#define SSD130X_CHARGE_PUMP			0x8d
+#define SSD130X_SEG_REMAP_ON			0xa1
+#define SSD130X_DISPLAY_OFF			0xae
+#define SSD130X_SET_MULTIPLEX_RATIO		0xa8
+#define SSD130X_DISPLAY_ON			0xaf
+#define SSD130X_START_PAGE_ADDRESS		0xb0
+#define SSD130X_SET_DISPLAY_OFFSET		0xd3
+#define SSD130X_SET_CLOCK_FREQ			0xd5
+#define SSD130X_SET_AREA_COLOR_MODE		0xd8
+#define SSD130X_SET_PRECHARGE_PERIOD		0xd9
+#define SSD130X_SET_COM_PINS_CONFIG		0xda
+#define SSD130X_SET_VCOMH			0xdb
+
+#define MAX_CONTRAST 255
+
+static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
+{
+	return container_of(drm, struct ssd130x_device, drm);
+}
+
+/*
+ * Helper to write data (SSD130X_DATA) to the device.
+ */
+static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, int count)
+{
+	int ret;
+
+	ret = regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Helper to write command (SSD130X_COMMAND). The fist variadic argument
+ * is the command to write and the following are the command options.
+ */
+static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
+				    /* u8 cmd, u8 option, ... */...)
+{
+	va_list ap;
+	u8 value;
+	int ret;
+
+	va_start(ap, count);
+
+	do {
+		value = va_arg(ap, int);
+		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
+		if (ret)
+			goto out_end;
+	} while (--count);
+
+out_end:
+	va_end(ap);
+
+	return ret;
+}
+
+static int ssd130x_set_col_range(struct ssd130x_device *ssd130x,
+				 u8 col_start, u8 cols)
+{
+	u8 col_end = col_start + cols - 1;
+	int ret;
+
+	if (col_start == ssd130x->col_start && col_end == ssd130x->col_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x, 3, SSD130X_SET_COL_RANGE, col_start, col_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->col_start = col_start;
+	ssd130x->col_end = col_end;
+	return 0;
+}
+
+static int ssd130x_set_page_range(struct ssd130x_device *ssd130x,
+				  u8 page_start, u8 pages)
+{
+	u8 page_end = page_start + pages - 1;
+	int ret;
+
+	if (page_start == ssd130x->page_start && page_end == ssd130x->page_end)
+		return 0;
+
+	ret = ssd130x_write_cmd(ssd130x, 3, SSD130X_SET_PAGE_RANGE, page_start, page_end);
+	if (ret < 0)
+		return ret;
+
+	ssd130x->page_start = page_start;
+	ssd130x->page_end = page_end;
+	return 0;
+}
+
+static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = ssd130x->dev;
+	struct pwm_state pwmstate;
+
+	ssd130x->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(ssd130x->pwm)) {
+		dev_err(dev, "Could not get PWM from firmware description!\n");
+		return PTR_ERR(ssd130x->pwm);
+	}
+
+	pwm_init_state(ssd130x->pwm, &pwmstate);
+	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
+	pwm_apply_state(ssd130x->pwm, &pwmstate);
+
+	/* Enable the PWM */
+	pwm_enable(ssd130x->pwm);
+
+	dev_dbg(dev, "Using PWM%d with a %lluns period.\n",
+		ssd130x->pwm->pwm, pwm_get_period(ssd130x->pwm));
+
+	return 0;
+}
+
+static void ssd130x_reset(struct ssd130x_device *ssd130x)
+{
+	/* Reset the screen */
+	gpiod_set_value_cansleep(ssd130x->reset, 1);
+	udelay(4);
+	gpiod_set_value_cansleep(ssd130x->reset, 0);
+	udelay(4);
+}
+
+static int ssd130x_power_on(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = ssd130x->dev;
+	int ret;
+
+	if (ssd130x->reset)
+		ssd130x_reset(ssd130x);
+
+	if (ssd130x->vbat_reg) {
+		ret = regulator_enable(ssd130x->vbat_reg);
+		if (ret) {
+			dev_err(dev, "Failed to enable VBAT: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (ssd130x->device_info->need_pwm) {
+		ret = ssd130x_pwm_enable(ssd130x);
+		if (ret) {
+			dev_err(dev, "Failed to enable PWM: %d\n", ret);
+			if (ssd130x->vbat_reg)
+				regulator_disable(ssd130x->vbat_reg);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_power_off(struct ssd130x_device *ssd130x)
+{
+	if (ssd130x->device_info->need_pwm) {
+		pwm_disable(ssd130x->pwm);
+		pwm_put(ssd130x->pwm);
+	}
+
+	if (ssd130x->vbat_reg)
+		regulator_disable(ssd130x->vbat_reg);
+}
+
+static int ssd130x_init(struct ssd130x_device *ssd130x)
+{
+	u32 precharge, dclk, com_invdir, compins, chargepump;
+	int ret;
+
+	/* Set initial contrast */
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_CONTRAST, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	/* Set segment re-map */
+	if (ssd130x->seg_remap) {
+		ret = ssd130x_write_cmd(ssd130x, 1, SSD130X_SEG_REMAP_ON);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set COM direction */
+	com_invdir = 0xc0 | ssd130x->com_invdir << 3;
+	ret = ssd130x_write_cmd(ssd130x,  1, com_invdir);
+	if (ret < 0)
+		return ret;
+
+	/* Set multiplex ratio value */
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_MULTIPLEX_RATIO, ssd130x->height - 1);
+	if (ret < 0)
+		return ret;
+
+	/* set display offset value */
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_DISPLAY_OFFSET, ssd130x->com_offset);
+	if (ret < 0)
+		return ret;
+
+	/* Set clock frequency */
+	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_CLOCK_FREQ, dclk);
+	if (ret < 0)
+		return ret;
+
+	/* Set Area Color Mode ON/OFF & Low Power Display Mode */
+	if (ssd130x->area_color_enable || ssd130x->low_power) {
+		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
+			    (ssd130x->low_power ? 5 : 0));
+
+		ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_AREA_COLOR_MODE, mode);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set precharge period in number of ticks from the internal clock */
+	precharge = (ssd130x->prechargep1 & 0xf) | (ssd130x->prechargep2 & 0xf) << 4;
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_PRECHARGE_PERIOD, precharge);
+	if (ret < 0)
+		return ret;
+
+	/* Set COM pins configuration */
+	compins = 0x02 | !ssd130x->com_seq << 4 | ssd130x->com_lrremap << 5;
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_COM_PINS_CONFIG, compins);
+	if (ret < 0)
+		return ret;
+
+
+	/* Set VCOMH */
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_VCOMH, ssd130x->vcomh);
+	if (ret < 0)
+		return ret;
+
+	/* Turn on the DC-DC Charge Pump */
+	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_CHARGE_PUMP, chargepump);
+	if (ret < 0)
+		return ret;
+
+	/* Set lookup table */
+	if (ssd130x->lookup_table_set) {
+		int i;
+
+		ret = ssd130x_write_cmd(ssd130x, 1, SSD130X_SET_LOOKUP_TABLE);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {
+			u8 val = ssd130x->lookup_table[i];
+
+			if (val < 31 || val > 63)
+				dev_warn(ssd130x->dev,
+					 "lookup table index %d value out of range 31 <= %d <= 63\n",
+					 i, val);
+			ret = ssd130x_write_cmd(ssd130x, 1, val);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	/* Switch to horizontal addressing mode */
+	return ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_ADDRESS_MODE,
+				 SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
+}
+
+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
+			       struct drm_rect *rect)
+{
+	unsigned int x = rect->x1;
+	unsigned int y = rect->y1;
+	unsigned int width = drm_rect_width(rect);
+	unsigned int height = drm_rect_height(rect);
+	unsigned int line_length = DIV_ROUND_UP(width, 8);
+	unsigned int pages = DIV_ROUND_UP(y % 8 + height, 8);
+	u32 array_idx = 0;
+	int ret, i, j, k;
+	u8 *data_array = NULL;
+
+	data_array = kcalloc(width, pages, GFP_KERNEL);
+	if (!data_array)
+		return -ENOMEM;
+
+	/*
+	 * The screen is divided in pages, each having a height of 8
+	 * pixels, and the width of the screen. When sending a byte of
+	 * data to the controller, it gives the 8 bits for the current
+	 * column. I.e, the first byte are the 8 bits of the first
+	 * column, then the 8 bits for the second column, etc.
+	 *
+	 *
+	 * Representation of the screen, assuming it is 5 bits
+	 * wide. Each letter-number combination is a bit that controls
+	 * one pixel.
+	 *
+	 * A0 A1 A2 A3 A4
+	 * B0 B1 B2 B3 B4
+	 * C0 C1 C2 C3 C4
+	 * D0 D1 D2 D3 D4
+	 * E0 E1 E2 E3 E4
+	 * F0 F1 F2 F3 F4
+	 * G0 G1 G2 G3 G4
+	 * H0 H1 H2 H3 H4
+	 *
+	 * If you want to update this screen, you need to send 5 bytes:
+	 *  (1) A0 B0 C0 D0 E0 F0 G0 H0
+	 *  (2) A1 B1 C1 D1 E1 F1 G1 H1
+	 *  (3) A2 B2 C2 D2 E2 F2 G2 H2
+	 *  (4) A3 B3 C3 D3 E3 F3 G3 H3
+	 *  (5) A4 B4 C4 D4 E4 F4 G4 H4
+	 */
+
+	ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width);
+	if (ret < 0)
+		goto out_free;
+
+	ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8, pages);
+	if (ret < 0)
+		goto out_free;
+
+	for (i = y / 8; i < y / 8 + pages; i++) {
+		int m = 8;
+
+		/* Last page may be partial */
+		if (8 * (i + 1) > ssd130x->height)
+			m = ssd130x->height % 8;
+		for (j = x; j < x + width; j++) {
+			u8 data = 0;
+
+			for (k = 0; k < m; k++) {
+				u8 byte = buf[(8 * i + k) * line_length + j / 8];
+				u8 bit = (byte >> (j % 8)) & 1;
+
+				data |= bit << k;
+			}
+			data_array[array_idx++] = data;
+		}
+	}
+
+	ret = ssd130x_write_data(ssd130x, data_array, width * pages);
+
+out_free:
+	kfree(data_array);
+	return ret;
+}
+
+static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
+{
+	u8 *buf = NULL;
+
+	struct drm_rect fullscreen = {
+		.x1 = 0,
+		.x2 = ssd130x->width,
+		.y1 = 0,
+		.y2 = ssd130x->height,
+	};
+
+	buf = kcalloc(ssd130x->width, ssd130x->height, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ssd130x_update_rect(ssd130x, buf, &fullscreen);
+
+	kfree(buf);
+}
+
+static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
+				struct drm_rect *rect)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	int ret = 0;
+	u8 *buf = NULL;
+
+	buf = kcalloc(fb->width, fb->height, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
+
+	ssd130x_update_rect(ssd130x, buf, rect);
+
+	kfree(buf);
+
+	return ret;
+}
+
+static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					   const struct drm_display_mode *mode)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+
+	if (mode->hdisplay != ssd130x->mode.hdisplay &&
+	    mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_SIZE;
+
+	if (mode->hdisplay != ssd130x->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+
+	if (mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+
+static void ssd130x_display_pipe_enable(struct drm_simple_display_pipe *pipe,
+					struct drm_crtc_state *crtc_state,
+					struct drm_plane_state *plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx, ret;
+
+	ret = ssd130x_power_on(ssd130x);
+	if (ret)
+		return;
+
+	ret = ssd130x_init(ssd130x);
+	if (ret)
+		goto power_off;
+
+	if (!drm_dev_enter(drm, &idx))
+		goto power_off;
+
+	ssd130x_clear_screen(ssd130x);
+
+	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
+
+	backlight_enable(ssd130x->bl_dev);
+
+	drm_dev_exit(idx);
+
+	return;
+power_off:
+	ssd130x_power_off(ssd130x);
+}
+
+static void ssd130x_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_clear_screen(ssd130x);
+
+	backlight_disable(ssd130x->bl_dev);
+
+	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
+
+	ssd130x_power_off(ssd130x);
+
+	drm_dev_exit(idx);
+}
+
+static void ssd130x_display_pipe_update(struct drm_simple_display_pipe *pipe,
+					struct drm_plane_state *old_plane_state)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+	struct drm_plane_state *plane_state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_device *drm = &ssd130x->drm;
+	struct drm_rect src_clip, dst_clip;
+	int idx;
+
+	if (!fb)
+		return;
+
+	if (!pipe->crtc.state->active)
+		return;
+
+	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+		return;
+
+	dst_clip = plane_state->dst;
+	if (!drm_rect_intersect(&dst_clip, &src_clip))
+		return;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
+
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs ssd130x_pipe_funcs = {
+	.mode_valid = ssd130x_display_pipe_mode_valid,
+	.enable = ssd130x_display_pipe_enable,
+	.disable = ssd130x_display_pipe_disable,
+	.update = ssd130x_display_pipe_update,
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+};
+
+static int ssd130x_connector_get_modes(struct drm_connector *connector)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = ssd130x->dev;
+
+	mode = drm_mode_duplicate(connector->dev, &ssd130x->mode);
+	if (!mode) {
+		dev_err(dev, "Failed to duplicated mode\n");
+		return 0;
+	}
+
+	drm_mode_probed_add(connector, mode);
+	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
+
+	/* There is only a single mode */
+	return 1;
+}
+
+static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
+	.get_modes = ssd130x_connector_get_modes,
+};
+
+static const struct drm_connector_funcs ssd130x_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+DEFINE_DRM_GEM_FOPS(ssd130x_fops);
+
+static const struct drm_driver ssd130x_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &ssd130x_fops,
+};
+
+static int ssd130x_update_bl(struct backlight_device *bdev)
+{
+	struct ssd130x_device *ssd130x = bl_get_data(bdev);
+	int brightness = backlight_get_brightness(bdev);
+	int ret;
+
+	ssd130x->contrast = brightness;
+
+	ret = ssd130x_write_cmd(ssd130x, 1, SSD130X_CONTRAST);
+	if (ret < 0)
+		return ret;
+
+	ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static const struct backlight_ops ssd130xfb_bl_ops = {
+	.update_status	= ssd130x_update_bl,
+};
+
+static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = ssd130x->dev;
+
+	if (device_property_read_u32(dev, "solomon,width", &ssd130x->width))
+		ssd130x->width = 96;
+
+	if (device_property_read_u32(dev, "solomon,height", &ssd130x->height))
+		ssd130x->height = 16;
+
+	if (device_property_read_u32(dev, "solomon,page-offset", &ssd130x->page_offset))
+		ssd130x->page_offset = 1;
+
+	if (device_property_read_u32(dev, "solomon,col-offset", &ssd130x->col_offset))
+		ssd130x->col_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,com-offset", &ssd130x->com_offset))
+		ssd130x->com_offset = 0;
+
+	if (device_property_read_u32(dev, "solomon,prechargep1", &ssd130x->prechargep1))
+		ssd130x->prechargep1 = 2;
+
+	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
+		ssd130x->prechargep2 = 2;
+
+	if (!device_property_read_u8_array(dev, "solomon,lookup-table",
+					   ssd130x->lookup_table,
+					   ARRAY_SIZE(ssd130x->lookup_table)))
+		ssd130x->lookup_table_set = 1;
+
+	ssd130x->seg_remap = !device_property_read_bool(dev, "solomon,segment-no-remap");
+	ssd130x->com_seq = device_property_read_bool(dev, "solomon,com-seq");
+	ssd130x->com_lrremap = device_property_read_bool(dev, "solomon,com-lrremap");
+	ssd130x->com_invdir = device_property_read_bool(dev, "solomon,com-invdir");
+	ssd130x->area_color_enable =
+		device_property_read_bool(dev, "solomon,area-color-enable");
+	ssd130x->low_power = device_property_read_bool(dev, "solomon,low-power");
+
+	ssd130x->contrast = 127;
+	ssd130x->vcomh = ssd130x->device_info->default_vcomh;
+
+	/* Setup display timing */
+	if (device_property_read_u32(dev, "solomon,dclk-div", &ssd130x->dclk_div))
+		ssd130x->dclk_div = ssd130x->device_info->default_dclk_div;
+	if (device_property_read_u32(dev, "solomon,dclk-frq", &ssd130x->dclk_frq))
+		ssd130x->dclk_frq = ssd130x->device_info->default_dclk_frq;
+}
+
+static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
+{
+	struct drm_display_mode *mode = &ssd130x->mode;
+	struct device *dev = ssd130x->dev;
+	struct drm_device *drm = &ssd130x->drm;
+	unsigned long max_width, max_height;
+	int ret;
+
+	ret = drmm_mode_config_init(drm);
+	if (ret) {
+		dev_err(dev, "DRM mode config init failed: %d\n", ret);
+		return ret;
+	}
+
+	mode->type = DRM_MODE_TYPE_DRIVER;
+	mode->clock = 1;
+	mode->hdisplay = mode->htotal = ssd130x->width;
+	mode->hsync_start = mode->hsync_end = ssd130x->width;
+	mode->vdisplay = mode->vtotal = ssd130x->height;
+	mode->vsync_start = mode->vsync_end = ssd130x->height;
+	mode->width_mm = 27;
+	mode->height_mm = 27;
+
+	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
+
+	drm->mode_config.min_width = mode->hdisplay;
+	drm->mode_config.max_width = max_width;
+	drm->mode_config.min_height = mode->vdisplay;
+	drm->mode_config.max_height = max_height;
+	drm->mode_config.preferred_depth = 32;
+	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
+
+	ret = drm_connector_init(drm, &ssd130x->connector, &ssd130x_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		dev_err(dev, "DRM connector init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_connector_helper_add(&ssd130x->connector, &ssd130x_connector_helper_funcs);
+
+	ret = drm_simple_display_pipe_init(drm, &ssd130x->pipe, &ssd130x_pipe_funcs,
+					   ssd130x_formats, ARRAY_SIZE(ssd130x_formats),
+					   NULL, &ssd130x->connector);
+	if (ret) {
+		dev_err(dev, "DRM simple display pipeline init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_plane_enable_fb_damage_clips(&ssd130x->pipe.plane);
+
+	drm_mode_config_reset(drm);
+
+	return 0;
+}
+
+static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
+{
+	struct device *dev = ssd130x->dev;
+	int ret;
+
+	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ssd130x->reset))
+		return dev_err_probe(dev, PTR_ERR(ssd130x->reset), "Failed to get reset gpio\n");
+
+	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
+	if (IS_ERR(ssd130x->vbat_reg)) {
+		ret = PTR_ERR(ssd130x->vbat_reg);
+		if (ret == -ENODEV)
+			ssd130x->vbat_reg = NULL;
+		else
+			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");
+	}
+
+	return 0;
+}
+
+struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
+{
+	struct ssd130x_device *ssd130x;
+	struct backlight_device *bl;
+	struct drm_device *drm;
+	int ret;
+
+	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
+				     struct ssd130x_device, drm);
+	if (IS_ERR(ssd130x)) {
+		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
+		return ssd130x;
+	}
+
+	drm = &ssd130x->drm;
+
+	ssd130x->dev = dev;
+	ssd130x->regmap = regmap;
+	ssd130x->device_info = device_get_match_data(dev);
+
+	ssd130x_parse_properties(ssd130x);
+
+	ret = ssd130x_get_resources(ssd130x);
+	if (ret)
+		return ERR_PTR(ret);
+
+	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
+					    &ssd130xfb_bl_ops, NULL);
+	if (IS_ERR(bl)) {
+		ret = PTR_ERR(bl);
+		dev_err(dev, "Unable to register backlight device: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	bl->props.brightness = ssd130x->contrast;
+	bl->props.max_brightness = MAX_CONTRAST;
+	ssd130x->bl_dev = bl;
+
+	ret = ssd130x_init_modeset(ssd130x);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "DRM device register failed: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return ssd130x;
+}
+EXPORT_SYMBOL_GPL(ssd130x_probe);
+
+int ssd130x_remove(struct ssd130x_device *ssd130x)
+{
+	drm_dev_unplug(&ssd130x->drm);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ssd130x_remove);
+
+void ssd130x_shutdown(struct ssd130x_device *ssd130x)
+{
+	drm_atomic_helper_shutdown(&ssd130x->drm);
+}
+EXPORT_SYMBOL_GPL(ssd130x_shutdown);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
new file mode 100644
index 000000000000..bc760fdf0dfe
--- /dev/null
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -0,0 +1,76 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Header file for:
+ * DRM driver for Solomon SSD130X OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ * Authors: Javier Martinez Canillas <javierm@redhat.com>
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ */
+
+#ifndef __SSD1307X_H__
+#define __SSD1307X_H__
+
+#include <drm/drm_drv.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#include <linux/regmap.h>
+
+struct ssd130x_deviceinfo {
+	u32 default_vcomh;
+	u32 default_dclk_div;
+	u32 default_dclk_frq;
+	int need_pwm;
+	int need_chargepump;
+};
+
+struct ssd130x_device {
+	struct drm_device drm;
+	struct device *dev;
+	struct drm_simple_display_pipe pipe;
+	struct drm_display_mode mode;
+	struct drm_connector connector;
+	struct i2c_client *client;
+
+	struct regmap *regmap;
+
+	const struct ssd130x_deviceinfo *device_info;
+
+	unsigned area_color_enable : 1;
+	unsigned com_invdir : 1;
+	unsigned com_lrremap : 1;
+	unsigned com_seq : 1;
+	unsigned lookup_table_set : 1;
+	unsigned low_power : 1;
+	unsigned seg_remap : 1;
+	u32 com_offset;
+	u32 contrast;
+	u32 dclk_div;
+	u32 dclk_frq;
+	u32 height;
+	u8 lookup_table[4];
+	u32 page_offset;
+	u32 col_offset;
+	u32 prechargep1;
+	u32 prechargep2;
+
+	struct backlight_device *bl_dev;
+	struct pwm_device *pwm;
+	struct gpio_desc *reset;
+	struct regulator *vbat_reg;
+	u32 vcomh;
+	u32 width;
+	/* Cached address ranges */
+	u8 col_start;
+	u8 col_end;
+	u8 page_start;
+	u8 page_end;
+};
+
+struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap);
+int ssd130x_remove(struct ssd130x_device *ssd130x);
+void ssd130x_shutdown(struct ssd130x_device *ssd130x);
+
+#endif /* __SSD1307X_H__ */