linux-kernel - Re: [PATCH 3/3] arm64: dts: rockchip: Add Radxa E24C

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dcbc1c43-748b-479a-878f-569428e09f45@kwiboo.se>
Date: Tue, 29 Jul 2025 23:15:04 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Chukun Pan <amadeus@....edu.cn>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, heiko@...ech.de,
 krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
 ziyao@...root.org
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: Add Radxa E24C

On 7/29/2025 3:20 PM, Chukun Pan wrote:
> Hi,
> 
>> Both avddl_1v1 and avddh_3v3 are controlled by the same gpio, I do not
>> remember if using two regulators with same gpios is supported, can only
>> remember it being an issue in the past, so I opted to just describe it
>> as a single regulator and gave it a new name and added labels for the
>> name used in schematic.
>>
>> Would calling it vdd_8367 (after gpio_8367_en) be better or do you have
>> any other suggestion on how to describe these?
> 
> Would it be better to just call it avddh_3v3 and add a comment?
> This makes it easier to find in the schematics and match phy-supply.

It already had a label for avddh_3v3, I am currently thinking something
like following, includes the names from schematic and a name for humans:

	/* Common enable line for the avdd rails mentioned in the labels */
	vdd_switch: avddl_1v1: avddh_3v3: regulator-vdd-switch {
		compatible = "regulator-fixed";
		enable-active-high;
		gpios = <&gpio1 RK_PC3 GPIO_ACTIVE_HIGH>;
		pinctrl-names = "default";
		pinctrl-0 = <&gpio_8367_en>;
		regulator-name = "vdd_switch";
		startup-delay-us = <10000>;
		vin-supply = <&vcc5v0_sys>;
	};

> 
>> See above, I had issues using the reset-gpios of the switch, because the
>> switch was probed twice, once deferred by gmac, and by the second probe
>> failed with -BUSY because of the reset-gpios still being claimd by the
>> first probe.
>>
>> I can change to describe the reset pin in the switch, however that will
>> likely mean Ethernet is unusable until the issue in devres/gpiolib is
>> tracked down and fixed by someone.
> 
> I don't think it's a devres/gpiolib issue.
> It looks like these two resets are competing:
> 
>   priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
>   priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> 
> reset-gpios works if reset-names is specified:
> 
> -	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> +	priv->reset_ctl = devm_reset_control_get_optional(dev, "switch");
> 
> Or just remove the reset controller, I'm not sure if it's really needed:
> 
> -	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> -	if (IS_ERR(priv->reset_ctl))
> -		return dev_err_cast_probe(dev, priv->reset_ctl,
> -					  "failed to get reset control\n");

Very interesting, my initial testing was to include a call to
devm_gpiod_put() in rtl83xx_remove() but was afraid it could result in a
WARN_ON in case driver was properly unloaded/removed.

  void rtl83xx_remove(struct realtek_priv *priv)
  {
	if (priv->reset)
		devm_gpiod_put(priv-dev, priv->reset);
  }

With reset_control_get using a gpio fallback, this complicates things a
little bit, will run some more tests and probably include a driver patch
for v2, thanks for finding this!

Regards,
Jonas

> 
> Thanks,
> Chukun
> 
> --
> 2.25.1
> 
>