Hello, On Fri, May 31, 2024 at 11:11:34PM +0900, Hironori KIKUCHI wrote: > @@ -20,8 +20,17 @@ > #include <linux/pwm.h> > #include <linux/reset.h> > > +#define SUN20I_PWM_REG_OFFSET_PER_D1 (0x0080) > +#define SUN20I_PWM_REG_OFFSET_PCR_D1 (0x0100 + 0x0000) > +#define SUN20I_PWM_REG_OFFSET_PPR_D1 (0x0100 + 0x0004) > +#define SUN20I_PWM_REG_OFFSET_PER_H616 (0x0040) > +#define SUN20I_PWM_REG_OFFSET_PCR_H616 (0x0060 + 0x0000) > +#define SUN20I_PWM_REG_OFFSET_PPR_H616 (0x0060 + 0x0004) Instead of having a conditional for each register, it would be easier to do: #define SUN20I_PWM_CHANBASE_D1 0x80 #define SUN20I_PWM_CHANBASE_H616 0x40 (maybe with a more suitable name) and then do: #define SUN20I_PWM_PER(sun20i_chip) ((sun20i_chip)->chanbase + 0) #define SUN20I_PWM_PCR(sun20i_chip) ((sun20i_chip)->chanbase + 0x20) #define SUN20I_PWM_PPR(sun20i_chip) ((sun20i_chip)->chanbase + 0x24) I would expect these definitions to appear in the order of register addresses, that is below SUN20I_PWM_CLK_CFG. This reduces the size of the private data struct, is easier to understnad to a human (I think) and I claim this results in more compact code (without having it verified). Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
