Hi Jernej,
On Tue 15 Mar 22, 20:46, Jernej Škrabec wrote:
> Dne petek, 11. marec 2022 ob 15:34:53 CET je Paul Kocialkowski napisal(a):
> > Wrap the clock and reset preparation into runtime pm functions
> > for better organization of the code. Also fix the clock and
> > reset enable order to first deassert reset, as recommended in
> > Allwinner litterature.
> >
> > Make the driver depend on PM while at it since runtime pm is
> > mandatory for the driver to work.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> > ---
> > .../media/platform/sunxi/sun6i-csi/Kconfig | 2 +-
> > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 94 ++++++++++++++-----
> > 2 files changed, 70 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/Kconfig b/drivers/media/
> platform/sunxi/sun6i-csi/Kconfig
> > index 586e3fb3a80d..fd03e48f0c8a 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/Kconfig
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > config VIDEO_SUN6I_CSI
> > tristate "Allwinner V3s Camera Sensor Interface driver"
> > - depends on VIDEO_V4L2 && COMMON_CLK && HAS_DMA
> > + depends on PM && VIDEO_V4L2 && COMMON_CLK && HAS_DMA
> > depends on ARCH_SUNXI || COMPILE_TEST
> > select MEDIA_CONTROLLER
> > select VIDEO_V4L2_SUBDEV_API
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/
> media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index 4a0d08e0ac25..7f045a9551a7 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -152,40 +152,18 @@ int sun6i_csi_set_power(struct sun6i_csi_device
> *csi_dev, bool enable)
> >
> > if (!enable) {
> > regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> 0);
> > + pm_runtime_put(dev);
> >
> > - clk_disable_unprepare(csi_dev->clk_ram);
> > - clk_disable_unprepare(csi_dev->clk_mod);
> > - reset_control_assert(csi_dev->reset);
> > return 0;
> > }
> >
> > - ret = clk_prepare_enable(csi_dev->clk_mod);
> > - if (ret) {
> > - dev_err(csi_dev->dev, "Enable csi clk err %d\n", ret);
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret < 0)
> > return ret;
> > - }
> > -
> > - ret = clk_prepare_enable(csi_dev->clk_ram);
> > - if (ret) {
> > - dev_err(csi_dev->dev, "Enable clk_dram_csi clk err
> %d\n", ret);
> > - goto clk_mod_disable;
> > - }
> > -
> > - ret = reset_control_deassert(csi_dev->reset);
> > - if (ret) {
> > - dev_err(csi_dev->dev, "reset err %d\n", ret);
> > - goto clk_ram_disable;
> > - }
> >
> > regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> CSI_EN_CSI_EN);
> >
> > return 0;
> > -
> > -clk_ram_disable:
> > - clk_disable_unprepare(csi_dev->clk_ram);
> > -clk_mod_disable:
> > - clk_disable_unprepare(csi_dev->clk_mod);
> > - return ret;
> > }
> >
> > static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device
> *csi_dev,
> > @@ -800,6 +778,65 @@ static irqreturn_t sun6i_csi_isr(int irq, void
> *private)
> > return IRQ_HANDLED;
> > }
> >
> > +static int sun6i_csi_suspend(struct device *dev)
> > +{
> > + struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> > +
> > + reset_control_assert(csi_dev->reset);
> > + clk_disable_unprepare(csi_dev->clk_ram);
> > + clk_disable_unprepare(csi_dev->clk_mod);
> > + clk_disable_unprepare(csi_dev->clk_bus);
> > +
> > + return 0;
> > +}
> > +
> > +static int sun6i_csi_resume(struct device *dev)
> > +{
> > + struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = reset_control_deassert(csi_dev->reset);
> > + if (ret) {
> > + dev_err(dev, "failed to deassert reset\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(csi_dev->clk_bus);
> > + if (ret) {
> > + dev_err(dev, "failed to enable bus clock\n");
> > + goto error_reset;
> > + }
> > +
> > + ret = clk_prepare_enable(csi_dev->clk_mod);
> > + if (ret) {
> > + dev_err(dev, "failed to enable module clock\n");
> > + goto error_clk_bus;
> > + }
> > +
> > + ret = clk_prepare_enable(csi_dev->clk_ram);
> > + if (ret) {
> > + dev_err(dev, "failed to enable ram clock\n");
> > + goto error_clk_mod;
> > + }
> > +
> > + return 0;
> > +
> > +error_clk_mod:
> > + clk_disable_unprepare(csi_dev->clk_mod);
> > +
> > +error_clk_bus:
> > + clk_disable_unprepare(csi_dev->clk_bus);
> > +
> > +error_reset:
> > + reset_control_assert(csi_dev->reset);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct dev_pm_ops sun6i_csi_pm_ops = {
> > + SET_RUNTIME_PM_OPS(sun6i_csi_suspend, sun6i_csi_resume, NULL)
>
> Since driver depends on PM, using SET_RUNTIME_PM_OPS macro doesn't make sense.
Can you ellaborate why? It's really not obvious why the macro should only be
used when PM is optional. I see that it facilitates that, but I really don't
see any issue using it with PM required too. Is it a common practice?
> > +};
> > +
> > static const struct regmap_config sun6i_csi_regmap_config = {
> > .reg_bits = 32,
> > .reg_stride = 4,
> > @@ -886,6 +923,11 @@ static int sun6i_csi_resources_setup(struct
> sun6i_csi_device *csi_dev,
> > goto error_clk_rate_exclusive;
> > }
> >
> > + /* Runtime PM */
> > +
> > + pm_runtime_enable(dev);
> > + pm_runtime_set_suspended(dev);
>
> pm_runtime_set_suspended() description says: "It is not valid to call this
> function for devices with runtime PM enabled."
Okay I need to revisit this, I think I misunderstood a bunch of things here.
Thanks,
Paul
> Best regards,
> Jernej
>
> > +
> > return 0;
> >
> > error_clk_rate_exclusive:
> > @@ -896,6 +938,7 @@ static int sun6i_csi_resources_setup(struct
> sun6i_csi_device *csi_dev,
> >
> > static void sun6i_csi_resources_cleanup(struct sun6i_csi_device *csi_dev)
> > {
> > + pm_runtime_disable(csi_dev->dev);
> > clk_rate_exclusive_put(csi_dev->clk_mod);
> > }
> >
> > @@ -978,6 +1021,7 @@ static struct platform_driver sun6i_csi_platform_driver
> = {
> > .driver = {
> > .name = SUN6I_CSI_NAME,
> > .of_match_table =
> of_match_ptr(sun6i_csi_of_match),
> > + .pm = &sun6i_csi_pm_ops,
> > },
> > };
> >
> > --
> > 2.35.1
> >
> >
>
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
