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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/