[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:

quoted

+
+#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
...

quoted

+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.

quoted

+#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.

quoted

+
+/* 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.

quoted

+
+	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.

quoted

+
+	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.

quoted

+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