| Message ID | a159566d0970fb5c43da7758d8281b594b6ca48e.1521209538.git.lorenzo.bianconi@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Fri, 2018-03-16 at 15:45 +0100, Lorenzo Bianconi wrote: > --- a/drivers/net/wireless/mediatek/mt76/Kconfig > +++ b/drivers/net/wireless/mediatek/mt76/Kconfig > @@ -1,5 +1,8 @@ > config MT76_CORE > tristate > + select MAC80211_LEDS Should drivers really mess with mac80211's configuration that way? I believe this is a user-visible config, no? johannes
On Wed, Mar 21, 2018 at 6:45 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2018-03-16 at 15:45 +0100, Lorenzo Bianconi wrote: >> --- a/drivers/net/wireless/mediatek/mt76/Kconfig >> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig >> @@ -1,5 +1,8 @@ >> config MT76_CORE >> tristate >> + select MAC80211_LEDS > > Should drivers really mess with mac80211's configuration that way? I > believe this is a user-visible config, no? We have a couple of drivers using 'select LEDS_CLASS' and others doing 'depends on LEDS_CLASS'. I think the latter is what we should have here for all those drivers. MAC80211_LEDS looks like it's designed to be optional, so nothing should select or depend on that. Arnd
On Mar 21, Arnd Bergmann wrote: > On Wed, Mar 21, 2018 at 6:45 AM, Johannes Berg > <johannes@sipsolutions.net> wrote: > > On Fri, 2018-03-16 at 15:45 +0100, Lorenzo Bianconi wrote: > >> --- a/drivers/net/wireless/mediatek/mt76/Kconfig > >> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig > >> @@ -1,5 +1,8 @@ > >> config MT76_CORE > >> tristate > >> + select MAC80211_LEDS > > > > Should drivers really mess with mac80211's configuration that way? I > > believe this is a user-visible config, no? > > We have a couple of drivers using 'select LEDS_CLASS' and others > doing 'depends on LEDS_CLASS'. I think the latter is what we should > have here for all those drivers. > > MAC80211_LEDS looks like it's designed to be optional, so nothing > should select or depend on that. > > Arnd Reviewing the current code we do not actually need MAC80211_LEDS, so I agree to remove it from Kconfig and let userspace selects the option. I would use select for LEDS_CLASS. If you agree I can send a v2 otherwise I fine to apply Arnd's patch. Felix what do you think? Regards, Lorenzo
On Wed, Mar 21, 2018, 23:27 Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > On Mar 21, Arnd Bergmann wrote: > > On Wed, Mar 21, 2018 at 6:45 AM, Johannes Berg > > <johannes@sipsolutions.net> wrote: > > > On Fri, 2018-03-16 at 15:45 +0100, Lorenzo Bianconi wrote: > > >> --- a/drivers/net/wireless/mediatek/mt76/Kconfig > > >> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig > > >> @@ -1,5 +1,8 @@ > > >> config MT76_CORE > > >> tristate > > >> + select MAC80211_LEDS > > > > > > Should drivers really mess with mac80211's configuration that way? I > > > believe this is a user-visible config, no? > > > > We have a couple of drivers using 'select LEDS_CLASS' and others > > doing 'depends on LEDS_CLASS'. I think the latter is what we should > > have here for all those drivers. > > > > MAC80211_LEDS looks like it's designed to be optional, so nothing > > should select or depend on that. > > > > Arnd > > Reviewing the current code we do not actually need MAC80211_LEDS, so I agree > to remove it from Kconfig and let userspace selects the option. I would use > select for LEDS_CLASS. If you agree I can send a v2 otherwise I fine to apply > Arnd's patch. > Felix what do you think? Looking at mt76 again, my impression is that the core driver should not have an dependency on LEDS at all, the dependency should instead be restricted to the CONFIG_MT76_LEDS symbol as my patch from January did (with the change to 'default y'). Arnd
On Mar 22, Arnd Bergmann wrote: > On Wed, Mar 21, 2018, 23:27 Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> wrote: > > > > On Mar 21, Arnd Bergmann wrote: > > > On Wed, Mar 21, 2018 at 6:45 AM, Johannes Berg > > > <johannes@sipsolutions.net> wrote: > > > > On Fri, 2018-03-16 at 15:45 +0100, Lorenzo Bianconi wrote: > > > >> --- a/drivers/net/wireless/mediatek/mt76/Kconfig > > > >> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig > > > >> @@ -1,5 +1,8 @@ > > > >> config MT76_CORE > > > >> tristate > > > >> + select MAC80211_LEDS > > > > > > > > Should drivers really mess with mac80211's configuration that way? I > > > > believe this is a user-visible config, no? > > > > > > We have a couple of drivers using 'select LEDS_CLASS' and others > > > doing 'depends on LEDS_CLASS'. I think the latter is what we should > > > have here for all those drivers. > > > > > > MAC80211_LEDS looks like it's designed to be optional, so nothing > > > should select or depend on that. > > > > > > Arnd > > > > Reviewing the current code we do not actually need MAC80211_LEDS, so I agree > > to remove it from Kconfig and let userspace selects the option. I would use > > select for LEDS_CLASS. If you agree I can send a v2 otherwise I fine to apply > > Arnd's patch. > > Felix what do you think? > > Looking at mt76 again, my impression is that the core driver should not have > an dependency on LEDS at all, the dependency should instead be restricted > to the CONFIG_MT76_LEDS symbol as my patch from January did (with the > change to 'default y'). > > Arnd I agree. Fell free to add Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Regards, Lorenzo
diff --git a/drivers/net/wireless/mediatek/mt76/Kconfig b/drivers/net/wireless/mediatek/mt76/Kconfig index fc05d79c80d0..0bbd492c2c2b 100644 --- a/drivers/net/wireless/mediatek/mt76/Kconfig +++ b/drivers/net/wireless/mediatek/mt76/Kconfig @@ -1,5 +1,8 @@ config MT76_CORE tristate + select MAC80211_LEDS + select LEDS_CLASS + select NEW_LEDS config MT76x2E tristate "MediaTek MT76x2E (PCIe) support"
Select mac80211 led support by default in order to fix the following compilation error that occurs when CONFIG_LEDS_CLASS/CONFIG_MAC80211_LEDS options are not enabled on the system ERROR: "devm_of_led_classdev_register" undefined! Fixes: 17f1de56df05 ("mt76: add common code shared between multiple chipsets") Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- drivers/net/wireless/mediatek/mt76/Kconfig | 3 +++ 1 file changed, 3 insertions(+)