Message-ID: <7a78d57342754a5d9bd3ce7c7bf3fa47@dh-electronics.com>
Date:   Mon, 13 Jun 2022 11:39:30 +0000
From:   Dominik Kierner <dkierner@...electronics.com>
To:     Javier Martinez Canillas <javierm@...hat.com>
CC:     "airlied@...ux.ie" <airlied@...ux.ie>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "daniel.vetter@...ll.ch" <daniel.vetter@...ll.ch>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "maxime@...no.tech" <maxime@...no.tech>,
        "noralf@...nnes.org" <noralf@...nnes.org>,
        "sam@...nborg.org" <sam@...nborg.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "tzimmermann@...e.de" <tzimmermann@...e.de>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

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

Powered by blists - more mailing lists