Re: Aw: Re: [RFC v1 5/5] arm64: dts: mediatek: Add mt7986 based Bananapi R3 Mini

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Il 09/05/24 12:30, Frank Wunderlich ha scritto:
Am 9. Mai 2024 12:10:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>:
Il 08/05/24 20:25, Frank Wunderlich ha scritto:
Hi

Gesendet: Dienstag, 07. Mai 2024 um 15:35 Uhr
Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@xxxxxxxxxxxxx>

Il 06/05/24 18:00, Frank Wunderlich ha scritto:

+	fan: pwm-fan {
+		compatible = "pwm-fan";
+		#cooling-cells = <2>;
+		/* cooling level (0, 1, 2) - pwm inverted */
+		cooling-levels = <255 96 0>;

Did you try to actually invert the PWM?

Look for PWM_POLARITY_INVERTED ;-)

Mtk pwm driver does not support it

https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211


You're right, sorry - I confused the general purpose PWM controller with the
rather specific DISP_PWM controller (which does support polarity inversion).

It's good - but I'd appreciate if you can please add a comment stating that
the PWM values are inverted in SW because the controller does *not* support
polarity inversion... so that next time someone looks at this will immediately
understand what's going on and why :-)

so i would change comment like this:

		/* cooling level (0, 1, 2)
		 * signal is inverted on board
		 * mtk pwm driver does not support
		 * PWM_POLARITY_INVERTED */


There you go:

/*
* The signal is inverted on this board and the general purpose
* PWM HW IP in this SoC does not support polarity inversion.
*/
/* Cooling level < 0  1  2> */
cooling-levels = <255 96 0>;

Thanks for clearing structure of the comment,but imho actually it is a driver issue (for all mtk SoC). Not sure it is really a hardware limitation. So i would change this to "... and the PWM driver does not support polarity inversion."

+		pwms = <&pwm 0 10000>;
+		status = "okay";
+	};
+
+	phy14: ethernet-phy@14 {
...
+		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
+		reset-assert-us = <10000>;
+		reset-deassert-us = <20000>;
+		phy-mode = "2500base-x";
+		full-duplex;
+		pause;
+		airoha,pnswap-rx;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 { /* en8811_a_gpio5 */
+				reg = <0>;
+				color = <LED_COLOR_ID_YELLOW>;
+				function = LED_FUNCTION_LAN;
+				function-enumerator = <1>;

Why aren't you simply using a label?

You mean the comment? I can add it of course like for regulators.


I mean in place of the function-enumerator... that's practically used to
distinguish between instances, it's not too common to see it, and usually
"label" replaces exactly that - just that, instead of a different number,
it gets a different name with no (usually) meaningless numbers :-)

as far as i understand using label also makes "function" property useless, after discussing
this with eric i would drop both on all 4 places by labels like these:

label = "yellow-lan";
label = "green-lan";
...

not sure if we should drop color property too...


I'm looking at the leds binding (leds/common.yaml) right now.

My suggestion of using 'label' was actually wrong - and your devicetree was
actually right!!! (apart from the default-trigger that may not work)

Infact, the documentation says, in brief:

- function-enumerator is ignored if label is present
- function doesn't say that gets ignored
- color doesn't say that gets ignored
- label says:
  - If not present -> get string from node name
  - function-enumerator ignored
  - This property is deprecated

...but the 'label' binding does not say 'deprecated: true', which is something
that must be fixed!

Ok,i can try to add the property to binding (independ of this series). Imho label was cleaner than function and function-enumerator...


Oh I sort of agree with you, I liked the label more, as it's more consistent with
everything else... but oh well. :-)

So, I'm sorry for the confusion, the noise and the useless loss of time around
this - you can keep the LED nodes as they are, and that's a lesson for the future
me reviewing another node like this one.

Don't worry, we are all humas...i missed looking in linux-next for the other binding-patches.

P.S.: This shouldn't have been a RFC, as the patches are more than RFC quality!!!

I sent it as RFC because i had not expected to be merged before next is closed.


Ah at least from my side, no worries... when I see RFC I generally expect to see
"dubious/head-scratching stuff", not "sub-optimal timing to send a patch" :-P

Cheers!
Angelo


+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+			led@1 { /* en8811_a_gpio4 */
+				reg = <1>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_LAN;
+				function-enumerator = <2>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+		};
+	};
+
+	phy15: ethernet-phy@15 {
+		reg = <15>;


regards Frank





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux