Re: [PATCH v4 08/21] PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines

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

On Tue, May 16, 2023 at 04:54:55PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:43 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > The DDRTYPE defines are named to be RK3399 specific, but they can be
> > used for other Rockchip SoCs as well, so replace the RK3399_PMUGRF_
> > prefix with ROCKCHIP_. They are defined in a SoC specific header
> > file, so when generalizing the prefix also move the new defines to
> > a SoC agnostic header file. While at it use GENMASK to define the
> > DDRTYPE bitfield and give it a name including the full register name.
> 
> Great - you covered this one a few patches later.
> 
> A few suggestions inline but this looks basically fine to me.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 10 ++++++----
> >  drivers/devfreq/rk3399_dmc.c         | 10 +++++-----
> >  include/soc/rockchip/rk3399_grf.h    |  7 +------
> >  include/soc/rockchip/rockchip_grf.h  | 15 +++++++++++++++
> >  4 files changed, 27 insertions(+), 15 deletions(-)
> >  create mode 100644 include/soc/rockchip/rockchip_grf.h
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 18d578730fd0c..7896cd8beb143 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -18,7 +18,10 @@
> >  #include <linux/list.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> 
> Why bits.h?

For GENMASK.

It's included indirectly anyway, but being explicit shouldn't hurt.

> 
> >  
> > +#include <soc/rockchip/rockchip_grf.h>
> >  #include <soc/rockchip/rk3399_grf.h>
> >  
> >  #define DMC_MAX_CHANNELS	2
> > @@ -74,9 +77,9 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> >  	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
> >  
> >  	/* set ddr type to dfi */
> > -	if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> > +	if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
> >  		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> > -	else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> > +	else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
> >  		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
> 
> Maybe a switch statement here as well?  In particular I'm interested
> that there is no sign of DDR3 or LPDDR2 here and I think it would be good to
> make that explicit given it's defined.

That's done later in this series, but you already noticed that.

> > diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> > new file mode 100644
> > index 0000000000000..dc77bb762a05a
> > --- /dev/null
> > +++ b/include/soc/rockchip/rockchip_grf.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Rockchip General Register Files definitions
> > + */
> > +
> > +#ifndef __SOC_ROCKCHIP_GRF_H
> > +#define __SOC_ROCKCHIP_GRF_H
> > +
> > +/* Rockchip DDRTYPE defines */
> > +#define ROCKCHIP_DDRTYPE_DDR3	3
> > +#define ROCKCHIP_DDRTYPE_LPDDR2	5
> > +#define ROCKCHIP_DDRTYPE_LPDDR3	6
> > +#define ROCKCHIP_DDRTYPE_LPDDR4	7
> 
> Maybe worth an enum so you can give ddr_type a named type and
> the compiler can see if you've handled all the cases for
> the switch statements?

Ok, will do.

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel