Re: [PATCH 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Maya,

I'm going through this now and addressing your comments.

On 12/22/22 23:57, Maya Matuszczyk wrote:

[...]

>> +       /* MIPI DSI panel 1.8v supply */
>> +       vcc1v8_lcd: vcc1v8-lcd {
> Node names should be generic, for example "vcc1v8-lcd-regulator".
>

Fixed.
 
>> +               compatible = "regulator-fixed";
>> +               enable-active-high;
> Is this really needed?
> You can set the polarity in "gpios" property.
>

I understand that it is needed. The regulator-fixing binding says:

  enable-active-high:
    description:
      Polarity of GPIO is Active high. If this property is missing,
      the default assumed is Active low.
    type: boolean

and indeed by looking at the source in drivers/gpio/gpiolib-of.c, there is
a check for this property in the of_gpio_flags_quirks() function:

static void of_gpio_flags_quirks(const struct device_node *np,
				 const char *propname,
				 enum of_gpio_flags *flags,
				 int index)
{
	/*
	 * Some GPIO fixed regulator quirks.
	 * Note that active low is the default.
	 */
	if (IS_ENABLED(CONFIG_REGULATOR) &&
	    (of_device_is_compatible(np, "regulator-fixed") ||
	     of_device_is_compatible(np, "reg-fixed-voltage") ||
	     (!(strcmp(propname, "enable-gpio") &&
		strcmp(propname, "enable-gpios")) &&
	      of_device_is_compatible(np, "regulator-gpio")))) {
		bool active_low = !of_property_read_bool(np,
							 "enable-active-high");
		/*
		 * The regulator GPIO handles are specified such that the
		 * presence or absence of "enable-active-high" solely controls
		 * the polarity of the GPIO line. Any phandle flags must
		 * be actively ignored.
		 */
		if ((*flags & OF_GPIO_ACTIVE_LOW) && !active_low) {
			pr_warn("%s GPIO handle specifies active low - ignored\n",
				of_node_full_name(np));
			*flags &= ~OF_GPIO_ACTIVE_LOW;
		}
		if (active_low)
			*flags |= OF_GPIO_ACTIVE_LOW;
	}
...
}

So I'll keep those.
 
>> +               regulator-name = "vcc1v8_lcd";
>> +               regulator-min-microvolt = <1800000>;
>> +               regulator-max-microvolt = <1800000>;
>> +               vin-supply = <&vcc3v3_sys>;
>> +               gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> Is this a typo? Documentation says "gpios"
>

Documentation/devicetree/bindings/gpio/gpio.txt indeed says "gpios" but "gpio"
is also supported for older DT that are using bindings that got it wrong. See
commits e7ae65ced7dd ("gpio: mention in DT binding doc that <name>-gpio is
deprecated") and dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").

The regulator-fixed bindings is such an example. See that its bindings schema
Documentation/devicetree/bindings/regulator/regulator.yaml mentions "gpio" and
not "gpios", also in the example.

So until that is fixed, I would prefer to stick with that's documented in the
regulator-fixed bindings doc.
 
[...]

>> +       touchscreen@14 {
>> +               compatible = "goodix,gt917s";
>> +               reg = <0x14>;
>> +               interrupt-parent = <&gpio3>;
>> +               interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
>> +               irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
>> +               reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
>> +               AVDD28-supply = <&vcc3v0_touch>;
>> +               VDDIO-supply = <&vcc3v0_touch>;
>> +               touchscreen-size-x = <720>;
>> +               touchscreen-size-y = <1440>;
>> +               poweroff-in-suspend;
> Are you really sure this property exists in touchscreen driver's dt bindings?
>

It's not indeed. I've dropped that now.

[...]

>> +               vcc-supply = <&vcc2v8_lcd>; // 2v8
> What is the purpose of that comment?
> 
>> +               iovcc-supply = <&vcc1v8_lcd>; // 1v8

Yeah, not that useful. I've removed those two now.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat