On Mon, Feb 13, 2012 at 5:48 AM, Jakub Kicinski <kubakici@xxxxx> wrote:
> rt2800pci_set_state was introduced to group requests preceded with
> common introduction. As that introduction is no longer used
> rt2800pci_set_state can be removed.
>
> Note that this patch changes behaviour for STATE_DEEP_SLEEP and
> STATE_STANDBY, but those states are not used in rt2800 anyway.
>
> Signed-off-by: Jakub Kicinski <kubakici@xxxxx>
I know this patch has been out there for a while, but I only now have
taken a good look at it.
First of all, I think that it would have been easy to not change the
behaviour for STATE_DEEP_SLEEP
and STATE_STANDBY, even though these are not used in rt2800. They can
simply be put as their
own cases in the switch statement in which nothing is done.
Also some comments to the actual code below.
> ---
> drivers/net/wireless/rt2x00/rt2800pci.c | 38 ++++++++++--------------------
> 1 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 6ad6929..6ab4637 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -517,23 +517,6 @@ static void rt2800pci_disable_radio(struct rt2x00_dev *rt2x00dev)
> }
> }
>
> -static int rt2800pci_set_state(struct rt2x00_dev *rt2x00dev,
> - enum dev_state state)
> -{
> - if (state == STATE_AWAKE) {
> - rt2800_mcu_request(rt2x00dev, MCU_WAKEUP, TOKEN_WAKUP, 0, 0x02);
> - rt2800pci_mcu_status(rt2x00dev, TOKEN_WAKUP);
> - } else if (state == STATE_SLEEP) {
> - rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_STATUS,
> - 0xffffffff);
> - rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CID,
> - 0xffffffff);
> - rt2800_mcu_request(rt2x00dev, MCU_SLEEP, 0x01, 0xff, 0x01);
> - }
> -
> - return 0;
> -}
> -
> static int rt2800pci_set_device_state(struct rt2x00_dev *rt2x00dev,
> enum dev_state state)
> {
> @@ -546,7 +529,7 @@ static int rt2800pci_set_device_state(struct rt2x00_dev *rt2x00dev,
> * to be woken up. After that it needs a bit of time
> * to be fully awake and then the radio can be enabled.
> */
> - rt2800pci_set_state(rt2x00dev, STATE_AWAKE);
> + rt2800pci_set_device_state(rt2x00dev, STATE_AWAKE);
> msleep(1);
> retval = rt2800pci_enable_radio(rt2x00dev);
> break;
I don't like this recursive call, but this is removed in patch 4, so I
don't mind it here.
> @@ -556,17 +539,22 @@ static int rt2800pci_set_device_state(struct rt2x00_dev *rt2x00dev,
> * be put to sleep for powersaving.
> */
> rt2800pci_disable_radio(rt2x00dev);
> - rt2800pci_set_state(rt2x00dev, STATE_SLEEP);
> - break;
> - case STATE_RADIO_IRQ_ON:
> - case STATE_RADIO_IRQ_OFF:
> - rt2800pci_toggle_irq(rt2x00dev, state);
> - break;
> + /* fall through */
I don't really like this falling through, which is just there out of
convenience, as the states have
some common code. However, in reality these states have not a lot in
common, so just a bit
of code duplication would have been better here, IMHO.
> case STATE_DEEP_SLEEP:
> case STATE_SLEEP:
> case STATE_STANDBY:
> + /* PCIe devices won't report status after SLEEP request. */
> + rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_STATUS, ~0);
> + rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CID, ~0);
> + rt2800_mcu_request(rt2x00dev, MCU_SLEEP, 0x01, 0xff, 0x01);
> + break;
> case STATE_AWAKE:
> - retval = rt2800pci_set_state(rt2x00dev, state);
> + rt2800_mcu_request(rt2x00dev, MCU_WAKEUP, TOKEN_WAKUP, 0, 0x02);
> + rt2800pci_mcu_status(rt2x00dev, TOKEN_WAKUP);
> + break;
> + case STATE_RADIO_IRQ_ON:
> + case STATE_RADIO_IRQ_OFF:
> + rt2800pci_toggle_irq(rt2x00dev, state);
> break;
> default:
> retval = -ENOTSUPP;
---
Gertjan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
