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
