Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Dominik Kierner]

Hello Javier,

I was working on a SSD130x driver as well, although with a different
(drm_panel) approach and hit a bit of a roadblock.
Now that Your driver is quite a bit maturer than mine,
I will happily provide You with the source of my draft,
so that any useful bits can be incorporated in Your driver.
I know that links are a bit frowned upon,
but I'd rather avoid cluttering the thread with my draft code,
which is unfinished and incompatible with the code in this thread.

https://github.com/dh-electronics/panel-solomon-ssd130x-draft
https://github.com/dh-electronics/panel-solomon-ssd130x-draft/tree/drm-ssd130x/drivers/gpu/drm/panel

The code was designed as a rewrite from scratch, as I assumed that a new
DRM driver that accommodates for I2C, 3- and 4-wire-SPI,
will probably need a new DTS interface for differentiating the
protocol-specific drivers, which would obviously break compatibility.

I do have few suggestions though:

# Atomic Configuration of Display Driving Regulator and the Charge Pump

The regulator VBAT is the SSD1306-specific low-voltage (3.3 V to 5 V)
regulator for the charge pump and takes the place of the voltage
regulator VCC, that would otherwise supply the OLED driver block with
7 V to 15 V.
The charge pump is never enabled when a VCC with 7 V to 15 V is present.
Configuring the charge pump based on the available regulators,
would provide an atomic configuration for either low-voltage +
charge pump or the regular voltage.

This way, the device tree boolean for enabling the charge pump could be
removed by probing for an optional VBAT first, which replaces VCC,
and falling back to a mandatory VCC otherwise:

```
[...]
struct ssd130x_panel {
...
	struct regulator *vdd;		/* Core logic supply */
	union {
		struct regulator *vcc;	/* Panel driving supply */
		struct regulator *vbat;	/* Charge pump regulator supply */
	};
	struct backlight_device *backlight;
		struct {
		unsigned int com_scan_dir_inv : 1;
		unsigned int com_seq_pin_cfg : 1;
		unsigned int com_lr_remap : 1;
		unsigned int seg_remap : 1;
		unsigned int inverse_display : 1;
		unsigned int use_charge_pump : 1;
		uint8_t height;
		uint8_t width;
		uint8_t height_mm;
		uint8_t width_mm;
		uint8_t display_start_line;
		uint8_t com_offset ;
		uint8_t contrast;
		uint8_t pre_charge_period_dclocks_phase1;
		uint8_t pre_charge_period_dclocks_phase2;
		uint8_t vcomh_deselect_level;
		uint8_t clock_divide_ratio;
		uint8_t oscillator_frequency;
	} display_settings;
	bool prepared;
	bool enabled;

[...]

ssd130x->vbat = devm_regulator_get_optional(dev, "vbat");
if (IS_ERR(ssd130x->vbat)) {
	ret = PTR_ERR(ssd130x->vbat);

	if (ret != -ENODEV) {
		if (ret != -EPROBE_DEFER)
			dev_err(dev,
				"failed to request regulator: %d\n",
				ret);
		return ret;
	}

	ssd130x->vbat = NULL;
}
else {
	ssd130x->display_settings.use_charge_pump = true;
}

/* Get panel driving supply */
If (!ssd130x->vbat) {
	ssd130x->vcc = devm_regulator_get(dev, "vcc");
	if (IS_ERR(ssd130x->vcc)){
		ret = PTR_ERR(ssd130x->vcc);
		return ret;
	}
	else {
		ssd130x->display_settings.use_charge_pump = false;
	}
}
```

Splitting in VCC/VBAT and VDD and enforcing their presence is
of course compatibility breaking.

https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm-ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.h#L85
https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm-ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.c#L80


# Static or Dynamic Configuration for SPI-Modes 3-Wire and 4-Wire

For the SPI-protocol drivers I see two possible approaches:
* Dynamic configuration by determining the presence/absence of the
  D/C-GPIO and assigning the functions accordingly.
  This way a single driver file for both SPI modes could be sufficient.
* Static configuration by using the device-tree names
  (ssd130x-spi-3wire/-4wire) to differentiate between the SPI protocol
  drivers.
  This would obviously necessitate two drivers files.

Which one do you think would be the best approach for this?


# DRM Mode Configuration via Device Tree

In the old fbdev driver, the display modes are hard-coded, which means
for every new display configuration, a new patch needs to be mainlined,
which slows down official Kernel support and
puts burden on the maintainers.
Additionally, with the DRM-subsystem supporting height and length
information, for scaling, this opens up a lot of new combinations.
The SSD1306 for example, is available in multiple resolutions like
128x64 and 96x16 and comes in different sizes per resolution as well.
Just to name a few:
* 128x64 0.96" (22x11mm)
* 128x64 1.3" (30x15mm)
* 96x16 0.69" (18x3mm)

Instead of hard-coding, I would suggest something along the lines of
of_get_drm_display_mode().
The displays won't need to support multiple modes at the same time,
let alone support for switching between them,
so the one-time invocation of this expensive function might be worth it. 
maybe a new and simpler function that could be named:
of_get_drm_display_mode_simple()

Providing a mode could later prove useful for a conversion to
drm_panel, if that is feasible.

But for a function like this, I have to chicken out.


# DRM Panel

The reason why I decided for the drm_panel approach in my code,
was power management and a clean handling of the software backlight
dependency, which requires powered display regulators to be powered.

Prepare/unprepare would power on/off the display logic regulator VDD.

Enable/disable would power on/off VCC/VBAT, optionally turn on/off
the charge pump and send the DISPLAY_ON/OFF commands.
The SSD1305's PWM part would likely be placed in enable/disable as well.

What is Your opinion on using drm_panel for Your driver?


Mit freundlichen Grüßen / Best regards

Dominik Kierner
Student Employee
Research & Development
DH electronics

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

Hello Dominik,

I missed this email before and never answered. Sorry about that.

On 3/10/22 14:11, Dominik Kierner wrote:
> Hello Javier,
> 
> I was working on a SSD130x driver as well, although with a different
> (drm_panel) approach and hit a bit of a roadblock.

My first attempt also was using a panel driver but then decided that it didn't
fit that well and a standalone DRM driver would be a better option.

> Now that Your driver is quite a bit maturer than mine,
> I will happily provide You with the source of my draft,
> so that any useful bits can be incorporated in Your driver.
> I know that links are a bit frowned upon,
> but I'd rather avoid cluttering the thread with my draft code,
> which is unfinished and incompatible with the code in this thread.
> 
> https://github.com/dh-electronics/panel-solomon-ssd130x-draft
> https://github.com/dh-electronics/panel-solomon-ssd130x-draft/tree/drm-ssd130x/drivers/gpu/drm/panel
>

Thanks, I looked at the code briefly and think that there are things there that we
could use. I.e the 3-wire SPI support that's in panel-solomon-ssd130x-spi-3wire.c.
 
> The code was designed as a rewrite from scratch, as I assumed that a new
> DRM driver that accommodates for I2C, 3- and 4-wire-SPI,
> will probably need a new DTS interface for differentiating the
> protocol-specific drivers, which would obviously break compatibility.
>

I wondered this too but then the drivers/staging/fbtft/fb_ssd1306.c supports both
both 4-wire/8-bit shift register and 3-wire/9-bits shift register SPI transports.

The driver does this by using a "buswidth" DT property to figure out which mode is
used. Since 4-wire seems to be the most common case and (at least with all the SPI
OLED displays I could find) one need to do some hardware modifications to support
the 3-wire SPI mode, I decided to just support 4-wire in the ssd130x-spi.c that is
going to be in v5.19.
 
> I do have few suggestions though:
> 
> # Atomic Configuration of Display Driving Regulator and the Charge Pump
> 
> The regulator VBAT is the SSD1306-specific low-voltage (3.3 V to 5 V)
> regulator for the charge pump and takes the place of the voltage
> regulator VCC, that would otherwise supply the OLED driver block with
> 7 V to 15 V.
> The charge pump is never enabled when a VCC with 7 V to 15 V is present.
> Configuring the charge pump based on the available regulators,
> would provide an atomic configuration for either low-voltage +
> charge pump or the regular voltage.
>

Ah I see, I didn't notice this when reading the ssd1306 datasheet.
 
> This way, the device tree boolean for enabling the charge pump could be
> removed by probing for an optional VBAT first, which replaces VCC,
> and falling back to a mandatory VCC otherwise:
>

Interesting, this makes sense to me.
 
> ```
> [...]
> struct ssd130x_panel {
> ...
> 	struct regulator *vdd;		/* Core logic supply */
> 	union {
> 		struct regulator *vcc;	/* Panel driving supply */
> 		struct regulator *vbat;	/* Charge pump regulator supply */
> 	};
> 	struct backlight_device *backlight;
> 		struct {
> 		unsigned int com_scan_dir_inv : 1;
> 		unsigned int com_seq_pin_cfg : 1;
> 		unsigned int com_lr_remap : 1;
> 		unsigned int seg_remap : 1;
> 		unsigned int inverse_display : 1;
> 		unsigned int use_charge_pump : 1;
> 		uint8_t height;
> 		uint8_t width;
> 		uint8_t height_mm;
> 		uint8_t width_mm;
> 		uint8_t display_start_line;
> 		uint8_t com_offset ;
> 		uint8_t contrast;
> 		uint8_t pre_charge_period_dclocks_phase1;
> 		uint8_t pre_charge_period_dclocks_phase2;
> 		uint8_t vcomh_deselect_level;
> 		uint8_t clock_divide_ratio;
> 		uint8_t oscillator_frequency;
> 	} display_settings;
> 	bool prepared;
> 	bool enabled;
> 
> [...]
> 
> ssd130x->vbat = devm_regulator_get_optional(dev, "vbat");
> if (IS_ERR(ssd130x->vbat)) {
> 	ret = PTR_ERR(ssd130x->vbat);
> 
> 	if (ret != -ENODEV) {
> 		if (ret != -EPROBE_DEFER)
> 			dev_err(dev,
> 				"failed to request regulator: %d\n",
> 				ret);
> 		return ret;
> 	}
> 
> 	ssd130x->vbat = NULL;
> }
> else {
> 	ssd130x->display_settings.use_charge_pump = true;
> }
> 
> /* Get panel driving supply */
> If (!ssd130x->vbat) {
> 	ssd130x->vcc = devm_regulator_get(dev, "vcc");
> 	if (IS_ERR(ssd130x->vcc)){
> 		ret = PTR_ERR(ssd130x->vcc);
> 		return ret;
> 	}
> 	else {
> 		ssd130x->display_settings.use_charge_pump = false;
> 	}
> }
> ```
> 
> Splitting in VCC/VBAT and VDD and enforcing their presence is
> of course compatibility breaking.
>
> https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm-ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.h#L85
> https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm-ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.c#L80
>

It is a break in the DT binding indeed but on the other hand it seems that the
regulator story is lacking in the current solomon,ssd1307fb.yaml anyways.

That is, the binding schema only mentions a "vbat-supply" but the DRM driver is
not looking for that but instead for "vcc-supply" (I think that was changed due
some feedback I got on some revisions, but didn't update the DT binding). The
fbdev drivers/video/fbdev/ssd1307fb.c driver does lookup "vbat-supply" but all
the DTS and DTS overlays I find don't set one.

Also the "vbat-supply" is not a required property in the current binding. One
thing to notice is that regulator_get() and regulator_get_optional() semantics
are confusing (at least for me). Since doesn't mean whether the regulator must
be present or not but rather if a dummy regulator must be provided if a supply
is not found.

In other words, I don't think that any of these supplies should be made required
in the DT binding but just leave the current "vbat-supply" and add properties for
"vcc-supply" and explain the relationship between these and just make the logic in
the driver to override struct ssd130x_deviceinfo .need_chargepump if are present.

> 
> # Static or Dynamic Configuration for SPI-Modes 3-Wire and 4-Wire
> 
> For the SPI-protocol drivers I see two possible approaches:
> * Dynamic configuration by determining the presence/absence of the
>   D/C-GPIO and assigning the functions accordingly.
>   This way a single driver file for both SPI modes could be sufficient.
> * Static configuration by using the device-tree names
>   (ssd130x-spi-3wire/-4wire) to differentiate between the SPI protocol
>   drivers.
>   This would obviously necessitate two drivers files.
> 
> Which one do you think would be the best approach for this?
>

I think that prefer the first approach. As mentioned the staging driver has a
"buswidth" property but as you said we could just use the "dc-gpios" presence
as indication on whether is a 4-wire or 3-wire SPI mode.
 
> 
> # DRM Mode Configuration via Device Tree
> 
> In the old fbdev driver, the display modes are hard-coded, which means
> for every new display configuration, a new patch needs to be mainlined,
> which slows down official Kernel support and
> puts burden on the maintainers.
> Additionally, with the DRM-subsystem supporting height and length
> information, for scaling, this opens up a lot of new combinations.
> The SSD1306 for example, is available in multiple resolutions like
> 128x64 and 96x16 and comes in different sizes per resolution as well.
> Just to name a few:
> * 128x64 0.96" (22x11mm)
> * 128x64 1.3" (30x15mm)
> * 96x16 0.69" (18x3mm)
>> Instead of hard-coding, I would suggest something along the lines of
> of_get_drm_display_mode().
> The displays won't need to support multiple modes at the same time,
> let alone support for switching between them,
> so the one-time invocation of this expensive function might be worth it. 
> maybe a new and simpler function that could be named:
> of_get_drm_display_mode_simple()
>

This makes sense to me as well.
 
> Providing a mode could later prove useful for a conversion to
> drm_panel, if that is feasible.
> 
> But for a function like this, I have to chicken out.
> 
> 
> # DRM Panel
> 
> The reason why I decided for the drm_panel approach in my code,
> was power management and a clean handling of the software backlight
> dependency, which requires powered display regulators to be powered.
> 
> Prepare/unprepare would power on/off the display logic regulator VDD.
> 
> Enable/disable would power on/off VCC/VBAT, optionally turn on/off
> the charge pump and send the DISPLAY_ON/OFF commands.

Yes, that's the reason why I started with a drm_panel driver as well.

> The SSD1305's PWM part would likely be placed in enable/disable as well.
> 
> What is Your opinion on using drm_panel for Your driver?
>

I can't remember exactly why I decided to stop using drm_panel, but I think
that was because struct drm_panel doesn't have a DRM device and so couldn't
use any of the helper functions that needed one?

At the end I found that having a standalone DRM driver and using the simple
display pipeline infrastructure was a much better way to implement it.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[andriy.shevchenko]

On Wed, May 25, 2022 at 09:46:24PM +0200, Javier Martinez Canillas wrote:
> On 3/10/22 14:11, Dominik Kierner wrote:

...

> > # DRM Mode Configuration via Device Tree
> > 
> > In the old fbdev driver, the display modes are hard-coded, which means
> > for every new display configuration, a new patch needs to be mainlined,
> > which slows down official Kernel support and
> > puts burden on the maintainers.
> > Additionally, with the DRM-subsystem supporting height and length
> > information, for scaling, this opens up a lot of new combinations.
> > The SSD1306 for example, is available in multiple resolutions like
> > 128x64 and 96x16 and comes in different sizes per resolution as well.
> > Just to name a few:
> > * 128x64 0.96" (22x11mm)
> > * 128x64 1.3" (30x15mm)
> > * 96x16 0.69" (18x3mm)
> >> Instead of hard-coding, I would suggest something along the lines of
> > of_get_drm_display_mode().
> > The displays won't need to support multiple modes at the same time,
> > let alone support for switching between them,
> > so the one-time invocation of this expensive function might be worth it. 
> > maybe a new and simpler function that could be named:
> > of_get_drm_display_mode_simple()
> 
> This makes sense to me as well.

What about non-OF platforms? Please, do not spread OF-only interfaces,
and use fwnode instead.

> > Providing a mode could later prove useful for a conversion to
> > drm_panel, if that is feasible.
> > 
> > But for a function like this, I have to chicken out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Dominik Kierner]

Hello Javier

On 6/14/22 13:39, Javier Martinez Canillas wrote:

> > I always understood regulator_get_optional() as a way of not having to rely on a dummy,
> > when a regulator is not present, but please correct me, if I am wrong on this.
> > The dummies would only be necessary for the mandatory supplies VCC and VDD.
> >
> 
> Yes, that's what I tried to say. That's regulator_get() and not _optional()
> the function that will provide a dummy regulator if isn't physically present:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2067
> 
> > You mean this part of the documentation of regulator_get_optional(), correct?:
> >
> >> * This is intended for use by consumers for devices which can have
> >> * some supplies unconnected in normal use, such as some MMC devices.
> >> * It can allow the regulator core to provide stub supplies for other
> >> * supplies requested using normal regulator_get() calls without
> >> * disrupting the operation of drivers that can handle absent
> >> * supplies.
> >
> >
> So for example when you just use a voltage rail in let's say a board pin header
> then you will need to define supply nodes with compatible = "regulator-
> fixed" ?

Exactly.

> That is indeed more accurate from a hardware description point of view but I'm
> not convinced that this is something worth to break DT backward compatibility.

> You also mentioned (IIUC) that the regulators could be made optional and their
> presence be used as an indication that an atomic charge pump configuration can
> be made instead of using the current ssd130x->display_settings.use_charge_pump.
> 
> I think that would prefer that the latter option, but will let others to chime
> in since maybe I'm not correct on the preferred approach.

Yes, here the reference for the former approach:
Chapter 2 "Charge Pump Regulator" on Page 62 of the SSD1306 datasheet:
https://cdn-shop.adafruit.com/datasheets/SSD1306.pdf

Just a TL;DR of the former approach for easier discussion:
The logic supply VDD would always be mandatory.
The low voltage supply VBAT would be optional and probed first.
If found, it would supersede the "high" voltage driving supply VCC and
the charge pump would be enabled. If VBAT is not found, then VCC is
mandatory and the charge pump will not be enabled.


--
Best regards

Dominik Kierner
Research & Development
DH electronics

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Dominik Kierner]

Hello Javier,

On 5/25/2022 21:46, Javier Martinez Canillas wrote:
> > Now that Your driver is quite a bit maturer than mine, I will happily
> > provide You with the source of my draft, so that any useful bits can
> > be incorporated in Your driver.
> > I know that links are a bit frowned upon, but I'd rather avoid
> > cluttering the thread with my draft code, which is unfinished and
> > incompatible with the code in this thread.
> >
> > https://github.com/dh-electronics/panel-solomon-ssd130x-draft
> > https://github.com/dh-electronics/panel-solomon-ssd130x-draft/tree/drm-ssd130x/drivers/gpu/drm/panel
> >
> 
> Thanks, I looked at the code briefly and think that there are things there that we
> could use. I.e the 3-wire SPI support that's in panel-solomon-ssd130x-spi-3wire.c.

When writing my driver code, I wasn't even considering using regmaps,
like You did in Your I2C-Code. If that's applicable for 3-wire-SPI,
it would likely be the better, more generic option. Your SPI-code
reuses these parts to some extent. For that case,
ssd130x_spi_regmap_config.reg_bits would need be changed to 1,
as the "register address" has a length of 1 bit and we are sending
9 bits over the line and not 16.
Since we still have 2 bytes of host memory,
it should still be compatible with the 4-wire write, right?
Or would 3-wire SPI require a second regmap?


> > Splitting in VCC/VBAT and VDD and enforcing their presence is of
> > course compatibility breaking.
> >
> > https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm
> > -ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.h#L85
> > https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm
> > -ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.c#L80
> >
> 
> It is a break in the DT binding indeed but on the other hand it seems that the
> regulator story is lacking in the current solomon,ssd1307fb.yaml anyways.
> 
> That is, the binding schema only mentions a "vbat-supply" but the DRM driver is not
> looking for that but instead for "vcc-supply" (I think that was changed due some
> feedback I got on some revisions, but didn't update the DT binding). The fbdev
> drivers/video/fbdev/ssd1307fb.c driver does lookup "vbat-supply" but all the DTS and
> DTS overlays I find don't set one.
> 
> Also the "vbat-supply" is not a required property in the current binding. One thing to
> notice is that regulator_get() and regulator_get_optional() semantics are confusing
> (at least for me). Since doesn't mean whether the regulator must be present or not
> but rather if a dummy regulator must be provided if a supply is not found.

I always understood regulator_get_optional() as a way of not having to rely on a dummy,
when a regulator is not present, but please correct me, if I am wrong on this.
The dummies would only be necessary for the mandatory supplies VCC and VDD. 

You mean this part of the documentation of regulator_get_optional(), correct?:

> * This is intended for use by consumers for devices which can have
> * some supplies unconnected in normal use, such as some MMC devices.
> * It can allow the regulator core to provide stub supplies for other
> * supplies requested using normal regulator_get() calls without
> * disrupting the operation of drivers that can handle absent
> * supplies.


> In other words, I don't think that any of these supplies should be made required in
> the DT binding but just leave the current "vbat-supply" and add properties for "vcc-
> supply" and explain the relationship between these and just make the logic in the
> driver to override struct ssd130x_deviceinfo .need_chargepump if are present.

My idea was to require these supplies, so that the binding correctly
reflects the manuals. Driving supply VCC and logic supply VDD, are
present throughout the SSD130x family. Only the VBAT supply is an
optional SSD1306 specific and would therefore use an optional
regulator.

The only other device specific supply is the SSD1305's VDDIO supply,
which is mandatory and seems to be commonly connected to VDD,
so including that is likely unnecessary.
I Just wanted to mention it for completeness.

If the device isn't controllable by Linux, a dummy would be connected
instead, just like the dummy regulator documentation states:

> * This is useful for systems with mixed controllable and
> * non-controllable regulators, as well as for allowing testing on
> * systems with no controllable regulators.

Which would be the case, with the SSD130x controllers.
Sometimes they are connected to external, non-controllable regulators.

I figured that the kernel developers might be more open to a compatibility
breaking change, under the circumstance, that this is more or less a new
driver for DRM, that it provides atomic charge pump configuration for the
SSD1306 and that some (embedded) user space software might need to be
rewritten to accommodate for the transition from fbdev to DRM anyway.
But I might be wrong on this.


> > # Static or Dynamic Configuration for SPI-Modes 3-Wire and 4-Wire
> >
> > For the SPI-protocol drivers I see two possible approaches:
> > * Dynamic configuration by determining the presence/absence of the
> >   D/C-GPIO and assigning the functions accordingly.
> >   This way a single driver file for both SPI modes could be sufficient.
> > * Static configuration by using the device-tree names
> >   (ssd130x-spi-3wire/-4wire) to differentiate between the SPI protocol
> >   drivers.
> >   This would obviously necessitate two drivers files.
> >
> > Which one do you think would be the best approach for this?
> >
> 
> I think that prefer the first approach. As mentioned the staging driver has a
> "buswidth" property but as you said we could just use the "dc-gpios" presence as
> indication on whether is a 4-wire or 3-wire SPI mode.

You are correct, I do prefer the first approach.
It would cut the additional file and code required for the second
approach and eliminate an additional device tree name,
that would have been necessary otherwise.


> > What is Your opinion on using drm_panel for Your driver?
> >
> 
> I can't remember exactly why I decided to stop using drm_panel, but I think that
> was because struct drm_panel doesn't have a DRM device and so couldn't use any of
> the helper functions that needed one?

I likely hit the same roadblock.
I would say, this approach should be revisited, when appropriate
helpers for this approach exist, as it would further clean up and
generify the ssd130x device configuration.

--
Best regards

Dominik Kierner
Research & Development
DH electronics

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[andriy.shevchenko]

On Mon, Jun 13, 2022 at 11:39:30AM +0000, Dominik Kierner wrote:
> On 5/25/2022 21:46, Javier Martinez Canillas wrote:

...

> > Thanks, I looked at the code briefly and think that there are things there that we
> > could use. I.e the 3-wire SPI support that's in panel-solomon-ssd130x-spi-3wire.c.
> 
> When writing my driver code, I wasn't even considering using regmaps,
> like You did in Your I2C-Code. If that's applicable for 3-wire-SPI,
> it would likely be the better, more generic option. Your SPI-code
> reuses these parts to some extent. For that case,
> ssd130x_spi_regmap_config.reg_bits would need be changed to 1,
> as the "register address" has a length of 1 bit and we are sending
> 9 bits over the line and not 16.
> Since we still have 2 bytes of host memory,
> it should still be compatible with the 4-wire write, right?
> Or would 3-wire SPI require a second regmap?

I believe the cleanest solution is to have different regmap configurations.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

On 6/13/22 17:35, andriy.shevchenko@linux.intel.com wrote:
> On Mon, Jun 13, 2022 at 11:39:30AM +0000, Dominik Kierner wrote:
>> On 5/25/2022 21:46, Javier Martinez Canillas wrote:
> 
> ...
> 
>>> Thanks, I looked at the code briefly and think that there are things there that we
>>> could use. I.e the 3-wire SPI support that's in panel-solomon-ssd130x-spi-3wire.c.
>>
>> When writing my driver code, I wasn't even considering using regmaps,
>> like You did in Your I2C-Code. If that's applicable for 3-wire-SPI,
>> it would likely be the better, more generic option. Your SPI-code
>> reuses these parts to some extent. For that case,
>> ssd130x_spi_regmap_config.reg_bits would need be changed to 1,
>> as the "register address" has a length of 1 bit and we are sending
>> 9 bits over the line and not 16.
>> Since we still have 2 bytes of host memory,
>> it should still be compatible with the 4-wire write, right?
>> Or would 3-wire SPI require a second regmap?
> 
> I believe the cleanest solution is to have different regmap configurations.
> 

I agree with this.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

Hello Dominik,

On 6/13/22 13:39, Dominik Kierner wrote:

Removed the regmap part since Andy already commented and I agree with him.

>>> Splitting in VCC/VBAT and VDD and enforcing their presence is of
>>> course compatibility breaking.
>>>
>>> https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm
>>> -ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.h#L85
>>> https://github.com/dh-electronics/panel-solomon-ssd130x-draft/blob/drm
>>> -ssd130x/drivers/gpu/drm/panel/panel-solomon-ssd130x.c#L80
>>>
>>
>> It is a break in the DT binding indeed but on the other hand it seems that the
>> regulator story is lacking in the current solomon,ssd1307fb.yaml anyways.
>>
>> That is, the binding schema only mentions a "vbat-supply" but the DRM driver is not
>> looking for that but instead for "vcc-supply" (I think that was changed due some
>> feedback I got on some revisions, but didn't update the DT binding). The fbdev
>> drivers/video/fbdev/ssd1307fb.c driver does lookup "vbat-supply" but all the DTS and
>> DTS overlays I find don't set one.
>>
>> Also the "vbat-supply" is not a required property in the current binding. One thing to
>> notice is that regulator_get() and regulator_get_optional() semantics are confusing
>> (at least for me). Since doesn't mean whether the regulator must be present or not
>> but rather if a dummy regulator must be provided if a supply is not found.
> 
> I always understood regulator_get_optional() as a way of not having to rely on a dummy,
> when a regulator is not present, but please correct me, if I am wrong on this.
> The dummies would only be necessary for the mandatory supplies VCC and VDD. 
> 

Yes, that's what I tried to say. That's regulator_get() and not _optional()
the function that will provide a dummy regulator if isn't physically present:

https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2067

> You mean this part of the documentation of regulator_get_optional(), correct?:
> 
>> * This is intended for use by consumers for devices which can have
>> * some supplies unconnected in normal use, such as some MMC devices.
>> * It can allow the regulator core to provide stub supplies for other
>> * supplies requested using normal regulator_get() calls without
>> * disrupting the operation of drivers that can handle absent
>> * supplies.
>
> 
>> In other words, I don't think that any of these supplies should be made required in
>> the DT binding but just leave the current "vbat-supply" and add properties for "vcc-
>> supply" and explain the relationship between these and just make the logic in the
>> driver to override struct ssd130x_deviceinfo .need_chargepump if are present.
> 
> My idea was to require these supplies, so that the binding correctly
> reflects the manuals. Driving supply VCC and logic supply VDD, are
> present throughout the SSD130x family. Only the VBAT supply is an
> optional SSD1306 specific and would therefore use an optional
> regulator.
> 
> The only other device specific supply is the SSD1305's VDDIO supply,
> which is mandatory and seems to be commonly connected to VDD,
> so including that is likely unnecessary.
> I Just wanted to mention it for completeness.
> 
> If the device isn't controllable by Linux, a dummy would be connected
> instead, just like the dummy regulator documentation states:
> 
>> * This is useful for systems with mixed controllable and
>> * non-controllable regulators, as well as for allowing testing on
>> * systems with no controllable regulators.
> 
> Which would be the case, with the SSD130x controllers.
> Sometimes they are connected to external, non-controllable regulators.
> 
> I figured that the kernel developers might be more open to a compatibility
> breaking change, under the circumstance, that this is more or less a new
> driver for DRM, that it provides atomic charge pump configuration for the
> SSD1306 and that some (embedded) user space software might need to be
> rewritten to accommodate for the transition from fbdev to DRM anyway.
> But I might be wrong on this.
> 

So for example when you just use a voltage rail in let's say a board pin header
then you will need to define supply nodes with compatible = "regulator-fixed" ?

That is indeed more accurate from a hardware description point of view but I'm
not convinced that this is something worth to break DT backward compatibility.

You also mentioned (IIUC) that the regulators could be made optional and their
presence be used as an indication that an atomic charge pump configuration can
be made instead of using the current ssd130x->display_settings.use_charge_pump.

I think that would prefer that the latter option, but will let others to chime
in since maybe I'm not correct on the preferred approach.

> 
>>> # Static or Dynamic Configuration for SPI-Modes 3-Wire and 4-Wire
>>>
>>> For the SPI-protocol drivers I see two possible approaches:
>>> * Dynamic configuration by determining the presence/absence of the
>>>   D/C-GPIO and assigning the functions accordingly.
>>>   This way a single driver file for both SPI modes could be sufficient.
>>> * Static configuration by using the device-tree names
>>>   (ssd130x-spi-3wire/-4wire) to differentiate between the SPI protocol
>>>   drivers.
>>>   This would obviously necessitate two drivers files.
>>>
>>> Which one do you think would be the best approach for this?
>>>
>>
>> I think that prefer the first approach. As mentioned the staging driver has a
>> "buswidth" property but as you said we could just use the "dc-gpios" presence as
>> indication on whether is a 4-wire or 3-wire SPI mode.
> 
> You are correct, I do prefer the first approach.
> It would cut the additional file and code required for the second
> approach and eliminate an additional device tree name,
> that would have been necessary otherwise.
>

Great that we are on the same page here.

> 
>>> What is Your opinion on using drm_panel for Your driver?
>>>
>>
>> I can't remember exactly why I decided to stop using drm_panel, but I think that
>> was because struct drm_panel doesn't have a DRM device and so couldn't use any of
>> the helper functions that needed one?
> 
> I likely hit the same roadblock.
> I would say, this approach should be revisited, when appropriate
> helpers for this approach exist, as it would further clean up and
> generify the ssd130x device configuration.
>

It's unlikely that drm_panel will get a drm_device since that was the case
but was changed by commit aa6c43644bc5 ("drm/panel: drop drm_device from
drm_panel"). But yes, I agree that we could revisit if there are helpers in
the future to manage a backlight device that is handled by a DRM driver.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v6 0/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Using the DRM fbdev emulation, all the tests from Geert Uytterhoeven repo
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes.

I've also tested it using the display as a VT output and even though fbcon
seems to work, it is mostly unusable on a 128x64 SSD1306 display.

This is a v6 that addresses the issues pointed in v5. Thanks a lot to all
reviewers that gave me feedback and comments.

I didn't include the patch that adds the SPI support this time, because it
will require changes in the existing Device Tree binding. And I wanted to
avoid that bikesheeding for now, to focus on the core and I2C parts.

Once this series land, I'll post patches for the SPI support. But the WIP
patch posted in v3 should still apply cleanly on top of this v6:

https://patchwork.kernel.org/project/dri-devel/patch/20220209091204.2513437-1-javierm@redhat.com/

Patch #1 splits per-line conversion logic in drm_fb_xrgb8888_to_gray8() to
a separate drm_fb_xrgb8888_to_gray8_line() helper function.

Patch #2 adds a new drm_fb_xrgb8888_to_mono_reversed() helper function to
convert from XR24 to reversed monochrome. The latter internally converts
each line first to 8-bit grayscale and then to 1-bit reversed monochrome.

Patch #3 adds the driver. This only has the core support and doesn't have
any bus specific code, separate drivers are needed for the transport used.

Patch #4 adds a driver to use the I2C bus to communicate with the device.

Patch #5 adds a MAINTAINERS entry for the DRM driver and patch #6 adds
myself as co-maintainer of the existing DT binding for the ssd1307fb,
since the same DT binding is used for both the fbdev and DRM drivers.

Best regards,
Javier

Changes in v6:
- Add Andy Shevchenko's Reviewed-by to patch #1.
- Add Andy Shevchenko's Reviewed-by to patch #2.
- Just return regmap_bulk_write() in ssd130x_write_data() (Andy Shevchenko)
- Remove unnecessary cast in ssd130x_write_cmd() (Andy Shevchenko)
- Return ERR_PTR(dev_err_probe(...)) everywhere in probe (Andy Shevchenko)

Changes in v5:
- Add Thomas Zimmermann's Reviewed-by to patch #1.
- Use drm_WARN_ON* macros instead of deprecated ones (Thomas Zimmermann)
- Include <linux/bits.h> header (Andy Shevchenko)
- Drop parenthesis for command options macros (Andy Shevchenko)
- Explain in ssd130x_write_cmd() comment how commands are sent (Andy Shevchenko)
- The pwm_*() functions check for NULL already (Andy Shevchenko)
- Remove unnecesary blank line (Andy Shevchenko)
- Simplify error handling for backlight registration failure (Geert Uytterhoeven)
- Don't clear screen on enable, instead send the full buffer (Thomas Zimmermann)
- Add Andy Shevchenko's Reviewed-by tag to patch #4.
- Add Andy Shevchenko's Reviewed-by tag to patch #5.
- Add Andy Shevchenko's Reviewed-by tag to patch #6.

Changes in v4:
- Rename end_offset to end_len (Thomas Zimmermann)
- Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann)
- Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann)
- Allocate single buffer for both copy cma memory and gray8 (Thomas Zimmermann)
- Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper.
- Rename vbat supply to vcc since is how's labeled in the device (Mark Brown)
- Don't make the regulator option since is always needed (Mark Brown)
- Add solomon Kconfig source and directory inclusion sorted (Andy Shevchenko)
- Use SSD130x instead of SSD130X to denote is not a model name (Andy Shevchenko)
- Check if there's a reset pin in the callee and not the caller (Andy Shevchenko)
- Define missing commands instead of using magic numbers (Andy Shevchenko)
- Use GENMASK() and FIELD_PREP() macros when possible (Andy Shevchenko)
- Avoid using ternary operators to ease code readablity (Andy Shevchenko)
- Use i++ instead of --i on some for loops (Andy Shevchenko)
- Remove redundant blank lines (Andy Shevchenko)
- Rename power_off label to out_power_off (Andy Shevchenko)
- Use dev_err_probe() even if no -EPROBE_DEFER (Andy Shevchenko)
- Don't use plural Authors if there's only one (Andy Shevchenko)
- Remove unnecessary casting (Geert Uytterhoeven)
- Remove redundant blank lines (Andy Shevchenko)
- Remove comma after of_device_id table terminator (Andy Shevchenko)
- Add Rob Herring Acked-by tag to patch adding as DT binding co-maintainer.

Changes in v3:
- Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann)
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann)
- Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann)
- 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)
- Add a separate driver for SSD130X chips I2C support (Andy Shevchenko)
- Adapt MAINTAINERS entry to point to the new drivers/gpu/drm/solomon directory.

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.
- Add Sam Ravnborg's acked-by to patch adding a MAINTAINERS entry (Sam Ravnborg)
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg).

Javier Martinez Canillas (6):
  drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
  drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()
  drm: Add driver for Solomon SSD130x OLED displays
  drm/solomon: Add SSD130x OLED displays I2C support
  MAINTAINERS: Add entry for Solomon SSD130x OLED displays DRM driver
  dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer

 .../bindings/display/solomon,ssd1307fb.yaml   |   1 +
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/Kconfig                       |   2 +
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/drm_format_helper.c           | 141 ++-
 drivers/gpu/drm/solomon/Kconfig               |  21 +
 drivers/gpu/drm/solomon/Makefile              |   2 +
 drivers/gpu/drm/solomon/ssd130x-i2c.c         | 116 +++
 drivers/gpu/drm/solomon/ssd130x.c             | 843 ++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.h             |  76 ++
 include/drm/drm_format_helper.h               |   4 +
 11 files changed, 1202 insertions(+), 12 deletions(-)
 create mode 100644 drivers/gpu/drm/solomon/Kconfig
 create mode 100644 drivers/gpu/drm/solomon/Makefile
 create mode 100644 drivers/gpu/drm/solomon/ssd130x-i2c.c
 create mode 100644 drivers/gpu/drm/solomon/ssd130x.c
 create mode 100644 drivers/gpu/drm/solomon/ssd130x.h

-- 
2.34.1

[PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

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 v6:
- Just return regmap_bulk_write() in ssd130x_write_data() (Andy Shevchenko)
- Remove unnecessary cast in ssd130x_write_cmd() (Andy Shevchenko)
- Return ERR_PTR(dev_err_probe(...)) everywhere in probe (Andy Shevchenko)

Changes in v5:
- Include <linux/bits.h> header (Andy Shevchenko)
- Drop parenthesis for command options macros (Andy Shevchenko)
- Explain in ssd130x_write_cmd() comment how commands are sent (Andy Shevchenko)
- The pwm_*() functions check for NULL already (Andy Shevchenko)
- Remove unnecesary blank line (Andy Shevchenko)
- Simplify error handling for backlight registration failure (Geert Uytterhoeven)
- Don't clear screen on enable, instead send the full buffer (Thomas Zimmermann)

Changes in v4:
- Rename vbat supply to vcc since is how's labeled in the device (Mark Brown)
- Don't make the regulator option since is always needed (Mark Brown)
- Add solomon Kconfig source and directory inclusion sorted (Andy Shevchenko)
- Use SSD130x instead of SSD130X to denote is not a model name (Andy Shevchenko)
- Check if there's a reset pin in the callee and not the caller (Andy Shevchenko)
- Define missing commands instead of using magic numbers (Andy Shevchenko)
- Use GENMASK() and FIELD_PREP() macros when possible (Andy Shevchenko)
- Avoid using ternary operators to ease code readablity (Andy Shevchenko)
- Use i++ instead of --i on some for loops (Andy Shevchenko)
- Remove redundant blank lines (Andy Shevchenko)
- Rename power_off label to out_power_off (Andy Shevchenko)
- Use dev_err_probe() even if no -EPROBE_DEFER (Andy Shevchenko)
- Don't use plural Authors if there's only one (Andy Shevchenko)

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 | 843 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/solomon/ssd130x.h |  76 +++
 6 files changed, 935 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

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dfdd3ec5f793..763355330b17 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -403,6 +403,8 @@ source "drivers/gpu/drm/xlnx/Kconfig"
 
 source "drivers/gpu/drm/gud/Kconfig"
 
+source "drivers/gpu/drm/solomon/Kconfig"
+
 source "drivers/gpu/drm/sprd/Kconfig"
 
 config DRM_HYPERV
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 8675c2af7ae1..c2ef5f9fce54 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -132,4 +132,5 @@ obj-$(CONFIG_DRM_TIDSS) += tidss/
 obj-y			+= xlnx/
 obj-y			+= gud/
 obj-$(CONFIG_DRM_HYPERV) += hyperv/
+obj-y			+= solomon/
 obj-$(CONFIG_DRM_SPRD) += sprd/
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
new file mode 100644
index 000000000000..7720a7039e8d
--- /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..2fcda1972e1e
--- /dev/null
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -0,0 +1,843 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Solomon SSD130x OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ * Author: Javier Martinez Canillas <javierm@redhat.com>
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ */
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/bits.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_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_COM_SCAN_DIR		0xc0
+#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 SSD130X_SET_COM_SCAN_DIR_MASK		GENMASK(3, 2)
+#define SSD130X_SET_COM_SCAN_DIR_SET(val)	FIELD_PREP(SSD130X_SET_COM_SCAN_DIR_MASK, (val))
+#define SSD130X_SET_CLOCK_DIV_MASK		GENMASK(3, 0)
+#define SSD130X_SET_CLOCK_DIV_SET(val)		FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))
+#define SSD130X_SET_CLOCK_FREQ_MASK		GENMASK(7, 4)
+#define SSD130X_SET_CLOCK_FREQ_SET(val)		FIELD_PREP(SSD130X_SET_CLOCK_FREQ_MASK, (val))
+#define SSD130X_SET_PRECHARGE_PERIOD1_MASK	GENMASK(3, 0)
+#define SSD130X_SET_PRECHARGE_PERIOD1_SET(val)	FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD1_MASK, (val))
+#define SSD130X_SET_PRECHARGE_PERIOD2_MASK	GENMASK(7, 4)
+#define SSD130X_SET_PRECHARGE_PERIOD2_SET(val)	FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))
+#define SSD130X_SET_COM_PINS_CONFIG1_MASK	GENMASK(4, 4)
+#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))
+#define SSD130X_SET_COM_PINS_CONFIG2_MASK	GENMASK(5, 5)
+#define SSD130X_SET_COM_PINS_CONFIG2_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))
+
+#define SSD130X_SET_ADDRESS_MODE_HORIZONTAL	0x00
+#define SSD130X_SET_ADDRESS_MODE_VERTICAL	0x01
+#define SSD130X_SET_ADDRESS_MODE_PAGE		0x02
+
+#define SSD130X_SET_AREA_COLOR_MODE_ENABLE	0x1e
+#define SSD130X_SET_AREA_COLOR_MODE_LOW_POWER	0x05
+
+#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)
+{
+	return regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
+}
+
+/*
+ * Helper to write command (SSD130X_COMMAND). The fist variadic argument
+ * is the command to write and the following are the command options.
+ *
+ * Note that the ssd130x protocol requires each command and option to be
+ * written as a SSD130X_COMMAND device register value. That is why a call
+ * to regmap_write(..., SSD130X_COMMAND, ...) is done for each argument.
+ */
+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, 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)
+{
+	if (!ssd130x->reset)
+		return;
+
+	/* 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;
+
+	ssd130x_reset(ssd130x);
+
+	ret = regulator_enable(ssd130x->vcc_reg);
+	if (ret) {
+		dev_err(dev, "Failed to enable VCC: %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);
+			regulator_disable(ssd130x->vcc_reg);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_power_off(struct ssd130x_device *ssd130x)
+{
+	pwm_disable(ssd130x->pwm);
+	pwm_put(ssd130x->pwm);
+
+	regulator_disable(ssd130x->vcc_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 = (SSD130X_SET_COM_SCAN_DIR |
+		      SSD130X_SET_COM_SCAN_DIR_SET(ssd130x->com_invdir));
+	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_SET_CLOCK_DIV_SET(ssd130x->dclk_div - 1) |
+		SSD130X_SET_CLOCK_FREQ_SET(ssd130x->dclk_frq));
+	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 = 0;
+
+		if (ssd130x->area_color_enable)
+			mode |= SSD130X_SET_AREA_COLOR_MODE_ENABLE;
+
+		if (ssd130x->low_power)
+			mode |= SSD130X_SET_AREA_COLOR_MODE_LOW_POWER;
+
+		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_SET_PRECHARGE_PERIOD1_SET(ssd130x->prechargep1) |
+		     SSD130X_SET_PRECHARGE_PERIOD1_SET(ssd130x->prechargep2));
+	ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_PRECHARGE_PERIOD, precharge);
+	if (ret < 0)
+		return ret;
+
+	/* Set COM pins configuration */
+	compins = BIT(1);
+	compins |= (SSD130X_SET_COM_PINS_CONFIG1_SET(ssd130x->com_seq) |
+		    SSD130X_SET_COM_PINS_CONFIG2_SET(ssd130x->com_lrremap));
+	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);
+
+	if (ssd130x->device_info->need_chargepump)
+		chargepump |= BIT(2);
+
+	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_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_device *drm = &ssd130x->drm;
+	int idx, ret;
+
+	ret = ssd130x_power_on(ssd130x);
+	if (ret)
+		return;
+
+	ret = ssd130x_init(ssd130x);
+	if (ret)
+		goto out_power_off;
+
+	if (!drm_dev_enter(drm, &idx))
+		goto out_power_off;
+
+	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst);
+
+	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
+
+	backlight_enable(ssd130x->bl_dev);
+
+	drm_dev_exit(idx);
+
+	return;
+out_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;
+
+	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->vcc_reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(ssd130x->vcc_reg))
+		return dev_err_probe(dev, PTR_ERR(ssd130x->vcc_reg),
+				     "Failed to get VCC 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))
+		return ERR_PTR(dev_err_probe(dev, PTR_ERR(ssd130x),
+					     "Failed to allocate DRM device\n"));
+
+	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))
+		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
+					     "Unable to register backlight device\n"));
+
+	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)
+		return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
+
+	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..cd21cdccb566
--- /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.
+ * Author: 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 *vcc_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__ */
-- 
2.34.1

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Mon, Feb 14, 2022 at 02:37:07PM +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.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
<javierm@redhat.com> 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.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for your patch, which is now commit a61732e808672cfa ("drm:
Add driver for Solomon SSD130x OLED displays") in drm/drm-next

Sorry for the delay, but finally I gave it a try on my Adafruit
FeatherWing 128x32 OLED.
Some of the weird issues (cursor disappears after printing some text,
more text also doesn't appear until I clear the display) are still there.
Unfortunately a regression was introduced since your v3: printed
text is mirrored upside-down. I.e. "E" is rendered correctly, but "L"
turns into "Γ" (Greek Gamma).
I suspect something went wrong with the display initialization
sequence.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

On 3/8/22 17:30, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
> <javierm@redhat.com> 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.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Thanks for your patch, which is now commit a61732e808672cfa ("drm:
> Add driver for Solomon SSD130x OLED displays") in drm/drm-next
> 
> Sorry for the delay, but finally I gave it a try on my Adafruit
> FeatherWing 128x32 OLED.
> Some of the weird issues (cursor disappears after printing some text,
> more text also doesn't appear until I clear the display) are still there.

I see. Thought that I tested using it as a console and it did work
correctly for me. I'll do more tests again.

> Unfortunately a regression was introduced since your v3: printed
> text is mirrored upside-down. I.e. "E" is rendered correctly, but "L"
> turns into "Γ" (Greek Gamma).
> I suspect something went wrong with the display initialization
> sequence.
>

Could you please try Chen-Yu's fix for the COM scan direction mask ?

https://lists.freedesktop.org/archives/dri-devel/2022-March/345915.html

I made a mistake when converting to use the GENMASK() and FIELD_PREP()
macros in v4 as suggested by Andy. The SSD130X_SET_COM_SCAN_DIR_MASK
wasn't correct which would explain the output to be vertically flipped.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Wed, Mar 9, 2022 at 1:14 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 3/8/22 17:30, Geert Uytterhoeven wrote:
> > Unfortunately a regression was introduced since your v3: printed
> > text is mirrored upside-down. I.e. "E" is rendered correctly, but "L"
> > turns into "Γ" (Greek Gamma).
> > I suspect something went wrong with the display initialization
> > sequence.
> >
>
> Could you please try Chen-Yu's fix for the COM scan direction mask ?
>
> https://lists.freedesktop.org/archives/dri-devel/2022-March/345915.html
>
> I made a mistake when converting to use the GENMASK() and FIELD_PREP()
> macros in v4 as suggested by Andy. The SSD130X_SET_COM_SCAN_DIR_MASK
> wasn't correct which would explain the output to be vertically flipped.

Thanks, confirmed fixed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 3/9/22 13:56, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Wed, Mar 9, 2022 at 1:14 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> On 3/8/22 17:30, Geert Uytterhoeven wrote:
>>> Unfortunately a regression was introduced since your v3: printed
>>> text is mirrored upside-down. I.e. "E" is rendered correctly, but "L"
>>> turns into "Γ" (Greek Gamma).
>>> I suspect something went wrong with the display initialization
>>> sequence.
>>>
>>
>> Could you please try Chen-Yu's fix for the COM scan direction mask ?
>>
>> https://lists.freedesktop.org/archives/dri-devel/2022-March/345915.html
>>
>> I made a mistake when converting to use the GENMASK() and FIELD_PREP()
>> macros in v4 as suggested by Andy. The SSD130X_SET_COM_SCAN_DIR_MASK
>> wasn't correct which would explain the output to be vertically flipped.
> 
> Thanks, confirmed fixed.
> 

Great, thanks a lot for testing and the confirmation!

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
<javierm@redhat.com> 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.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

> +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];

As buf does not point to (0, 0), the above is not correct if rect.x1 !=
0 or rect.y1 != 0.  After fixing that, writing more than one text line
to the console works, but I still see an issue with updates where the
rectangle size and/or position are not aligned to 8 pixels horizontally.
Will do more investigation, and send fixes...

> +                               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);

This buffer is larger than needed. Will send a fix.

> +       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);

This buffer is much larger than needed. Will send a fix.

> +       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;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 3/9/22 21:04, Geert Uytterhoeven wrote:

[snip]

>> +
>> +               /* 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];
> 
> As buf does not point to (0, 0), the above is not correct if rect.x1 !=
> 0 or rect.y1 != 0.  After fixing that, writing more than one text line
> to the console works, but I still see an issue with updates where the
> rectangle size and/or position are not aligned to 8 pixels horizontally.
> Will do more investigation, and send fixes...
>

Right, I believe this is a consequence of introducing shadow buffers at
some point and not adjusting the logic in this function.

Thanks a lot for tracking down the issues and working on fixes for them!
 -- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat