Re: [PATCH v4 04/21] PM / devfreq: rockchip-dfi: Add SoC specific init function

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, May 16, 2023 at 04:40:17PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:39 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Move the RK3399 specifics to a SoC specific init function to make
> > the way free for supporting other SoCs later.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Hi Sascha,
> 
> A few things inline,
> 
> Jonathan
> 
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 59 +++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 47cc9e48dafab..f317d3d063e9c 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/list.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  
> >  #include <soc/rockchip/rk3399_grf.h>
> >  
> > @@ -55,27 +56,21 @@ struct rockchip_dfi {
> >  	void __iomem *regs;
> >  	struct regmap *regmap_pmu;
> >  	struct clk *clk;
> > +	u32 ddr_type;
> >  };
> >  
> >  static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> >  {
> >  	struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> >  	void __iomem *dfi_regs = dfi->regs;
> > -	u32 val;
> > -	u32 ddr_type;
> > -
> > -	/* get ddr type */
> > -	regmap_read(dfi->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> > -	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> > -		    RK3399_PMUGRF_DDRTYPE_MASK;
> >  
> >  	/* clear DDRMON_CTRL setting */
> >  	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
> >  
> >  	/* set ddr type to dfi */
> > -	if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> > +	if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> >  		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> > -	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> > +	else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> >  		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
> >  
> >  	/* enable count, use software mode */
> > @@ -167,8 +162,34 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
> >  	.set_event = rockchip_dfi_set_event,
> >  };
> >  
> > +static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> > +{
> > +	struct regmap *regmap_pmu = dfi->regmap_pmu;
> > +	u32 val;
> > +
> > +	dfi->clk = devm_clk_get(dfi->dev, "pclk_ddr_mon");
> > +	if (IS_ERR(dfi->clk))
> > +		return dev_err_probe(dfi->dev, PTR_ERR(dfi->clk),
> > +				     "Cannot get the clk pclk_ddr_mon\n");
> > +
> > +	/* get ddr type */
> > +	regmap_read(regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> > +	dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> > +			RK3399_PMUGRF_DDRTYPE_MASK;
> 
> I don't suppose you fancy converting whole driver to
> FIELD_GET / FIELD_PREP and masks that include the shift? :)
> Guess probably not or maybe you do it later in this set.

That's done later for the cases that make sense, but you already noticed
that ;)

> 
> > +
> > +	return 0;
> > +};
> > +
> > +struct rockchip_dfi_devtype_data {
> > +	int (*init)(struct rockchip_dfi *dfi);
> > +};
> > +
> > +static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
> > +	.init = rk3399_dfi_init,
> 
> I guess the 'why' will become apparent later in the set, but at this
> stage a callback to get the ddr type would have made more sense and
> kept the data local to where it was needed.

struct rockchip_dfi_devtype_data will still have a single member by the
end of this series. The reason I still prefer a struct type is that I've
seen enough drivers that passed a single ad-hoc value in devtype data
that I had to convert to pass a struct type because I had to add another
value.

Well, I just realized that I have configured several values that I could
have put in that struct in the SoC specific init function instead, so
there's no real point in using a struct type.

> 
> > +};
> > +
> >  static const struct of_device_id rockchip_dfi_id_match[] = {
> > -	{ .compatible = "rockchip,rk3399-dfi" },
> > +	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> > @@ -179,6 +200,15 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> >  	struct rockchip_dfi *dfi;
> >  	struct devfreq_event_desc *desc;
> >  	struct device_node *np = pdev->dev.of_node, *node;
> > +	const struct of_device_id *of_id;
> > +	const struct rockchip_dfi_devtype_data *devtype;
> > +	int ret;
> > +
> > +	of_id = of_match_device(rockchip_dfi_id_match, &pdev->dev);
> > +	if (!of_id)
> > +		return -ENODEV;
> > +
> > +	devtype = of_id->data;
> 
> device_get_match_data()
> 
> or I guess if you really want stick to the of specific form.
> 
> of_device_get_match_data()

Yes, will change.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip