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-ssd13...
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
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.
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.
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#L206...
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.
dri-devel@lists.freedesktop.org
-
andriy.shevchenko＠linux.intel.com -
Dominik Kierner -
Javier Martinez Canillas