| Message ID | 20200306174413.20634-7-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Raspbery Pi 4 vmmc regulator support | expand |
On 6/03/20 7:44 pm, Nicolas Saenz Julienne wrote: > The sdhci core provides a helper function with the same functionality as > this controller's set_power() callback. Use it instead. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/mmc/host/sdhci-xenon.c | 20 +------------------- > 1 file changed, 1 insertion(+), 19 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > index 1dea1ba66f7b..1e9a7a76f2ba 100644 > --- a/drivers/mmc/host/sdhci-xenon.c > +++ b/drivers/mmc/host/sdhci-xenon.c > @@ -213,24 +213,6 @@ static void xenon_set_uhs_signaling(struct sdhci_host *host, > sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); > } > > -static void xenon_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > -{ > - struct mmc_host *mmc = host->mmc; > - u8 pwr = host->pwr; > - > - sdhci_set_power_noreg(host, mode, vdd); > - > - if (host->pwr == pwr) > - return; > - > - if (host->pwr == 0) > - vdd = 0; > - > - if (!IS_ERR(mmc->supply.vmmc)) > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > -} This code is different. The commit message should explain why it is equivalent. Has it been tested? > - > static void xenon_voltage_switch(struct sdhci_host *host) > { > /* Wait for 5ms after set 1.8V signal enable bit */ > @@ -240,7 +222,7 @@ static void xenon_voltage_switch(struct sdhci_host *host) > static const struct sdhci_ops sdhci_xenon_ops = { > .voltage_switch = xenon_voltage_switch, > .set_clock = sdhci_set_clock, > - .set_power = xenon_set_power, > + .set_power = sdhci_set_power_and_bus_voltage, > .set_bus_width = sdhci_set_bus_width, > .reset = xenon_reset, > .set_uhs_signaling = xenon_set_uhs_signaling, >
On Mon, 2020-03-09 at 09:20 +0200, Adrian Hunter wrote: > > -static void xenon_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > -{ > > - struct mmc_host *mmc = host->mmc; > > - u8 pwr = host->pwr; > > - > > - sdhci_set_power_noreg(host, mode, vdd); > > - > > - if (host->pwr == pwr) > > - return; > > - > > - if (host->pwr == 0) > > - vdd = 0; > > - > > - if (!IS_ERR(mmc->supply.vmmc)) > > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > -} > > This code is different. The commit message should explain why it is > equivalent. Has it been tested? Yes, I should've pointed it out. The rationale behind including sdhci-xenon and sdhci-pxav3 is based on xenon's original commit message (99c14fc360dbb): mmc: sdhci-xenon: add set_power callback Xenon sdh controller requests proper SD bus voltage select bits programmed even with vmmc power supply. Any reserved value(100b-000b) programmed in this field will lead to controller ignore SD bus power bit and keep its value at zero. Add set_power callback to handle this. I can't test it, but it felt to me as the implementation differences are only there as different people wrote the code. Ultimately, I'll be happy to drop them from the series if you feel it's too much of an assumption, I can see how the controllers could react badly to the ordering change. If not I can send a v3 with fixed commit messages. Regards, Nicolas
On 9/03/20 12:53 pm, Nicolas Saenz Julienne wrote: > On Mon, 2020-03-09 at 09:20 +0200, Adrian Hunter wrote: >>> -static void xenon_set_power(struct sdhci_host *host, unsigned char mode, >>> - unsigned short vdd) >>> -{ >>> - struct mmc_host *mmc = host->mmc; >>> - u8 pwr = host->pwr; >>> - >>> - sdhci_set_power_noreg(host, mode, vdd); >>> - >>> - if (host->pwr == pwr) >>> - return; >>> - >>> - if (host->pwr == 0) >>> - vdd = 0; >>> - >>> - if (!IS_ERR(mmc->supply.vmmc)) >>> - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); >>> -} >> >> This code is different. The commit message should explain why it is >> equivalent. Has it been tested? > > Yes, I should've pointed it out. The rationale behind including sdhci-xenon and > sdhci-pxav3 is based on xenon's original commit message (99c14fc360dbb): > > mmc: sdhci-xenon: add set_power callback > > Xenon sdh controller requests proper SD bus voltage select > bits programmed even with vmmc power supply. Any reserved > value(100b-000b) programmed in this field will lead to controller > ignore SD bus power bit and keep its value at zero. > Add set_power callback to handle this. > > I can't test it, but it felt to me as the implementation differences are only > there as different people wrote the code. Ultimately, I'll be happy to drop > them from the series if you feel it's too much of an assumption, I can see how > the controllers could react badly to the ordering change. If not I can send a > v3 with fixed commit messages. We can wait a bit and see if anyone provides a Tested-by tag, otherwise safer to drop it.
+ Zhoujie Wu <zjwu@marvell.com> On 9/03/20 1:55 pm, Adrian Hunter wrote: > On 9/03/20 12:53 pm, Nicolas Saenz Julienne wrote: >> On Mon, 2020-03-09 at 09:20 +0200, Adrian Hunter wrote: >>>> -static void xenon_set_power(struct sdhci_host *host, unsigned char mode, >>>> - unsigned short vdd) >>>> -{ >>>> - struct mmc_host *mmc = host->mmc; >>>> - u8 pwr = host->pwr; >>>> - >>>> - sdhci_set_power_noreg(host, mode, vdd); >>>> - >>>> - if (host->pwr == pwr) >>>> - return; >>>> - >>>> - if (host->pwr == 0) >>>> - vdd = 0; >>>> - >>>> - if (!IS_ERR(mmc->supply.vmmc)) >>>> - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); >>>> -} >>> >>> This code is different. The commit message should explain why it is >>> equivalent. Has it been tested? >> >> Yes, I should've pointed it out. The rationale behind including sdhci-xenon and >> sdhci-pxav3 is based on xenon's original commit message (99c14fc360dbb): >> >> mmc: sdhci-xenon: add set_power callback >> >> Xenon sdh controller requests proper SD bus voltage select >> bits programmed even with vmmc power supply. Any reserved >> value(100b-000b) programmed in this field will lead to controller >> ignore SD bus power bit and keep its value at zero. >> Add set_power callback to handle this. >> >> I can't test it, but it felt to me as the implementation differences are only >> there as different people wrote the code. Ultimately, I'll be happy to drop >> them from the series if you feel it's too much of an assumption, I can see how >> the controllers could react badly to the ordering change. If not I can send a >> v3 with fixed commit messages. > > We can wait a bit and see if anyone provides a Tested-by tag, otherwise > safer to drop it. >
diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c index 1dea1ba66f7b..1e9a7a76f2ba 100644 --- a/drivers/mmc/host/sdhci-xenon.c +++ b/drivers/mmc/host/sdhci-xenon.c @@ -213,24 +213,6 @@ static void xenon_set_uhs_signaling(struct sdhci_host *host, sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); } -static void xenon_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) -{ - struct mmc_host *mmc = host->mmc; - u8 pwr = host->pwr; - - sdhci_set_power_noreg(host, mode, vdd); - - if (host->pwr == pwr) - return; - - if (host->pwr == 0) - vdd = 0; - - if (!IS_ERR(mmc->supply.vmmc)) - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); -} - static void xenon_voltage_switch(struct sdhci_host *host) { /* Wait for 5ms after set 1.8V signal enable bit */ @@ -240,7 +222,7 @@ static void xenon_voltage_switch(struct sdhci_host *host) static const struct sdhci_ops sdhci_xenon_ops = { .voltage_switch = xenon_voltage_switch, .set_clock = sdhci_set_clock, - .set_power = xenon_set_power, + .set_power = sdhci_set_power_and_bus_voltage, .set_bus_width = sdhci_set_bus_width, .reset = xenon_reset, .set_uhs_signaling = xenon_set_uhs_signaling,
The sdhci core provides a helper function with the same functionality as this controller's set_power() callback. Use it instead. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/mmc/host/sdhci-xenon.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)