Re: [PATCH v4 3/5] drm: add SimpleDRM driver

From: Noralf TrÃnnes
Date: Thu Aug 25 2016 - 14:46:25 EST

Den 25.08.2016 15:09, skrev Rob Herring:
On Mon, Aug 22, 2016 at 3:25 PM, Noralf TrÃnnes <noralf@xxxxxxxxxxx> wrote:
The SimpleDRM driver binds to simple-framebuffer devices and provides a
DRM/KMS API. It provides only a single CRTC+encoder+connector combination
plus one initial mode.

Userspace can create dumb-buffers which can be blit into the real
framebuffer similar to UDL. No access to the real framebuffer is allowed
(compared to earlier version of this driver) to avoid security issues.
Furthermore, this way we can support arbitrary modes as long as we have a

The driver was originally written by David Herrmann in 2014.
My main contribution is to make use of drm_simple_kms_helper and
rework the probe path to avoid use of the deprecated drm_platform_init()
and drm_driver.{load,unload}().
Additions have also been made for later changes to the Device Tree binding
document, like support for clocks, regulators and having the node under
/chosen. This code was lifted verbatim from simplefb.c.

Cc: dh.herrmann@xxxxxxxxx
Cc: libv@xxxxxxxxx
Signed-off-by: Noralf TrÃnnes <noralf@xxxxxxxxxxx>

+ /* Count the number of regulator supplies */
+ for_each_property_of_node(np, prop) {
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (p && p != prop->name)
+ count++;
+ }
The regulator API should have functions for this rather than open coding it.

I couldn't find anything that matches this usecase. There are regulator
bulk functions, but they require the names to be known in advance.
And I don't know how many regulators there are nor their names.

From Documentation/devicetree/bindings/display/simple-framebuffer.txt:

- *-supply : Any number of regulators used by the framebuffer. These should
be named according to the names in the device's design.

+ if (!count)
+ return 0;
+ sdrm->regulators = devm_kcalloc(&pdev->dev, count,
+ sizeof(struct regulator *),
+ if (!sdrm->regulators)
+ return -ENOMEM;
+ /* Get all the regulators */
+ for_each_property_of_node(np, prop) {
+ char name[32]; /* 32 is max size of property name */
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (!p || p == prop->name)
+ continue;
This too.

+ strlcpy(name, prop->name,
+ strlen(prop->name) - strlen(SUPPLY_SUFFIX) + 1);
+ regulator = devm_regulator_get_optional(&pdev->dev, name);

+ if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
+ struct device_node *np;
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
Rather than exporting of_chosen, this whole chunk can be replaced with
a of_find_compatible_node call. Yes, that would match if
simple-framebuffer exists somewhere else in the DT, but it is not the
kernel's job to do DT validation.

This seems to do the job:

* The binding doc states that simplefb nodes should be sub-nodes of
* chosen and that older devicetree files can have them in a different
* place. of_platform_device_create() just returns NULL if a device
* has already been created for the node.
for_each_compatible_node(np, NULL, "simple-framebuffer")
of_platform_device_create(np, NULL, NULL);


+ of_platform_device_create(np, NULL, NULL);
+ }
+ }