Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Thierry Reding]

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

Attachment: signature.asc
Description: PGP signature