Hello Javier,
On 5/25/2022 21:46, Javier Martinez Canillas wrote:
quoted
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?
quoted
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.
quoted
# 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.
quoted
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:
...
quoted
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
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:
quoted
On 5/25/2022 21:46, Javier Martinez Canillas wrote:
...
quoted
quoted
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
Hello Dominik,
On 6/13/22 13:39, Dominik Kierner wrote:
Removed the regmap part since Andy already commented and I agree with him.
quoted
quoted
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?:
quoted
* 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.
quoted
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:
quoted
* 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.
quoted
quoted
# 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.
quoted
quoted
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