LinuxLists.cc - [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Christian Marangi <[email protected]> writes:

> From: Sebastian Gottschall <[email protected]>
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
> based chipsets with on chipset connected led's using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and
> can be controlled with various triggers.
> Adds also debugfs interface for gpio control.
>
> Signed-off-by: Sebastian Gottschall <[email protected]>
> Reviewed-by: Steve deRosier <[email protected]>
> [kvalo: major reorg and cleanup]
> Signed-off-by: Kalle Valo <[email protected]>
> [ansuel: rebase and small cleanup]
> Signed-off-by: Christian Marangi <[email protected]>
> Tested-by: Stefan Lippers-Hollmann <[email protected]>
> ---
>
> Hi,
> this is a very old patch from 2018 that somehow was talked till 2020
> with Kavlo asked to rebase and resubmit and nobody did.
> So here we are in 2023 with me trying to finally have this upstream.
>
> A summarize of the situation.
> - The patch is from years in OpenWRT. Used by anything that has ath10k
> card and a LED connected.
> - This patch is also used by the fw variant from Candela Tech with no
> problem reported.
> - It was pointed out that this caused some problem with ipq4019 SoC
> but the problem was actually caused by a different bug related to
> interrupts.
>
> I honestly hope we can have this feature merged since it's really
> funny to have something that was so near merge and jet still not
> present and with devices not supporting this simple but useful
> feature.

Indeed, we should finally get this in. Thanks for working on it.

I did some minor changes to the patch, they are in my pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1

My comments below, please review my changes. No need to resend because
of these.

> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
>
> If unsure, say Y to make it easier to debug problems.
>
> +config ATH10K_LEDS
> + bool "Atheros ath10k LED support"
> + depends on ATH10K
> + select MAC80211_LEDS
> + select LEDS_CLASS
> + select NEW_LEDS
> + default y
> + help
> + This option enables LEDs support for chipset LED pins.
> + Each pin is connected via GPIO and can be controlled using
> + WMI Firmware API.
> +
> + The LED device will get available named as "ath10k-phyX" at sysfs and
> + can be controlled with various triggers.
> +
> + Say Y, if you have LED pins connected to the ath10k wireless card.

I'm not sure anymore if we should ask anything from the user, better to
enable automatically if LED support is enabled in the kernel. So I
simplified this to:

config ATH10K_LEDS
bool
depends on ATH10K
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
default y

This follows what mt76 does:

config MT76_LEDS
bool
depends on MT76_CORE
depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
default y

> @@ -65,6 +66,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
> .dev_id = QCA988X_2_0_DEVICE_ID,
> .bus = ATH10K_BUS_PCI,
> .name = "qca988x hw2.0",
> + .led_pin = 1,
> .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> .uart_pin = 7,
> .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,

I prefer following the field order from struct ath10k_hw_params
declaration and also setting fields explicitly to zero (even though
there are gaps still) so I changed that for every entry.

> +int ath10k_leds_register(struct ath10k *ar)
> +{
> + int ret;
> +
> + if (ar->hw_params.led_pin == 0)
> + /* leds not supported */
> + return 0;
> +
> + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
> + wiphy_name(ar->hw->wiphy));
> + ar->leds.wifi_led.active_low = 1;
> + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
> + ar->leds.wifi_led.name = ar->leds.label;
> + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
> +
> + ar->leds.cdev.name = ar->leds.label;
> + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
> +
> + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
> + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;

But what to do with this FIXME?

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2023-06-16 22:15:22

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> Christian Marangi <[email protected]> writes:
>
> > From: Sebastian Gottschall <[email protected]>
> >
> > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
> > based chipsets with on chipset connected led's using WMI Firmware API.
> > The LED device will get available named as "ath10k-phyX" at sysfs and
> > can be controlled with various triggers.
> > Adds also debugfs interface for gpio control.
> >
> > Signed-off-by: Sebastian Gottschall <[email protected]>
> > Reviewed-by: Steve deRosier <[email protected]>
> > [kvalo: major reorg and cleanup]
> > Signed-off-by: Kalle Valo <[email protected]>
> > [ansuel: rebase and small cleanup]
> > Signed-off-by: Christian Marangi <[email protected]>
> > Tested-by: Stefan Lippers-Hollmann <[email protected]>
> > ---
> >
> > Hi,
> > this is a very old patch from 2018 that somehow was talked till 2020
> > with Kavlo asked to rebase and resubmit and nobody did.
> > So here we are in 2023 with me trying to finally have this upstream.
> >
> > A summarize of the situation.
> > - The patch is from years in OpenWRT. Used by anything that has ath10k
> > card and a LED connected.
> > - This patch is also used by the fw variant from Candela Tech with no
> > problem reported.
> > - It was pointed out that this caused some problem with ipq4019 SoC
> > but the problem was actually caused by a different bug related to
> > interrupts.
> >
> > I honestly hope we can have this feature merged since it's really
> > funny to have something that was so near merge and jet still not
> > present and with devices not supporting this simple but useful
> > feature.
>
> Indeed, we should finally get this in. Thanks for working on it.
>
> I did some minor changes to the patch, they are in my pending branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
>
> My comments below, please review my changes. No need to resend because
> of these.
>

Hi,
very happy this is going further.

> > --- a/drivers/net/wireless/ath/ath10k/Kconfig
> > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> > @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
> >
> > If unsure, say Y to make it easier to debug problems.
> >
> > +config ATH10K_LEDS
> > + bool "Atheros ath10k LED support"
> > + depends on ATH10K
> > + select MAC80211_LEDS
> > + select LEDS_CLASS
> > + select NEW_LEDS
> > + default y
> > + help
> > + This option enables LEDs support for chipset LED pins.
> > + Each pin is connected via GPIO and can be controlled using
> > + WMI Firmware API.
> > +
> > + The LED device will get available named as "ath10k-phyX" at sysfs and
> > + can be controlled with various triggers.
> > +
> > + Say Y, if you have LED pins connected to the ath10k wireless card.
>
> I'm not sure anymore if we should ask anything from the user, better to
> enable automatically if LED support is enabled in the kernel. So I
> simplified this to:
>
> config ATH10K_LEDS
> bool
> depends on ATH10K
> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> default y
>
> This follows what mt76 does:
>
> config MT76_LEDS
> bool
> depends on MT76_CORE
> depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
> default y
>

I remember there was the same discussion in a previous series. OK for me
for making this by default, only concern is any buildbot error (if any)

Anyway OK for the change.

> > @@ -65,6 +66,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
> > .dev_id = QCA988X_2_0_DEVICE_ID,
> > .bus = ATH10K_BUS_PCI,
> > .name = "qca988x hw2.0",
> > + .led_pin = 1,
> > .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> > .uart_pin = 7,
> > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
>
> I prefer following the field order from struct ath10k_hw_params
> declaration and also setting fields explicitly to zero (even though
> there are gaps still) so I changed that for every entry.
>

Thanks for the change, np for me.

> > +int ath10k_leds_register(struct ath10k *ar)
> > +{
> > + int ret;
> > +
> > + if (ar->hw_params.led_pin == 0)
> > + /* leds not supported */
> > + return 0;
> > +
> > + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
> > + wiphy_name(ar->hw->wiphy));
> > + ar->leds.wifi_led.active_low = 1;
> > + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
> > + ar->leds.wifi_led.name = ar->leds.label;
> > + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
> > +
> > + ar->leds.cdev.name = ar->leds.label;
> > + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
> > +
> > + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
> > + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
>
> But what to do with this FIXME?
>

It was pushed by you in v13.

I could be wrong but your idea was to prepare for future support of
other patch that would set the default_trigger to the mac80211 tpt.

We might got both confused by default_trigger and default_state.
default_trigger is actually never set and is NULL (actually it's 0)

We have other 2 patch that adds tpt rates for the mac80211 LED trigger
and set this trigger as the default one but honestly I would chose a
different implementation than hardcoding everything.

If it's ok for you, I would drop the comment and the default_trigger and
I will send a follow-up patch to this adding DT support by using
led_classdev_register_ext and defining init_data.
(and this indirectly would permit better LED naming and defining of
default-trigger in DT)

Also ideally I will also send a patch for default_state following
standard LED implementation. (to set default_state in DT)

I would prefer this approach as the LED patch already took way too much
time and I think it's better to merge this initial version and then
improve it.

--
Ansuel

2023-06-17 17:43:31

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
> On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> > Christian Marangi <[email protected]> writes:
> >
> > > From: Sebastian Gottschall <[email protected]>
> > >
> > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
> > > based chipsets with on chipset connected led's using WMI Firmware API.
> > > The LED device will get available named as "ath10k-phyX" at sysfs and
> > > can be controlled with various triggers.
> > > Adds also debugfs interface for gpio control.
> > >
> > > Signed-off-by: Sebastian Gottschall <[email protected]>
> > > Reviewed-by: Steve deRosier <[email protected]>
> > > [kvalo: major reorg and cleanup]
> > > Signed-off-by: Kalle Valo <[email protected]>
> > > [ansuel: rebase and small cleanup]
> > > Signed-off-by: Christian Marangi <[email protected]>
> > > Tested-by: Stefan Lippers-Hollmann <[email protected]>
> > > ---
> > >
> > > Hi,
> > > this is a very old patch from 2018 that somehow was talked till 2020
> > > with Kavlo asked to rebase and resubmit and nobody did.
> > > So here we are in 2023 with me trying to finally have this upstream.
> > >
> > > A summarize of the situation.
> > > - The patch is from years in OpenWRT. Used by anything that has ath10k
> > > card and a LED connected.
> > > - This patch is also used by the fw variant from Candela Tech with no
> > > problem reported.
> > > - It was pointed out that this caused some problem with ipq4019 SoC
> > > but the problem was actually caused by a different bug related to
> > > interrupts.
> > >
> > > I honestly hope we can have this feature merged since it's really
> > > funny to have something that was so near merge and jet still not
> > > present and with devices not supporting this simple but useful
> > > feature.
> >
> > Indeed, we should finally get this in. Thanks for working on it.
> >
> > I did some minor changes to the patch, they are in my pending branch:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
> >
> > My comments below, please review my changes. No need to resend because
> > of these.
> >
>
> Hi,
> very happy this is going further.
>
> > > --- a/drivers/net/wireless/ath/ath10k/Kconfig
> > > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> > > @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
> > >
> > > If unsure, say Y to make it easier to debug problems.
> > >
> > > +config ATH10K_LEDS
> > > + bool "Atheros ath10k LED support"
> > > + depends on ATH10K
> > > + select MAC80211_LEDS
> > > + select LEDS_CLASS
> > > + select NEW_LEDS
> > > + default y
> > > + help
> > > + This option enables LEDs support for chipset LED pins.
> > > + Each pin is connected via GPIO and can be controlled using
> > > + WMI Firmware API.
> > > +
> > > + The LED device will get available named as "ath10k-phyX" at sysfs and
> > > + can be controlled with various triggers.
> > > +
> > > + Say Y, if you have LED pins connected to the ath10k wireless card.
> >
> > I'm not sure anymore if we should ask anything from the user, better to
> > enable automatically if LED support is enabled in the kernel. So I
> > simplified this to:
> >
> > config ATH10K_LEDS
> > bool
> > depends on ATH10K
> > depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> > default y
> >
> > This follows what mt76 does:
> >
> > config MT76_LEDS
> > bool
> > depends on MT76_CORE
> > depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
> > default y
> >
>
> I remember there was the same discussion in a previous series. OK for me
> for making this by default, only concern is any buildbot error (if any)
>
> Anyway OK for the change.
>
> > > @@ -65,6 +66,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
> > > .dev_id = QCA988X_2_0_DEVICE_ID,
> > > .bus = ATH10K_BUS_PCI,
> > > .name = "qca988x hw2.0",
> > > + .led_pin = 1,
> > > .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> > > .uart_pin = 7,
> > > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
> >
> > I prefer following the field order from struct ath10k_hw_params
> > declaration and also setting fields explicitly to zero (even though
> > there are gaps still) so I changed that for every entry.
> >
>
> Thanks for the change, np for me.
>
> > > +int ath10k_leds_register(struct ath10k *ar)
> > > +{
> > > + int ret;
> > > +
> > > + if (ar->hw_params.led_pin == 0)
> > > + /* leds not supported */
> > > + return 0;
> > > +
> > > + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
> > > + wiphy_name(ar->hw->wiphy));
> > > + ar->leds.wifi_led.active_low = 1;
> > > + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
> > > + ar->leds.wifi_led.name = ar->leds.label;
> > > + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
> > > +
> > > + ar->leds.cdev.name = ar->leds.label;
> > > + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
> > > +
> > > + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
> > > + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
> >
> > But what to do with this FIXME?
> >
>
> It was pushed by you in v13.
>
> I could be wrong but your idea was to prepare for future support of
> other patch that would set the default_trigger to the mac80211 tpt.
>
> We might got both confused by default_trigger and default_state.
> default_trigger is actually never set and is NULL (actually it's 0)
>
> We have other 2 patch that adds tpt rates for the mac80211 LED trigger
> and set this trigger as the default one but honestly I would chose a
> different implementation than hardcoding everything.
>
> If it's ok for you, I would drop the comment and the default_trigger and
> I will send a follow-up patch to this adding DT support by using
> led_classdev_register_ext and defining init_data.
> (and this indirectly would permit better LED naming and defining of
> default-trigger in DT)
>
> Also ideally I will also send a patch for default_state following
> standard LED implementation. (to set default_state in DT)
>
> I would prefer this approach as the LED patch already took way too much
> time and I think it's better to merge this initial version and then
> improve it.

If you want to check out I attached the 2 patch (one dt-bindings and the
one for the code) that I will submit when this will be merged (the
change is with the assumption that the FIXME line is dropped)

Tested and works correctly with my use case of wifi card attached with
pcie. This implementation permits to declare the default trigger in DT
instead of hardcoding.

--
Ansuel

Attachments:

(No filename) (6.75 kB)
0001-dt-bindings-net-wireless-qcom-ath10k-Document-LED-no.patch (2.30 kB)
0002-wifi-ath10k-start-LED-with-the-previous-defined-cdev.patch (1.67 kB)
0003-wifi-ath10k-add-DT-support-for-LED-definition.patch (2.61 kB)
Download all attachments

2024-05-09 04:52:17

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Ansuel Smith <[email protected]> writes:

> Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
> <[email protected]> ha scritto:
>
>>
>> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
>> > On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
>> > > Christian Marangi <[email protected]> writes:
>> > >
>> > > > From: Sebastian Gottschall <[email protected]>
>> > > >
>> > > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
>> > > > based chipsets with on chipset connected led's using WMI Firmware API.
>> > > > The LED device will get available named as "ath10k-phyX" at sysfs and
>> > > > can be controlled with various triggers.
>> > > > Adds also debugfs interface for gpio control.
>> > > >
>> > > > Signed-off-by: Sebastian Gottschall <[email protected]>
>> > > > Reviewed-by: Steve deRosier <[email protected]>
>> > > > [kvalo: major reorg and cleanup]
>> > > > Signed-off-by: Kalle Valo <[email protected]>
>> > > > [ansuel: rebase and small cleanup]
>> > > > Signed-off-by: Christian Marangi <[email protected]>
>> > > > Tested-by: Stefan Lippers-Hollmann <[email protected]>
>> > > > ---
>> > > >
>> > > > Hi,
>> > > > this is a very old patch from 2018 that somehow was talked till 2020
>> > > > with Kavlo asked to rebase and resubmit and nobody did.
>> > > > So here we are in 2023 with me trying to finally have this upstream.
>> > > >
>> > > > A summarize of the situation.
>> > > > - The patch is from years in OpenWRT. Used by anything that has ath10k
>> > > > card and a LED connected.
>> > > > - This patch is also used by the fw variant from Candela Tech with no
>> > > > problem reported.
>> > > > - It was pointed out that this caused some problem with ipq4019 SoC
>> > > > but the problem was actually caused by a different bug related to
>> > > > interrupts.
>> > > >
>> > > > I honestly hope we can have this feature merged since it's really
>> > > > funny to have something that was so near merge and jet still not
>> > > > present and with devices not supporting this simple but useful
>> > > > feature.
>> > >
>> > > Indeed, we should finally get this in. Thanks for working on it.
>> > >
>> > > I did some minor changes to the patch, they are in my pending branch:
>> > >
>> > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
>> > >
>> > > My comments below, please review my changes. No need to resend because
>> > > of these.
>> > >
>> >
>> > Hi,
>> > very happy this is going further.
>> >
>> > > > --- a/drivers/net/wireless/ath/ath10k/Kconfig
>> > > > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
>> > > > @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
>> > > >
>> > > > If unsure, say Y to make it easier to debug problems.
>> > > >
>> > > > +config ATH10K_LEDS
>> > > > + bool "Atheros ath10k LED support"
>> > > > + depends on ATH10K
>> > > > + select MAC80211_LEDS
>> > > > + select LEDS_CLASS
>> > > > + select NEW_LEDS
>> > > > + default y
>> > > > + help
>> > > > + This option enables LEDs support for chipset LED pins.
>> > > > + Each pin is connected via GPIO and can be controlled using
>> > > > + WMI Firmware API.
>> > > > +
>> > > > + The LED device will get available named as "ath10k-phyX" at sysfs and
>> > > > + can be controlled with various triggers.
>> > > > +
>> > > > + Say Y, if you have LED pins connected to the ath10k wireless card.
>> > >
>> > > I'm not sure anymore if we should ask anything from the user, better to
>> > > enable automatically if LED support is enabled in the kernel. So I
>> > > simplified this to:
>> > >
>> > > config ATH10K_LEDS
>> > > bool
>> > > depends on ATH10K
>> > > depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
>> > > default y
>> > >
>> > > This follows what mt76 does:
>> > >
>> > > config MT76_LEDS
>> > > bool
>> > > depends on MT76_CORE
>> > > depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
>> > > default y
>> > >
>> >
>> > I remember there was the same discussion in a previous series. OK for me
>> > for making this by default, only concern is any buildbot error (if any)
>> >
>> > Anyway OK for the change.
>> >
>> > > > @@ -65,6 +66,7 @@ static const struct ath10k_hw_params
>> > > > ath10k_hw_params_list[] = {
>> > > > .dev_id = QCA988X_2_0_DEVICE_ID,
>> > > > .bus = ATH10K_BUS_PCI,
>> > > > .name = "qca988x hw2.0",
>> > > > + .led_pin = 1,
>> > > > .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
>> > > > .uart_pin = 7,
>> > > > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
>> > >
>> > > I prefer following the field order from struct ath10k_hw_params
>> > > declaration and also setting fields explicitly to zero (even though
>> > > there are gaps still) so I changed that for every entry.
>> > >
>> >
>> > Thanks for the change, np for me.
>> >
>> > > > +int ath10k_leds_register(struct ath10k *ar)
>> > > > +{
>> > > > + int ret;
>> > > > +
>> > > > + if (ar->hw_params.led_pin == 0)
>> > > > + /* leds not supported */
>> > > > + return 0;
>> > > > +
>> > > > + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
>> > > > + wiphy_name(ar->hw->wiphy));
>> > > > + ar->leds.wifi_led.active_low = 1;
>> > > > + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
>> > > > + ar->leds.wifi_led.name = ar->leds.label;
>> > > > + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
>> > > > +
>> > > > + ar->leds.cdev.name = ar->leds.label;
>> > > > + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
>> > > > +
>> > > > + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
>> > > > + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
>> > >
>> > > But what to do with this FIXME?
>> > >
>> >
>> > It was pushed by you in v13.
>> >
>> > I could be wrong but your idea was to prepare for future support of
>> > other patch that would set the default_trigger to the mac80211 tpt.
>> >
>> > We might got both confused by default_trigger and default_state.
>> > default_trigger is actually never set and is NULL (actually it's 0)
>> >
>> > We have other 2 patch that adds tpt rates for the mac80211 LED trigger
>> > and set this trigger as the default one but honestly I would chose a
>> > different implementation than hardcoding everything.
>> >
>> > If it's ok for you, I would drop the comment and the default_trigger and
>> > I will send a follow-up patch to this adding DT support by using
>> > led_classdev_register_ext and defining init_data.
>> > (and this indirectly would permit better LED naming and defining of
>> > default-trigger in DT)
>> >
>> > Also ideally I will also send a patch for default_state following
>> > standard LED implementation. (to set default_state in DT)
>> >
>> > I would prefer this approach as the LED patch already took way too much
>> > time and I think it's better to merge this initial version and then
>> > improve it.
>>
>> If you want to check out I attached the 2 patch (one dt-bindings and the
>> one for the code) that I will submit when this will be merged (the
>> change is with the assumption that the FIXME line is dropped)
>>
>> Tested and works correctly with my use case of wifi card attached with
>> pcie. This implementation permits to declare the default trigger in DT
>> instead of hardcoding.
>>
>
> Any news with this? Did I notice the LEDs patch are still in pending...

Sorry for the delay but finally I looked at this again. I decided to
just remove the fixme and otherwise it looks good for me. Please check
my changes:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-05-09 10:05:02

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On Thu, May 09, 2024 at 07:50:40AM +0300, Kalle Valo wrote:
> Ansuel Smith <[email protected]> writes:
>
> > Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
> > <[email protected]> ha scritto:
> >
> >>
> >> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
> >> > On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> >> > > Christian Marangi <[email protected]> writes:
> >> > >
> >> > > > From: Sebastian Gottschall <[email protected]>
> >> > > >
> >> > > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
> >> > > > based chipsets with on chipset connected led's using WMI Firmware API.
> >> > > > The LED device will get available named as "ath10k-phyX" at sysfs and
> >> > > > can be controlled with various triggers.
> >> > > > Adds also debugfs interface for gpio control.
> >> > > >
> >> > > > Signed-off-by: Sebastian Gottschall <[email protected]>
> >> > > > Reviewed-by: Steve deRosier <[email protected]>
> >> > > > [kvalo: major reorg and cleanup]
> >> > > > Signed-off-by: Kalle Valo <[email protected]>
> >> > > > [ansuel: rebase and small cleanup]
> >> > > > Signed-off-by: Christian Marangi <[email protected]>
> >> > > > Tested-by: Stefan Lippers-Hollmann <[email protected]>
> >> > > > ---
> >> > > >
> >> > > > Hi,
> >> > > > this is a very old patch from 2018 that somehow was talked till 2020
> >> > > > with Kavlo asked to rebase and resubmit and nobody did.
> >> > > > So here we are in 2023 with me trying to finally have this upstream.
> >> > > >
> >> > > > A summarize of the situation.
> >> > > > - The patch is from years in OpenWRT. Used by anything that has ath10k
> >> > > > card and a LED connected.
> >> > > > - This patch is also used by the fw variant from Candela Tech with no
> >> > > > problem reported.
> >> > > > - It was pointed out that this caused some problem with ipq4019 SoC
> >> > > > but the problem was actually caused by a different bug related to
> >> > > > interrupts.
> >> > > >
> >> > > > I honestly hope we can have this feature merged since it's really
> >> > > > funny to have something that was so near merge and jet still not
> >> > > > present and with devices not supporting this simple but useful
> >> > > > feature.
> >> > >
> >> > > Indeed, we should finally get this in. Thanks for working on it.
> >> > >
> >> > > I did some minor changes to the patch, they are in my pending branch:
> >> > >
> >> > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
> >> > >
> >> > > My comments below, please review my changes. No need to resend because
> >> > > of these.
> >> > >
> >> >
> >> > Hi,
> >> > very happy this is going further.
> >> >
> >> > > > --- a/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > > @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
> >> > > >
> >> > > > If unsure, say Y to make it easier to debug problems.
> >> > > >
> >> > > > +config ATH10K_LEDS
> >> > > > + bool "Atheros ath10k LED support"
> >> > > > + depends on ATH10K
> >> > > > + select MAC80211_LEDS
> >> > > > + select LEDS_CLASS
> >> > > > + select NEW_LEDS
> >> > > > + default y
> >> > > > + help
> >> > > > + This option enables LEDs support for chipset LED pins.
> >> > > > + Each pin is connected via GPIO and can be controlled using
> >> > > > + WMI Firmware API.
> >> > > > +
> >> > > > + The LED device will get available named as "ath10k-phyX" at sysfs and
> >> > > > + can be controlled with various triggers.
> >> > > > +
> >> > > > + Say Y, if you have LED pins connected to the ath10k wireless card.
> >> > >
> >> > > I'm not sure anymore if we should ask anything from the user, better to
> >> > > enable automatically if LED support is enabled in the kernel. So I
> >> > > simplified this to:
> >> > >
> >> > > config ATH10K_LEDS
> >> > > bool
> >> > > depends on ATH10K
> >> > > depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> >> > > default y
> >> > >
> >> > > This follows what mt76 does:
> >> > >
> >> > > config MT76_LEDS
> >> > > bool
> >> > > depends on MT76_CORE
> >> > > depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
> >> > > default y
> >> > >
> >> >
> >> > I remember there was the same discussion in a previous series. OK for me
> >> > for making this by default, only concern is any buildbot error (if any)
> >> >
> >> > Anyway OK for the change.
> >> >
> >> > > > @@ -65,6 +66,7 @@ static const struct ath10k_hw_params
> >> > > > ath10k_hw_params_list[] = {
> >> > > > .dev_id = QCA988X_2_0_DEVICE_ID,
> >> > > > .bus = ATH10K_BUS_PCI,
> >> > > > .name = "qca988x hw2.0",
> >> > > > + .led_pin = 1,
> >> > > > .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> >> > > > .uart_pin = 7,
> >> > > > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
> >> > >
> >> > > I prefer following the field order from struct ath10k_hw_params
> >> > > declaration and also setting fields explicitly to zero (even though
> >> > > there are gaps still) so I changed that for every entry.
> >> > >
> >> >
> >> > Thanks for the change, np for me.
> >> >
> >> > > > +int ath10k_leds_register(struct ath10k *ar)
> >> > > > +{
> >> > > > + int ret;
> >> > > > +
> >> > > > + if (ar->hw_params.led_pin == 0)
> >> > > > + /* leds not supported */
> >> > > > + return 0;
> >> > > > +
> >> > > > + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
> >> > > > + wiphy_name(ar->hw->wiphy));
> >> > > > + ar->leds.wifi_led.active_low = 1;
> >> > > > + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
> >> > > > + ar->leds.wifi_led.name = ar->leds.label;
> >> > > > + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
> >> > > > +
> >> > > > + ar->leds.cdev.name = ar->leds.label;
> >> > > > + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
> >> > > > +
> >> > > > + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
> >> > > > + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
> >> > >
> >> > > But what to do with this FIXME?
> >> > >
> >> >
> >> > It was pushed by you in v13.
> >> >
> >> > I could be wrong but your idea was to prepare for future support of
> >> > other patch that would set the default_trigger to the mac80211 tpt.
> >> >
> >> > We might got both confused by default_trigger and default_state.
> >> > default_trigger is actually never set and is NULL (actually it's 0)
> >> >
> >> > We have other 2 patch that adds tpt rates for the mac80211 LED trigger
> >> > and set this trigger as the default one but honestly I would chose a
> >> > different implementation than hardcoding everything.
> >> >
> >> > If it's ok for you, I would drop the comment and the default_trigger and
> >> > I will send a follow-up patch to this adding DT support by using
> >> > led_classdev_register_ext and defining init_data.
> >> > (and this indirectly would permit better LED naming and defining of
> >> > default-trigger in DT)
> >> >
> >> > Also ideally I will also send a patch for default_state following
> >> > standard LED implementation. (to set default_state in DT)
> >> >
> >> > I would prefer this approach as the LED patch already took way too much
> >> > time and I think it's better to merge this initial version and then
> >> > improve it.
> >>
> >> If you want to check out I attached the 2 patch (one dt-bindings and the
> >> one for the code) that I will submit when this will be merged (the
> >> change is with the assumption that the FIXME line is dropped)
> >>
> >> Tested and works correctly with my use case of wifi card attached with
> >> pcie. This implementation permits to declare the default trigger in DT
> >> instead of hardcoding.
> >>
> >
> > Any news with this? Did I notice the LEDs patch are still in pending...
>
> Sorry for the delay but finally I looked at this again. I decided to
> just remove the fixme and otherwise it looks good for me. Please check
> my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>

All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
that intended?

Aside from this very happy that we are finally finishing with this long
lasting feature!

--
Ansuel

2024-05-09 16:38:20

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On 5/8/2024 9:50 PM, Kalle Valo wrote:
> Sorry for the delay but finally I looked at this again. I decided to
> just remove the fixme and otherwise it looks good for me. Please check
> my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>

I have a question about the copyrights in the two new files:
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.

My understanding is that Qualcomm's affiliation with Linux Foundation via Code
Aurora ended in December 2021, and hence any contributions in 2022-2023 should
be the copyright of Qualcomm Innovation Center, Inc.

2024-05-09 16:48:39

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On 5/9/2024 9:37 AM, Jeff Johnson wrote:
> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
>
> I have a question about the copyrights in the two new files:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
> be the copyright of Qualcomm Innovation Center, Inc.
>
>

ok it seems like Kalle's v13 had:
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.

and Ansuel's v14 has:
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.

So Ansuel, is your work on behalf of The Linux Foundation?

2024-05-10 08:53:47

by Sebastian Gottschall

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Am 09.05.2024 um 12:04 schrieb Christian Marangi:
> On Thu, May 09, 2024 at 07:50:40AM +0300, Kalle Valo wrote:
>> Ansuel Smith <[email protected]> writes:
>>
>>> Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
>>> <[email protected]> ha scritto:
>>>
>>>> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
>>>>> On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
>>>>>> Christian Marangi <[email protected]> writes:
>>>>>>
>>>>>>> From: Sebastian Gottschall <[email protected]>
>>>>>>>
>>>>>>> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
>>>>>>> based chipsets with on chipset connected led's using WMI Firmware API.
>>>>>>> The LED device will get available named as "ath10k-phyX" at sysfs and
>>>>>>> can be controlled with various triggers.
>>>>>>> Adds also debugfs interface for gpio control.
>>>>>>>
>>>>>>> Signed-off-by: Sebastian Gottschall <[email protected]>
>>>>>>> Reviewed-by: Steve deRosier <[email protected]>
>>>>>>> [kvalo: major reorg and cleanup]
>>>>>>> Signed-off-by: Kalle Valo <[email protected]>
>>>>>>> [ansuel: rebase and small cleanup]
>>>>>>> Signed-off-by: Christian Marangi <[email protected]>
>>>>>>> Tested-by: Stefan Lippers-Hollmann <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> Hi,
>>>>>>> this is a very old patch from 2018 that somehow was talked till 2020
>>>>>>> with Kavlo asked to rebase and resubmit and nobody did.
>>>>>>> So here we are in 2023 with me trying to finally have this upstream.
>>>>>>>
>>>>>>> A summarize of the situation.
>>>>>>> - The patch is from years in OpenWRT. Used by anything that has ath10k
>>>>>>> card and a LED connected.
>>>>>>> - This patch is also used by the fw variant from Candela Tech with no
>>>>>>> problem reported.
>>>>>>> - It was pointed out that this caused some problem with ipq4019 SoC
>>>>>>> but the problem was actually caused by a different bug related to
>>>>>>> interrupts.
>>>>>>>
>>>>>>> I honestly hope we can have this feature merged since it's really
>>>>>>> funny to have something that was so near merge and jet still not
>>>>>>> present and with devices not supporting this simple but useful
>>>>>>> feature.
>>>>>> Indeed, we should finally get this in. Thanks for working on it.
>>>>>>
>>>>>> I did some minor changes to the patch, they are in my pending branch:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
>>>>>>
>>>>>> My comments below, please review my changes. No need to resend because
>>>>>> of these.
>>>>>>
>>>>> Hi,
>>>>> very happy this is going further.
>>>>>
>>>>>>> --- a/drivers/net/wireless/ath/ath10k/Kconfig
>>>>>>> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
>>>>>>> @@ -67,6 +67,23 @@ config ATH10K_DEBUGFS
>>>>>>>
>>>>>>> If unsure, say Y to make it easier to debug problems.
>>>>>>>
>>>>>>> +config ATH10K_LEDS
>>>>>>> + bool "Atheros ath10k LED support"
>>>>>>> + depends on ATH10K
>>>>>>> + select MAC80211_LEDS
>>>>>>> + select LEDS_CLASS
>>>>>>> + select NEW_LEDS
>>>>>>> + default y
>>>>>>> + help
>>>>>>> + This option enables LEDs support for chipset LED pins.
>>>>>>> + Each pin is connected via GPIO and can be controlled using
>>>>>>> + WMI Firmware API.
>>>>>>> +
>>>>>>> + The LED device will get available named as "ath10k-phyX" at sysfs and
>>>>>>> + can be controlled with various triggers.
>>>>>>> +
>>>>>>> + Say Y, if you have LED pins connected to the ath10k wireless card.
>>>>>> I'm not sure anymore if we should ask anything from the user, better to
>>>>>> enable automatically if LED support is enabled in the kernel. So I
>>>>>> simplified this to:
>>>>>>
>>>>>> config ATH10K_LEDS
>>>>>> bool
>>>>>> depends on ATH10K
>>>>>> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
>>>>>> default y
>>>>>>
>>>>>> This follows what mt76 does:
>>>>>>
>>>>>> config MT76_LEDS
>>>>>> bool
>>>>>> depends on MT76_CORE
>>>>>> depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
>>>>>> default y
>>>>>>
>>>>> I remember there was the same discussion in a previous series. OK for me
>>>>> for making this by default, only concern is any buildbot error (if any)
>>>>>
>>>>> Anyway OK for the change.
>>>>>
>>>>>>> @@ -65,6 +66,7 @@ static const struct ath10k_hw_params
>>>>>>> ath10k_hw_params_list[] = {
>>>>>>> .dev_id = QCA988X_2_0_DEVICE_ID,
>>>>>>> .bus = ATH10K_BUS_PCI,
>>>>>>> .name = "qca988x hw2.0",
>>>>>>> + .led_pin = 1,
>>>>>>> .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
>>>>>>> .uart_pin = 7,
>>>>>>> .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
>>>>>> I prefer following the field order from struct ath10k_hw_params
>>>>>> declaration and also setting fields explicitly to zero (even though
>>>>>> there are gaps still) so I changed that for every entry.
>>>>>>
>>>>> Thanks for the change, np for me.
>>>>>
>>>>>>> +int ath10k_leds_register(struct ath10k *ar)
>>>>>>> +{
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + if (ar->hw_params.led_pin == 0)
>>>>>>> + /* leds not supported */
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
>>>>>>> + wiphy_name(ar->hw->wiphy));
>>>>>>> + ar->leds.wifi_led.active_low = 1;
>>>>>>> + ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
>>>>>>> + ar->leds.wifi_led.name = ar->leds.label;
>>>>>>> + ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
>>>>>>> +
>>>>>>> + ar->leds.cdev.name = ar->leds.label;
>>>>>>> + ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
>>>>>>> +
>>>>>>> + /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
>>>>>>> + ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
>>>>>> But what to do with this FIXME?
>>>>>>
>>>>> It was pushed by you in v13.
>>>>>
>>>>> I could be wrong but your idea was to prepare for future support of
>>>>> other patch that would set the default_trigger to the mac80211 tpt.
>>>>>
>>>>> We might got both confused by default_trigger and default_state.
>>>>> default_trigger is actually never set and is NULL (actually it's 0)
>>>>>
>>>>> We have other 2 patch that adds tpt rates for the mac80211 LED trigger
>>>>> and set this trigger as the default one but honestly I would chose a
>>>>> different implementation than hardcoding everything.
>>>>>
>>>>> If it's ok for you, I would drop the comment and the default_trigger and
>>>>> I will send a follow-up patch to this adding DT support by using
>>>>> led_classdev_register_ext and defining init_data.
>>>>> (and this indirectly would permit better LED naming and defining of
>>>>> default-trigger in DT)
>>>>>
>>>>> Also ideally I will also send a patch for default_state following
>>>>> standard LED implementation. (to set default_state in DT)
>>>>>
>>>>> I would prefer this approach as the LED patch already took way too much
>>>>> time and I think it's better to merge this initial version and then
>>>>> improve it.
>>>> If you want to check out I attached the 2 patch (one dt-bindings and the
>>>> one for the code) that I will submit when this will be merged (the
>>>> change is with the assumption that the FIXME line is dropped)
>>>>
>>>> Tested and works correctly with my use case of wifi card attached with
>>>> pcie. This implementation permits to declare the default trigger in DT
>>>> instead of hardcoding.
>>>>
>>> Any news with this? Did I notice the LEDs patch are still in pending...
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
> All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> that intended?
>
> Aside from this very happy that we are finally finishing with this long
> lasting feature!

since ATH10K_LEDS is no exposed option anymore. how is this feature
enabled then? Its not selected by any dependency

Sebastian

>

2024-05-10 13:51:10

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Christian Marangi <[email protected]> writes:

>>
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
>
> All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> that intended?

Yes. It follows the same idea as other wireless drivers do, for example iwlwifi:

config IWLWIFI_LEDS
bool
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
depends on IWLMVM || IWLDVM
select LEDS_TRIGGERS
select MAC80211_LEDS
default y

So what this patch now does:

config ATH10K_LEDS
bool
depends on ATH10K
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
default y

The idea being that if LEDS_CLASS is enabled then ATH10K_LEDS is
automatically enabled. But please let us know if something is wrong
here.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-05-10 13:52:30

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Jeff Johnson <[email protected]> writes:

> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>> Sorry for the delay but finally I looked at this again. I decided to
>>> just remove the fixme and otherwise it looks good for me. Please check
>>> my changes:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>
>>
>> I have a question about the copyrights in the two new files:
>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>
>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>> be the copyright of Qualcomm Innovation Center, Inc.
>>
>>
>
> ok it seems like Kalle's v13 had:
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>
> and Ansuel's v14 has:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> So Ansuel, is your work on behalf of The Linux Foundation?

BTW in the pending branch I can change the copyright back to original so
no need to resend because of this. But I'll need guidance from Ansuel.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-05-10 14:11:13

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On Fri, May 10, 2024 at 04:50:52PM +0300, Kalle Valo wrote:
> Christian Marangi <[email protected]> writes:
>
> >>
> >> Sorry for the delay but finally I looked at this again. I decided to
> >> just remove the fixme and otherwise it looks good for me. Please check
> >> my changes:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
> >>
> >
> > All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> > that intended?
>
> Yes. It follows the same idea as other wireless drivers do, for example iwlwifi:
>
> config IWLWIFI_LEDS
> bool
> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> depends on IWLMVM || IWLDVM
> select LEDS_TRIGGERS
> select MAC80211_LEDS
> default y
>
> So what this patch now does:
>
> config ATH10K_LEDS
> bool
> depends on ATH10K
> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> default y
>
> The idea being that if LEDS_CLASS is enabled then ATH10K_LEDS is
> automatically enabled. But please let us know if something is wrong
> here.
>

Sure, was just asking some clarification about it. Not a problem for me
since we are using other pattern to handle this.

--
Ansuel

2024-05-10 14:14:58

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
> > On 5/8/2024 9:50 PM, Kalle Valo wrote:
> >> Sorry for the delay but finally I looked at this again. I decided to
> >> just remove the fixme and otherwise it looks good for me. Please check
> >> my changes:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
> >>
> >
> > I have a question about the copyrights in the two new files:
> > + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
> >
> > My understanding is that Qualcomm's affiliation with Linux Foundation via Code
> > Aurora ended in December 2021, and hence any contributions in 2022-2023 should
> > be the copyright of Qualcomm Innovation Center, Inc.
> >
> >
>
> ok it seems like Kalle's v13 had:
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>
> and Ansuel's v14 has:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> So Ansuel, is your work on behalf of The Linux Foundation?
>

When I resubmitted this at times, I just updated the copyright to the
current year so I guess it was wrong doing that?

As you can see from the copyright header this patch went all around and
I think at the end (around 2018) the Linux copyright was added as it was
submitted upstream. (can't remember if maintainers were asking that)

So me watching the old year and resubmitting it, just updated the date.

Soo I think we should revert to 2018?

--
Ansuel

2024-05-10 14:54:50

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

On 5/10/2024 7:14 AM, Christian Marangi wrote:
> On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
>> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>>> Sorry for the delay but finally I looked at this again. I decided to
>>>> just remove the fixme and otherwise it looks good for me. Please check
>>>> my changes:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>>
>>>
>>> I have a question about the copyrights in the two new files:
>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>
>>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>>> be the copyright of Qualcomm Innovation Center, Inc.
>>>
>>>
>>
>> ok it seems like Kalle's v13 had:
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>
>> and Ansuel's v14 has:
>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>
>> So Ansuel, is your work on behalf of The Linux Foundation?
>>
>
> When I resubmitted this at times, I just updated the copyright to the
> current year so I guess it was wrong doing that?
>
> As you can see from the copyright header this patch went all around and
> I think at the end (around 2018) the Linux copyright was added as it was
> submitted upstream. (can't remember if maintainers were asking that)
>
> So me watching the old year and resubmitting it, just updated the date.
>
> Soo I think we should revert to 2018?
>

Yes, in this case changing the Linux Foundation copyright back to 2018 is correct.

/jeff

2024-05-11 14:18:03

[permalink] [raw]

Subject: Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets

Jeff Johnson <[email protected]> writes:

> On 5/10/2024 7:14 AM, Christian Marangi wrote:
>
>> On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
>>> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>>>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>>>> Sorry for the delay but finally I looked at this again. I decided to
>>>>> just remove the fixme and otherwise it looks good for me. Please check
>>>>> my changes:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>>>
>>>>
>>>> I have a question about the copyrights in the two new files:
>>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>>
>>>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>>>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>>>> be the copyright of Qualcomm Innovation Center, Inc.
>>>>
>>>>
>>>
>>> ok it seems like Kalle's v13 had:
>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>
>>> and Ansuel's v14 has:
>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>
>>> So Ansuel, is your work on behalf of The Linux Foundation?
>>>
>>
>> When I resubmitted this at times, I just updated the copyright to the
>> current year so I guess it was wrong doing that?
>>
>> As you can see from the copyright header this patch went all around and
>> I think at the end (around 2018) the Linux copyright was added as it was
>> submitted upstream. (can't remember if maintainers were asking that)
>>
>> So me watching the old year and resubmitting it, just updated the date.
>>
>> Soo I think we should revert to 2018?
>>
>
> Yes, in this case changing the Linux Foundation copyright back to 2018 is correct.

I changed it now back to 2018, please check:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5eff06bef76b6d4e1553c2d4978025c329d8db35

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-05-13 15:09:13