Discussion:
[PATCH 2/3] ath5k: led, cleanup and prepare macros
(too old to reply)
Jiri Slaby
2009-05-20 20:08:58 UTC
Permalink
From: Jiri Slaby <jirislaby-***@public.gmane.org>

We need to use driver_data for more than pin and polaroty data.
Remove driver_data designator from ATH_LED definition and write
it explicitly.

Signed-off-by: Jiri Slaby <jirislaby-***@public.gmane.org>
---
drivers/net/wireless/ath/ath5k/led.c | 33 +++++++++++++++++++++------------
1 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/led.c b/drivers/net/wireless/ath/ath5k/led.c
index 762e40d..0374b1f 100644
--- a/drivers/net/wireless/ath/ath5k/led.c
+++ b/drivers/net/wireless/ath/ath5k/led.c
@@ -47,30 +47,39 @@
.vendor = PCI_ANY_ID, .device = PCI_ANY_ID, \
.subvendor = (subv), .subdevice = (subd)

-#define ATH_LED(pin,polarity) .driver_data = (((pin) << 8) | (polarity))
-#define ATH_PIN(data) ((data) >> 8)
-#define ATH_POLARITY(data) ((data) & 0xff)
+#define ATH_LED(pin, polarity) (((pin) << 8) | (polarity))
+#define ATH_PIN(data) (((data) >> 8) & 0xffff)
+#define ATH_POLARITY(data) ((data) & 0xff)

/* Devices we match on for LED config info (typically laptops) */
static const struct pci_device_id ath5k_led_devices[] = {
/* AR5211 */
- { PCI_VDEVICE(ATHEROS, PCI_DEVICE_ID_ATHEROS_AR5211), ATH_LED(0, 0) },
+ { PCI_VDEVICE(ATHEROS, PCI_DEVICE_ID_ATHEROS_AR5211),
+ .driver_data = ATH_LED(0, 0) },
/* HP Compaq nc6xx, nc4000, nx6000 */
- { ATH_SDEVICE(PCI_VENDOR_ID_COMPAQ, PCI_ANY_ID), ATH_LED(1, 1) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_COMPAQ, PCI_ANY_ID),
+ .driver_data = ATH_LED(1, 1) },
/* Acer Aspire One A150 (maximlevitsky-***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_FOXCONN, 0xe008), ATH_LED(3, 0) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_FOXCONN, 0xe008),
+ .driver_data = ATH_LED(3, 0) },
/* Acer Ferrari 5000 (russ.dill-***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_AMBIT, 0x0422), ATH_LED(1, 1) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_AMBIT, 0x0422),
+ .driver_data = ATH_LED(1, 1) },
/* E-machines E510 (tuliom-***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_AMBIT, 0x0428), ATH_LED(3, 0) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_AMBIT, 0x0428),
+ .driver_data = ATH_LED(3, 0) },
/* Acer Extensa 5620z (nekoreeve-***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_QMI, 0x0105), ATH_LED(3, 0) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_QMI, 0x0105),
+ .driver_data = ATH_LED(3, 0) },
/* Fukato Datacask Jupiter 1014a (mrb74-***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_AZWAVE, 0x1026), ATH_LED(3, 0) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_AZWAVE, 0x1026),
+ .driver_data = ATH_LED(3, 0) },
/* IBM ThinkPad AR5BXB6 (legovini-ul0cmuUPbMe6zEtDuAPGx/***@public.gmane.org) */
- { ATH_SDEVICE(PCI_VENDOR_ID_IBM, 0x058a), ATH_LED(1, 0) },
+ { ATH_SDEVICE(PCI_VENDOR_ID_IBM, 0x058a),
+ .driver_data = ATH_LED(1, 0) },
/* IBM-specific AR5212 (all others) */
- { PCI_VDEVICE(ATHEROS, PCI_DEVICE_ID_ATHEROS_AR5212_IBM), ATH_LED(0, 0) },
+ { PCI_VDEVICE(ATHEROS, PCI_DEVICE_ID_ATHEROS_AR5212_IBM),
+ .driver_data = ATH_LED(0, 0) },
{ }
};
--
1.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Tobias Doerffel
2009-05-20 20:16:42 UTC
Permalink
Hi,
I found these patches on the web not being merged since Jan. Was
there any reason why they shouldn't be accepted?
Yes - they needed further work. The approach was wrong because the GPIO does
not control the LED rather than some internal rfkill mechanism (which itself
controls the LED). I therefore reworked things to have general RFKILL support
in ath5k and proposed the patches. They were reviewed by Bob Copeland and I
addressed most of the issues so far but didn't come up with a new patch yet,
as there're still minor things to be worked out. If there's interest, I'll
prepare a new patch which represents the current state.

Toby
Jiri Slaby
2009-05-20 20:28:19 UTC
Permalink
Post by Tobias Doerffel
Hi,
I found these patches on the web not being merged since Jan. Was
there any reason why they shouldn't be accepted?
Yes - they needed further work. The approach was wrong because the GPIO does
not control the LED rather than some internal rfkill mechanism (which itself
controls the LED). I therefore reworked things to have general RFKILL support
in ath5k and proposed the patches. They were reviewed by Bob Copeland and I
addressed most of the issues so far but didn't come up with a new patch yet,
as there're still minor things to be worked out. If there's interest, I'll
prepare a new patch which represents the current state.
I think we are definitely interested. Please send an updated patch.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bob Copeland
2009-05-20 20:29:42 UTC
Permalink
If there's interest, I'll prepare a new patch which represents the
current state.
I'm definitely interested in seeing rfkill gpio support getting in,
it's overdue. So I say repost it and we can fasttrack that. Digging
up your last patchset was already on my todo list.

As for patch 2/3 it might be a good idea to have support for
which-led-does-what encoded into the driver data anyway; other
people have asked for that and the "solution" of adding various
echos to sysfs LED trigger files is rather sad. OTOH it could
eventually be a lot of data to carry around.
--
Bob Copeland %% www.bobcopeland.com
Tobias Doerffel
2009-05-25 23:24:02 UTC
Permalink
Hi,
Post by Bob Copeland
I'm definitely interested in seeing rfkill gpio support getting in,
it's overdue. So I say repost it and we can fasttrack that. Digging
up your last patchset was already on my todo list.
Here we go. I attached a patch (against wireless-testing) which represents my
latest work on RFKILL support for ath5k. It still lacks testing with a real
physical rfkill switch (for I don't have one with my NC10) but everything else
(SW rfkill) works fine after testing various scenarios. Furthermore I think I
addressed all remarks in the last review by Bob Copeland.

Regards,

Toby
Johannes Berg
2009-05-25 23:32:46 UTC
Permalink
+ err = rfkill_register(sc->rf_kill.rfkill);
+ if (err) {
+ sc->rf_kill.rfkill = NULL;
+ return -1;
+ }
Missing rfkill_free, and why -EPERM? Could you try to work against v11
of my rfkill patch, or better even against the cfg80211 rfkill instead?

johannes
Tobias Doerffel
2009-05-29 22:33:07 UTC
Permalink
Hi,
Post by Johannes Berg
+ err = rfkill_register(sc->rf_kill.rfkill);
+ if (err) {
+ sc->rf_kill.rfkill = NULL;
+ return -1;
+ }
Missing rfkill_free, and why -EPERM?
Thanks for this remark. I fixed the code and will post an updated patch soon.

Any other comments on the patch? If not, any chance to get it merged into
wireless-testing branch?
Post by Johannes Berg
Could you try to work against v11
of my rfkill patch, or better even against the cfg80211 rfkill instead?
I didn't look at both of them yet. However I think we first should try to get
current rfkill support into mainline as fast as possible. Improvements and
adaptations to reworked rfkill framework can follow.

Regards,

Toby
Johannes Berg
2009-05-29 23:14:30 UTC
Permalink
Post by Tobias Doerffel
Post by Johannes Berg
Could you try to work against v11
of my rfkill patch, or better even against the cfg80211 rfkill instead?
I didn't look at both of them yet. However I think we first should try to get
current rfkill support into mainline as fast as possible. Improvements and
adaptations to reworked rfkill framework can follow.
No other comments from me -- but since the v11 of the rfkill rewrite
will certainly hit the tree before your patch (it's almost in) you
_will_ have to work against it. I'll also try to make the cfg80211
rfkill hit the tree very soon for reasons I'll explain elsewhere -- you
would be better off working against that because then your rfkill code
is very very very simple and doesn't need to do much at all.

johannes
Alan Jenkins
2009-05-30 15:26:13 UTC
Permalink
Post by Johannes Berg
Post by Tobias Doerffel
Post by Johannes Berg
Could you try to work against v11
of my rfkill patch, or better even against the cfg80211 rfkill instead?
I didn't look at both of them yet. However I think we first should try to get
current rfkill support into mainline as fast as possible. Improvements and
adaptations to reworked rfkill framework can follow.
No other comments from me -- but since the v11 of the rfkill rewrite
will certainly hit the tree before your patch (it's almost in) you
_will_ have to work against it. I'll also try to make the cfg80211
rfkill hit the tree very soon for reasons I'll explain elsewhere -- you
would be better off working against that because then your rfkill code
is very very very simple and doesn't need to do much at all.
johannes
Btw, I believe the new rfkill core behaviour should avoid the concern
I raised about the EeePC.

Reminder: the eeepc-laptop implements platform rfkill on the original
model(s) by hotplugging the entire ath5k device. If you boot with it
in the blocked state, the old rfkill core would "lock in" that state
as the default. If you then hotplug the ath5k device _and it comes up
with an rfkill device of it's own_, my concern is that the new rfkill
device would be initialized to a blocked state. It would be
impossible to enable the wireless.

The new core removes this idea of a separate "default" state, so it
can't have this problem.

Regards
Alan

Jiri Slaby
2009-05-20 20:08:57 UTC
Permalink
From: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>

I found these patches on the web not being merged since Jan. Was
there any reason why they shouldn't be accepted?
As there were no replies to this version of them, I'm resending
them for review and comments :). I rebased them and moved to the
current approach in the led file.
--
From: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>

Signed-off-by: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>
Signed-off-by: Jiri Slaby <jirislaby-***@public.gmane.org> [rebase]
---
drivers/net/wireless/ath/ath5k/base.h | 2 ++
drivers/net/wireless/ath/ath5k/led.c | 8 ++++++++
2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 852b2c1..e0e55ee 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -149,6 +149,8 @@ struct ath5k_softc {
unsigned int led_pin, /* GPIO pin for driving LED */
led_on; /* pin setting for LED on */

+ struct ath5k_led radio_led; /* Radio LED */
+
struct tasklet_struct restq; /* reset tasklet */

unsigned int rxbufsize; /* rx size based on mtu */
diff --git a/drivers/net/wireless/ath/ath5k/led.c b/drivers/net/wireless/ath/ath5k/led.c
index 876725f..762e40d 100644
--- a/drivers/net/wireless/ath/ath5k/led.c
+++ b/drivers/net/wireless/ath/ath5k/led.c
@@ -141,6 +141,7 @@ ath5k_unregister_led(struct ath5k_led *led)

void ath5k_unregister_leds(struct ath5k_softc *sc)
{
+ ath5k_unregister_led(&sc->radio_led);
ath5k_unregister_led(&sc->rx_led);
ath5k_unregister_led(&sc->tx_led);
}
@@ -174,6 +175,13 @@ int ath5k_init_leds(struct ath5k_softc *sc)
snprintf(name, sizeof(name), "ath5k-%s::tx", wiphy_name(hw->wiphy));
ret = ath5k_register_led(sc, &sc->tx_led, name,
ieee80211_get_tx_led_name(hw));
+ if (ret)
+ goto out;
+
+ snprintf(name, sizeof(name), "ath5k-%s::radio", wiphy_name(hw->wiphy));
+ ret = ath5k_register_led(sc, &sc->radio_led, name,
+ ieee80211_get_radio_led_name(hw));
+
out:
return ret;
}
--
1.6.3
Jiri Slaby
2009-05-20 20:08:59 UTC
Permalink
From: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>

From: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>

Add support for a radio led on devices with subvendor 0x144f.

That means don't register rx and tx leds for this particular device.
Add a flag for that into driver_data.

Signed-off-by: Tobias Doerffel <tobias.doerffel-***@public.gmane.org>
Signed-off-by: Jiri Slaby <jirislaby-***@public.gmane.org> [rebase]
---
drivers/net/wireless/ath/ath5k/led.c | 32 +++++++++++++++++++++-----------
1 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/led.c b/drivers/net/wireless/ath/ath5k/led.c
index 0374b1f..29bbb8b 100644
--- a/drivers/net/wireless/ath/ath5k/led.c
+++ b/drivers/net/wireless/ath/ath5k/led.c
@@ -43,10 +43,13 @@
#include "ath5k.h"
#include "base.h"

+#define PCI_VENDOR_ID_ASKEY 0x144f
+
#define ATH_SDEVICE(subv,subd) \
.vendor = PCI_ANY_ID, .device = PCI_ANY_ID, \
.subvendor = (subv), .subdevice = (subd)

+#define ATH_NO_RX_TX 0x01000000
#define ATH_LED(pin, polarity) (((pin) << 8) | (polarity))
#define ATH_PIN(data) (((data) >> 8) & 0xffff)
#define ATH_POLARITY(data) ((data) & 0xff)
@@ -68,6 +71,9 @@ static const struct pci_device_id ath5k_led_devices[] = {
/* E-machines E510 (tuliom-***@public.gmane.org) */
{ ATH_SDEVICE(PCI_VENDOR_ID_AMBIT, 0x0428),
.driver_data = ATH_LED(3, 0) },
+ /* Softled on PIN0 on Samsung NC10 (tobias.doerffel-***@public.gmane.org) */
+ { ATH_SDEVICE(PCI_VENDOR_ID_ASKEY, PCI_ANY_ID),
+ .driver_data = ATH_LED(0, 1) | ATH_NO_RX_TX },
/* Acer Extensa 5620z (nekoreeve-***@public.gmane.org) */
{ ATH_SDEVICE(PCI_VENDOR_ID_QMI, 0x0105),
.driver_data = ATH_LED(3, 0) },
@@ -175,17 +181,21 @@ int ath5k_init_leds(struct ath5k_softc *sc)

ath5k_led_enable(sc);

- snprintf(name, sizeof(name), "ath5k-%s::rx", wiphy_name(hw->wiphy));
- ret = ath5k_register_led(sc, &sc->rx_led, name,
- ieee80211_get_rx_led_name(hw));
- if (ret)
- goto out;
-
- snprintf(name, sizeof(name), "ath5k-%s::tx", wiphy_name(hw->wiphy));
- ret = ath5k_register_led(sc, &sc->tx_led, name,
- ieee80211_get_tx_led_name(hw));
- if (ret)
- goto out;
+ if (!(match && (match->driver_data & ATH_NO_RX_TX))) {
+ snprintf(name, sizeof(name), "ath5k-%s::rx",
+ wiphy_name(hw->wiphy));
+ ret = ath5k_register_led(sc, &sc->rx_led, name,
+ ieee80211_get_rx_led_name(hw));
+ if (ret)
+ goto out;
+
+ snprintf(name, sizeof(name), "ath5k-%s::tx",
+ wiphy_name(hw->wiphy));
+ ret = ath5k_register_led(sc, &sc->tx_led, name,
+ ieee80211_get_tx_led_name(hw));
+ if (ret)
+ goto out;
+ }

snprintf(name, sizeof(name), "ath5k-%s::radio", wiphy_name(hw->wiphy));
ret = ath5k_register_led(sc, &sc->radio_led, name,
--
1.6.3
Continue reading on narkive:
Loading...