| Message ID | 20220205185429.2278860-22-paul.kocialkowski@bootlin.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support | expand |
On Sat, Feb 05, 2022 at 07:53:44PM +0100, Paul Kocialkowski wrote: > In some situations the default rate of the module clock is not the > required one for operation (for example when reconfiguring the clock > tree to use a different parent). As a result, always set the correct > rate for the clock (and take care of cleanup). > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++----- > 1 file changed, 41 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 8155e9560164..2355088fdc37 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -154,9 +154,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0); > > clk_disable_unprepare(csi_dev->clk_ram); > - if (of_device_is_compatible(dev->of_node, > - "allwinner,sun50i-a64-csi")) > - clk_rate_exclusive_put(csi_dev->clk_mod); > clk_disable_unprepare(csi_dev->clk_mod); > reset_control_assert(csi_dev->reset); > return 0; > @@ -168,9 +165,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > return ret; > } > > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > - clk_set_rate_exclusive(csi_dev->clk_mod, 300000000); > - > ret = clk_prepare_enable(csi_dev->clk_ram); > if (ret) { > dev_err(csi_dev->dev, "Enable clk_dram_csi clk err %d\n", ret); > @@ -190,8 +184,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > clk_ram_disable: > clk_disable_unprepare(csi_dev->clk_ram); > clk_mod_disable: > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > - clk_rate_exclusive_put(csi_dev->clk_mod); > clk_disable_unprepare(csi_dev->clk_mod); > return ret; > } > @@ -819,6 +811,7 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > struct platform_device *platform_dev) > { > struct device *dev = csi_dev->dev; > + unsigned long clk_mod_rate; > void __iomem *io_base; > int ret; > int irq; > @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > return PTR_ERR(csi_dev->clk_ram); > } > > + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > + clk_mod_rate = 300000000; > + else > + clk_mod_rate = 297000000; > + > + ret = clk_set_rate_exclusive(csi_dev->clk_mod, clk_mod_rate); > + if (ret) { > + dev_err(dev, "failed to set mod clock rate\n"); > + return ret; > + } > + > /* Reset */ > > csi_dev->reset = devm_reset_control_get_shared(dev, NULL); > if (IS_ERR(csi_dev->reset)) { > dev_err(dev, "failed to acquire reset\n"); > - return PTR_ERR(csi_dev->reset); > + ret = PTR_ERR(csi_dev->reset); > + goto error_clk_rate_exclusive; > } > > /* Interrupt */ > > irq = platform_get_irq(platform_dev, 0); > - if (irq < 0) > - return -ENXIO; > + if (irq < 0) { > + dev_err(dev, "failed to get interrupt\n"); > + ret = -ENXIO; > + goto error_clk_rate_exclusive; > + } > > ret = devm_request_irq(dev, irq, sun6i_csi_isr, 0, SUN6I_CSI_NAME, > csi_dev); > if (ret) { > dev_err(dev, "failed to request interrupt\n"); > - return ret; > + goto error_clk_rate_exclusive; > } > > return 0; > + > +error_clk_rate_exclusive: > + clk_rate_exclusive_put(csi_dev->clk_mod); > + > + return ret; > +} > + > +static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev) > +{ > + clk_rate_exclusive_put(csi_dev->clk_mod); > } If you're going to have that function anyway, let's use devm_add_action_or_reset, it'll simplify the rest of the patch. Maxime
Hi, Thanks for the review, On Mon 07 Feb 22, 10:14, Maxime Ripard wrote: > On Sat, Feb 05, 2022 at 07:53:44PM +0100, Paul Kocialkowski wrote: > > In some situations the default rate of the module clock is not the > > required one for operation (for example when reconfiguring the clock > > tree to use a different parent). As a result, always set the correct > > rate for the clock (and take care of cleanup). > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++----- > > 1 file changed, 41 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > index 8155e9560164..2355088fdc37 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > @@ -154,9 +154,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > > regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0); > > > > clk_disable_unprepare(csi_dev->clk_ram); > > - if (of_device_is_compatible(dev->of_node, > > - "allwinner,sun50i-a64-csi")) > > - clk_rate_exclusive_put(csi_dev->clk_mod); > > clk_disable_unprepare(csi_dev->clk_mod); > > reset_control_assert(csi_dev->reset); > > return 0; > > @@ -168,9 +165,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > > return ret; > > } > > > > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > > - clk_set_rate_exclusive(csi_dev->clk_mod, 300000000); > > - > > ret = clk_prepare_enable(csi_dev->clk_ram); > > if (ret) { > > dev_err(csi_dev->dev, "Enable clk_dram_csi clk err %d\n", ret); > > @@ -190,8 +184,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) > > clk_ram_disable: > > clk_disable_unprepare(csi_dev->clk_ram); > > clk_mod_disable: > > - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > > - clk_rate_exclusive_put(csi_dev->clk_mod); > > clk_disable_unprepare(csi_dev->clk_mod); > > return ret; > > } > > @@ -819,6 +811,7 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > > struct platform_device *platform_dev) > > { > > struct device *dev = csi_dev->dev; > > + unsigned long clk_mod_rate; > > void __iomem *io_base; > > int ret; > > int irq; > > @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > > return PTR_ERR(csi_dev->clk_ram); > > } > > > > + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > > + clk_mod_rate = 300000000; > > + else > > + clk_mod_rate = 297000000; > > + > > + ret = clk_set_rate_exclusive(csi_dev->clk_mod, clk_mod_rate); > > + if (ret) { > > + dev_err(dev, "failed to set mod clock rate\n"); > > + return ret; > > + } > > + > > /* Reset */ > > > > csi_dev->reset = devm_reset_control_get_shared(dev, NULL); > > if (IS_ERR(csi_dev->reset)) { > > dev_err(dev, "failed to acquire reset\n"); > > - return PTR_ERR(csi_dev->reset); > > + ret = PTR_ERR(csi_dev->reset); > > + goto error_clk_rate_exclusive; > > } > > > > /* Interrupt */ > > > > irq = platform_get_irq(platform_dev, 0); > > - if (irq < 0) > > - return -ENXIO; > > + if (irq < 0) { > > + dev_err(dev, "failed to get interrupt\n"); > > + ret = -ENXIO; > > + goto error_clk_rate_exclusive; > > + } > > > > ret = devm_request_irq(dev, irq, sun6i_csi_isr, 0, SUN6I_CSI_NAME, > > csi_dev); > > if (ret) { > > dev_err(dev, "failed to request interrupt\n"); > > - return ret; > > + goto error_clk_rate_exclusive; > > } > > > > return 0; > > + > > +error_clk_rate_exclusive: > > + clk_rate_exclusive_put(csi_dev->clk_mod); > > + > > + return ret; > > +} > > + > > +static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev) > > +{ > > + clk_rate_exclusive_put(csi_dev->clk_mod); > > } > > If you're going to have that function anyway, let's use > devm_add_action_or_reset, it'll simplify the rest of the patch. Well, this will cause issues later on when adding runtime pm support to sun6i_csi_resources_cleanup: then it will no longer be equivalent to the error case label. Also I feel like making the resources_cleanup call a devm action would not help clarify the general flow of the driver, where the matching setup/cleanup calls are ordered in probe/remove. The driver is quite big with various parts and I'd rather have them behave in a coherent and similar way. What do you think? Paul
Hi Paul, Thanks for the patchbomb. On Sat, Feb 05, 2022 at 07:53:44PM +0100, Paul Kocialkowski wrote: > In some situations the default rate of the module clock is not the > required one for operation (for example when reconfiguring the clock > tree to use a different parent). As a result, always set the correct > rate for the clock (and take care of cleanup). > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++----- > 1 file changed, 41 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 8155e9560164..2355088fdc37 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > return PTR_ERR(csi_dev->clk_ram); > } > > + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > + clk_mod_rate = 300000000; > + else > + clk_mod_rate = 297000000; This would be nice to put in OF match data. Of course the driver did this already before the patch. The approach still scales badly.
Hi Sakari, On Mon 14 Feb 22, 18:31, Sakari Ailus wrote: > Hi Paul, > > Thanks for the patchbomb. I'll split it in the next revision. > On Sat, Feb 05, 2022 at 07:53:44PM +0100, Paul Kocialkowski wrote: > > In some situations the default rate of the module clock is not the > > required one for operation (for example when reconfiguring the clock > > tree to use a different parent). As a result, always set the correct > > rate for the clock (and take care of cleanup). > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++----- > > 1 file changed, 41 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > index 8155e9560164..2355088fdc37 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, > > return PTR_ERR(csi_dev->clk_ram); > > } > > > > + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) > > + clk_mod_rate = 300000000; > > + else > > + clk_mod_rate = 297000000; > > This would be nice to put in OF match data. > > Of course the driver did this already before the patch. The approach still > scales badly. Agreed, that could be another follow-up patch in the sun6i-csi rework series. Paul
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index 8155e9560164..2355088fdc37 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c @@ -154,9 +154,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0); clk_disable_unprepare(csi_dev->clk_ram); - if (of_device_is_compatible(dev->of_node, - "allwinner,sun50i-a64-csi")) - clk_rate_exclusive_put(csi_dev->clk_mod); clk_disable_unprepare(csi_dev->clk_mod); reset_control_assert(csi_dev->reset); return 0; @@ -168,9 +165,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) return ret; } - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) - clk_set_rate_exclusive(csi_dev->clk_mod, 300000000); - ret = clk_prepare_enable(csi_dev->clk_ram); if (ret) { dev_err(csi_dev->dev, "Enable clk_dram_csi clk err %d\n", ret); @@ -190,8 +184,6 @@ int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) clk_ram_disable: clk_disable_unprepare(csi_dev->clk_ram); clk_mod_disable: - if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) - clk_rate_exclusive_put(csi_dev->clk_mod); clk_disable_unprepare(csi_dev->clk_mod); return ret; } @@ -819,6 +811,7 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, struct platform_device *platform_dev) { struct device *dev = csi_dev->dev; + unsigned long clk_mod_rate; void __iomem *io_base; int ret; int irq; @@ -856,28 +849,53 @@ static int sun6i_csi_resources_setup(struct sun6i_csi_device *csi_dev, return PTR_ERR(csi_dev->clk_ram); } + if (of_device_is_compatible(dev->of_node, "allwinner,sun50i-a64-csi")) + clk_mod_rate = 300000000; + else + clk_mod_rate = 297000000; + + ret = clk_set_rate_exclusive(csi_dev->clk_mod, clk_mod_rate); + if (ret) { + dev_err(dev, "failed to set mod clock rate\n"); + return ret; + } + /* Reset */ csi_dev->reset = devm_reset_control_get_shared(dev, NULL); if (IS_ERR(csi_dev->reset)) { dev_err(dev, "failed to acquire reset\n"); - return PTR_ERR(csi_dev->reset); + ret = PTR_ERR(csi_dev->reset); + goto error_clk_rate_exclusive; } /* Interrupt */ irq = platform_get_irq(platform_dev, 0); - if (irq < 0) - return -ENXIO; + if (irq < 0) { + dev_err(dev, "failed to get interrupt\n"); + ret = -ENXIO; + goto error_clk_rate_exclusive; + } ret = devm_request_irq(dev, irq, sun6i_csi_isr, 0, SUN6I_CSI_NAME, csi_dev); if (ret) { dev_err(dev, "failed to request interrupt\n"); - return ret; + goto error_clk_rate_exclusive; } return 0; + +error_clk_rate_exclusive: + clk_rate_exclusive_put(csi_dev->clk_mod); + + return ret; +} + +static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev) +{ + clk_rate_exclusive_put(csi_dev->clk_mod); } static int sun6i_csi_probe(struct platform_device *platform_dev) @@ -897,7 +915,16 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) if (ret) return ret; - return sun6i_csi_v4l2_init(csi_dev); + ret = sun6i_csi_v4l2_init(csi_dev); + if (ret) + goto error_resources; + + return 0; + +error_resources: + sun6i_csi_resources_cleanup(csi_dev); + + return ret; } static int sun6i_csi_remove(struct platform_device *pdev) @@ -905,6 +932,7 @@ static int sun6i_csi_remove(struct platform_device *pdev) struct sun6i_csi_device *csi_dev = platform_get_drvdata(pdev); sun6i_csi_v4l2_cleanup(csi_dev); + sun6i_csi_resources_cleanup(csi_dev); return 0; }
In some situations the default rate of the module clock is not the required one for operation (for example when reconfiguring the clock tree to use a different parent). As a result, always set the correct rate for the clock (and take care of cleanup). Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 ++++++++++++++----- 1 file changed, 41 insertions(+), 13 deletions(-)