Re: [PATCH 2/5] pwm: sun20i: Add support for Allwinner H616 PWM

Uwe Kleine-König
2024-07-13T12:37:38Z

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]