* [PATCH 0/3] Fix fixed regulators support @ 2012-11-13 8:48 Marek Szyprowski 2012-11-13 8:48 ` [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() Marek Szyprowski ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 8:48 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Hello, I've noticed that the voltage check in regulator_is_supported_voltage() function was inverted, what resulted in strange side effects. One of such side effects appeared in sdhci driver, which has some build-in support for special case of fixed regulators. Now, after fixing regulator_is_supported_voltage(), the sdhci driver stopped working with fixed regulators. The provided patch series fixes regulator_is_supported_voltage() function and updates sdhci driver to correctly operate with fixed voltage regulators. The second patch unifies handling of fixed regulators and regulators with disabled voltage change due to their constraints. This restores support for such regulators in sdhci driver (such case is present on Samsung GONI board). If possible, I would recomment to push those patches to v3.7-rc series. Best regards Marek Szyprowski Samsung Poland R&D Center Patch summary: Marek Szyprowski (3): regulator: fix voltage check in regulator_is_supported_voltage() regulator: threat regulators with constant volatage as fixed mmc: sdhci: apply voltage range check only for non-fixed regulators drivers/mmc/host/sdhci.c | 2 +- drivers/regulator/core.c | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() 2012-11-13 8:48 [PATCH 0/3] Fix fixed regulators support Marek Szyprowski @ 2012-11-13 8:48 ` Marek Szyprowski 2012-11-13 9:01 ` Mark Brown 2012-11-13 8:48 ` [PATCH 2/3] regulator: threat regulators with constant volatage as fixed Marek Szyprowski 2012-11-13 8:48 ` [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators Marek Szyprowski 2 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 8:48 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball regulator_is_supported_voltage() should return true only if the voltage of fixed/constant regulator is between min_uV and max_uV. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/regulator/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 1a35251..042c1ff 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1974,7 +1974,7 @@ int regulator_is_supported_voltage(struct regulator *regulator, if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) { ret = regulator_get_voltage(regulator); if (ret >= 0) - return (min_uV >= ret && ret <= max_uV); + return (min_uV <= ret && ret <= max_uV); else return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() 2012-11-13 8:48 ` [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() Marek Szyprowski @ 2012-11-13 9:01 ` Mark Brown 0 siblings, 0 replies; 21+ messages in thread From: Mark Brown @ 2012-11-13 9:01 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood, Chris Ball [-- Attachment #1: Type: text/plain, Size: 218 bytes --] On Tue, Nov 13, 2012 at 09:48:51AM +0100, Marek Szyprowski wrote: > regulator_is_supported_voltage() should return true only if the voltage > of fixed/constant regulator is between min_uV and max_uV. Applied, thanks. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] regulator: threat regulators with constant volatage as fixed 2012-11-13 8:48 [PATCH 0/3] Fix fixed regulators support Marek Szyprowski 2012-11-13 8:48 ` [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() Marek Szyprowski @ 2012-11-13 8:48 ` Marek Szyprowski 2012-11-13 9:00 ` Mark Brown 2012-11-13 8:48 ` [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators Marek Szyprowski 2 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 8:48 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Some drivers has additional logic for fixed regulators. Let regulator core to threat regulators which cannot change their voltage due to applied constraints as fixed. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/regulator/core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 042c1ff..271182e 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1872,7 +1872,10 @@ int regulator_count_voltages(struct regulator *regulator) { struct regulator_dev *rdev = regulator->rdev; - return rdev->desc->n_voltages ? : -EINVAL; + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) + return rdev->desc->n_voltages ? : -EINVAL; + else + return 1; } EXPORT_SYMBOL_GPL(regulator_count_voltages); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] regulator: threat regulators with constant volatage as fixed 2012-11-13 8:48 ` [PATCH 2/3] regulator: threat regulators with constant volatage as fixed Marek Szyprowski @ 2012-11-13 9:00 ` Mark Brown 2012-11-13 9:35 ` [PATCH v2] regulator: treat " Marek Szyprowski 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2012-11-13 9:00 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood, Chris Ball [-- Attachment #1: Type: text/plain, Size: 569 bytes --] On Tue, Nov 13, 2012 at 09:48:52AM +0100, Marek Szyprowski wrote: > Some drivers has additional logic for fixed regulators. Let regulator core > to threat regulators which cannot change their voltage due to applied YM "treat". > + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) > + return rdev->desc->n_voltages ? : -EINVAL; Please don't perpetuate the use of ? : as it's not a triumph of legibility (even worse than the regular ternery operator). I realise that the original code did this but there's no need to carry on doing the same thing. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] regulator: treat regulators with constant volatage as fixed 2012-11-13 9:00 ` Mark Brown @ 2012-11-13 9:35 ` Marek Szyprowski 2012-11-13 9:42 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 9:35 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Some drivers has additional logic for fixed regulators. Let regulator core to treat regulators which cannot change their voltage due to applied constraints as fixed. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/regulator/core.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 042c1ff..78b34b7 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1872,7 +1872,11 @@ int regulator_count_voltages(struct regulator *regulator) { struct regulator_dev *rdev = regulator->rdev; - return rdev->desc->n_voltages ? : -EINVAL; + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) + return rdev->desc->n_voltages ? rdev->desc->n_voltages : + -EINVAL; + else + return 1; } EXPORT_SYMBOL_GPL(regulator_count_voltages); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] regulator: treat regulators with constant volatage as fixed 2012-11-13 9:35 ` [PATCH v2] regulator: treat " Marek Szyprowski @ 2012-11-13 9:42 ` Mark Brown 2012-11-13 9:49 ` [PATCH v3] " Marek Szyprowski 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2012-11-13 9:42 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood, Chris Ball [-- Attachment #1: Type: text/plain, Size: 281 bytes --] On Tue, Nov 13, 2012 at 10:35:34AM +0100, Marek Szyprowski wrote: > + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) > + return rdev->desc->n_voltages ? rdev->desc->n_voltages : > + -EINVAL; The idea here was to avoid the ternery operator completely. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] regulator: treat regulators with constant volatage as fixed 2012-11-13 9:42 ` Mark Brown @ 2012-11-13 9:49 ` Marek Szyprowski 2012-11-14 2:01 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 9:49 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Some drivers has additional logic for fixed regulators. Let regulator core to treat regulators which cannot change their voltage due to applied constraints as fixed. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/regulator/core.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 042c1ff..d07c240 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1872,7 +1872,14 @@ int regulator_count_voltages(struct regulator *regulator) { struct regulator_dev *rdev = regulator->rdev; - return rdev->desc->n_voltages ? : -EINVAL; + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) { + if (rdev->desc->n_voltages) + return rdev->desc->n_voltages; + else + return -EINVAL; + } else { + return 1; + } } EXPORT_SYMBOL_GPL(regulator_count_voltages); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3] regulator: treat regulators with constant volatage as fixed 2012-11-13 9:49 ` [PATCH v3] " Marek Szyprowski @ 2012-11-14 2:01 ` Mark Brown 2012-11-20 13:20 ` Marek Szyprowski 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2012-11-14 2:01 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood, Chris Ball [-- Attachment #1: Type: text/plain, Size: 918 bytes --] On Tue, Nov 13, 2012 at 10:49:37AM +0100, Marek Szyprowski wrote: > + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) { > + if (rdev->desc->n_voltages) > + return rdev->desc->n_voltages; > + else > + return -EINVAL; > + } else { > + return 1; > + } Hrm, now I can read the logic I'm not convinced this is a good idea. This will report that we have an available voltage for devices which don't know their voltage (things like battery supplies often do this as the voltage is unregulated) and it will mean that we are doing something different for the case where there's only one voltage (reporting the restricted count instead of the physically supported count). I think we want a regulator_can_change_voltage() or possibly a count function (though I can't see any use cases except this) which answers the question directly instead of layering on top of this function. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] regulator: treat regulators with constant volatage as fixed 2012-11-14 2:01 ` Mark Brown @ 2012-11-20 13:20 ` Marek Szyprowski 0 siblings, 0 replies; 21+ messages in thread From: Marek Szyprowski @ 2012-11-20 13:20 UTC (permalink / raw) To: Mark Brown Cc: linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood, Chris Ball Hello, On 11/14/2012 3:01 AM, Mark Brown wrote: > On Tue, Nov 13, 2012 at 10:49:37AM +0100, Marek Szyprowski wrote: > > > + if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) { > > + if (rdev->desc->n_voltages) > > + return rdev->desc->n_voltages; > > + else > > + return -EINVAL; > > + } else { > > + return 1; > > + } > > Hrm, now I can read the logic I'm not convinced this is a good idea. > This will report that we have an available voltage for devices which > don't know their voltage (things like battery supplies often do this as > the voltage is unregulated) and it will mean that we are doing something > different for the case where there's only one voltage (reporting the > restricted count instead of the physically supported count). > > I think we want a regulator_can_change_voltage() or possibly a count > function (though I can't see any use cases except this) which answers > the question directly instead of layering on top of this function. Right, regulator_can_change_voltage() sounds much better than my hacky approach. The first client would be probably sdhci/mmc driver, as 'can_change_voltage' check sounds much more appropriate than counting available voltage values. I will prepare patches soon. Best regards -- Marek Szyprowski Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 8:48 [PATCH 0/3] Fix fixed regulators support Marek Szyprowski 2012-11-13 8:48 ` [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() Marek Szyprowski 2012-11-13 8:48 ` [PATCH 2/3] regulator: threat regulators with constant volatage as fixed Marek Szyprowski @ 2012-11-13 8:48 ` Marek Szyprowski 2012-11-13 12:45 ` Chris Ball 2 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 8:48 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Fixed regulators cannot change their voltage, so disable all voltage range checking for them, otherwise the driver fails to operate with fixed regulators. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/mmc/host/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c7851c0..6f6534e 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) regulator_enable(host->vmmc); #ifdef CONFIG_REGULATOR - if (host->vmmc) { + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { ret = regulator_is_supported_voltage(host->vmmc, 3300000, 3300000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 8:48 ` [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators Marek Szyprowski @ 2012-11-13 12:45 ` Chris Ball 2012-11-13 13:32 ` [PATCH v2] " Marek Szyprowski 2012-11-14 1:10 ` [PATCH 3/3] " Mark Brown 0 siblings, 2 replies; 21+ messages in thread From: Chris Ball @ 2012-11-13 12:45 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood Hi Marek, On Tue, Nov 13 2012, Marek Szyprowski wrote: > Fixed regulators cannot change their voltage, so disable all voltage range > checking for them, otherwise the driver fails to operate with fixed > regulators. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Perhaps it's a good idea to mention that the regulator API is changing (being fixed) at the same time, and that's why this patch is necessary. > --- > drivers/mmc/host/sdhci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c7851c0..6f6534e 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) > regulator_enable(host->vmmc); > > #ifdef CONFIG_REGULATOR > - if (host->vmmc) { > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { > ret = regulator_is_supported_voltage(host->vmmc, 3300000, > 3300000); > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) What about other occasions where is_supported_voltage() is used, like this one -- is it necessary here too? else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000)) regulator_enable(host->vqmmc); Thanks, - Chris. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 12:45 ` Chris Ball @ 2012-11-13 13:32 ` Marek Szyprowski 2012-11-13 13:45 ` Chris Ball 2012-11-14 1:10 ` [PATCH 3/3] " Mark Brown 1 sibling, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 13:32 UTC (permalink / raw) To: linux-kernel, linux-mmc Cc: Marek Szyprowski, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Fixed regulators cannot change their voltage, so disable all voltage range checking for them, otherwise the driver fails to operate with fixed regulators. Up to now it worked only by luck, because regulator_is_supported_voltage() function returned incorrect values. Commit "regulator: fix voltage check in regulator_is_supported_voltage()" fixed that function and now additional check is needed for fixed regulators. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/mmc/host/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c7851c0..6f6534e 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) regulator_enable(host->vmmc); #ifdef CONFIG_REGULATOR - if (host->vmmc) { + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { ret = regulator_is_supported_voltage(host->vmmc, 3300000, 3300000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 13:32 ` [PATCH v2] " Marek Szyprowski @ 2012-11-13 13:45 ` Chris Ball 2012-11-13 14:09 ` Marek Szyprowski 0 siblings, 1 reply; 21+ messages in thread From: Chris Ball @ 2012-11-13 13:45 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood Hi Marek, On Tue, Nov 13 2012, Marek Szyprowski wrote: > Fixed regulators cannot change their voltage, so disable all voltage > range checking for them, otherwise the driver fails to operate with > fixed regulators. Up to now it worked only by luck, because > regulator_is_supported_voltage() function returned incorrect values. > Commit "regulator: fix voltage check in regulator_is_supported_voltage()" > fixed that function and now additional check is needed for fixed > regulators. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/mmc/host/sdhci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c7851c0..6f6534e 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) > regulator_enable(host->vmmc); > > #ifdef CONFIG_REGULATOR > - if (host->vmmc) { > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { > ret = regulator_is_supported_voltage(host->vmmc, 3300000, > 3300000); > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) Thanks for the longer explanation. I'm still missing something, though; what's wrong with running the check as it was with the new regulator code? (I haven't tried it yet.) #ifdef CONFIG_REGULATOR if (host->vmmc) { ret = regulator_is_supported_voltage(host->vmmc, 3300000, 3300000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) caps[0] &= ~SDHCI_CAN_VDD_330; ret = regulator_is_supported_voltage(host->vmmc, 3000000, 3000000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) caps[0] &= ~SDHCI_CAN_VDD_300; ret = regulator_is_supported_voltage(host->vmmc, 1800000, 1800000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) caps[0] &= ~SDHCI_CAN_VDD_180; } #endif /* CONFIG_REGULATOR */ The point is to remove unsupported voltages, so if someone sets up a fixed regulator at 3300000, all of the other caps are disabled. Why wouldn't that work without this change, and how are we supposed to remove those caps on a fixed regulator after your patchset? Thanks, sorry if I'm missing something obvious, - Chris. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 13:45 ` Chris Ball @ 2012-11-13 14:09 ` Marek Szyprowski 2012-11-13 14:14 ` Chris Ball 0 siblings, 1 reply; 21+ messages in thread From: Marek Szyprowski @ 2012-11-13 14:09 UTC (permalink / raw) To: Chris Ball Cc: linux-kernel, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood Hello, On 11/13/2012 2:45 PM, Chris Ball wrote: > Hi Marek, > > On Tue, Nov 13 2012, Marek Szyprowski wrote: > > Fixed regulators cannot change their voltage, so disable all voltage > > range checking for them, otherwise the driver fails to operate with > > fixed regulators. Up to now it worked only by luck, because > > regulator_is_supported_voltage() function returned incorrect values. > > Commit "regulator: fix voltage check in regulator_is_supported_voltage()" > > fixed that function and now additional check is needed for fixed > > regulators. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/mmc/host/sdhci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index c7851c0..6f6534e 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) > > regulator_enable(host->vmmc); > > > > #ifdef CONFIG_REGULATOR > > - if (host->vmmc) { > > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { > > ret = regulator_is_supported_voltage(host->vmmc, 3300000, > > 3300000); > > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) > > Thanks for the longer explanation. I'm still missing something, though; > what's wrong with running the check as it was with the new regulator code? > (I haven't tried it yet.) > > #ifdef CONFIG_REGULATOR > if (host->vmmc) { > ret = regulator_is_supported_voltage(host->vmmc, 3300000, > 3300000); > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) > caps[0] &= ~SDHCI_CAN_VDD_330; > ret = regulator_is_supported_voltage(host->vmmc, 3000000, > 3000000); > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) > caps[0] &= ~SDHCI_CAN_VDD_300; > ret = regulator_is_supported_voltage(host->vmmc, 1800000, > 1800000); > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) > caps[0] &= ~SDHCI_CAN_VDD_180; > } > #endif /* CONFIG_REGULATOR */ > > The point is to remove unsupported voltages, so if someone sets up a > fixed regulator at 3300000, all of the other caps are disabled. Why > wouldn't that work without this change, and how are we supposed to > remove those caps on a fixed regulator after your patchset? > > Thanks, sorry if I'm missing something obvious, On our boards eMMC is connected to fixed 2.8V regulator, what results in clearing all available voltages and fail. The same situation is when one enable dummy regulator and try to use sdhci with it. My patch fixes this and restores sdhci to working state as it was before (before fixing regulator regulator_is_supported_voltage() function and earlier when MMC_BROKEN_VOLATGE capability was used). Best regards -- Marek Szyprowski Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 14:09 ` Marek Szyprowski @ 2012-11-13 14:14 ` Chris Ball 2012-11-13 21:23 ` Philip Rakity 0 siblings, 1 reply; 21+ messages in thread From: Chris Ball @ 2012-11-13 14:14 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood, Philip Rakity Hi, On Tue, Nov 13 2012, Marek Szyprowski wrote: >> On Tue, Nov 13 2012, Marek Szyprowski wrote: >> > Fixed regulators cannot change their voltage, so disable all voltage >> > range checking for them, otherwise the driver fails to operate with >> > fixed regulators. Up to now it worked only by luck, because >> > regulator_is_supported_voltage() function returned incorrect values. >> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()" >> > fixed that function and now additional check is needed for fixed >> > regulators. >> > >> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> > --- >> > drivers/mmc/host/sdhci.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> > index c7851c0..6f6534e 100644 >> > --- a/drivers/mmc/host/sdhci.c >> > +++ b/drivers/mmc/host/sdhci.c >> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) >> > regulator_enable(host->vmmc); >> > >> > #ifdef CONFIG_REGULATOR >> > - if (host->vmmc) { >> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { >> > ret = regulator_is_supported_voltage(host->vmmc, 3300000, >> > 3300000); >> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >> >> Thanks for the longer explanation. I'm still missing something, though; >> what's wrong with running the check as it was with the new regulator code? >> (I haven't tried it yet.) >> >> #ifdef CONFIG_REGULATOR >> if (host->vmmc) { >> ret = regulator_is_supported_voltage(host->vmmc, 3300000, >> 3300000); >> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >> caps[0] &= ~SDHCI_CAN_VDD_330; >> ret = regulator_is_supported_voltage(host->vmmc, 3000000, >> 3000000); >> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) >> caps[0] &= ~SDHCI_CAN_VDD_300; >> ret = regulator_is_supported_voltage(host->vmmc, 1800000, >> 1800000); >> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) >> caps[0] &= ~SDHCI_CAN_VDD_180; >> } >> #endif /* CONFIG_REGULATOR */ >> >> The point is to remove unsupported voltages, so if someone sets up a >> fixed regulator at 3300000, all of the other caps are disabled. Why >> wouldn't that work without this change, and how are we supposed to >> remove those caps on a fixed regulator after your patchset? >> >> Thanks, sorry if I'm missing something obvious, > > On our boards eMMC is connected to fixed 2.8V regulator, what results in > clearing all available voltages and fail. The same situation is when one > enable dummy regulator and try to use sdhci with it. My patch fixes this > and restores sdhci to working state as it was before (before fixing > regulator regulator_is_supported_voltage() function and earlier when > MMC_BROKEN_VOLATGE capability was used). I see. Sounds like a separate bug -- Philip (or anyone else), any idea how we should be treating eMMCs with a fixed voltage here? Thanks, - Chris. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 14:14 ` Chris Ball @ 2012-11-13 21:23 ` Philip Rakity 2012-11-20 8:42 ` Marek Szyprowski 0 siblings, 1 reply; 21+ messages in thread From: Philip Rakity @ 2012-11-13 21:23 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel@vger.kernel.org List, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball Hi Marek, Is the regulator dedicated ? or is it shared ? Is it used for eMMC ? If it cannot be turned off -- then just don't list it in the regulators list for vmmc. If it CAN be turned off then need to get back to you. Philip On Nov 13, 2012, at 2:14 PM, Chris Ball <cjb@laptop.org> wrote: > Hi, > > On Tue, Nov 13 2012, Marek Szyprowski wrote: >>> On Tue, Nov 13 2012, Marek Szyprowski wrote: >>>> Fixed regulators cannot change their voltage, so disable all voltage >>>> range checking for them, otherwise the driver fails to operate with >>>> fixed regulators. Up to now it worked only by luck, because >>>> regulator_is_supported_voltage() function returned incorrect values. >>>> Commit "regulator: fix voltage check in regulator_is_supported_voltage()" >>>> fixed that function and now additional check is needed for fixed >>>> regulators. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/mmc/host/sdhci.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index c7851c0..6f6534e 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) >>>> regulator_enable(host->vmmc); >>>> >>>> #ifdef CONFIG_REGULATOR >>>> - if (host->vmmc) { >>>> + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { >>>> ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>>> 3300000); >>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> >>> Thanks for the longer explanation. I'm still missing something, though; >>> what's wrong with running the check as it was with the new regulator code? >>> (I haven't tried it yet.) >>> >>> #ifdef CONFIG_REGULATOR >>> if (host->vmmc) { >>> ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>> 3300000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> caps[0] &= ~SDHCI_CAN_VDD_330; >>> ret = regulator_is_supported_voltage(host->vmmc, 3000000, >>> 3000000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) >>> caps[0] &= ~SDHCI_CAN_VDD_300; >>> ret = regulator_is_supported_voltage(host->vmmc, 1800000, >>> 1800000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) >>> caps[0] &= ~SDHCI_CAN_VDD_180; >>> } >>> #endif /* CONFIG_REGULATOR */ >>> >>> The point is to remove unsupported voltages, so if someone sets up a >>> fixed regulator at 3300000, all of the other caps are disabled. Why >>> wouldn't that work without this change, and how are we supposed to >>> remove those caps on a fixed regulator after your patchset? >>> >>> Thanks, sorry if I'm missing something obvious, >> >> On our boards eMMC is connected to fixed 2.8V regulator, what results in >> clearing all available voltages and fail. The same situation is when one >> enable dummy regulator and try to use sdhci with it. My patch fixes this >> and restores sdhci to working state as it was before (before fixing >> regulator regulator_is_supported_voltage() function and earlier when >> MMC_BROKEN_VOLATGE capability was used). > > I see. Sounds like a separate bug -- Philip (or anyone else), any > idea how we should be treating eMMCs with a fixed voltage here? > > Thanks, > > - Chris. > -- > Chris Ball <cjb@laptop.org> <http://printf.net/> > One Laptop Per Child ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 21:23 ` Philip Rakity @ 2012-11-20 8:42 ` Marek Szyprowski 0 siblings, 0 replies; 21+ messages in thread From: Marek Szyprowski @ 2012-11-20 8:42 UTC (permalink / raw) To: Philip Rakity Cc: linux-kernel@vger.kernel.org List, linux-mmc, Kyungmin Park, Mark Brown, Liam Girdwood, Chris Ball On 11/13/2012 10:23 PM, Philip Rakity wrote: > Hi Marek, > > Is the regulator dedicated ? or is it shared ? Is it used for eMMC ? > > If it cannot be turned off -- then just don't list it in the regulators list for vmmc. > > If it CAN be turned off then need to get back to you. It is dedicated to eMMC device and can be turned off. Patch "mmc: sdhci: apply voltage range check only for non-fixed regulators" restored sdhci to working state after merging the regulator fix. However there are lots of error messages from sdhci driver: s3c-sdhci s3c-sdhci.0: could not set regulator OCR (-1) The only remaining problem is sdhci driver operation with dummy regulator. Right now it simply fails to initialize if dummy regulator is enabled. Do you have any idea how to fix it properly? Best regards -- Marek Szyprowski Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-13 12:45 ` Chris Ball 2012-11-13 13:32 ` [PATCH v2] " Marek Szyprowski @ 2012-11-14 1:10 ` Mark Brown 1 sibling, 0 replies; 21+ messages in thread From: Mark Brown @ 2012-11-14 1:10 UTC (permalink / raw) To: Chris Ball Cc: Marek Szyprowski, linux-kernel, linux-mmc, Kyungmin Park, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 1213 bytes --] On Tue, Nov 13, 2012 at 07:45:07AM -0500, Chris Ball wrote: > On Tue, Nov 13 2012, Marek Szyprowski wrote: > > Fixed regulators cannot change their voltage, so disable all voltage range > > checking for them, otherwise the driver fails to operate with fixed > > regulators. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Perhaps it's a good idea to mention that the regulator API is changing > (being fixed) at the same time, and that's why this patch is necessary. This should be totally unrelated to any externally visible change in the regulator API, if anything I would expect fixes in the API to *improve* the ability of clients to work with fixed voltage regulators. What's missing here is some explanation as to what the problem is and how it's being fixed. As I understand it this should really be a workaround for hardware which runs cards at out of spec voltages; we have a fixed voltage regulator which claims to not support any of the voltages that are in spec (due to this being what the hardware does) so if we try to use the voltage management stuff in the MMC API it gets upset. As a workaround if we can't change the voltage we just ignore the voltage aspects of the API. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <25B60CDC2F704E4E9D88FFD52780CB4C060FBEA29A@SC-VEXCH1.marvell.com>]
* Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators [not found] <25B60CDC2F704E4E9D88FFD52780CB4C060FBEA29A@SC-VEXCH1.marvell.com> @ 2012-11-14 7:11 ` Kevin Liu 2012-11-14 7:28 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: Kevin Liu @ 2012-11-14 7:11 UTC (permalink / raw) To: m.szyprowski, Chris Ball, linux-mmc, linux-kernel, kyungmin.park, Mark Brown, lrg, Philip Rakity > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Chris Ball > Sent: Tuesday, November 13, 2012 10:14 PM > To: Marek Szyprowski > Cc: linux-kernel@vger.kernel.org; linux-mmc@vger.kernel.org; Kyungmin Park; Mark Brown; Liam Girdwood; Philip Rakity > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators > > Hi, > > On Tue, Nov 13 2012, Marek Szyprowski wrote: >>> On Tue, Nov 13 2012, Marek Szyprowski wrote: >>> > Fixed regulators cannot change their voltage, so disable all voltage >>> > range checking for them, otherwise the driver fails to operate with >>> > fixed regulators. Up to now it worked only by luck, because >>> > regulator_is_supported_voltage() function returned incorrect values. >>> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()" >>> > fixed that function and now additional check is needed for fixed >>> > regulators. >>> > >>> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> > --- >>> > drivers/mmc/host/sdhci.c | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> > >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> > index c7851c0..6f6534e 100644 >>> > --- a/drivers/mmc/host/sdhci.c >>> > +++ b/drivers/mmc/host/sdhci.c >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) >>> > regulator_enable(host->vmmc); >>> > >>> > #ifdef CONFIG_REGULATOR >>> > - if (host->vmmc) { >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { >>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>> > 3300000); >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> >>> Thanks for the longer explanation. I'm still missing something, though; >>> what's wrong with running the check as it was with the new regulator code? >>> (I haven't tried it yet.) >>> >>> #ifdef CONFIG_REGULATOR >>> if (host->vmmc) { >>> ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>> 3300000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> caps[0] &= ~SDHCI_CAN_VDD_330; >>> ret = regulator_is_supported_voltage(host->vmmc, 3000000, >>> 3000000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) >>> caps[0] &= ~SDHCI_CAN_VDD_300; >>> ret = regulator_is_supported_voltage(host->vmmc, 1800000, >>> 1800000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) >>> caps[0] &= ~SDHCI_CAN_VDD_180; >>> } >>> #endif /* CONFIG_REGULATOR */ >>> >>> The point is to remove unsupported voltages, so if someone sets up a >>> fixed regulator at 3300000, all of the other caps are disabled. Why >>> wouldn't that work without this change, and how are we supposed to >>> remove those caps on a fixed regulator after your patchset? >>> >>> Thanks, sorry if I'm missing something obvious, >> >> On our boards eMMC is connected to fixed 2.8V regulator, what results in >> clearing all available voltages and fail. The same situation is when one >> enable dummy regulator and try to use sdhci with it. My patch fixes this >> and restores sdhci to working state as it was before (before fixing >> regulator regulator_is_supported_voltage() function and earlier when >> MMC_BROKEN_VOLATGE capability was used). > > I see. Sounds like a separate bug -- Philip (or anyone else), any > idea how we should be treating eMMCs with a fixed voltage here? > I think we should check the voltage range rather than the voltage point accoring to the spec. Otherwise some valid voltage like 2.8v will be discarded by mistake. My below old patch aim to fix this issue. How do you think? -----Original Message----- From: Kevin Liu [mailto:keyuan.liu@gmail.com] Sent: Friday, September 28, 2012 3:56 PM To: linux-mmc@vger.kernel.org; cjb@laptop.org; pierre@ossman.eu; ulf.hansson@linaro.org; Zhangfei Gao Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage range according to spec From: Kevin Liu <kliu5@marvell.com> For regulator vmmc/vmmcq, use voltage range as below 3.3v/3.0v: (2.7v, 3.6v) 1.8v: (1.7v, 1.95v) Original code use the specific value which may fail in regulator driver if it does NOT support the specific voltage. Signed-off-by: Jialing Fu <jlfu@marvell.com> Signed-off-by: Kevin Liu <kliu5@marvell.com> --- drivers/mmc/host/sdhci.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 3aef580..36afd47 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1628,7 +1628,7 @@ static int sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host, sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); if (host->vqmmc) { - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000); + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000); if (ret) { pr_warning("%s: Switching to 3.3V signalling voltage " " failed\n", mmc_hostname(host->mmc)); @@ -1672,7 +1672,7 @@ static int sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host, */ if (host->vqmmc) ret = regulator_set_voltage(host->vqmmc, - 1800000, 1800000); + 1700000, 1950000); else ret = 0; @@ -2856,7 +2856,7 @@ int sdhci_add_host(struct sdhci_host *host) pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc)); host->vqmmc = NULL; } - else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000)) + else if (regulator_is_supported_voltage(host->vqmmc, 1700000, 1950000)) regulator_enable(host->vqmmc); else caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | @@ -2927,16 +2927,14 @@ int sdhci_add_host(struct sdhci_host *host) #ifdef CONFIG_REGULATOR if (host->vmmc) { - ret = regulator_is_supported_voltage(host->vmmc, 3300000, - 3300000); + ret = regulator_is_supported_voltage(host->vmmc, 2700000, + 3600000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330)) caps[0] &= ~SDHCI_CAN_VDD_330; - ret = regulator_is_supported_voltage(host->vmmc, 3000000, - 3000000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300)) caps[0] &= ~SDHCI_CAN_VDD_300; - ret = regulator_is_supported_voltage(host->vmmc, 1800000, - 1800000); + ret = regulator_is_supported_voltage(host->vmmc, 1700000, + 1950000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180)) caps[0] &= ~SDHCI_CAN_VDD_180; } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 7:11 ` FW: [PATCH v2] " Kevin Liu @ 2012-11-14 7:28 ` Mark Brown 2012-11-14 8:36 ` Kevin Liu 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2012-11-14 7:28 UTC (permalink / raw) To: Kevin Liu Cc: m.szyprowski, Chris Ball, linux-mmc, linux-kernel, kyungmin.park, lrg, Philip Rakity [-- Attachment #1: Type: text/plain, Size: 463 bytes --] On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote: > - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000); > + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000); Should this be regulator_set_voltage_tol()? Otherwise it'd be good to explain where the numbers come from. > + ret = regulator_is_supported_voltage(host->vmmc, 1700000, > + 1950000); We should really add a regulator_is_supported_voltage_tol... let me just do that. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 7:28 ` Mark Brown @ 2012-11-14 8:36 ` Kevin Liu 2012-11-14 8:41 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: Kevin Liu @ 2012-11-14 8:36 UTC (permalink / raw) To: Mark Brown Cc: m.szyprowski, Chris Ball, linux-mmc, linux-kernel, kyungmin.park, lrg, Philip Rakity 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: > On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote: > >> - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000); >> + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000); > > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to > explain where the numbers come from. In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for voltage range is defined as below: Vdd(min) = 2.7V while Vdd(max) = 3.6V. The card should work within the voltage range. If you are afraid the voltage value is too aggressive, maybe we can use regulator_set_voltage_tol() to set a smaller range. But which range should be reasonable? >> + ret = regulator_is_supported_voltage(host->vmmc, 1700000, >> + 1950000); > > We should really add a regulator_is_supported_voltage_tol... let me > just do that. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 8:36 ` Kevin Liu @ 2012-11-14 8:41 ` Mark Brown 2012-11-14 8:57 ` Kevin Liu 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2012-11-14 8:41 UTC (permalink / raw) To: Kevin Liu Cc: m.szyprowski, Chris Ball, linux-mmc, linux-kernel, kyungmin.park, lrg, Philip Rakity [-- Attachment #1: Type: text/plain, Size: 787 bytes --] On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: > 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: > > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to > > explain where the numbers come from. > In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for > voltage range is defined as below: > Vdd(min) = 2.7V while Vdd(max) = 3.6V. > The card should work within the voltage range. > If you are afraid the voltage value is too aggressive, maybe we can > use regulator_set_voltage_tol() to set a smaller range. > But which range should be reasonable? The above makes total sense - thanks! I just wasn't aware that the range was specified in this fashion in the spec. Might be worth a comment in the code if you need to respin. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 8:41 ` Mark Brown @ 2012-11-14 8:57 ` Kevin Liu 2012-11-14 9:08 ` Philip Rakity 0 siblings, 1 reply; 21+ messages in thread From: Kevin Liu @ 2012-11-14 8:57 UTC (permalink / raw) To: Mark Brown Cc: m.szyprowski, Chris Ball, linux-mmc, linux-kernel, kyungmin.park, lrg, Philip Rakity 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: > On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >> 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: > >> > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to >> > explain where the numbers come from. > >> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for >> voltage range is defined as below: >> Vdd(min) = 2.7V while Vdd(max) = 3.6V. >> The card should work within the voltage range. > >> If you are afraid the voltage value is too aggressive, maybe we can >> use regulator_set_voltage_tol() to set a smaller range. >> But which range should be reasonable? > > The above makes total sense - thanks! I just wasn't aware that the > range was specified in this fashion in the spec. Might be worth a > comment in the code if you need to respin. Sure, I will update the patch. Thanks! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 8:57 ` Kevin Liu @ 2012-11-14 9:08 ` Philip Rakity 2012-11-14 12:05 ` Kevin Liu 0 siblings, 1 reply; 21+ messages in thread From: Philip Rakity @ 2012-11-14 9:08 UTC (permalink / raw) To: Chris Ball, linux-mmc Cc: Mark Brown, m.szyprowski, linux-kernel@vger.kernel.org List, kyungmin.park, lrg, Kevin Liu On Nov 14, 2012, at 8:57 AM, Kevin Liu <keyuan.liu@gmail.com> wrote: > 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: >> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >>> 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: >> >>>> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to >>>> explain where the numbers come from. >> >>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for >>> voltage range is defined as below: >>> Vdd(min) = 2.7V while Vdd(max) = 3.6V. >>> The card should work within the voltage range. >> >>> If you are afraid the voltage value is too aggressive, maybe we can >>> use regulator_set_voltage_tol() to set a smaller range. >>> But which range should be reasonable? >> >> The above makes total sense - thanks! I just wasn't aware that the >> range was specified in this fashion in the spec. Might be worth a >> comment in the code if you need to respin. > > Sure, I will update the patch. Thanks! > -- > 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 re-read spec. Please apply Kevin;s patch. Reviewed-by: Philip Rakity <prakity@nvidia.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators 2012-11-14 9:08 ` Philip Rakity @ 2012-11-14 12:05 ` Kevin Liu 0 siblings, 0 replies; 21+ messages in thread From: Kevin Liu @ 2012-11-14 12:05 UTC (permalink / raw) To: Philip Rakity Cc: Chris Ball, linux-mmc, Mark Brown, m.szyprowski, linux-kernel@vger.kernel.org List, kyungmin.park, lrg 2012/11/14 Philip Rakity <prakity@nvidia.com>: > > On Nov 14, 2012, at 8:57 AM, Kevin Liu <keyuan.liu@gmail.com> wrote: > >> 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: >>> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >>>> 2012/11/14 Mark Brown <broonie@opensource.wolfsonmicro.com>: >>> >>>>> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to >>>>> explain where the numbers come from. >>> >>>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for >>>> voltage range is defined as below: >>>> Vdd(min) = 2.7V while Vdd(max) = 3.6V. >>>> The card should work within the voltage range. >>> >>>> If you are afraid the voltage value is too aggressive, maybe we can >>>> use regulator_set_voltage_tol() to set a smaller range. >>>> But which range should be reasonable? >>> >>> The above makes total sense - thanks! I just wasn't aware that the >>> range was specified in this fashion in the spec. Might be worth a >>> comment in the code if you need to respin. >> >> Sure, I will update the patch. Thanks! >> -- >> 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 > > > re-read spec. Please apply Kevin;s patch. > > Reviewed-by: Philip Rakity <prakity@nvidia.com> Philip, thanks! ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-11-20 13:20 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-13 8:48 [PATCH 0/3] Fix fixed regulators support Marek Szyprowski 2012-11-13 8:48 ` [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage() Marek Szyprowski 2012-11-13 9:01 ` Mark Brown 2012-11-13 8:48 ` [PATCH 2/3] regulator: threat regulators with constant volatage as fixed Marek Szyprowski 2012-11-13 9:00 ` Mark Brown 2012-11-13 9:35 ` [PATCH v2] regulator: treat " Marek Szyprowski 2012-11-13 9:42 ` Mark Brown 2012-11-13 9:49 ` [PATCH v3] " Marek Szyprowski 2012-11-14 2:01 ` Mark Brown 2012-11-20 13:20 ` Marek Szyprowski 2012-11-13 8:48 ` [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators Marek Szyprowski 2012-11-13 12:45 ` Chris Ball 2012-11-13 13:32 ` [PATCH v2] " Marek Szyprowski 2012-11-13 13:45 ` Chris Ball 2012-11-13 14:09 ` Marek Szyprowski 2012-11-13 14:14 ` Chris Ball 2012-11-13 21:23 ` Philip Rakity 2012-11-20 8:42 ` Marek Szyprowski 2012-11-14 1:10 ` [PATCH 3/3] " Mark Brown [not found] <25B60CDC2F704E4E9D88FFD52780CB4C060FBEA29A@SC-VEXCH1.marvell.com> 2012-11-14 7:11 ` FW: [PATCH v2] " Kevin Liu 2012-11-14 7:28 ` Mark Brown 2012-11-14 8:36 ` Kevin Liu 2012-11-14 8:41 ` Mark Brown 2012-11-14 8:57 ` Kevin Liu 2012-11-14 9:08 ` Philip Rakity 2012-11-14 12:05 ` Kevin Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).