Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overlay for Adafruit 1.8" TFT LCD on SPI1 with tinydrm st7735r #68

Merged
merged 3 commits into from
Feb 23, 2018

Conversation

pdp7
Copy link

@pdp7 pdp7 commented Feb 22, 2018

Summary

Testing

modetest -M "st7735r" -c #this will display connector id
modetest -M "st7735r" -s 28:128x160 #connector id and resolution

Resources:

This file was copied from src/arm/BB-SPIDEV1-00A0.dts and modified
by Drew Fustini based on an exmample from David Lechner.

This Adafruit 1.8" TFT LCD should be connected to SPI1 bus:
https://www.adafruit.com/products/358

This overlay will load the inydrm st7735r driver by David Lechner:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/tinydrm/st7735r.c

Tested with 4.16.0-rc2-bone3 kernel on Debian 9.3 image

Run libdrm modetest for colorbar test based on instructions from:
https://github.com/notro/tinydrm/wiki/Development#modetest

modetest -M "st7735r" -c #this will display connector id
modetest -M "st7735r" -s 28:128x160 #connector id and resolution

Mailing list post with more information:
https://groups.google.com/d/msg/beagleboard/GuMQIP_XCW0/b3lxbx_8AwAJ

GitHub Gist with testing results:
https://gist.github.com/pdp7/aee5664598059c9b9a9020f107957f80

Discussion with notro on how to test tinydrm driver:
notro/tinydrm#1 (comment)

Signed-off-by: Drew Fustini <drew@pdp7.com>
@pdp7
Copy link
Author

pdp7 commented Feb 22, 2018

@RobertCNelson @dlech @notro I would appreciate any feedback on this device tree overlay for Adafruit 1.8" TFT LCD with tinydrm st7735r

@dlech
Copy link

dlech commented Feb 22, 2018

It would be useful to add some info on how to set up a backlight too, at least in the comments. You could give an example for both compatible = "gpio-backlight" and compatible="pwm-backlight".

@RobertCNelson
Copy link
Member

@pdp7 looks pretty good, replace this:

+	compatible = "ti,beaglebone", "ti,beaglebone-black", "ti,beaglebone-green";
+
+	/* identification */
+	part-number = "BB-LCD-ADAFRUIT-18-SPI1";
+	version = "00A0";
+
+	/* state the resources this cape uses */
+	exclusive-use =
+		/* the pin header uses */
+		"P9.31",	/* spi1_sclk */
+		"P9.29",	/* spi1_d0 */
+		"P9.30",	/* spi1_d1 */
+		"P9.28",	/* spi1_cs0 */
+		// "P9.42",	/* spi1_cs1 */
+		/* the hardware ip uses */
+		"spi1";

Which actually isn't utilized, with this:

	/*
	 * Free up the pins used by the cape from the pinmux helpers.
	 */
	fragment@0 {
		target = <&ocp>;
		__overlay__ {
			P9_28_pinmux { status = "disabled"; };	/* spi1_cs0 */
			P9_29_pinmux { status = "disabled"; };	/* spi1_d0 */
			P9_30_pinmux { status = "disabled"; };	/* spi1_d1 */
			P9_31_pinmux { status = "disabled"; };	/* spi1_sclk */
		};
	};

That's needed for the rewrite.. (you'll have to increment the fragment #'s)
You should be able to drop:

ti,pio-mode; /* disable dma when used as an overlay, dma gets stuck at 160 bits... */

it's not used in the kernel anymore..
Regards,

@RobertCNelson
Copy link
Member

RobertCNelson commented Feb 22, 2018

and make sure to declare these too:

+ dc-gpios = <&gpio1 16 0>;    /* dc:48    P9.15 GPIO1_16 */
+ reset-gpios = <&gpio1 28 0>; /* reset:60 P9.12 GPIO1_28 */

as

	P9_12_pinmux { status = "disabled"; };	/* reset:60 P9.12 GPIO1_28 */
	P9_15_pinmux { status = "disabled"; };	/* dc:48    P9.15 GPIO1_16 */

and create a pinmux node, otherwise they'll get stolen by gpio-of-helper/cape-universal for userspace access with the rewrite i'm working on..

pdp7 added 2 commits February 23, 2018 00:20
Add a fragment for ocp overlay and declare use of spi1, dc, and reset pins.

Based on feedback from @RobertNelson in my pull request #68:
#68 (comment)

Signed-off-by: Drew Fustini <drew@pdp7.com>
Signed-off-by: Drew Fustini <drew@pdp7.com>
@RobertCNelson RobertCNelson merged commit eadf11d into beagleboard:master Feb 23, 2018
RobertCNelson pushed a commit that referenced this pull request Feb 23, 2018
Add a fragment for ocp overlay and declare use of spi1, dc, and reset pins.

Based on feedback from @RobertNelson in my pull request #68:
#68 (comment)

Signed-off-by: Drew Fustini <drew@pdp7.com>
@RobertCNelson
Copy link
Member

and merged! thanks @pdp7 !!!

@pdp7
Copy link
Author

pdp7 commented Feb 23, 2018

@dlech @RobertCNelson I tried to configure a gpio backlight per David's suggestion, but it seems I haven't gotten it quite right.

I added this to the dts file:

	adafruitbacklight: adafruitbacklight {
                // P9.23 <--> lite (gpio-backlight GPIO_49 gpio1[17])
		compatible = "gpio-backlight";
		gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
	};

and backlight = &adafruitbacklight; inside display@0

Here is the full dts file: BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

However, the BeagleBone fails to boot. From the serial console:

loading /boot/vmlinuz-4.16.0-rc2-bone3 ...
10260992 bytes read in 783 ms (12.5 MiB/s)
uboot_overlays: [uboot_base_dtb=am335x-boneblack-uboot.dtb] ...
uboot_overlays: Switching too: dtb=am335x-boneblack-uboot.dtb ...
loading /boot/dtbs/4.16.0-rc2-bone3/am335x-boneblack-uboot.dtb ...
54186 bytes read in 83 ms (636.7 KiB/s)
uboot_overlays: [fdt_buffer=0x60000] ...
uboot_overlays: loading /lib/firmware/BB-LCD-ADAFRUIT-18-SPI1-00A0.dtbo ...
1717 bytes read in 1455 ms (1000 Bytes/s)
failed on fdt_overlay_apply(): FDT_ERR_BADOVERLAY

Any ideas as to what I am doing wrong? Thanks!

@dlech
Copy link

dlech commented Feb 23, 2018

backlight = &adafruitbacklight;

You need angle braces.

backlight = <&adafruitbacklight>;

@pdp7
Copy link
Author

pdp7 commented Feb 24, 2018

@dlech thanks. I made that change in syntax. The boot still failed with the same error. I looked at some of the other overlay files in the repo, and realized I probably needed to put the definition adafruitblacklight inside a fragment. I added:

	fragment@0 {
		target-path = "/";
		__overlay__ {
			adafruitbacklight: adafruitbacklight {
				// P9.23 <--> lite (gpio-backlight GPIO_49 gpio1[17])
				compatible = "gpio-backlight";
				gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
			};
		};
	};

And the BeagleBone then booted OK, and LCD correctly displays the console and the blacklight turns on.

Here is the full dts file: BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

Any additional feedback?

Thanks for you help @dlech!

@pdp7
Copy link
Author

pdp7 commented Feb 25, 2018

@dlech @RobertCNelson I'm looking into how to add PWM backlight to this overlay.

Is this a good example from BB-BONE-4D4C-01-00A1.dts?

        fragment@1 {
                target = <&am33xx_pinmux>;
                __overlay__ {

                        bb_lcd_pwm_backlight_pins: pinmux_bb_lcd_pwm_backlight_pins {
                                pinctrl-single,pins = <
                                        BONE_P9_14 0x06 /* gpmc_a2.ehrpwm1a, OMAP_MUX_MODE6 | AM33XX_PIN_OUTPUT */
                                >;
                        };
        fragment@6 {
                target-path="/";
                __overlay__ {

                        /* avoid stupid warning */
                        #address-cells = <1>;
                        #size-cells = <1>;

                        backlight {
                                status = "okay";
                                compatible = "pwm-backlight";
                                pwms = <&ehrpwm1 0 500000 0>;
                                brightness-levels = <
                                        0  1  2  3  4  5  6  7  8  9
                                        10 11 12 13 14 15 16 17 18 19
                                        20 21 22 23 24 25 26 27 28 29
                                        30 31 32 33 34 35 36 37 38 39
                                        40 41 42 43 44 45 46 47 48 49
                                        50 51 52 53 54 55 56 57 58 59
                                        60 61 62 63 64 65 66 67 68 69
                                        70 71 72 73 74 75 76 77 78 79
                                        80 81 82 83 84 85 86 87 88 89
                                        90 91 92 93 94 95 96 97 98 99
                                        100
                                >;
                                default-brightness-level = <100>;
                        };

@pdp7
Copy link
Author

pdp7 commented Feb 25, 2018

I have tried adding pwm backlight. Here is the updated dts:
BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

However, I get this at bootup:

[    0.857793] pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator
[    1.184409] pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator
[    1.209977] pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator
[   24.512675] pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator

I connected the LCD lite pin to P9.14 but there is no backlight.

@pdp7
Copy link
Author

pdp7 commented Feb 25, 2018

This is what the output looks like when cape universal is active and set PWM output to P9_14. BB-LCD-ADAFRUIT-18-SPI1-00A0.dts is not loaded during this test.

open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC) = 4
fstat64(4, {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
getdents64(4, /* 3 entries */, 32768)   = 80
getdents64(4, /* 0 entries */, 32768)   = 0
close(4)                                = 0
stat64("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm0", 0xbec67e28) = -1 ENOENT (No such file or directory)
open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/export", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 4
fstat64(4, {st_mode=S_IFREG|0660, st_size=4096, ...}) = 0
write(4, "0", 1)                        = -1 EBUSY (Device or resource busy)
close(4)                                = 0
nanosleep({tv_sec=0, tv_nsec=100000000}, NULL) = 0
stat64("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm0", 0xbec67e28) = -1 ENOENT (No such file or directory)
stat64("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm-3:0", {st_mode=S_IFDIR|0775, st_size=0, ...}) = 0
open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm-3:0/period", O_RDWR) = 4
open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm-3:0/duty_cycle", O_RDWR) = 5
open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm-3:0/polarity", O_RDWR) = 6
open("/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip3/pwm-3:0/enable", O_RDWR) = 7
lseek(4, 0, SEEK_SET)                   = 0
read(4, "500000\n", 20)                 = 7
lseek(5, 0, SEEK_SET)                   = 0
read(5, "127500\n", 20)                 = 7
lseek(5, 0, SEEK_SET)                   = 0
write(5, "500000", 6)                   = 6
lseek(7, 0, SEEK_SET)                   = 0
read(7, "0", 1)                         = 1
lseek(6, 0, SEEK_SET)                   = 0
write(6, "normal", 6)                   = 6
lseek(7, 0, SEEK_SET)                   = 0
write(7, "1", 1)                        = 1
lseek(7, 0, SEEK_SET)                   = 0
write(7, "1", 1)                        = 1

@dlech
Copy link

dlech commented Feb 25, 2018

pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator

This is normal. The reason the message shows more than once is probably because the driver probe is being deferred because some other device is not ready yet.

pwms = <&ehrpwm1 0 500000 0>;

I've noticed that setting a duty cycle of 0 in DT tends to cause the PWM to not be initialized correctly. Try this:

pwms = <&ehrpwm1 1 500000 0>;

Also, you are missing the pinctrl properties on the backlight node.

                    backlight {
                            pinctrl-names = "default";
                            pinctrl-0 = <&bb_lcd_pwm_backlight_pins>;
                            status = "okay";
                            ...

After you get it working, you may want to play with the brightness levels. 5 or 10 levels is plenty for me and usually, it needs to be non-linear so that the apparent brightness looks linear.

@pdp7
Copy link
Author

pdp7 commented Feb 26, 2018

@dlech Thanks for the suggestions.

Here is the dts file for the overlay that I am using:
BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

I added some print statements in tinydrm code. tinydrm_of_find_backlight() in drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c fails to find the backlight. The call to of_find_backlight_by_node(np) returns null so backlight is set to null.

dmesg when overlay has backlight = <&backlight_pwm>:

[   20.847735] tinydrm/st7735r: st7735r_probe(): begin
[   21.129420] tinydrm/st7735r: st7735r_probe(): call devm_gpiod_get(reset)
[   21.444411] tinydrm/st7735r: st7735r_probe(): call devm_gpiod_get(dc)
[   21.748351] tinydrm/st7735r: st7735r_probe(): call tinydrm_of_find_backlight()
[   21.755682] core/tinydrm-helpers: call tinydrm_of_find_backlight()
[   21.761940] core/tinydrm-helpers: np=f2d1668b
[   21.766332] core/tinydrm-helpers: call of_find_backlight_by_node()
[   21.772557] core/tinydrm-helpers: backlight=  (null)
[   21.777552] core/tinydrm-helpers: call of_node_put(np)
[   22.944586] random: crng init done

dmesg when overlay has backlight = <&backlight_gpio>:

[   20.944650] tinydrm/st7735r: st7735r_probe(): begin
[   21.163941] tinydrm/st7735r: st7735r_probe(): call devm_gpiod_get(reset)
[   21.401606] tinydrm/st7735r: st7735r_probe(): call devm_gpiod_get(dc)
[   21.696296] tinydrm/st7735r: st7735r_probe(): call tinydrm_of_find_backlight()
[   21.703615] core/tinydrm-helpers: call tinydrm_of_find_backlight()
[   21.709880] core/tinydrm-helpers: np=8d4da18e
[   21.714268] core/tinydrm-helpers: call of_find_backlight_by_node()
[   21.720495] core/tinydrm-helpers: backlight=f48f14ac
[   21.725497] core/tinydrm-helpers: call of_node_put(np)
[   21.730675] tinydrm/st7735r: st7735r_probe(): call mipi_dbi_spi_init()
[   21.737248] tinydrm/st7735r: st7735r_probe(): call mipi_dbi_init()
[   23.341384] tinydrm/st7735r: jd_t18003_t01_pipe_enable(): begin
[   23.341390] tinydrm/st7735r: jd_t18003_t01_pipe_enable(): call mipi_dbi_hw_reset()
[   23.500199] tinydrm/st7735r: jd_t18003_t01_pipe_enable(): call mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET)
[   23.664233] tinydrm/st7735r: jd_t18003_t01_pipe_enable(): call mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE)
[   24.332223] tinydrm/st7735r: jd_t18003_t01_pipe_enable(): call mipi_dbi_pipe_enable(pipe, crtc_state)
[   24.356898] core/tinydrm-helpers: tinydrm_enable_backlight(): backlight=f48f14ac
[   24.356907] core/tinydrm-helpers: tinydrm_enable_backlight(): backlight->props.state=0x0
[   24.356911] core/tinydrm-helpers: tinydrm_enable_backlight(): Backlight state: 0x0 -> 0x0
[   24.356915] core/tinydrm-helpers: tinydrm_enable_backlight(): call backlight_update_status()
[   24.356926] core/tinydrm-helpers: tinydrm_enable_backlight(): ret=0

Any ideas how I can make tinydrm_of_find_backlight() find the pwm backlight?

@dlech
Copy link

dlech commented Feb 27, 2018

It looks like you have pinctrl-0 = <&backlight_pwm_pins>; on both the PWM backlight node and the EHRPWM node. This could be why the PWM backlight is not working. Try removing the pinctrl-* properties from fragment@3.

It looks to me like the GPIO backlight is working?

@dlech
Copy link

dlech commented Feb 27, 2018

Also, the GPIO backlight node doesn't have pinctrl-*, so it might not be muxing the GPIO correctly. Maybe that could be why that one does not appear to be working?

@pdp7
Copy link
Author

pdp7 commented Feb 27, 2018

@dlech GPIO backlight control does work OK.

I made the changes you suggested for PWM backlight:
BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

I added some printk() debugging output to drivers/video/backlight/pwm_bl.c. I discovered that pwm_backlight_probe() fails. I can determine that the code reaches line 328:

        pb->pwm = devm_pwm_get(&pdev->dev, NULL);
        if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
                dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
                pb->legacy = true;
                pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
        }
        if (IS_ERR(pb->pwm)) {
                ret = PTR_ERR(pb->pwm);  /******** LINE 328 ********/
                if (ret != -EPROBE_DEFER)
                        dev_err(&pdev->dev, "unable to request PWM\n");
                goto err_alloc;
        }

Any idea how I could troubleshoot further? Thanks!

@dlech
Copy link

dlech commented Feb 28, 2018

I think I mislead you earlier. I forgot there are 2 PWMs per EHRPWM, so the first parameter of the phandle is the index.

pwms = <&ehrpwm1 1 500000 0>;

should be

pwms = <&ehrpwm1 0 500000 0>;

if you are using EHRPWM1A

(500000 is the value that should be non-zero, so it is OK)

@pdp7
Copy link
Author

pdp7 commented Feb 28, 2018

@dlech Thanks, I made that change to the overlay but the results are still the same.

It seems the issue is still that this call fails in the probe:
pb->pwm = devm_pwm_get(&pdev->dev, NULL);

I'll have to dig deeper to try to learn why it fails.

@dlech
Copy link

dlech commented Feb 28, 2018

What is the error code that is returned?

@pdp7
Copy link
Author

pdp7 commented Mar 4, 2018

@dlech Thanks. I've added more print statements and devm_pwm_get(&pdev->dev, NULL) returns -EPROBE_DEFER:

[   21.897975] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): -EPROBE_DEFER=-517
[   21.906127] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): pb->pwm=a6da1f93
[   21.914030] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): node=2cd548a6
[   21.921658] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): if (IS_ERR(pb->pwm)): ret=-517

Here is the complete output related to the display and backlight:

debian@beaglebone:~$ dmesg |grep -E '(pwm|tinydrm|st7735|backlight|class)'
[    0.291482] drivers/video/backlight/backlight.c: backlight_class_init(): class_create(THIS_MODULE, "backlight"): backlight_class=a3996fa7
[   15.748479] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): pdev=d66e4744
[   15.756147] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): data=  (null)
[   15.763794] drivers/video/backlight/pwm_bl.c: pwm_backlight_parse_dt()
[   16.849221] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): pwm_backlight_parse_dt(&pdev->dev, &defdata): ret=0
[   16.860235] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): data->init=  (null)
[   17.702234] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): devm_kzalloc(): pb=fcb9c237
[   17.711178] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): data->levels=eb4d7be8
[   18.410006] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): devm_gpiod_get_optional(): pb->enable_gpio=  (null)
[   18.421059] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): &pdev->dev=58bee375
[   18.429224] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): devm_regulator_get(&pdev->dev, power)
[   19.385753] pwm-backlight backlight_pwm: backlight_pwm supply power not found, using dummy regulator
[   19.385883] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): pb->power_supply=7db6d316
[   19.394660] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): devm_pwm_get(&pdev->dev, NULL)
[   19.987089] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): -EPROBE_DEFER=-517
[   19.995225] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): pb->pwm=36d38477
[   20.003127] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): node=e6420cf0
[   20.010777] drivers/video/backlight/pwm_bl.c: pwm_backlight_probe(): if (IS_ERR(pb->pwm)): ret=-517
[   21.148383] tinydrm/st7735r: st7735r_probe(): begin
[   21.352430] tinydrm/st7735r: st7735r_probe(): devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL): mipi=aaf31902
[   21.727869] tinydrm/st7735r: st7735r_probe(): devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH): mipi->reset=8e1a8fab
[   22.172448] tinydrm/st7735r: st7735r_probe(): devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW): dc=5033d86a
[   22.181595] tinydrm/st7735r: st7735r_probe(): call tinydrm_of_find_backlight(dev=e7efaf3d)
[   22.189939] core/tinydrm-helpers: tinydrm_of_find_backlight(): dev=e7efaf3d
[   22.196952] core/tinydrm-helpers: tinydrm_of_find_backlight(): dev->of_node=5a2538eb
[   22.204768] core/tinydrm-helpers: tinydrm_of_find_backlight(): of_parse_phandle(dev->of_node, "backlight", 0) np=e6420cf0
[   22.215786] core/tinydrm-helpers: tinydrm_of_find_backlight(): call of_find_backlight_by_node()
[   22.224533] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): node=e6420cf0
[   22.232936] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): backlight_class=a3996fa7
[   22.242296] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): backlight_class->name=backlight
[   22.252294] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): call class_find_device(): backlight_class=a3996fa7
[   22.263920] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): call class_find_device(): node=e6420cf0
[   22.274585] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): call class_find_device(): of_parent_match=a1fa7d0e
[   22.286209] KERNEL/drivers/base/class.c: class_find_device(): class=a3996fa7
[   22.293298] KERNEL/drivers/base/class.c: class_find_device(): start=  (null)
[   22.300387] KERNEL/drivers/base/class.c: class_find_device(): data=e6420cf0
[   22.307386] KERNEL/drivers/base/class.c: class_find_device(): match=a1fa7d0e
[   22.314472] KERNEL/drivers/base/class.c: class_find_device(): class->name=backlight
[   22.322176] KERNEL/drivers/base/class.c: class_find_device(): &iter=992aba59
[   22.329268] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): dev=  (null)
[   22.337587] drivers/video/backlight/backlight.c: of_find_backlight_by_node(): return ret=0
[   22.345897] core/tinydrm-helpers: tinydrm_of_find_backlight(): of_find_backlight_by_node() returned backlight=  (null)
[   22.356647] core/tinydrm-helpers: tinydrm_of_find_backlight(): call of_node_put(np)
[   22.364345] core/tinydrm-helpers: tinydrm_of_find_backlight(): if (!backlight): return ERR_PTR(-EPROBE_DEFER)
[   22.374312] tinydrm/st7735r: st7735r_probe(): tinydrm_of_find_backlight(dev): mipi->backlight=36d38477
[   22.383667] tinydrm/st7735r: st7735r_probe(): IS_ERR(mipi->backlight): PTR_ERR(mipi->backlight)

@dlech
Copy link

dlech commented Mar 4, 2018

It looks like the error is getting propagated as it should. I'm not sure why the driver is not getting probed again later. Is it possible that something like universal cape manager is tying up the PWM later in boot?

@pdp7
Copy link
Author

pdp7 commented Mar 5, 2018

@dlech It looks like the problem was that I was failing to activate epwmss1. I've added that now:

    fragment@2 {
        target = <&epwmss1>;
        __overlay__ {
            status = "okay";
        };
    };

And the st7735r driver loads OK!

@dlech
Copy link

dlech commented Mar 5, 2018

cool beans

@pdp7
Copy link
Author

pdp7 commented Mar 6, 2018

@dlech Here is the dts overlay I am using for PWM backlight: BB-LCD-ADAFRUIT-18-SPI1-00A0.dts

While no errors appeared to occur, the pwm backlight would not turn on. I found the issue is with pwm_backlight_update_status() located in drivers/video/backlight/pwm_bl.c.

I found that the following would evaluated to true and thus set brightness to 0:

	if (bl->props.power != FB_BLANK_UNBLANK ||
	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
	    bl->props.state & BL_CORE_FBBLANK)
		brightness = 0;

I removed the above lines:

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1c2289ddd555..68c039614892 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -98,11 +98,6 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
        int brightness = bl->props.brightness;
        int duty_cycle;
 
-       if (bl->props.power != FB_BLANK_UNBLANK ||
-           bl->props.fb_blank != FB_BLANK_UNBLANK ||
-           bl->props.state & BL_CORE_FBBLANK)
-               brightness = 0;
-
        if (pb->notify)
                brightness = pb->notify(pb->dev, brightness);

and the backlight does now turn on OK, and I can set brightness via /sys/devices/platform/backlight/backlight/backlight/brightness.

It seems unlikely this is actually a bug in drivers/video/backlight/pwm_bl.c.

Any suggestions as to how to address this issue?

Maybe I need to add something to BB-LCD-ADAFRUIT-18-SPI1-00A0.dts?

@dlech
Copy link

dlech commented Mar 6, 2018

Do you know which condition specifically is keeping it off? I wonder if you create an "always-on" regulator in device tree if that would help.

bl_reg: backlight-regulator {
	compatible = "regulator-fixed";
	regulator-name = "backlight";
	regulator-always-on;
	regulator-boot-on;
};

@pdp7
Copy link
Author

pdp7 commented Mar 6, 2018

@dlech thanks, I will try that out. BTW, will you be at ELC next week?

@dlech
Copy link

dlech commented Mar 6, 2018

No, I won't sadly. I really enjoyed it last year and have been wishing I was going again.

@pdp7
Copy link
Author

pdp7 commented Mar 7, 2018

@dlech Thanks, I added the power regulator to BB-LCD-ADAFRUIT-18-SPI1-00A0.dts but the results are still the same.

It seems the issue is that pwm is not enabled when pwm_backlight_probe() is called.

From pwm_backlight_probe() in drivers/video/backlight/pwm_bl.c:

        bl->props.power = pwm_backlight_initial_power_state(pb);

pwm_backlight_initial_power_state() returns FB_BLANK_POWERDOWN
because !pwm_is_enabled(pb->pwm).

From include/uapi/linux/fb.h:

#define VESA_POWERDOWN          3
	FB_BLANK_POWERDOWN     = VESA_POWERDOWN + 1

From pwm_backlight_update_status() in drivers/video/backlight/pwm_bl.c:

        if (bl->props.power != FB_BLANK_UNBLANK ||
            bl->props.fb_blank != FB_BLANK_UNBLANK ||
            bl->props.state & BL_CORE_FBBLANK) {
                brightness = 0;
        }

From printk() I can determine that:

  • FB_BLANK_UNBLANK=0x0
  • BL_CORE_FBBLANK=0x2
  • bl->props.power=0x4
  • bl->props.fb_blank=0x0
  • bl->props.state=0x0

Here is the if statement with the values:

        if ( 0x4 != 0x0 ||
             0x0 != 0x0 ||
             0x0 & 0x2 )) {
                brightness = 0;
        }

@pdp7
Copy link
Author

pdp7 commented Mar 7, 2018

@dlech I'm looking at occurrences of FB_BLANK_UNBLANK within drivers/gpu/drm/ and I see a pattern of backlight->props.power = FB_BLANK_UNBLANK before calling backlight_update_status().

Do you think think that needs to be done in st7735r driver?

Here are some examples I was looking at within drivers/gpu/drm/:

static int st7789v_enable(struct drm_panel *panel)
{
        struct st7789v *ctx = panel_to_st7789v(panel);

        if (ctx->backlight) {
                ctx->backlight->props.state &= ~BL_CORE_FBBLANK;
                ctx->backlight->props.power = FB_BLANK_UNBLANK;
                backlight_update_status(ctx->backlight);
        }

        return st7789v_write_command(ctx, MIPI_DCS_SET_DISPLAY_ON);
}
static int panel_simple_enable(struct drm_panel *panel)
{
        struct panel_simple *p = to_panel_simple(panel);

        if (p->enabled)
                return 0;

        if (p->desc->delay.enable)
                msleep(p->desc->delay.enable);

        if (p->backlight) {
                p->backlight->props.state &= ~BL_CORE_FBBLANK;
                p->backlight->props.power = FB_BLANK_UNBLANK;
                backlight_update_status(p->backlight);
        }

        p->enabled = true;

        return 0;
}

@pdp7
Copy link
Author

pdp7 commented Mar 7, 2018

@dlech I added mipi->backlight->props.power = FB_BLANK_UNBLANK to st7735r_probe() and it appears to solve the issue.

Do you think this is a good approach?

diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 98ff447f40b4..65e6cd4e801d 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -171,6 +171,7 @@ static int st7735r_probe(struct spi_device *spi)
        mipi->backlight = tinydrm_of_find_backlight(dev);
        if (IS_ERR(mipi->backlight))
                return PTR_ERR(mipi->backlight);
+       mipi->backlight->props.power = FB_BLANK_UNBLANK;
 
        device_property_read_u32(dev, "rotation", &rotation);
 

Related dmesg output with printk()'s for debugging:

[   25.908483] tinydrm/st7735r: st7735r_probe(): mipi->backlight->props=574c5eb5
[   25.915658] tinydrm/st7735r: st7735r_probe(): mipi->backlight->props.power=0x4
[   25.922919] tinydrm/st7735r: st7735r_probe(): mipi->backlight->props.power = FB_BLANK_UNBLANK
[   25.931492] tinydrm/st7735r: st7735r_probe(): mipi->backlight->props.power=0x0
[   25.938754] tinydrm/st7735r: st7735r_probe(): device_property_read_u32(dev, "rotation", &rotation)
[   25.947765] tinydrm/st7735r: st7735r_probe(): call mipi_dbi_spi_init()
[   25.954338] tinydrm/st7735r: st7735r_probe(): mipi_dbi_spi_init(): ret=0
[   25.961077] tinydrm/st7735r: st7735r_probe(): call mipi_dbi_init()

@notro
Copy link

notro commented Mar 7, 2018

The backlight helper functions in tinydrm are gone now in linux-next. You could try to apply the 6 first patches in this series: Add backlight helper functions
But I don't think it will solve your problem since the change that works for you indicates that the problems has to do with the initial props.power value. Have you tried to turn the userspace knobs (/sys/class/backlight) to see if you can turn on backlight that way?

@pdp7
Copy link
Author

pdp7 commented Mar 7, 2018

@notro thanks. I will apply the patch set and see if the result changes.

In regards to /sys/class/backlight, I did write to the brightness file. Are there other files that I could try writing to?

@notro
Copy link

notro commented Mar 7, 2018

I was thinking of bl_power which sets props.power.
https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-backlight
https://elixir.bootlin.com/linux/latest/ident/bl_power_store

@pdp7
Copy link
Author

pdp7 commented Mar 22, 2018

@notro thanks. The backlight does turn on when I set bl_power to 0 (which is FB_BLANK_UNBLANK).

echo 0 > /sys/devices/platform/backlight/backlight/backlight/bl_power

Is the expectation that userspace will do this as needed and that the driver probe should not?

@pdp7
Copy link
Author

pdp7 commented Mar 22, 2018

@dlech @notro thanks for your assistance. I have created PR #75

@notro
Copy link

notro commented Mar 22, 2018

Is the expectation that userspace will do this as needed and that the driver probe should not?

No, the tinydrm driver should make sure the backlight turns on when the display pipeline is enabled. I mentioned it just as an aid in debugging the problem.

RobertCNelson pushed a commit that referenced this pull request Mar 22, 2018
Add defintion for PWM backlight control on Adafruit 1.8" TFT LCD.

Connect lite pin on lcd to P9.14.

Turn on backlight on by setting bl_power to 0 (FB_BLANK_UNBLACK):
echo 0 > /sys/devices/platform/backlight_pwm/backlight/backlight_pwm/bl_power

That value is specified in:
https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-backlight

What:		/sys/class/backlight/<backlight>/bl_power
Date:		April 2005
KernelVersion:	2.6.12
Contact:	Richard Purdie <rpurdie@rpsys.net>
Description:
		Control BACKLIGHT power, values are FB_BLANK_* from fb.h
		 - FB_BLANK_UNBLANK (0)   : power on.
		 - FB_BLANK_POWERDOWN (4) : power off

Thanks to @notro for the suggestion in PR #68 comment:
#68 (comment)

Signed-off-by: Drew Fustini <drew@pdp7.com>
@pdp7
Copy link
Author

pdp7 commented Mar 22, 2018

@notro @dlech Thanks. Do you think I should create an issue in the tinydrm repo to further discuss a patch for st7735r driver?

@dlech
Copy link

dlech commented Mar 22, 2018

Starting a new issue sounds like a good idea since this PR has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants