2014-04-22 06:57:36

by Seungwon Jeon

[permalink] [raw]
Subject: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

As commit 3957848(mmc: drop the speed mode of card's state) applies,
this change should be followed.

drivers/net/wireless/rsi/rsi_91x_sdio.c:288:20: error: 'MMC_STATE_HIGHSPEED' undeclared (first use in this function)
drivers/net/wireless/rsi/rsi_91x_sdio.c:299:4: error: implicit declaration of function 'mmc_card_set_highspeed' [-Werror=implicit-function-declaration]
drivers/net/wireless/rsi/rsi_91x_sdio.c:306:2: error: implicit declaration of function 'mmc_card_highspeed' [-Werror=implicit-function-declaration]

Signed-off-by: Seungwon Jeon <[email protected]>
---
drivers/net/wireless/rsi/rsi_91x_sdio.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index 2e39d38..46e7af4 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -285,7 +285,6 @@ static void rsi_reset_card(struct sdio_func *pfunction)
if (err) {
rsi_dbg(ERR_ZONE, "%s: CCCR speed reg read failed: %d\n",
__func__, err);
- card->state &= ~MMC_STATE_HIGHSPEED;
} else {
err = rsi_cmd52writebyte(card,
SDIO_CCCR_SPEED,
@@ -296,14 +295,13 @@ static void rsi_reset_card(struct sdio_func *pfunction)
__func__, err);
return;
}
- mmc_card_set_highspeed(card);
host->ios.timing = MMC_TIMING_SD_HS;
host->ops->set_ios(host, &host->ios);
}
}

/* Set clock */
- if (mmc_card_highspeed(card))
+ if (mmc_card_hs(card))
clock = 50000000;
else
clock = card->cis.max_dtr;
--
1.7.0.4


2014-04-22 08:28:41

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

On 22 April 2014 08:57, Seungwon Jeon <[email protected]> wrote:
> As commit 3957848(mmc: drop the speed mode of card's state) applies,
> this change should be followed.
>
> drivers/net/wireless/rsi/rsi_91x_sdio.c:288:20: error: 'MMC_STATE_HIGHSPEED' undeclared (first use in this function)
> drivers/net/wireless/rsi/rsi_91x_sdio.c:299:4: error: implicit declaration of function 'mmc_card_set_highspeed' [-Werror=implicit-function-declaration]
> drivers/net/wireless/rsi/rsi_91x_sdio.c:306:2: error: implicit declaration of function 'mmc_card_highspeed' [-Werror=implicit-function-declaration]
>
> Signed-off-by: Seungwon Jeon <[email protected]>

Hi Seungwon,

Thanks for responding quickly!

> ---
> drivers/net/wireless/rsi/rsi_91x_sdio.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> index 2e39d38..46e7af4 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> @@ -285,7 +285,6 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> if (err) {
> rsi_dbg(ERR_ZONE, "%s: CCCR speed reg read failed: %d\n",
> __func__, err);
> - card->state &= ~MMC_STATE_HIGHSPEED;
> } else {
> err = rsi_cmd52writebyte(card,
> SDIO_CCCR_SPEED,
> @@ -296,14 +295,13 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> __func__, err);
> return;
> }
> - mmc_card_set_highspeed(card);
> host->ios.timing = MMC_TIMING_SD_HS;
> host->ops->set_ios(host, &host->ios);
> }
> }
>
> /* Set clock */
> - if (mmc_card_highspeed(card))
> + if (mmc_card_hs(card))
> clock = 50000000;
> else
> clock = card->cis.max_dtr;
> --
> 1.7.0.4
>
>

I am not sure it's safe to carry this patch through Chris' mmc tree
due to merge conflicts with John's wireless tree.

Though, since you have changed the mmc_card_highspeed() function to be
named to mmc_card_hs(), we need to work out the dependency.

We have some options to handle this, I suggest the following.

Re-spin this patch to keep using mmc_card_highspeed() and let John
take it through his wireless tree. Thus you also need to keep the
name mmc_card_highspeed() function from the patches to the mmc core -
could you please re-spin and post new version of those patches as
well!?

Kind regards
Ulf Hansson

2014-04-22 10:53:08

by Seungwon Jeon

[permalink] [raw]
Subject: RE: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

On Tue, April 22, 2014, Ulf Hansson wrote:
> On 22 April 2014 08:57, Seungwon Jeon <[email protected]> wrote:
> > As commit 3957848(mmc: drop the speed mode of card's state) applies,
> > this change should be followed.
> >
> > drivers/net/wireless/rsi/rsi_91x_sdio.c:288:20: error: 'MMC_STATE_HIGHSPEED' undeclared (first use
> in this function)
> > drivers/net/wireless/rsi/rsi_91x_sdio.c:299:4: error: implicit declaration of function
> 'mmc_card_set_highspeed' [-Werror=implicit-function-declaration]
> > drivers/net/wireless/rsi/rsi_91x_sdio.c:306:2: error: implicit declaration of function
> 'mmc_card_highspeed' [-Werror=implicit-function-declaration]
> >
> > Signed-off-by: Seungwon Jeon <[email protected]>
>
> Hi Seungwon,
>
> Thanks for responding quickly!
>
> > ---
> > drivers/net/wireless/rsi/rsi_91x_sdio.c | 4 +---
> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > index 2e39d38..46e7af4 100644
> > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > @@ -285,7 +285,6 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> > if (err) {
> > rsi_dbg(ERR_ZONE, "%s: CCCR speed reg read failed: %d\n",
> > __func__, err);
> > - card->state &= ~MMC_STATE_HIGHSPEED;
> > } else {
> > err = rsi_cmd52writebyte(card,
> > SDIO_CCCR_SPEED,
> > @@ -296,14 +295,13 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> > __func__, err);
> > return;
> > }
> > - mmc_card_set_highspeed(card);
> > host->ios.timing = MMC_TIMING_SD_HS;
> > host->ops->set_ios(host, &host->ios);
> > }
> > }
> >
> > /* Set clock */
> > - if (mmc_card_highspeed(card))
> > + if (mmc_card_hs(card))
> > clock = 50000000;
> > else
> > clock = card->cis.max_dtr;
> > --
> > 1.7.0.4
> >
> >
>
> I am not sure it's safe to carry this patch through Chris' mmc tree
> due to merge conflicts with John's wireless tree.
>
> Though, since you have changed the mmc_card_highspeed() function to be
> named to mmc_card_hs(), we need to work out the dependency.
>
> We have some options to handle this, I suggest the following.
>
> Re-spin this patch to keep using mmc_card_highspeed() and let John
> take it through his wireless tree. Thus you also need to keep the
It would be better if this patch can be handled in Chris's.
Because if the rest of changes of this patch with keeping "mmc_card_highspeed" is applied,
high-speed SDIO will be ignored in John's tree.

Thanks,
Seungwon Jeon

> name mmc_card_highspeed() function from the patches to the mmc core -
> could you please re-spin and post new version of those patches as
> well!?
>
> Kind regards
> Ulf Hansson

2014-04-22 11:36:16

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

On 22 April 2014 12:53, Seungwon Jeon <[email protected]> wrote:
> On Tue, April 22, 2014, Ulf Hansson wrote:
>> On 22 April 2014 08:57, Seungwon Jeon <[email protected]> wrote:
>> > As commit 3957848(mmc: drop the speed mode of card's state) applies,
>> > this change should be followed.
>> >
>> > drivers/net/wireless/rsi/rsi_91x_sdio.c:288:20: error: 'MMC_STATE_HIGHSPEED' undeclared (first use
>> in this function)
>> > drivers/net/wireless/rsi/rsi_91x_sdio.c:299:4: error: implicit declaration of function
>> 'mmc_card_set_highspeed' [-Werror=implicit-function-declaration]
>> > drivers/net/wireless/rsi/rsi_91x_sdio.c:306:2: error: implicit declaration of function
>> 'mmc_card_highspeed' [-Werror=implicit-function-declaration]
>> >
>> > Signed-off-by: Seungwon Jeon <[email protected]>
>>
>> Hi Seungwon,
>>
>> Thanks for responding quickly!
>>
>> > ---
>> > drivers/net/wireless/rsi/rsi_91x_sdio.c | 4 +---
>> > 1 files changed, 1 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > index 2e39d38..46e7af4 100644
>> > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > @@ -285,7 +285,6 @@ static void rsi_reset_card(struct sdio_func *pfunction)
>> > if (err) {
>> > rsi_dbg(ERR_ZONE, "%s: CCCR speed reg read failed: %d\n",
>> > __func__, err);
>> > - card->state &= ~MMC_STATE_HIGHSPEED;
>> > } else {
>> > err = rsi_cmd52writebyte(card,
>> > SDIO_CCCR_SPEED,
>> > @@ -296,14 +295,13 @@ static void rsi_reset_card(struct sdio_func *pfunction)
>> > __func__, err);
>> > return;
>> > }
>> > - mmc_card_set_highspeed(card);
>> > host->ios.timing = MMC_TIMING_SD_HS;
>> > host->ops->set_ios(host, &host->ios);
>> > }
>> > }
>> >
>> > /* Set clock */
>> > - if (mmc_card_highspeed(card))
>> > + if (mmc_card_hs(card))
>> > clock = 50000000;
>> > else
>> > clock = card->cis.max_dtr;
>> > --
>> > 1.7.0.4
>> >
>> >
>>
>> I am not sure it's safe to carry this patch through Chris' mmc tree
>> due to merge conflicts with John's wireless tree.
>>
>> Though, since you have changed the mmc_card_highspeed() function to be
>> named to mmc_card_hs(), we need to work out the dependency.
>>
>> We have some options to handle this, I suggest the following.
>>
>> Re-spin this patch to keep using mmc_card_highspeed() and let John
>> take it through his wireless tree. Thus you also need to keep the
> It would be better if this patch can be handled in Chris's.
> Because if the rest of changes of this patch with keeping "mmc_card_highspeed" is applied,
> high-speed SDIO will be ignored in John's tree.

You are right Seungwon!

Please re-spin the patchset on the mmc core an squash the code from
this patch, into the patch "mmc: drop the speed mode of card's state".
We then need to get acks from John and Fariya.

BTW, I had a look at the rsi_reset_card() function
(drivers/net/wireless/rsi/rsi_91x_sdio.c), which handles a complete
SDIO re-initialization and I think performs "layering violations"
while doing that. I suppose it's because of lack of documentation
about the mmc/sdio core, but I really don't think an SDIO func driver
should be doing that kind of stuff by itself. There are APIs to use to
perform an SDIO reset. Two options exists:

a) Use pm_runtime_get|put in combination with a MMC_CAP_POWER_OFF_CARD
enabled host.
b) Invoke mmc_power_save|restore_host() API.

Kind regards
Ulf Hansson

>
> Thanks,
> Seungwon Jeon
>
>> name mmc_card_highspeed() function from the patches to the mmc core -
>> could you please re-spin and post new version of those patches as
>> well!?
>>
>> Kind regards
>> Ulf Hansson
>

2014-04-22 12:32:19

by Seungwon Jeon

[permalink] [raw]
Subject: RE: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

On Tue, April 22, 2014, Ulf Hansson wrote:
> On 22 April 2014 12:53, Seungwon Jeon <[email protected]> wrote:
> > On Tue, April 22, 2014, Ulf Hansson wrote:
> >> On 22 April 2014 08:57, Seungwon Jeon <[email protected]> wrote:
> >> > As commit 3957848(mmc: drop the speed mode of card's state) applies,
> >> > this change should be followed.
> >> >
> >> > drivers/net/wireless/rsi/rsi_91x_sdio.c:288:20: error: 'MMC_STATE_HIGHSPEED' undeclared (first
> use
> >> in this function)
> >> > drivers/net/wireless/rsi/rsi_91x_sdio.c:299:4: error: implicit declaration of function
> >> 'mmc_card_set_highspeed' [-Werror=implicit-function-declaration]
> >> > drivers/net/wireless/rsi/rsi_91x_sdio.c:306:2: error: implicit declaration of function
> >> 'mmc_card_highspeed' [-Werror=implicit-function-declaration]
> >> >
> >> > Signed-off-by: Seungwon Jeon <[email protected]>
> >>
> >> Hi Seungwon,
> >>
> >> Thanks for responding quickly!
> >>
> >> > ---
> >> > drivers/net/wireless/rsi/rsi_91x_sdio.c | 4 +---
> >> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >> > index 2e39d38..46e7af4 100644
> >> > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >> > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >> > @@ -285,7 +285,6 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> >> > if (err) {
> >> > rsi_dbg(ERR_ZONE, "%s: CCCR speed reg read failed: %d\n",
> >> > __func__, err);
> >> > - card->state &= ~MMC_STATE_HIGHSPEED;
> >> > } else {
> >> > err = rsi_cmd52writebyte(card,
> >> > SDIO_CCCR_SPEED,
> >> > @@ -296,14 +295,13 @@ static void rsi_reset_card(struct sdio_func *pfunction)
> >> > __func__, err);
> >> > return;
> >> > }
> >> > - mmc_card_set_highspeed(card);
> >> > host->ios.timing = MMC_TIMING_SD_HS;
> >> > host->ops->set_ios(host, &host->ios);
> >> > }
> >> > }
> >> >
> >> > /* Set clock */
> >> > - if (mmc_card_highspeed(card))
> >> > + if (mmc_card_hs(card))
> >> > clock = 50000000;
> >> > else
> >> > clock = card->cis.max_dtr;
> >> > --
> >> > 1.7.0.4
> >> >
> >> >
> >>
> >> I am not sure it's safe to carry this patch through Chris' mmc tree
> >> due to merge conflicts with John's wireless tree.
> >>
> >> Though, since you have changed the mmc_card_highspeed() function to be
> >> named to mmc_card_hs(), we need to work out the dependency.
> >>
> >> We have some options to handle this, I suggest the following.
> >>
> >> Re-spin this patch to keep using mmc_card_highspeed() and let John
> >> take it through his wireless tree. Thus you also need to keep the
> > It would be better if this patch can be handled in Chris's.
> > Because if the rest of changes of this patch with keeping "mmc_card_highspeed" is applied,
> > high-speed SDIO will be ignored in John's tree.
>
> You are right Seungwon!
>
> Please re-spin the patchset on the mmc core an squash the code from
> this patch, into the patch "mmc: drop the speed mode of card's state".
> We then need to get acks from John and Fariya.
Sure! I'll send before long.
You still want to get back naming for mmc_card_highspeed?

>
> BTW, I had a look at the rsi_reset_card() function
> (drivers/net/wireless/rsi/rsi_91x_sdio.c), which handles a complete
> SDIO re-initialization and I think performs "layering violations"
> while doing that. I suppose it's because of lack of documentation
> about the mmc/sdio core, but I really don't think an SDIO func driver
> should be doing that kind of stuff by itself. There are APIs to use to
> perform an SDIO reset. Two options exists:
>
> a) Use pm_runtime_get|put in combination with a MMC_CAP_POWER_OFF_CARD
> enabled host.
> b) Invoke mmc_power_save|restore_host() API.
Yes, we could expect better changes in rsi sdio soon.

Thanks,
Seungwon Jeon
>
> Kind regards
> Ulf Hansson
>
> >
> > Thanks,
> > Seungwon Jeon
> >
> >> name mmc_card_highspeed() function from the patches to the mmc core -
> >> could you please re-spin and post new version of those patches as
> >> well!?
> >>
> >> Kind regards
> >> Ulf Hansson
> >

2014-04-22 13:05:10

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH] linux-next: rsi: fix using of removed stuff from mmc

>> >>
>> >> I am not sure it's safe to carry this patch through Chris' mmc tree
>> >> due to merge conflicts with John's wireless tree.
>> >>
>> >> Though, since you have changed the mmc_card_highspeed() function to be
>> >> named to mmc_card_hs(), we need to work out the dependency.
>> >>
>> >> We have some options to handle this, I suggest the following.
>> >>
>> >> Re-spin this patch to keep using mmc_card_highspeed() and let John
>> >> take it through his wireless tree. Thus you also need to keep the
>> > It would be better if this patch can be handled in Chris's.
>> > Because if the rest of changes of this patch with keeping "mmc_card_highspeed" is applied,
>> > high-speed SDIO will be ignored in John's tree.
>>
>> You are right Seungwon!
>>
>> Please re-spin the patchset on the mmc core an squash the code from
>> this patch, into the patch "mmc: drop the speed mode of card's state".
>> We then need to get acks from John and Fariya.
> Sure! I'll send before long.
> You still want to get back naming for mmc_card_highspeed?

It's up to you, I have no strong opinion.

Kind regards
Ulf Hansson

>
>>
>> BTW, I had a look at the rsi_reset_card() function
>> (drivers/net/wireless/rsi/rsi_91x_sdio.c), which handles a complete
>> SDIO re-initialization and I think performs "layering violations"
>> while doing that. I suppose it's because of lack of documentation
>> about the mmc/sdio core, but I really don't think an SDIO func driver
>> should be doing that kind of stuff by itself. There are APIs to use to
>> perform an SDIO reset. Two options exists:
>>
>> a) Use pm_runtime_get|put in combination with a MMC_CAP_POWER_OFF_CARD
>> enabled host.
>> b) Invoke mmc_power_save|restore_host() API.
> Yes, we could expect better changes in rsi sdio soon.
>
> Thanks,
> Seungwon Jeon
>>
>> Kind regards
>> Ulf Hansson
>>
>> >
>> > Thanks,
>> > Seungwon Jeon
>> >
>> >> name mmc_card_highspeed() function from the patches to the mmc core -
>> >> could you please re-spin and post new version of those patches as
>> >> well!?
>> >>
>> >> Kind regards
>> >> Ulf Hansson
>> >
>