diff mbox series

[v3,3/3] spi: geni-qcom: Use devm functions to simplify code

Message ID 20240909073141.951494-4-ruanjinjie@huawei.com (mailing list archive)
State Superseded
Headers show
Series spi: geni-qcom: Undo runtime PM changes at driver exit time | expand

Commit Message

Jinjie Ruan Sept. 9, 2024, 7:31 a.m. UTC
Use devm_pm_runtime_enable(), devm_request_irq() and
devm_spi_register_controller() to simplify code.

And also register a callback spi_geni_release_dma_chan() with
devm_add_action_or_reset(), to release dma channel in both error
and device detach path, which can make sure the release sequence is
consistent with the original one.

1. Unregister spi controller.
2. Free the IRQ.
3. Free DMA chans
4. Disable runtime PM.

So the remove function can also be removed.

Suggested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Land the rest of the cleanups afterwards.
---
 drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

Comments

Dmitry Baryshkov Sept. 9, 2024, 9:49 a.m. UTC | #1
On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
> Use devm_pm_runtime_enable(), devm_request_irq() and
> devm_spi_register_controller() to simplify code.
> 
> And also register a callback spi_geni_release_dma_chan() with
> devm_add_action_or_reset(), to release dma channel in both error
> and device detach path, which can make sure the release sequence is
> consistent with the original one.
> 
> 1. Unregister spi controller.
> 2. Free the IRQ.
> 3. Free DMA chans
> 4. Disable runtime PM.
> 
> So the remove function can also be removed.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v3:
> - Land the rest of the cleanups afterwards.
> ---
>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 6f4057330444..8b0039d14605 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>  	return ret;
>  }
>  
> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
> +static void spi_geni_release_dma_chan(void *data)
>  {
> +	struct spi_geni_master *mas = data;
> +
>  	if (mas->rx) {
>  		dma_release_channel(mas->rx);
>  		mas->rx = NULL;
> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);

This should be mas, not spi.

Doesn't looks like this was tested. Please correct me if I'm wrong.
Jinjie Ruan Sept. 9, 2024, 11:46 a.m. UTC | #2
On 2024/9/9 17:49, Dmitry Baryshkov wrote:
> On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
>> Use devm_pm_runtime_enable(), devm_request_irq() and
>> devm_spi_register_controller() to simplify code.
>>
>> And also register a callback spi_geni_release_dma_chan() with
>> devm_add_action_or_reset(), to release dma channel in both error
>> and device detach path, which can make sure the release sequence is
>> consistent with the original one.
>>
>> 1. Unregister spi controller.
>> 2. Free the IRQ.
>> 3. Free DMA chans
>> 4. Disable runtime PM.
>>
>> So the remove function can also be removed.
>>
>> Suggested-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> v3:
>> - Land the rest of the cleanups afterwards.
>> ---
>>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>>  1 file changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>> index 6f4057330444..8b0039d14605 100644
>> --- a/drivers/spi/spi-geni-qcom.c
>> +++ b/drivers/spi/spi-geni-qcom.c
>> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>>  	return ret;
>>  }
>>  
>> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
>> +static void spi_geni_release_dma_chan(void *data)
>>  {
>> +	struct spi_geni_master *mas = data;
>> +
>>  	if (mas->rx) {
>>  		dma_release_channel(mas->rx);
>>  		mas->rx = NULL;
>> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
> 
> This should be mas, not spi.
> 
> Doesn't looks like this was tested. Please correct me if I'm wrong.

Yes, you are right, the data should be struct spi_geni_master, which is
mas. Sorry, only compile passed.

> 
>
Dmitry Baryshkov Sept. 9, 2024, 11:51 a.m. UTC | #3
On Mon, 9 Sept 2024 at 14:46, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/9/9 17:49, Dmitry Baryshkov wrote:
> > On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
> >> Use devm_pm_runtime_enable(), devm_request_irq() and
> >> devm_spi_register_controller() to simplify code.
> >>
> >> And also register a callback spi_geni_release_dma_chan() with
> >> devm_add_action_or_reset(), to release dma channel in both error
> >> and device detach path, which can make sure the release sequence is
> >> consistent with the original one.
> >>
> >> 1. Unregister spi controller.
> >> 2. Free the IRQ.
> >> 3. Free DMA chans
> >> 4. Disable runtime PM.
> >>
> >> So the remove function can also be removed.
> >>
> >> Suggested-by: Doug Anderson <dianders@chromium.org>
> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> >> ---
> >> v3:
> >> - Land the rest of the cleanups afterwards.
> >> ---
> >>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
> >>  1 file changed, 13 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> >> index 6f4057330444..8b0039d14605 100644
> >> --- a/drivers/spi/spi-geni-qcom.c
> >> +++ b/drivers/spi/spi-geni-qcom.c
> >> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
> >>      return ret;
> >>  }
> >>
> >> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
> >> +static void spi_geni_release_dma_chan(void *data)
> >>  {
> >> +    struct spi_geni_master *mas = data;
> >> +
> >>      if (mas->rx) {
> >>              dma_release_channel(mas->rx);
> >>              mas->rx = NULL;
> >> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
> >>      if (ret)
> >>              return ret;
> >>
> >> +    ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
> >
> > This should be mas, not spi.
> >
> > Doesn't looks like this was tested. Please correct me if I'm wrong.
>
> Yes, you are right, the data should be struct spi_geni_master, which is
> mas. Sorry, only compile passed.

Please perform a runtime test or mention it in the cover letter that
it was only compile-tested.
Jinjie Ruan Sept. 9, 2024, 12:01 p.m. UTC | #4
On 2024/9/9 19:51, Dmitry Baryshkov wrote:
> On Mon, 9 Sept 2024 at 14:46, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2024/9/9 17:49, Dmitry Baryshkov wrote:
>>> On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
>>>> Use devm_pm_runtime_enable(), devm_request_irq() and
>>>> devm_spi_register_controller() to simplify code.
>>>>
>>>> And also register a callback spi_geni_release_dma_chan() with
>>>> devm_add_action_or_reset(), to release dma channel in both error
>>>> and device detach path, which can make sure the release sequence is
>>>> consistent with the original one.
>>>>
>>>> 1. Unregister spi controller.
>>>> 2. Free the IRQ.
>>>> 3. Free DMA chans
>>>> 4. Disable runtime PM.
>>>>
>>>> So the remove function can also be removed.
>>>>
>>>> Suggested-by: Doug Anderson <dianders@chromium.org>
>>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>>> ---
>>>> v3:
>>>> - Land the rest of the cleanups afterwards.
>>>> ---
>>>>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>>>>  1 file changed, 13 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>>> index 6f4057330444..8b0039d14605 100644
>>>> --- a/drivers/spi/spi-geni-qcom.c
>>>> +++ b/drivers/spi/spi-geni-qcom.c
>>>> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>>>>      return ret;
>>>>  }
>>>>
>>>> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
>>>> +static void spi_geni_release_dma_chan(void *data)
>>>>  {
>>>> +    struct spi_geni_master *mas = data;
>>>> +
>>>>      if (mas->rx) {
>>>>              dma_release_channel(mas->rx);
>>>>              mas->rx = NULL;
>>>> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>      if (ret)
>>>>              return ret;
>>>>
>>>> +    ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
>>>
>>> This should be mas, not spi.
>>>
>>> Doesn't looks like this was tested. Please correct me if I'm wrong.
>>
>> Yes, you are right, the data should be struct spi_geni_master, which is
>> mas. Sorry, only compile passed.
> 
> Please perform a runtime test or mention it in the cover letter that
> it was only compile-tested.

Thank you! I'll add this information next version.

> 
> 
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 6f4057330444..8b0039d14605 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -632,8 +632,10 @@  static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
 	return ret;
 }
 
-static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
+static void spi_geni_release_dma_chan(void *data)
 {
+	struct spi_geni_master *mas = data;
+
 	if (mas->rx) {
 		dma_release_channel(mas->rx);
 		mas->rx = NULL;
@@ -1132,6 +1134,12 @@  static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
+	if (ret) {
+		dev_err(dev, "Unable to add action.\n");
+		return ret;
+	}
+
 	/*
 	 * check the mode supported and set_cs for fifo mode only
 	 * for dma (gsi) mode, the gsi will set cs based on params passed in
@@ -1146,33 +1154,15 @@  static int spi_geni_probe(struct platform_device *pdev)
 	if (mas->cur_xfer_mode == GENI_GPI_DMA)
 		spi->flags = SPI_CONTROLLER_MUST_TX;
 
-	ret = request_irq(mas->irq, geni_spi_isr, 0, dev_name(dev), spi);
+	ret = devm_request_irq(dev, mas->irq, geni_spi_isr, 0, dev_name(dev), spi);
 	if (ret)
-		goto spi_geni_release_dma;
+		return ret;
 
-	ret = spi_register_controller(spi);
+	ret = devm_spi_register_controller(dev, spi);
 	if (ret)
-		goto spi_geni_probe_free_irq;
+		return ret;
 
 	return 0;
-spi_geni_probe_free_irq:
-	free_irq(mas->irq, spi);
-spi_geni_release_dma:
-	spi_geni_release_dma_chan(mas);
-	return ret;
-}
-
-static void spi_geni_remove(struct platform_device *pdev)
-{
-	struct spi_controller *spi = platform_get_drvdata(pdev);
-	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
-
-	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
-	spi_unregister_controller(spi);
-
-	free_irq(mas->irq, spi);
-
-	spi_geni_release_dma_chan(mas);
 }
 
 static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
@@ -1254,7 +1244,6 @@  MODULE_DEVICE_TABLE(of, spi_geni_dt_match);
 
 static struct platform_driver spi_geni_driver = {
 	.probe  = spi_geni_probe,
-	.remove_new = spi_geni_remove,
 	.driver = {
 		.name = "geni_spi",
 		.pm = &spi_geni_pm_ops,