[PATCH 0/6] mmc: tmio, renesas_shci: cleanups, core code change for new driver

[PATCH 0/6] mmc: tmio, renesas_shci: cleanups, core code change for new driver

[Masahiro Yamada]

When I was implementing my dirver, I just noticed some candidates
for cleanups.  Some are required changes for my driver.



Masahiro Yamada (6):
  mmc: renesas_sdhc: remove eprobe jump label
  mmc: tmio: set tmio_mmc_host to driver data
  mmc: tmio: use devm_ioremap_resource() instead of devm_ioremap()
  mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data
  mmc: tmio, renesas_sdhi: set mmc_host_ops hooks directly
  mmc: tmio: move mmc_of_parse() out of tmio_mmc_host_probe()

 drivers/mmc/host/renesas_sdhi_core.c | 24 ++++++++++++------------
 drivers/mmc/host/tmio_mmc.c          | 16 ++++++++--------
 drivers/mmc/host/tmio_mmc.h          |  4 +---
 drivers/mmc/host/tmio_mmc_core.c     | 31 +++++++++----------------------
 4 files changed, 30 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH 1/6] mmc: renesas_sdhc: remove eprobe jump label

[Masahiro Yamada]

"goto eprobe" does nothing useful.  Return directly.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/mmc/host/renesas_sdhi_core.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index fcf7235..4b7a4e2 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -495,9 +495,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
-		ret = PTR_ERR(priv->clk);
-		dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
-		goto eprobe;
+		dev_err(&pdev->dev, "cannot get clock\n");
+		return PTR_ERR(priv->clk);
 	}
 
 	/*
@@ -524,10 +523,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	}
 
 	host = tmio_mmc_host_alloc(pdev);
-	if (!host) {
-		ret = -ENOMEM;
-		goto eprobe;
-	}
+	if (!host)
+		return -ENOMEM;
 
 	if (of_data) {
 		mmc_data->flags |= of_data->tmio_flags;
@@ -652,7 +649,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	tmio_mmc_host_remove(host);
 efree:
 	tmio_mmc_host_free(host);
-eprobe:
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(renesas_sdhi_probe);
-- 
2.7.4

[PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Masahiro Yamada]

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(-)

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);
-- 
2.7.4

[PATCH 3/6] mmc: tmio: use devm_ioremap_resource() instead of devm_ioremap()

[Masahiro Yamada]

The TMIO core misses to request_mem_region().  devm_ioremap_resource()
takes care of it and makes the code cleaner.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/mmc/host/tmio_mmc_core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 9b8ef73..0db6cac 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1182,8 +1182,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 		_host->write16_hook = NULL;
 
 	res_ctl = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res_ctl)
-		return -EINVAL;
+	_host->ctl = devm_ioremap_resource(&pdev->dev, res_ctl);
+	if (IS_ERR(_host->ctl))
+		return PTR_ERR(_host->ctl);
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
@@ -1199,11 +1200,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 	if (ret < 0)
 		return ret;
 
-	_host->ctl = devm_ioremap(&pdev->dev,
-				  res_ctl->start, resource_size(res_ctl));
-	if (!_host->ctl)
-		return -ENOMEM;
-
 	tmio_mmc_ops.card_busy = _host->card_busy;
 	tmio_mmc_ops.start_signal_voltage_switch =
 		_host->start_signal_voltage_switch;
-- 
2.7.4

[PATCH 4/6] mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data

[Masahiro Yamada]

Currently, tmio_mmc_ops is static data and tmio_mmc_host_probe()
updates some hooks in the static data.  This is a problem when
two or more instances call tmio_mmc_host_probe() and each of them
requests to use its own card_busy/start_signal_voltage_switch.

We can borrow a solution from sdhci_alloc_host().  Copy the whole
ops structure to host->mmc_host_ops, then override the hooks in
malloc'ed data.  Constify tmio_mmc_ops since it is a template ops
used by default.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/mmc/host/tmio_mmc.h      | 1 +
 drivers/mmc/host/tmio_mmc_core.c | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 3e6ff89..b251f1c 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -134,6 +134,7 @@ struct tmio_mmc_host {
 	struct mmc_request      *mrq;
 	struct mmc_data         *data;
 	struct mmc_host         *mmc;
+	struct mmc_host_ops     mmc_host_ops;
 
 	/* Callbacks for clock / power control */
 	void (*set_pwr)(struct platform_device *host, int state);
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 0db6cac..3c84044 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1098,7 +1098,7 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static struct mmc_host_ops tmio_mmc_ops = {
+static const struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
 	.get_ro         = tmio_mmc_get_ro,
@@ -1155,6 +1155,8 @@ tmio_mmc_host_alloc(struct platform_device *pdev)
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
 	host->pdev = pdev;
+	host->mmc_host_ops = tmio_mmc_ops;
+	mmc->ops = &host->mmc_host_ops;
 
 	return host;
 }
@@ -1200,10 +1202,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 	if (ret < 0)
 		return ret;
 
-	tmio_mmc_ops.card_busy = _host->card_busy;
-	tmio_mmc_ops.start_signal_voltage_switch =
+	_host->mmc_host_ops.card_busy = _host->card_busy;
+	_host->mmc_host_ops.start_signal_voltage_switch =
 		_host->start_signal_voltage_switch;
-	mmc->ops = &tmio_mmc_ops;
 
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
-- 
2.7.4

[PATCH 5/6] mmc: tmio, renesas_sdhi: set mmc_host_ops hooks directly

[Masahiro Yamada]

Drivers can set any mmc_host_ops hooks between tmio_mmc_host_alloc()
and tmio_mmc_host_probe().  Remove duplicated hooks in tmio_mmc_host.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/mmc/host/renesas_sdhi_core.c | 4 ++--
 drivers/mmc/host/tmio_mmc.h          | 3 ---
 drivers/mmc/host/tmio_mmc_core.c     | 4 ----
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 23c250e..7bb0252 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -548,8 +548,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	/* SDR speeds are only available on Gen2+ */
 	if (mmc_data->flags & TMIO_MMC_MIN_RCAR2) {
 		/* card_busy caused issues on r8a73a4 (pre-Gen2) CD-less SDHI */
-		host->card_busy	= renesas_sdhi_card_busy;
-		host->start_signal_voltage_switch =
+		host->mmc_host_ops.card_busy = renesas_sdhi_card_busy;
+		host->mmc_host_ops.start_signal_voltage_switch =
 			renesas_sdhi_start_signal_voltage_switch;
 	}
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b251f1c..319a2d6 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -186,9 +186,6 @@ struct tmio_mmc_host {
 	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
-	int (*card_busy)(struct mmc_host *mmc);
-	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
-					   struct mmc_ios *ios);
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	void (*hw_reset)(struct tmio_mmc_host *host);
 	void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 3c84044..695b4f4 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1202,10 +1202,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 	if (ret < 0)
 		return ret;
 
-	_host->mmc_host_ops.card_busy = _host->card_busy;
-	_host->mmc_host_ops.start_signal_voltage_switch =
-		_host->start_signal_voltage_switch;
-
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
 	mmc->max_segs = pdata->max_segs ? : 32;
-- 
2.7.4

[PATCH 6/6] mmc: tmio: move mmc_of_parse() out of tmio_mmc_host_probe()

[Masahiro Yamada]

mmc_of_parse() parses various DT properties and sets capability flags
accordingly.  However, drivers have no chance to run platform init
code depending on such flags because mmc_of_parse() is called from
tmio_mmc_host_probe().

Move mmc_of_parse() out of tmio_mmc_host_probe() so that drivers can
handle capabilities before mmc_add_host().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/mmc/host/renesas_sdhi_core.c | 4 ++++
 drivers/mmc/host/tmio_mmc.c          | 4 ++++
 drivers/mmc/host/tmio_mmc_core.c     | 4 ----
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 7bb0252..4658fb5 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -526,6 +526,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	if (!host)
 		return -ENOMEM;
 
+	ret = mmc_of_parse(host->mmc);
+	if (ret)
+		goto efree;
+
 	if (of_data) {
 		mmc_data->flags |= of_data->tmio_flags;
 		mmc_data->ocr_mask = of_data->tmio_ocr_mask;
diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c
index ccfbc15..87052b3 100644
--- a/drivers/mmc/host/tmio_mmc.c
+++ b/drivers/mmc/host/tmio_mmc.c
@@ -96,6 +96,10 @@ static int tmio_mmc_probe(struct platform_device *pdev)
 	if (!host)
 		goto cell_disable;
 
+	ret = mmc_of_parse(host->mmc);
+	if (ret)
+		goto host_free;
+
 	/* SD control register space size is 0x200, 0x400 for bus_shift=1 */
 	host->bus_shift = resource_size(res) >> 10;
 
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 695b4f4..c2f759b 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1188,10 +1188,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 	if (IS_ERR(_host->ctl))
 		return PTR_ERR(_host->ctl);
 
-	ret = mmc_of_parse(mmc);
-	if (ret < 0)
-		return ret;
-
 	_host->pdata = pdata;
 	platform_set_drvdata(pdev, _host);
 
-- 
2.7.4

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Masahiro Yamada]

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?





-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]


> 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Masahiro Yamada]

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!

-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/6] mmc: renesas_sdhc: remove eprobe jump label

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]


> -		ret = PTR_ERR(priv->clk);
> -		dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
> -		goto eprobe;
> +		dev_err(&pdev->dev, "cannot get clock\n");
> +		return PTR_ERR(priv->clk);

Why dropping the 'ret' printout? Will it be printed by the core?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] mmc: tmio: set tmio_mmc_host to driver data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

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>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] mmc: tmio: use devm_ioremap_resource() instead of devm_ioremap()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Tue, Nov 07, 2017 at 05:09:29PM +0900, Masahiro Yamada wrote:
> The TMIO core misses to request_mem_region().  devm_ioremap_resource()
> takes care of it and makes the code cleaner.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Tue, Nov 07, 2017 at 05:09:30PM +0900, Masahiro Yamada wrote:
> Currently, tmio_mmc_ops is static data and tmio_mmc_host_probe()
> updates some hooks in the static data.  This is a problem when
> two or more instances call tmio_mmc_host_probe() and each of them
> requests to use its own card_busy/start_signal_voltage_switch.
> 
> We can borrow a solution from sdhci_alloc_host().  Copy the whole
> ops structure to host->mmc_host_ops, then override the hooks in
> malloc'ed data.  Constify tmio_mmc_ops since it is a template ops
> used by default.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] mmc: tmio, renesas_sdhi: set mmc_host_ops hooks directly

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On Tue, Nov 07, 2017 at 05:09:31PM +0900, Masahiro Yamada wrote:
> Drivers can set any mmc_host_ops hooks between tmio_mmc_host_alloc()
> and tmio_mmc_host_probe().  Remove duplicated hooks in tmio_mmc_host.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Yes, this is much cleaner. I like it!

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] mmc: tmio: move mmc_of_parse() out of tmio_mmc_host_probe()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Tue, Nov 07, 2017 at 05:09:32PM +0900, Masahiro Yamada wrote:
> mmc_of_parse() parses various DT properties and sets capability flags
> accordingly.  However, drivers have no chance to run platform init
> code depending on such flags because mmc_of_parse() is called from
> tmio_mmc_host_probe().
> 
> Move mmc_of_parse() out of tmio_mmc_host_probe() so that drivers can
> handle capabilities before mmc_add_host().
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

I'll just add this:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

because you wanted to still try which solution is better.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 145 bytes --]


> +	struct mmc_host_ops     mmc_host_ops;

Just came to think of it: maybe a shorter name?

 host->ops

is still nicely readable, I'd think...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] mmc: renesas_sdhc: remove eprobe jump label

[Masahiro Yamada]

2017-11-21 5:31 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
>
>> -             ret = PTR_ERR(priv->clk);
>> -             dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
>> -             goto eprobe;
>> +             dev_err(&pdev->dev, "cannot get clock\n");
>> +             return PTR_ERR(priv->clk);
>
> Why dropping the 'ret' printout? Will it be printed by the core?
>

No.
I just wanted to save "ret = PTR_ERR(priv->clk)" line.


I will restore the printout as follows:

priv->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(priv->clk)) {
        ret = PTR_ERR(priv->clk);
        dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
        return ret;
}



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 4/6] mmc: tmio: move mmc_host_ops to struct tmio_mmc_host from static data

[Masahiro Yamada]

2017-11-21 5:48 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
>
>> +     struct mmc_host_ops     mmc_host_ops;
>
> Just came to think of it: maybe a shorter name?
>
>  host->ops
>
> is still nicely readable, I'd think...
>

I just thought it should be clear which ops,
but nobody would be confused with "dma_ops".

OK, I will rename it to "ops" in v2.



-- 
Best Regards
Masahiro Yamada