Re: [PATCH v3] clk: imx: imx8mp: Add pm_runtime support for power saving

[Marc Kleine-Budde]

On 21.03.2024 16:36:51, Shengjiu Wang wrote:
> Add pm_runtime support for power saving. In pm runtime suspend
> state the registers will be reseted, so add registers save
> in pm runtime suspend and restore them in pm runtime resume.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> Reviewed-by: Peng Fan <peng.fan@xxxxxxx>
> ---
> changes in v3:
> - remove REGS_NUM, use the ARRAY_SIZE
> - merge clk_imx8mp_audiomix_drvdata and clk_hw_onecell_data together.

Look way nicer, but still room for improvement.

> changes in v2:
> - move pm_runtime_enable before the clk register
> 
>  drivers/clk/imx/clk-imx8mp-audiomix.c | 157 ++++++++++++++++++++++----
>  1 file changed, 136 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
> index 55ed211a5e0b..5ae33bce8ad8 100644
> --- a/drivers/clk/imx/clk-imx8mp-audiomix.c
> +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
> @@ -7,10 +7,12 @@
>  
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
> +#include <linux/io.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <dt-bindings/clock/imx8mp-clock.h>
>  
> @@ -18,6 +20,7 @@
>  
>  #define CLKEN0			0x000
>  #define CLKEN1			0x004
> +#define EARC			0x200
>  #define SAI1_MCLK_SEL		0x300
>  #define SAI2_MCLK_SEL		0x304
>  #define SAI3_MCLK_SEL		0x308
> @@ -26,6 +29,11 @@
>  #define SAI7_MCLK_SEL		0x314
>  #define PDM_SEL			0x318
>  #define SAI_PLL_GNRL_CTL	0x400
> +#define SAI_PLL_FDIVL_CTL0	0x404
> +#define SAI_PLL_FDIVL_CTL1	0x408
> +#define SAI_PLL_SSCG_CTL	0x40C
> +#define SAI_PLL_MNIT_CTL	0x410
> +#define IPG_LP_CTRL		0x504
>  
>  #define SAIn_MCLK1_PARENT(n)						\
>  static const struct clk_parent_data					\
> @@ -182,26 +190,82 @@ static struct clk_imx8mp_audiomix_sel sels[] = {
>  	CLK_SAIn(7)
>  };
>  
> +static const u16 audiomix_regs[] = {
> +	CLKEN0,
> +	CLKEN1,
> +	EARC,
> +	SAI1_MCLK_SEL,
> +	SAI2_MCLK_SEL,
> +	SAI3_MCLK_SEL,
> +	SAI5_MCLK_SEL,
> +	SAI6_MCLK_SEL,
> +	SAI7_MCLK_SEL,
> +	PDM_SEL,
> +	SAI_PLL_GNRL_CTL,
> +	SAI_PLL_FDIVL_CTL0,
> +	SAI_PLL_FDIVL_CTL1,
> +	SAI_PLL_SSCG_CTL,
> +	SAI_PLL_MNIT_CTL,
> +	IPG_LP_CTRL,
> +};
> +
> +struct clk_imx8mp_audiomix_priv {
> +	void __iomem *base;
> +	struct clk_hw_onecell_data *clk_hw_data;
> +	u32 regs_save[ARRAY_SIZE(audiomix_regs)];

Put the "struct clk_hw_onecell_data clk_hw_data" at the end of the
struct. Then allocating should be easier and you don't need the
additional pointer.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachment: signature.asc
Description: PGP signature