| Message ID | 1510042172-27220-3-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 07, 2017 at 05:09:28PM +0900, Masahiro Yamada wrote: > The remove, suspend, resume hooks need tmio_mmc_host. It is tedious > to get mmc_host from the driver_data and pass it to mmc_priv(). > We can directly set tmio_mmc_host to driver data to clean up the code. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> ... > diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c > index 64b7e9f..ccfbc15 100644 > --- a/drivers/mmc/host/tmio_mmc.c > +++ b/drivers/mmc/host/tmio_mmc.c > @@ -128,15 +128,11 @@ static int tmio_mmc_probe(struct platform_device *pdev) > static int tmio_mmc_remove(struct platform_device *pdev) > { > const struct mfd_cell *cell = mfd_get_cell(pdev); > - struct mmc_host *mmc = platform_get_drvdata(pdev); > + struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > - if (mmc) { > - struct tmio_mmc_host *host = mmc_priv(mmc); > - > - tmio_mmc_host_remove(host); > - if (cell->disable) > - cell->disable(pdev); > - } > + tmio_mmc_host_remove(host); > + if (cell->disable) > + cell->disable(pdev); Hmmm, this changes the code logic. Any reason this driver checks for a valid 'mmc' and can we safely drop it?
2017-11-20 4:49 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>: > On Tue, Nov 07, 2017 at 05:09:28PM +0900, Masahiro Yamada wrote: >> The remove, suspend, resume hooks need tmio_mmc_host. It is tedious >> to get mmc_host from the driver_data and pass it to mmc_priv(). >> We can directly set tmio_mmc_host to driver data to clean up the code. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > ... > >> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c >> index 64b7e9f..ccfbc15 100644 >> --- a/drivers/mmc/host/tmio_mmc.c >> +++ b/drivers/mmc/host/tmio_mmc.c >> @@ -128,15 +128,11 @@ static int tmio_mmc_probe(struct platform_device *pdev) >> static int tmio_mmc_remove(struct platform_device *pdev) >> { >> const struct mfd_cell *cell = mfd_get_cell(pdev); >> - struct mmc_host *mmc = platform_get_drvdata(pdev); >> + struct tmio_mmc_host *host = platform_get_drvdata(pdev); >> >> - if (mmc) { >> - struct tmio_mmc_host *host = mmc_priv(mmc); >> - >> - tmio_mmc_host_remove(host); >> - if (cell->disable) >> - cell->disable(pdev); >> - } >> + tmio_mmc_host_remove(host); >> + if (cell->disable) >> + cell->disable(pdev); > > Hmmm, this changes the code logic. Any reason this driver checks for a > valid 'mmc' and can we safely drop it? > This code has been here since the initial support of TMIO by commit 4a48998fa16121d0fe3436cce43afd6f47424103. So, we have no way to know the reason except asking the author, Ian Molton. My best guess is unnecessary if-conditional was added and overlooked in the review process. mmc has been allocated, and platform_set_drvdata() has been called in the driver probe. I do not see any case for mmc==NULL.
Yamada-san, > My best guess is unnecessary if-conditional was added > and overlooked in the review process. I agree. So, from visual review, most patches look good to me. I will give them testing on HW this evening and add my Reviewed-by tags then. I'll also reply to the driver rename thread later today. Thanks! Wolfram
2017-11-20 18:20 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>: > Yamada-san, > >> My best guess is unnecessary if-conditional was added >> and overlooked in the review process. > > I agree. > > So, from visual review, most patches look good to me. I will give them > testing on HW this evening and add my Reviewed-by tags then. I'll also > reply to the driver rename thread later today. > > Thanks! > > Wolfram > Last week I was working on the TMIO driver and I have more bug-fix and clean-up patches in hand now. (about 20 patches) The patch order is getting a mess, so I am planning to put all patches in one series. If not in hurry, can you wait for a little?
> Last week I was working on the TMIO driver > and I have more bug-fix and clean-up patches in hand now. > (about 20 patches) > > The patch order is getting a mess, > so I am planning to put all patches in one series. > > If not in hurry, can you wait for a little? So, did I get this correct: all your current patches are obsolete meanwhile? And I don't need to test or tag them until your new series comes out?
Hi Wolfram, 2017-11-20 22:22 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>: > >> Last week I was working on the TMIO driver >> and I have more bug-fix and clean-up patches in hand now. >> (about 20 patches) >> >> The patch order is getting a mess, >> so I am planning to put all patches in one series. >> >> If not in hurry, can you wait for a little? > > So, did I get this correct: all your current patches are obsolete > meanwhile? And I don't need to test or tag them until your new series > comes out? > No, that is not what I meant. I a bit felt sorry to ask you to test my patches several times. I thought I could ask you to test for a larger chunk (since 4.16 is far away anyway), but if you do not mind it, please go a head for testing the current patches. I fixed some typos, but no change in the code diff. Once you test them and issue Reviewed-by, you do not need to re-review them. Please let me postpone only the following one: https://patchwork.kernel.org/patch/10045861/ I had two ideas in my mind [1] move mmc_of_parse() to each driver's probe [2] move mmc_of_parse() to tmio_mmc_host_alloc() After I thought a bit, perhaps [2] might be cleaner... Thanks!
On Tue, Nov 07, 2017 at 05:09:28PM +0900, Masahiro Yamada wrote: > The remove, suspend, resume hooks need tmio_mmc_host. It is tedious > to get mmc_host from the driver_data and pass it to mmc_priv(). > We can directly set tmio_mmc_host to driver data to clean up the code. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 4b7a4e2..23c250e 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -656,8 +656,7 @@ EXPORT_SYMBOL_GPL(renesas_sdhi_probe); int renesas_sdhi_remove(struct platform_device *pdev) { - struct mmc_host *mmc = platform_get_drvdata(pdev); - struct tmio_mmc_host *host = mmc_priv(mmc); + struct tmio_mmc_host *host = platform_get_drvdata(pdev); tmio_mmc_host_remove(host); diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c index 64b7e9f..ccfbc15 100644 --- a/drivers/mmc/host/tmio_mmc.c +++ b/drivers/mmc/host/tmio_mmc.c @@ -128,15 +128,11 @@ static int tmio_mmc_probe(struct platform_device *pdev) static int tmio_mmc_remove(struct platform_device *pdev) { const struct mfd_cell *cell = mfd_get_cell(pdev); - struct mmc_host *mmc = platform_get_drvdata(pdev); + struct tmio_mmc_host *host = platform_get_drvdata(pdev); - if (mmc) { - struct tmio_mmc_host *host = mmc_priv(mmc); - - tmio_mmc_host_remove(host); - if (cell->disable) - cell->disable(pdev); - } + tmio_mmc_host_remove(host); + if (cell->disable) + cell->disable(pdev); return 0; } diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 9c4e619..9b8ef73 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1190,7 +1190,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host, return ret; _host->pdata = pdata; - platform_set_drvdata(pdev, mmc); + platform_set_drvdata(pdev, _host); _host->set_pwr = pdata->set_pwr; _host->set_clk_div = pdata->set_clk_div; @@ -1347,8 +1347,7 @@ EXPORT_SYMBOL_GPL(tmio_mmc_host_remove); #ifdef CONFIG_PM int tmio_mmc_host_runtime_suspend(struct device *dev) { - struct mmc_host *mmc = dev_get_drvdata(dev); - struct tmio_mmc_host *host = mmc_priv(mmc); + struct tmio_mmc_host *host = dev_get_drvdata(dev); tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); @@ -1368,8 +1367,7 @@ static bool tmio_mmc_can_retune(struct tmio_mmc_host *host) int tmio_mmc_host_runtime_resume(struct device *dev) { - struct mmc_host *mmc = dev_get_drvdata(dev); - struct tmio_mmc_host *host = mmc_priv(mmc); + struct tmio_mmc_host *host = dev_get_drvdata(dev); tmio_mmc_reset(host); tmio_mmc_clk_enable(host);
The remove, suspend, resume hooks need tmio_mmc_host. It is tedious to get mmc_host from the driver_data and pass it to mmc_priv(). We can directly set tmio_mmc_host to driver data to clean up the code. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mmc/host/renesas_sdhi_core.c | 3 +-- drivers/mmc/host/tmio_mmc.c | 12 ++++-------- drivers/mmc/host/tmio_mmc_core.c | 8 +++----- 3 files changed, 8 insertions(+), 15 deletions(-)