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