Messages in this thread | | | Date | Thu, 9 May 2024 12:10:59 +0200 | Subject | Re: Aw: Re: [RFC v1 5/5] arm64: dts: mediatek: Add mt7986 based Bananapi R3 Mini | From | AngeloGioacchino Del Regno <> |
| |
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@collabora.com> >> >> 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>;
>>>>> + 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!
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.
P.S.: This shouldn't have been a RFC, as the patches are more than RFC quality!!!
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>; >>>> >>>> Same here. >>>> >>>> Cheers, >>>> Angelo > > regards Frank
| |