Add QSPI clock operation in device probe.
Signed-off-by: William Qiu <[email protected]>
Reviewed-by: Hal Feng <[email protected]>
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Reported-by: Julia Lawall <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
---
drivers/spi/spi-cadence-quadspi.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
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);
--
2.34.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 <[email protected]>
> Reviewed-by: Hal Feng <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
>
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 <[email protected]>
> > Reviewed-by: Hal Feng <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> 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...
On 04/07/2023 11:04, William Qiu wrote:
> Add QSPI clock operation in device probe.
>
> Signed-off-by: William Qiu <[email protected]>
> Reviewed-by: Hal Feng <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Reported-by: Julia Lawall <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]/
>
> @@ -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
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 <[email protected]>
>> Reviewed-by: Hal Feng <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> 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
>>
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 <[email protected]>
>> Reviewed-by: Hal Feng <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>> Reported-by: Julia Lawall <[email protected]>
>> Closes: https://lore.kernel.org/r/[email protected]/
>
>
>>
>> @@ -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
>
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 <[email protected]>
>>> Reviewed-by: Hal Feng <[email protected]>
>>> Reported-by: kernel test robot <[email protected]>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>> Reported-by: Julia Lawall <[email protected]>
>>> Closes: https://lore.kernel.org/r/[email protected]/
>>
>>
>>>
>>> @@ -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
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 <[email protected]>
>>>> Reviewed-by: Hal Feng <[email protected]>
>>>> Reported-by: kernel test robot <[email protected]>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>>> Reported-by: Julia Lawall <[email protected]>
>>>> Closes: https://lore.kernel.org/r/[email protected]/
>>>
>>>
>>>>
>>>> @@ -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
>