diff mbox series

[v4,2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

Message ID 20230704090453.83980-3-william.qiu@starfivetech.com
State Superseded
Headers show
Series Add SPI module for StarFive JH7110 SoC | expand

Commit Message

William Qiu July 4, 2023, 9:04 a.m. UTC 
Add QSPI clock operation in device probe.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/
---
 drivers/spi/spi-cadence-quadspi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Conor Dooley July 4, 2023, 4:36 p.m. UTC | #1 
Hey William,

On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote:
> Add QSPI clock operation in device probe.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/

These Reported-by tags don't seem correct, given they were reports about
this patch, not the reason for it - but did you actually check that you
fixed the errors that the patch produces?

This particular one seems to complain about a hunk that is still in the
patch & the CI running on the RISC-V patchwork is complaining about it.

Cheers,
Conor.

> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>  	struct spi_master *master = dev_get_drvdata(dev);
>  
>  	clk_prepare_enable(cqspi->clk);
> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
> +		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
>  	cqspi_wait_idle(cqspi);
>  	cqspi_controller_init(cqspi);
>  
> -- 
> 2.34.1
>
Mark Brown July 4, 2023, 4:41 p.m. UTC | #2 
On Tue, Jul 04, 2023 at 05:36:03PM +0100, Conor Dooley wrote:
> On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote:
> > Add QSPI clock operation in device probe.
> > 
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/

> These Reported-by tags don't seem correct, given they were reports about
> this patch, not the reason for it - but did you actually check that you
> fixed the errors that the patch produces?

Yeah, the Reported-bys that LKP sends in response to on list patches are
a menace, they just generate noise.

> This particular one seems to complain about a hunk that is still in the
> patch & the CI running on the RISC-V patchwork is complaining about it.

I'm surprised that builds cleanly anywhere...
Krzysztof Kozlowski July 5, 2023, 6:21 a.m. UTC | #3 
On 04/07/2023 11:04, William Qiu wrote:
> Add QSPI clock operation in device probe.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/


>  
> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>  	struct spi_master *master = dev_get_drvdata(dev);
>  
>  	clk_prepare_enable(cqspi->clk);
> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))

Don't add compatible checks inside the code. It does not scale. We
expect compatibles to be listed only in one place - of_device_id - and
customize driver with match data / quirks / flags.

Comment applies to all your diff hunks.

> +		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
>  	cqspi_wait_idle(cqspi);
>  	cqspi_controller_init(cqspi);
>  

Best regards,
Krzysztof
William Qiu July 5, 2023, 7:02 a.m. UTC | #4 
On 2023/7/5 0:36, Conor Dooley wrote:
> Hey William,
> 
> On Tue, Jul 04, 2023 at 05:04:52PM +0800, William Qiu wrote:
>> Add QSPI clock operation in device probe.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
> 
> These Reported-by tags don't seem correct, given they were reports about
> this patch, not the reason for it - but did you actually check that you
> fixed the errors that the patch produces?
> 
> This particular one seems to complain about a hunk that is still in the
> patch & the CI running on the RISC-V patchwork is complaining about it.
> 
> Cheers,
> Conor.
> 
I checked and found that I had only partially fixed it. I'll fix it in next
version.
Thanks for your comments.

Best regards,
William
>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>>  	struct spi_master *master = dev_get_drvdata(dev);
>>  
>>  	clk_prepare_enable(cqspi->clk);
>> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
>> +		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
>>  	cqspi_wait_idle(cqspi);
>>  	cqspi_controller_init(cqspi);
>>  
>> -- 
>> 2.34.1
>>
William Qiu July 5, 2023, 7:04 a.m. UTC | #5 
On 2023/7/5 14:21, Krzysztof Kozlowski wrote:
> On 04/07/2023 11:04, William Qiu wrote:
>> Add QSPI clock operation in device probe.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
>> Reported-by: Julia Lawall <julia.lawall@inria.fr>
>> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/
> 
> 
>>  
>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>>  	struct spi_master *master = dev_get_drvdata(dev);
>>  
>>  	clk_prepare_enable(cqspi->clk);
>> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
> 
> Don't add compatible checks inside the code. It does not scale. We
> expect compatibles to be listed only in one place - of_device_id - and
> customize driver with match data / quirks / flags.
> 
> Comment applies to all your diff hunks.
> 
I'll use "of_device_get_match_data" to replace it. But the way I added
reset before is also by compatible checks. Should I change this place to 
"of_device_get_match_data" as well?
>> +		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
>>  	cqspi_wait_idle(cqspi);
>>  	cqspi_controller_init(cqspi);
>>  
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski July 5, 2023, 7:23 a.m. UTC | #6 
On 05/07/2023 09:04, William Qiu wrote:
> 
> 
> On 2023/7/5 14:21, Krzysztof Kozlowski wrote:
>> On 04/07/2023 11:04, William Qiu wrote:
>>> Add QSPI clock operation in device probe.
>>>
>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
>>> Reported-by: Julia Lawall <julia.lawall@inria.fr>
>>> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/
>>
>>
>>>  
>>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>>>  	struct spi_master *master = dev_get_drvdata(dev);
>>>  
>>>  	clk_prepare_enable(cqspi->clk);
>>> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
>>
>> Don't add compatible checks inside the code. It does not scale. We
>> expect compatibles to be listed only in one place - of_device_id - and
>> customize driver with match data / quirks / flags.
>>
>> Comment applies to all your diff hunks.
>>
> I'll use "of_device_get_match_data" to replace it. But the way I added
> reset before is also by compatible checks. Should I change this place to 
> "of_device_get_match_data" as well?

I don't know what's there, but in general driver should be written in a
consistent style.

Best regards,
Krzysztof
William Qiu July 5, 2023, 8:31 a.m. UTC | #7 
On 2023/7/5 15:23, Krzysztof Kozlowski wrote:
> On 05/07/2023 09:04, William Qiu wrote:
>> 
>> 
>> On 2023/7/5 14:21, Krzysztof Kozlowski wrote:
>>> On 04/07/2023 11:04, William Qiu wrote:
>>>> Add QSPI clock operation in device probe.
>>>>
>>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
>>>> Reported-by: Julia Lawall <julia.lawall@inria.fr>
>>>> Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/
>>>
>>>
>>>>  
>>>> @@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
>>>>  	struct spi_master *master = dev_get_drvdata(dev);
>>>>  
>>>>  	clk_prepare_enable(cqspi->clk);
>>>> +	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
>>>
>>> Don't add compatible checks inside the code. It does not scale. We
>>> expect compatibles to be listed only in one place - of_device_id - and
>>> customize driver with match data / quirks / flags.
>>>
>>> Comment applies to all your diff hunks.
>>>
>> I'll use "of_device_get_match_data" to replace it. But the way I added
>> reset before is also by compatible checks. Should I change this place to 
>> "of_device_get_match_data" as well?
> 
> I don't know what's there, but in general driver should be written in a
> consistent style.
>It's in line 1719, inside the "cqspi_probe", but this part of the code is
already merged in the main line. Should I keep it in a consistent style?

Best regards,
William
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 6ddb2dfc0f00..8774f9aaff61 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -63,6 +63,8 @@  struct cqspi_st {
 	struct platform_device	*pdev;
 	struct spi_master	*master;
 	struct clk		*clk;
+	struct clk_bulk_data	*clks;
+	int			num_clks;
 	unsigned int		sclk;
 
 	void __iomem		*iobase;
@@ -1715,6 +1717,16 @@  static int cqspi_probe(struct platform_device *pdev)
 	}
 
 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) {
+		cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks);
+		if (cqspi->num_clks < 0) {
+			dev_err(dev, "Cannot claim clock: %u\n", cqspi->num_clks);
+			return -EINVAL;
+		}
+
+		ret = clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
+		if (ret)
+			dev_err(dev, "Cannot enable clock clks\n");
+
 		rstc_ref = devm_reset_control_get_optional_exclusive(dev, "rstc_ref");
 		if (IS_ERR(rstc_ref)) {
 			ret = PTR_ERR(rstc_ref);
@@ -1816,6 +1828,9 @@  static void cqspi_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(cqspi->clk);
 
+	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi"))
+		clk_bulk_disable_unprepare(cqspi->num_clks, cqspi->clks);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 }
@@ -1831,6 +1846,9 @@  static int cqspi_suspend(struct device *dev)
 
 	clk_disable_unprepare(cqspi->clk);
 
+	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
+		clk_bulk_disable_unprepare(cqspi->num_clks, cqspi->clks);
+
 	return ret;
 }
 
@@ -1840,6 +1858,8 @@  static int cqspi_resume(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 
 	clk_prepare_enable(cqspi->clk);
+	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
+		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
 	cqspi_wait_idle(cqspi);
 	cqspi_controller_init(cqspi);