FR: LED: add optional wifi\led\status DT binding to omit LED creation

[filimonic]

Want to know your mention.

Feature Request: add optional wifi\led\status DT binding option to omit LED creation.

Problem:

LED creation depends on CONFIG_MT76_LEDS build variable and can not be configured using DTS.
It there is no LED connected and CONFIG_MT76_LEDS is set (which is by default in OpenWrt), it creates /sys/class/leds/mt76-* by default. Changing build flags seem to be bad idea depending on such a minor device hardware difference.

Proposed solution:

The led section already allowed for mt76 in DT, but is designed in expectation that LED exists.
It'll be good if status = "disabled" within wifi\led section could be processed by mt76 driver, and in case when status is not okay, driver should omit /sys/class/leds/mt76-* device creation.

Example configuration of proposed solution:

&pcie0 {
    wifi@0,0 {
        compatible = "mediatek,mt76";
        reg = <0x0000 0 0 0 0>;
        mediatek,mtd-eeprom = <&factory 0x8000>;
        led {
            // There is no LED connected
            status = "disabled"; 
        };
    };
};

[ryderlee1110]

Could you please test this patch https://pastebin.mozilla.org/SM0Vcpru/raw?

[filimonic]

Could you please test this patch https://pastebin.mozilla.org/SM0Vcpru/raw?

Thanks, I will report back.

[DragonBluep]

These changes may break the LED support of some dbdc devices which don't have 'led' node. And using node name to find node is not a good idea.

[filimonic]

These changes may break the LED support of some dbdc devices which don't have 'led' node.

Should be this, i think

np = of_get_child_by_name(np, "led");
// If "led" is defined in DT and it has status option defined and status is not "okay", then return
if (np && !of_device_is_available(np))
    return 0;

I'm also afraid if led_classdev_unregister in mt76_led_cleanup maybe should check something about not null

And using node name to find node is not a good idea.

Any alternative?

[DragonBluep]

And using node name to find node is not a good idea.

Any alternative?

I don't know but Linux upstream seems don't like this way. I've seen some developers suggest this in the mailing list.

[filimonic]

And using node name to find node is not a good idea.

Any alternative?

I don't know but Linux upstream seems don't like this way. I've seen some developers suggest this in the mailing list.

I see there is of_find_node_by_type maybe better

[ryderlee1110]

led_classdev_unregister would check IS_ERR_OR_NULL(), so you don't need to worry about this.

Also, of_get_child_by_name has existed in mt76 for years, and this patch does not change the way it gets node, but maybe you can make a patch to change to of_find_node_by_type if that works better to you. Lastly, If you want to explicitly control LED creation via "status = disabled/okay" you must explicitly check sub-node, and this does break devices which don't have 'led' node not just DBDC since we currently create led_classdev regardless the DT node status.

If you just want to explicitly disable it when LED node exists, then you can do "if (np && !of_device_is_available(np))"

Does that make sense?

[filimonic]

led_classdev_unregister would check IS_ERR_OR_NULL(), so you don't need to worry about this.

if(IS_ERR_OR_NULL(led_cdev->dev)) return; inside led_classdev_unregister was introduced in kernel 5.4. Before 5.4 there seem to be no check. IDK if it is important or not.

If you just want to explicitly disable it when LED node exists, then you can do "if (np && !of_device_is_available(np))"

Thanks.

[ryderlee1110]

so please upgrade to 5.4+
@filimonic does this https://pastebin.mozilla.org/jscfu1gx/raw work for your case?

[filimonic]

so please upgrade to 5.4+ @filimonic does this https://pastebin.mozilla.org/jscfu1gx/raw work for your case?

When !of_device_is_available(np), of_node_put(np) is never called - I think this is not good.

Created PR #811

[ryderlee1110]

Oh right, just add one more of_node_put(np) into !of_device_is_available(np) check.

I think we can close this ticket.

[filimonic]

Oh right, just add one more of_node_put(np) into !of_device_is_available(np) check.

I think we can close this ticket.

I think it should be done if PR will be accepted ...

[ryderlee1110]

I think the proper way is upstream patch for public review. Felix will apply if there's no issue and then sync to openwrt/mt76. I've posted to linux-wireless including your changes.

[filimonic]

I think the proper way is upstream patch for public review. Felix will apply if there's no issue and then sync to openwrt/mt76. I've posted to linux-wireless including your changes.

Thanks.
(link here)
https://lore.kernel.org/linux-wireless/b08ef4d7c91c54e273c61784272e5c6f70c3c137.1692896654.git.ryder.lee@mediatek.com/T/#u