On 29-01-19, 14:42, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-01-28 03:53:58)
> > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> > index e5eca8a1abe4..f06783c20688 100644
> > --- a/drivers/clk/qcom/clk-rcg.h
> > +++ b/drivers/clk/qcom/clk-rcg.h
> > @@ -140,6 +140,7 @@ extern const struct clk_ops clk_dyn_rcg_ops;
> > * @parent_map: map from software's parent index to hardware's src_sel field
> > * @freq_tbl: frequency table
> > * @clkr: regmap clock handle
> > + * @cfg_off: defines the cfg register offset from the CMD_RCGR
> > *
>
> Please remove this extra line here. Also, shouldn't it say offset from
> CMD_RCGR + CFG_REG?
Sure but I dont like to mix so will send that as a separate patch :)
and will update the comment too.
>
>
> > */
> > struct clk_rcg2 {
> > @@ -150,6 +151,7 @@ struct clk_rcg2 {
> > const struct parent_map *parent_map;
> > const struct freq_tbl *freq_tbl;
> > struct clk_regmap clkr;
> > + u8 cfg_off;
> > };
> >
> > #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr)
> > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> > index 6e3bd195d012..106848e3313f 100644
> > --- a/drivers/clk/qcom/clk-rcg2.c
> > +++ b/drivers/clk/qcom/clk-rcg2.c
> > @@ -74,7 +74,8 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw)
> > u32 cfg;
> > int i, ret;
> >
> > - ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
> > + ret = regmap_read(rcg->clkr.regmap,
> > + rcg->cmd_rcgr + rcg->cfg_off + CFG_REG, &cfg);
>
> Maybe we should define CFG_REG as CFG_REG(rcg) and then do the math
> there?
>
> #define CFG_REG(rcg) (rcg)->cfg_off + 0x4
Sure that looks better
>
> > if (ret)
> > goto err;
> >
> > @@ -263,17 +268,20 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
> > if (rcg->mnd_width && f->n) {
> > mask = BIT(rcg->mnd_width) - 1;
> > ret = regmap_update_bits(rcg->clkr.regmap,
> > - rcg->cmd_rcgr + M_REG, mask, f->m);
> > + rcg->cmd_rcgr + rcg->cfg_off + M_REG,
> > + mask, f->m);
> > if (ret)
> > return ret;
> >
> > ret = regmap_update_bits(rcg->clkr.regmap,
> > - rcg->cmd_rcgr + N_REG, mask, ~(f->n - f->m));
> > + rcg->cmd_rcgr + rcg->cfg_off + N_REG,
> > + mask, ~(f->n - f->m));
> > if (ret)
> > return ret;
> >
> > ret = regmap_update_bits(rcg->clkr.regmap,
> > - rcg->cmd_rcgr + D_REG, mask, ~f->n);
> > + rcg->cmd_rcgr + rcg->cfg_off + D_REG,
> > + mask, ~f->n);
>
> Ah the MND registers also move. Wow that's so sad. Do a similar thing
> for all these too?
>
> #define D_REG(rcg) (rcg)->cfg_off + 0x8
> etc...
>
> All just to make things fit on the same number of lines as before! We
> could also throw the rcg->cmd_rcgr part into the register named macros
> to make things even shorter. It was mostly OK when it was just adding
> the offset to the base, but now we have another offset so I think we can
> roll it all into the macro so that we can just read "regmap_read
> FOO_REG" and ignore the rest.
Correct that will make it neater, will do so
--
~Vinod
