Patch 1 fixes an issue about an error code that is erroneously logged.
Patch 2-4 are just clean-ups spotted while fixing it.
Additional comments are added below --- in patches 2 and 3.
Christophe JAILLET (4):
spi: mt7621: Fix an error message in mt7621_spi_probe()
spi: mt7621: Use the devm_clk_get_enabled() helper to simplify error
handling
spi: mt7621: Use devm_spi_register_controller()
spi: mt7621: Remove 'clk' from 'struct mt7621_spi'
drivers/spi/spi-mt7621.c | 42 ++++++----------------------------------
1 file changed, 6 insertions(+), 36 deletions(-)
--
2.34.1
'status' is known to be 0 at this point. The expected error code is
PTR_ERR(clk).
Switch to dev_err_probe() in order to display the expected error code (in a
human readable way).
This also filters -EPROBE_DEFER cases, should it happen.
Fixes: 1ab7f2a43558 ("staging: mt7621-spi: add mt7621 support")
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/spi/spi-mt7621.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
index b4b9b7309b5e..351b0ef52bbc 100644
--- a/drivers/spi/spi-mt7621.c
+++ b/drivers/spi/spi-mt7621.c
@@ -340,11 +340,9 @@ static int mt7621_spi_probe(struct platform_device *pdev)
return PTR_ERR(base);
clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(clk)) {
- dev_err(&pdev->dev, "unable to get SYS clock, err=%d\n",
- status);
- return PTR_ERR(clk);
- }
+ if (IS_ERR(clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ "unable to get SYS clock\n");
status = clk_prepare_enable(clk);
if (status)
--
2.34.1
The devm_clk_get_enabled() helper:
- calls devm_clk_get()
- calls clk_prepare_enable() and registers what is needed in order to
call clk_disable_unprepare() when needed, as a managed resource.
This helper is well suited for cases where the clock would be kept
prepared or enabled for the whole lifetime of the driver.
This simplifies the error handling a lot.
The order between spi_unregister_controller() (in the remove function) and
the clk_disable_unprepare() (now handle by a managed resource) is kept
the same.
(see commit 46b5c4fb87ce ("spi: mt7621: Don't leak SPI master in probe
error path") to see why it matters)
Signed-off-by: Christophe JAILLET <[email protected]>
---
The order with devm_spi_release_controller() (which undoes
devm_spi_alloc_master()) is reversed, but I don't think it is an issue.
---
drivers/spi/spi-mt7621.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
index 351b0ef52bbc..2580b28042be 100644
--- a/drivers/spi/spi-mt7621.c
+++ b/drivers/spi/spi-mt7621.c
@@ -327,7 +327,6 @@ static int mt7621_spi_probe(struct platform_device *pdev)
struct spi_controller *master;
struct mt7621_spi *rs;
void __iomem *base;
- int status = 0;
struct clk *clk;
int ret;
@@ -339,19 +338,14 @@ static int mt7621_spi_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- clk = devm_clk_get(&pdev->dev, NULL);
+ clk = devm_clk_get_enabled(&pdev->dev, NULL);
if (IS_ERR(clk))
return dev_err_probe(&pdev->dev, PTR_ERR(clk),
"unable to get SYS clock\n");
- status = clk_prepare_enable(clk);
- if (status)
- return status;
-
master = devm_spi_alloc_master(&pdev->dev, sizeof(*rs));
if (!master) {
dev_info(&pdev->dev, "master allocation failed\n");
- clk_disable_unprepare(clk);
return -ENOMEM;
}
@@ -376,13 +370,10 @@ static int mt7621_spi_probe(struct platform_device *pdev)
ret = device_reset(&pdev->dev);
if (ret) {
dev_err(&pdev->dev, "SPI reset failed!\n");
- clk_disable_unprepare(clk);
return ret;
}
ret = spi_register_controller(master);
- if (ret)
- clk_disable_unprepare(clk);
return ret;
}
@@ -390,13 +381,10 @@ static int mt7621_spi_probe(struct platform_device *pdev)
static int mt7621_spi_remove(struct platform_device *pdev)
{
struct spi_controller *master;
- struct mt7621_spi *rs;
master = dev_get_drvdata(&pdev->dev);
- rs = spi_controller_get_devdata(master);
spi_unregister_controller(master);
- clk_disable_unprepare(rs->clk);
return 0;
}
--
2.34.1
The 'clk' field in 'struct mt7621_spi' is useless, remove it.
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/spi/spi-mt7621.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
index 114f98dcae5e..c4cc8e2f85e2 100644
--- a/drivers/spi/spi-mt7621.c
+++ b/drivers/spi/spi-mt7621.c
@@ -55,7 +55,6 @@ struct mt7621_spi {
void __iomem *base;
unsigned int sys_freq;
unsigned int speed;
- struct clk *clk;
int pending_write;
};
@@ -361,9 +360,8 @@ static int mt7621_spi_probe(struct platform_device *pdev)
rs = spi_controller_get_devdata(master);
rs->base = base;
- rs->clk = clk;
rs->master = master;
- rs->sys_freq = clk_get_rate(rs->clk);
+ rs->sys_freq = clk_get_rate(clk);
rs->pending_write = 0;
dev_info(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
--
2.34.1
Now that clk_disable_unprepare(clk) is handled with a managed resource,
we can use devm_spi_register_controller() and axe the .remove function.
The order between spi_unregister_controller() and clk_disable_unprepare()
is still the same.
(see commit 46b5c4fb87ce ("spi: mt7621: Don't leak SPI master in probe
error path") to see why it matters)
Signed-off-by: Christophe JAILLET <[email protected]>
---
I guess that the dev_set_drvdata() in the probe can be removed as-well.
But it is also harmless to leave it as-is.
---
drivers/spi/spi-mt7621.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
index 2580b28042be..114f98dcae5e 100644
--- a/drivers/spi/spi-mt7621.c
+++ b/drivers/spi/spi-mt7621.c
@@ -373,20 +373,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
return ret;
}
- ret = spi_register_controller(master);
-
- return ret;
-}
-
-static int mt7621_spi_remove(struct platform_device *pdev)
-{
- struct spi_controller *master;
-
- master = dev_get_drvdata(&pdev->dev);
-
- spi_unregister_controller(master);
-
- return 0;
+ return devm_spi_register_controller(&pdev->dev, master);
}
MODULE_ALIAS("platform:" DRIVER_NAME);
@@ -397,7 +384,6 @@ static struct platform_driver mt7621_spi_driver = {
.of_match_table = mt7621_spi_match,
},
.probe = mt7621_spi_probe,
- .remove = mt7621_spi_remove,
};
module_platform_driver(mt7621_spi_driver);
--
2.34.1
On 27/08/2022 13:42, Christophe JAILLET wrote:
> The devm_clk_get_enabled() helper:
> - calls devm_clk_get()
> - calls clk_prepare_enable() and registers what is needed in order to
> call clk_disable_unprepare() when needed, as a managed resource.
>
> This helper is well suited for cases where the clock would be kept
> prepared or enabled for the whole lifetime of the driver.
>
> This simplifies the error handling a lot.
>
> The order between spi_unregister_controller() (in the remove function) and
> the clk_disable_unprepare() (now handle by a managed resource) is kept
> the same.
> (see commit 46b5c4fb87ce ("spi: mt7621: Don't leak SPI master in probe
> error path") to see why it matters)
>
> Signed-off-by: Christophe JAILLET <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> The order with devm_spi_release_controller() (which undoes
> devm_spi_alloc_master()) is reversed, but I don't think it is an issue.
> ---
> drivers/spi/spi-mt7621.c | 14 +-------------
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
> index 351b0ef52bbc..2580b28042be 100644
> --- a/drivers/spi/spi-mt7621.c
> +++ b/drivers/spi/spi-mt7621.c
> @@ -327,7 +327,6 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> struct spi_controller *master;
> struct mt7621_spi *rs;
> void __iomem *base;
> - int status = 0;
> struct clk *clk;
> int ret;
>
> @@ -339,19 +338,14 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> + clk = devm_clk_get_enabled(&pdev->dev, NULL);
> if (IS_ERR(clk))
> return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> "unable to get SYS clock\n");
>
> - status = clk_prepare_enable(clk);
> - if (status)
> - return status;
> -
> master = devm_spi_alloc_master(&pdev->dev, sizeof(*rs));
> if (!master) {
> dev_info(&pdev->dev, "master allocation failed\n");
> - clk_disable_unprepare(clk);
> return -ENOMEM;
> }
>
> @@ -376,13 +370,10 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> ret = device_reset(&pdev->dev);
> if (ret) {
> dev_err(&pdev->dev, "SPI reset failed!\n");
> - clk_disable_unprepare(clk);
> return ret;
> }
>
> ret = spi_register_controller(master);
> - if (ret)
> - clk_disable_unprepare(clk);
>
> return ret;
> }
> @@ -390,13 +381,10 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> static int mt7621_spi_remove(struct platform_device *pdev)
> {
> struct spi_controller *master;
> - struct mt7621_spi *rs;
>
> master = dev_get_drvdata(&pdev->dev);
> - rs = spi_controller_get_devdata(master);
>
> spi_unregister_controller(master);
> - clk_disable_unprepare(rs->clk);
>
> return 0;
> }
On 27/08/2022 13:42, Christophe JAILLET wrote:
> 'status' is known to be 0 at this point. The expected error code is
> PTR_ERR(clk).
>
> Switch to dev_err_probe() in order to display the expected error code (in a
> human readable way).
> This also filters -EPROBE_DEFER cases, should it happen.
>
> Fixes: 1ab7f2a43558 ("staging: mt7621-spi: add mt7621 support")
> Signed-off-by: Christophe JAILLET <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/spi/spi-mt7621.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
> index b4b9b7309b5e..351b0ef52bbc 100644
> --- a/drivers/spi/spi-mt7621.c
> +++ b/drivers/spi/spi-mt7621.c
> @@ -340,11 +340,9 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> return PTR_ERR(base);
>
> clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(clk)) {
> - dev_err(&pdev->dev, "unable to get SYS clock, err=%d\n",
> - status);
> - return PTR_ERR(clk);
> - }
> + if (IS_ERR(clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> + "unable to get SYS clock\n");
>
> status = clk_prepare_enable(clk);
> if (status)
On 27/08/2022 13:42, Christophe JAILLET wrote:
> The 'clk' field in 'struct mt7621_spi' is useless, remove it.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
IMHO should be part of patch 2/4.
Regards,
Matthias
> ---
> drivers/spi/spi-mt7621.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
> index 114f98dcae5e..c4cc8e2f85e2 100644
> --- a/drivers/spi/spi-mt7621.c
> +++ b/drivers/spi/spi-mt7621.c
> @@ -55,7 +55,6 @@ struct mt7621_spi {
> void __iomem *base;
> unsigned int sys_freq;
> unsigned int speed;
> - struct clk *clk;
> int pending_write;
> };
>
> @@ -361,9 +360,8 @@ static int mt7621_spi_probe(struct platform_device *pdev)
>
> rs = spi_controller_get_devdata(master);
> rs->base = base;
> - rs->clk = clk;
> rs->master = master;
> - rs->sys_freq = clk_get_rate(rs->clk);
> + rs->sys_freq = clk_get_rate(clk);
> rs->pending_write = 0;
> dev_info(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
>
On 27/08/2022 13:42, Christophe JAILLET wrote:
> Now that clk_disable_unprepare(clk) is handled with a managed resource,
> we can use devm_spi_register_controller() and axe the .remove function.
>
> The order between spi_unregister_controller() and clk_disable_unprepare()
> is still the same.
> (see commit 46b5c4fb87ce ("spi: mt7621: Don't leak SPI master in probe
> error path") to see why it matters)
>
> Signed-off-by: Christophe JAILLET <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> I guess that the dev_set_drvdata() in the probe can be removed as-well.
> But it is also harmless to leave it as-is.
> ---
> drivers/spi/spi-mt7621.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
> index 2580b28042be..114f98dcae5e 100644
> --- a/drivers/spi/spi-mt7621.c
> +++ b/drivers/spi/spi-mt7621.c
> @@ -373,20 +373,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
> return ret;
> }
>
> - ret = spi_register_controller(master);
> -
> - return ret;
> -}
> -
> -static int mt7621_spi_remove(struct platform_device *pdev)
> -{
> - struct spi_controller *master;
> -
> - master = dev_get_drvdata(&pdev->dev);
> -
> - spi_unregister_controller(master);
> -
> - return 0;
> + return devm_spi_register_controller(&pdev->dev, master);
> }
>
> MODULE_ALIAS("platform:" DRIVER_NAME);
> @@ -397,7 +384,6 @@ static struct platform_driver mt7621_spi_driver = {
> .of_match_table = mt7621_spi_match,
> },
> .probe = mt7621_spi_probe,
> - .remove = mt7621_spi_remove,
> };
>
> module_platform_driver(mt7621_spi_driver);
Le 29/08/2022 à 13:18, Matthias Brugger a écrit :
>
>
> On 27/08/2022 13:42, Christophe JAILLET wrote:
>> The 'clk' field in 'struct mt7621_spi' is useless, remove it.
>>
>> Signed-off-by: Christophe JAILLET <[email protected]>
>
> IMHO should be part of patch 2/4.
Ok, I'll send a v2 of the serie to merge patch 2 & 4.
CJ
>
> Regards,
> Matthias
>
>> ---
>> drivers/spi/spi-mt7621.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
>> index 114f98dcae5e..c4cc8e2f85e2 100644
>> --- a/drivers/spi/spi-mt7621.c
>> +++ b/drivers/spi/spi-mt7621.c
>> @@ -55,7 +55,6 @@ struct mt7621_spi {
>> void __iomem *base;
>> unsigned int sys_freq;
>> unsigned int speed;
>> - struct clk *clk;
>> int pending_write;
>> };
>> @@ -361,9 +360,8 @@ static int mt7621_spi_probe(struct platform_device
>> *pdev)
>> rs = spi_controller_get_devdata(master);
>> rs->base = base;
>> - rs->clk = clk;
>> rs->master = master;
>> - rs->sys_freq = clk_get_rate(rs->clk);
>> + rs->sys_freq = clk_get_rate(clk);
>> rs->pending_write = 0;
>> dev_info(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
On Sat, 27 Aug 2022 13:41:39 +0200, Christophe JAILLET wrote:
> Patch 1 fixes an issue about an error code that is erroneously logged.
>
> Patch 2-4 are just clean-ups spotted while fixing it.
>
> Additional comments are added below --- in patches 2 and 3.
>
> Christophe JAILLET (4):
> spi: mt7621: Fix an error message in mt7621_spi_probe()
> spi: mt7621: Use the devm_clk_get_enabled() helper to simplify error
> handling
> spi: mt7621: Use devm_spi_register_controller()
> spi: mt7621: Remove 'clk' from 'struct mt7621_spi'
>
> [...]
Applied to
broonie/spi.git for-next
Thanks!
[1/4] spi: mt7621: Fix an error message in mt7621_spi_probe()
commit: 2b2bf6b7faa9010fae10dc7de76627a3fdb525b3
[2/4] spi: mt7621: Use the devm_clk_get_enabled() helper to simplify error handling
commit: 3d6af96814d753f34cf897466e7ddf041d0cdf3b
[3/4] spi: mt7621: Use devm_spi_register_controller()
commit: 30b31b29a866d32fc381b406d4c4f5db2636e5ec
[4/4] spi: mt7621: Remove 'clk' from 'struct mt7621_spi'
commit: 4a5cc683543f5f6ed586944095c65cb4da4b9273
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Il 27/08/22 13:42, Christophe JAILLET ha scritto:
> The devm_clk_get_enabled() helper:
> - calls devm_clk_get()
> - calls clk_prepare_enable() and registers what is needed in order to
> call clk_disable_unprepare() when needed, as a managed resource.
>
> This helper is well suited for cases where the clock would be kept
> prepared or enabled for the whole lifetime of the driver.
>
> This simplifies the error handling a lot.
>
> The order between spi_unregister_controller() (in the remove function) and
> the clk_disable_unprepare() (now handle by a managed resource) is kept
> the same.
> (see commit 46b5c4fb87ce ("spi: mt7621: Don't leak SPI master in probe
> error path") to see why it matters)
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> Reviewed-by: Matthias Brugger <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 27/08/22 13:42, Christophe JAILLET ha scritto:
> 'status' is known to be 0 at this point. The expected error code is
> PTR_ERR(clk).
>
> Switch to dev_err_probe() in order to display the expected error code (in a
> human readable way).
> This also filters -EPROBE_DEFER cases, should it happen.
>
> Fixes: 1ab7f2a43558 ("staging: mt7621-spi: add mt7621 support")
> Signed-off-by: Christophe JAILLET <[email protected]>
> Reviewed-by: Matthias Brugger <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>