Message-ID: <20150925091524.GB19573@kwain>
Date:	Fri, 25 Sep 2015 11:15:24 +0200
From:	Antoine Tenart <antoine.tenart@...e-electrons.com>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Antoine Tenart <antoine.tenart@...e-electrons.com>,
	sebastian.hesselbarth@...il.com, zmxu@...vell.com,
	jszhang@...vell.com, linux-arm-kernel@...ts.infradead.org,
	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/5] pwm: add the Berlin pwm controller driver

On Mon, Sep 21, 2015 at 10:40:08AM +0200, Thierry Reding wrote:
> On Thu, Sep 17, 2015 at 12:13:04PM +0200, Antoine Tenart wrote:
> > +
> > +#define BERLIN_PWM_EN			0x0
> > +#define BERLIN_PWM_CONTROL		0x4
> > +#define BERLIN_PWM_DUTY			0x8
> > +#define BERLIN_PWM_TCNT			0xc
> > +
> > +#define BERLIN_PWM_ENABLE		BIT(0)
> > +#define BERLIN_PWM_INVERT_POLARITY	BIT(3)
> > +#define BERLIN_PWM_PRESCALE_MASK	0x7
> > +#define BERLIN_PWM_PRESCALE_MAX		4096
> > +#define BERLIN_PWM_MAX_TCNT		65535
> 
> It'd be nice to see some sort of connection between the register
> definitions and which fields belong to them.

Something like:

#define BERLIN_PWM_EN		0x0
#define  BERLIN_PWM_ENABLE	BIT(0)
#define BERLIN_PWM_CONTROL	0x4
...

> > +struct berlin_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	spinlock_t lock;
> 
> I don't think that lock is necessary here. You have per-channel
> registers and each channel can only be used by one consumer at a time
> anyway.

Sure. I'll make some tests and remove the lock if possible.

> > +#define to_berlin_pwm_chip(chip)	\
> > +	container_of((chip), struct berlin_pwm_chip, chip)
> > +
> > +#define berlin_pwm_readl(chip, channel, offset)		\
> > +	readl_relaxed((chip)->base + (channel) * 0x10 + offset)
> > +#define berlin_pwm_writel(val, chip, channel, offset)	\
> > +	writel_relaxed(val, (chip)->base + (channel) * 0x10 + offset)
> 
> These should be static inline functions. Also I think for
> berlin_pwm_writel() val should come after chip and channel to preserve a
> more natural ordering of parameters.

What's the benefit of using static inline functions here?

I'm not convinced having val after chip and channel is more natural, but
this is not a big matter. I'll update.

> > +
> > +/* prescaler table: {1, 4, 8, 16, 64, 256, 1024, 4096} */
> > +static const u32 prescaler_diff_table[] = {
> > +	1, 4, 2, 2, 4, 4, 4, 4,
> > +};
> 
> I don't see any relationship between these values and the prescaler
> table given in the comment. Please expand the comment to explain the
> connection.
> 
> After reading the remainder of the code, I see that the values in this
> table are the multiplication factors for each of the prescalers. It
> shouldn't be necessary to read the code to find out, so please clarify
> in the comment (and perhaps rename the table to something more related
> to its purpose, such as prescale_factors).

Will do.

> Perhaps an even more easily digestible alternative would be to make this
> a list of prescaler values and then use the values directly to compute
> the number of cycles rather than iteratively dividing and needing this
> unintuitive mapping.

Would something like the following be better?

"""
static const prescaler_table = {1, 4, 8, 16, 64, 256, 1024, 4096};
unsigned int prescale;
u64 tmp;

for (prescale = 0; prescale < ARRAY_SIZE(prescaler_table); prescale++) {
	tmp = cycles;
	do_div(tmp, prescaler_table[prescale]);

	if (tmp <= BERLIN_PWM_MAX_TCNT)
		break;
}

if (tmp > BERLIN_PWM_MAX_TCNT)
	return -ERANGE;

cycles = tmp;
"""

I personally prefer the prescale factors implementation, but I admit
this is maybe more readable.

> > +
> > +	while (cycles > BERLIN_PWM_MAX_TCNT)
> > +		do_div(cycles, prescaler_diff_table[++prescale]);
> 
> Don't you need to make sure that prescale doesn't exceed the table size?

Sure.

> > +
> > +	ret = pwmchip_add(&pwm->chip);
> > +	if (ret < 0) {
> > +		clk_disable_unprepare(pwm->clk);
> 
> Why not enable the clock until after successful registration? It doesn't
> seem like you need access before that. Doing so would introduce a subtle
> race condition between adding the chip (and hence exposing it via sysfs)
> and enabling the clock, so perhaps an even better approach would be to
> add more fine-grained clock management by enabling or disabling it only
> when necessary (clock enables are reference counted, so ->request() and
> ->free() would probably work fine in this case).
> 
> This isn't a real objection, though. If you prefer to keep things simple
> the current code is fine with me.

That was the idea. We may update this latter.

> > +static int berlin_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct berlin_pwm_chip *pwm = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&pwm->chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_disable_unprepare(pwm->clk);
> 
> You might want to disable the clock regardless. The driver will be
> unloaded regardless of whether pwmchip_remove() returns failure or
> not. The above would leak a clock enable, which may not be what you
> want.

Yes, I'll call clk_disable_unprepare() regardless of what pwmchip_remove()
returns.

Thanks for the review!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists