Re: [alsa-devel] [PATCH v5 1/6] ASoC: Intel: Skylake: Add ssp clock driver

On 12/11, Sriram Periyasamy wrote:

+static int skl_clk_set_rate(struct clk_hw *hw, unsigned long rate,

• 			unsigned long parent_rate)
  

+{

• struct skl_clk *clkdev = to_skl_clk(hw);
• struct skl_clk_rate_cfg_table *rcfg;
• int clk_type;
• if (!clkdev)

• return -ENODEV;
  

These checks don't make sense. container_of() on clk_hw structures returning NULL wouldn't happen.

• if (!rate)

• return -EINVAL;
  

• if (__clk_is_enabled(hw->clk) && (clkdev->rate != rate))

Any chance you can directly read the hardware instead of going through the framework to find out if the clk is enabled? Seems circular to do it this way.

• return -EBUSY;
  

• rcfg = skl_get_rate_cfg(clkdev->pdata->ssp_clks[clkdev->id].rate_cfg,

• 					rate);
  

• if (!rcfg)

• return -EINVAL;
  

• clk_type = skl_get_clk_type(clkdev->id);
• if (clk_type < 0)

• return clk_type;
  

• skl_fill_clk_ipc(rcfg, clk_type);
• clkdev->rate = rate;
• return 0;

+}

+static unsigned long skl_clk_recalc_rate(struct clk_hw *hw,

• 		unsigned long parent_rate)
  

+{

• struct skl_clk *clkdev = to_skl_clk(hw);
• struct skl_clk_rate_cfg_table *rcfg;
• int clk_type;
• if (!clkdev)

• return 0;
  

• if (clkdev->rate)

• return clkdev->rate;
  

Why is the rate being cached? We should always be able to calculate the rate based on parent_rate that gets passed to this function?

• rcfg = skl_get_rate_cfg(clkdev->pdata->ssp_clks[clkdev->id].rate_cfg,

• 			parent_rate);
  

• if (!rcfg)

• return 0;
  

• clk_type = skl_get_clk_type(clkdev->id);
• if (clk_type < 0)

• return 0;
  

• skl_fill_clk_ipc(rcfg, clk_type);
• clkdev->rate = rcfg->rate;
• return clkdev->rate;

+}

+/* Not supported by clk driver. Implemented to satisfy clk fw */ +long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,

• 		unsigned long *parent_rate)
  

+{

• return rate;

+}