On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote: > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote: [...] > > +#define PWM_GET_CLK_OFFSET(chan) (0x20 + ((chan >> 1) * 0x4)) > > +#define PWM_CLK_APB_SCR BIT(7) > > +#define PWM_DIV_M 0 > > +#define PWM_DIV_M_WIDTH 0x4 > > + > > +#define PWM_CLK_REG 0x40 > > +#define PWM_CLK_GATING BIT(0) > > + > > +#define PWM_ENABLE_REG 0x80 > > +#define PWM_EN BIT(0) > > + > > +#define PWM_CTL_REG(chan) (0x100 + 0x20 * chan) > > +#define PWM_ACT_STA BIT(8) > > +#define PWM_PRESCAL_K 0 > > +#define PWM_PRESCAL_K_WIDTH 0x8 > > + > > +#define PWM_PERIOD_REG(chan) (0x104 + 0x20 * chan) > > +#define PWM_ENTIRE_CYCLE 16 > > +#define PWM_ENTIRE_CYCLE_WIDTH 0x10 > > +#define PWM_ACT_CYCLE 0 > > +#define PWM_ACT_CYCLE_WIDTH 0x10 > > Please use a driver specific prefix for these defines.
These are nice and to the point, so I think it'd be fine to leave these
as-is. Unless of course if they conflict with something from the PWM
core, which I don't think any of these do.
> > +struct sun50i_pwm_data {
> > + unsigned int npwm;
> > +};
> > +
> > +struct sun50i_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + struct reset_control *rst_clk;
> > + void __iomem *base;
> > + const struct sun50i_pwm_data *data;
> > +};
> > +
> > +static inline struct sun50i_pwm_chip *sun50i_pwm_from_chip(struct pwm_chip
> > *chip)
> > +{
> > + return container_of(chip, struct sun50i_pwm_chip, chip);
> > +}
I wanted to reply to Uwe's comment on v1 but you sent this before I got
around to it, so I'll mention it here. I recognize the usefulness of
having a common prefix for function names, though admittedly it's not
totally necessary in these drivers because these are all local symbols.
But it does makes things a bit consistent and helps when looking at
backtraces and such, so that's all good.
That said, I consider these casting helpers a bit of a special case
because they usually won't show up in backtraces, or anywhere else for
that matter. Traditionally these have been named to_*(), so it'd be
entirely consistent to keep doing that.
But I don't have any strong objections to this variant either, so pick
whichever you want. Although, please, nobody take that as a hint that
any existing helpers should be converted.
[...]
> > +static int sun50i_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct sun50i_pwm_chip *pwm;
>
> "pwm" isn't a good name, in PWM code this name is usually used for
> struct pwm_device pointers (and sometimes the global pwm id). I usually
> use "ddata" for driver data (and would have called "sun50i_pwm_chip"
> "sun50i_pwm_ddata" instead). Another common name is "priv".
This driver already declares sun50i_pwm_data for per-SoC data, so having
a struct sun50i_pwm_ddata would just be confusing. sun50i_pwm_chip is
consistent with what other drivers name this, so I think that's fine.
Agreed on "pwm" being a bad choice here, though.
Thierry
signature.asc
Description: PGP signature
