* Re: [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg()
2014-10-11 4:16 [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg() Doug Anderson
@ 2014-10-14 12:02 ` Alim Akhtar
2014-10-14 16:27 ` Doug Anderson
2014-10-15 1:40 ` Jaehoon Chung
2014-10-27 14:15 ` Ulf Hansson
2 siblings, 1 reply; 6+ messages in thread
From: Alim Akhtar @ 2014-10-14 12:02 UTC (permalink / raw)
To: Doug Anderson
Cc: Ulf Hansson, Seungwon Jeon, Jaehoon Chung, Addy Ke, Sonny Rao,
Alim Akhtar, Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Doug,
On Sat, Oct 11, 2014 at 9:46 AM, Doug Anderson <dianders@chromium.org> wrote:
> In (28f92b5 mmc: core: Try other signal levels during power up) we can
> see that there are times when it's valid to try several signal
> voltages. Don't print an ugly error in the logs when that happens.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> drivers/mmc/host/dw_mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 69f0cc6..c4afbdd 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1075,7 +1075,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
> ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>
> if (ret) {
> - dev_err(&mmc->class_dev,
> + dev_dbg(&mmc->class_dev,
> "Regulator set error %d: %d - %d\n",
> ret, min_uv, max_uv);
> return ret;
Well, I am ok with this but this info is very useful, what if PMIC
failed to actually set the voltage? may be because of some PMIC driver
bug or i2c driver bug? Ofcourse this can be found by turning MMC_DEBUG
ON, but is that worth in this case. Or is there a way to print that,
this failure is because of a regulator re-try?
your thoughts?
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Regards,
Alim
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg()
2014-10-14 12:02 ` Alim Akhtar
@ 2014-10-14 16:27 ` Doug Anderson
2014-10-14 20:04 ` Alim Akhtar
0 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2014-10-14 16:27 UTC (permalink / raw)
To: Alim Akhtar
Cc: Ulf Hansson, Seungwon Jeon, Jaehoon Chung, Addy Ke, Sonny Rao,
Alim Akhtar, Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Alim,
On Tue, Oct 14, 2014 at 5:02 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1075,7 +1075,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>> ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>>
>> if (ret) {
>> - dev_err(&mmc->class_dev,
>> + dev_dbg(&mmc->class_dev,
>> "Regulator set error %d: %d - %d\n",
>> ret, min_uv, max_uv);
>> return ret;
> Well, I am ok with this but this info is very useful, what if PMIC
> failed to actually set the voltage? may be because of some PMIC driver
> bug or i2c driver bug? Ofcourse this can be found by turning MMC_DEBUG
> ON, but is that worth in this case. Or is there a way to print that,
> this failure is because of a regulator re-try?
> your thoughts?
I think that the regulator framework and the i2c framework are
supposed to be reliable. If they aren't reliable there will be lots
of places that will have problems. I think that you _could_:
* In your regulator driver print an error when an i2c transfer fails.
* In your regulator driver print an error if some unexpected event
happens (like a regulator reports that the voltage didn't actually
change).
That would get you want you want, right? ...but an error here doesn't
belong and that's pretty much determined by (28f92b5 mmc: core: Try
other signal levels during power up). That patch wants to be able to
try several different voltage levels and if we print an error in that
case then it's going to be very confusing to the user.
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg()
2014-10-14 16:27 ` Doug Anderson
@ 2014-10-14 20:04 ` Alim Akhtar
0 siblings, 0 replies; 6+ messages in thread
From: Alim Akhtar @ 2014-10-14 20:04 UTC (permalink / raw)
To: Doug Anderson
Cc: Ulf Hansson, Seungwon Jeon, Jaehoon Chung, Addy Ke, Sonny Rao,
Alim Akhtar, Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Doug,
On Tue, Oct 14, 2014 at 9:57 PM, Doug Anderson <dianders@chromium.org> wrote:
> Alim,
>
> On Tue, Oct 14, 2014 at 5:02 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1075,7 +1075,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>>> ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>>>
>>> if (ret) {
>>> - dev_err(&mmc->class_dev,
>>> + dev_dbg(&mmc->class_dev,
>>> "Regulator set error %d: %d - %d\n",
>>> ret, min_uv, max_uv);
>>> return ret;
>> Well, I am ok with this but this info is very useful, what if PMIC
>> failed to actually set the voltage? may be because of some PMIC driver
>> bug or i2c driver bug? Ofcourse this can be found by turning MMC_DEBUG
>> ON, but is that worth in this case. Or is there a way to print that,
>> this failure is because of a regulator re-try?
>> your thoughts?
>
> I think that the regulator framework and the i2c framework are
> supposed to be reliable. If they aren't reliable there will be lots
> of places that will have problems. I think that you _could_:
>
> * In your regulator driver print an error when an i2c transfer fails.
>
> * In your regulator driver print an error if some unexpected event
> happens (like a regulator reports that the voltage didn't actually
> change).
>
> That would get you want you want, right? ...but an error here doesn't
> belong and that's pretty much determined by (28f92b5 mmc: core: Try
> other signal levels during power up). That patch wants to be able to
> try several different voltage levels and if we print an error in that
> case then it's going to be very confusing to the user.
>
Hmm...Ok, convincing enough to me, so
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> -Doug
--
Regards,
Alim
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg()
2014-10-11 4:16 [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg() Doug Anderson
2014-10-14 12:02 ` Alim Akhtar
@ 2014-10-15 1:40 ` Jaehoon Chung
2014-10-27 14:15 ` Ulf Hansson
2 siblings, 0 replies; 6+ messages in thread
From: Jaehoon Chung @ 2014-10-15 1:40 UTC (permalink / raw)
To: Doug Anderson, Ulf Hansson, Seungwon Jeon
Cc: Addy Ke, Sonny Rao, Alim Akhtar, chris, linux-mmc, linux-kernel
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
On 10/11/2014 01:16 PM, Doug Anderson wrote:
> In (28f92b5 mmc: core: Try other signal levels during power up) we can
> see that there are times when it's valid to try several signal
> voltages. Don't print an ugly error in the logs when that happens.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> drivers/mmc/host/dw_mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 69f0cc6..c4afbdd 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1075,7 +1075,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
> ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>
> if (ret) {
> - dev_err(&mmc->class_dev,
> + dev_dbg(&mmc->class_dev,
> "Regulator set error %d: %d - %d\n",
> ret, min_uv, max_uv);
> return ret;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg()
2014-10-11 4:16 [PATCH] mmc: dw_mmc: Change signal voltage error to dev_dbg() Doug Anderson
2014-10-14 12:02 ` Alim Akhtar
2014-10-15 1:40 ` Jaehoon Chung
@ 2014-10-27 14:15 ` Ulf Hansson
2 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2014-10-27 14:15 UTC (permalink / raw)
To: Doug Anderson
Cc: Seungwon Jeon, Jaehoon Chung, Addy Ke, Sonny Rao, Alim Akhtar,
Chris Ball, linux-mmc, linux-kernel@vger.kernel.org
On 11 October 2014 06:16, Doug Anderson <dianders@chromium.org> wrote:
> In (28f92b5 mmc: core: Try other signal levels during power up) we can
> see that there are times when it's valid to try several signal
> voltages. Don't print an ugly error in the logs when that happens.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
Thanks! Applied for next!
Kind regards
Uffe
> ---
> drivers/mmc/host/dw_mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 69f0cc6..c4afbdd 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1075,7 +1075,7 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
> ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
>
> if (ret) {
> - dev_err(&mmc->class_dev,
> + dev_dbg(&mmc->class_dev,
> "Regulator set error %d: %d - %d\n",
> ret, min_uv, max_uv);
> return ret;
> --
> 2.1.0.rc2.206.gedb03e5
>
^ permalink raw reply [flat|nested] 6+ messages in thread