[U-Boot] [PATCH] pwm: add MACRO to limit some code which only for rk3288

[Kever Yang]

Hi Simon,

On 07/09/2016 10:39 PM, Simon Glass wrote:
> Hi Kever,
>
> On 7 July 2016 at 20:45, Kever Yang <kever.yang at rock-chips.com> wrote:
>> The grf setting for rkpwm is only need in rk3288, other SoCs like
>> RK3399 which also use rkpwm do not need set the grf, let's add a
>> MACRO to make the code only for RK3288.
>>
>> Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
> patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
>
>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>> ---
>>   drivers/pwm/rk_pwm.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c
>> index 2d289a4..e34adf0 100644
>> --- a/drivers/pwm/rk_pwm.c
>> +++ b/drivers/pwm/rk_pwm.c
>> @@ -13,8 +13,10 @@
>>   #include <syscon.h>
>>   #include <asm/io.h>
>>   #include <asm/arch/clock.h>
>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>   #include <asm/arch/cru_rk3288.h>
>>   #include <asm/arch/grf_rk3288.h>
>> +#endif
>>   #include <asm/arch/pwm.h>
>>   #include <power/regulator.h>
>>
>> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>>   struct rk_pwm_priv {
>>          struct rk3288_pwm *regs;
>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>          struct rk3288_grf *grf;
>> +#endif
>>   };
>>
>>   static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
>> @@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)
>>          struct regmap *map;
>>
>>          priv->regs = (struct rk3288_pwm *)dev_get_addr(dev);
>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>          map = syscon_get_regmap_by_driver_data(ROCKCHIP_SYSCON_GRF);
>>          if (IS_ERR(map))
>>                  return PTR_ERR(map);
>>          priv->grf = regmap_get_range(map, 0);
>> +#endif
> I'd like to find a better way. Do all chips have a grf? If so then
> perhaps we can have a function like grf_enable_pwm() in the core SoC
> code and call it here. The #ifdef can be there.
>
> Or perhaps we should have a general soc.c, with its own struct
> containing pointers to grf, sgrf, etc. It can be set up at the start,
> and then provide a soc_enable_pwm() function to enable the pwm.
>
> What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like
grf. The content in grf are very different for different SoC, it gathers
all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in
RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is
better to stay in driver file like rk_pwm.c.

For these system control registers, I'm sure the dedicate general soc.c is
needed, because they are not so common for different module and different
Soc. We are not able to have a common framework for them, a general soc.c
won't help much except all the system control are gather in one file .

I think the GRF setting is part of the module and driver, so we can 
leave it as it is,
and a simple syscon driver is enough for grf like what is kernel do.

Thanks,
- Kever
>
>>          return 0;
>>   }
>>
>>   static int rk_pwm_probe(struct udevice *dev)
>>   {
>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>          struct rk_pwm_priv *priv = dev_get_priv(dev);
>>
>>          rk_setreg(&priv->grf->soc_con2, 1 << 0);
>> +#endif
>>
>>          return 0;
>>   }
>> --
>> 1.9.1
>>
>>
> Regards,
> Simon
>
>
>