lkml.org 
[lkml]   [2018]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 4/4] ARM: PWM: add allwinner sun8i pwm support.
Hi,

Thanks for respinning this serie. It looks mostly good, but you still
have a quite significant number of checkpatch (--strict) warnings that
you should address.

On Sun, Feb 25, 2018 at 09:53:08PM +0800, hao_zhang wrote:
> +#define CAPTURE_IRQ_ENABLE_REG 0x0010
> +#define CFIE(ch) BIT(ch << 1 + 1)
> +#define CRIE(ch) BIT(ch << 1)

You should also put your argument between parentheses here (and in all
your other macros).

> +static const u16 div_m_table[] = {
> + 1,
> + 2,
> + 4,
> + 8,
> + 16,
> + 32,
> + 64,
> + 128,
> + 256
> +};

If this is just a power of two, you can use either the power of two /
ilog2 to switch back and forth, instead of using that table.

> +static int sun8i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + int ret;
> + struct sun8i_pwm_chip *sun8i_pwm = to_sun8i_pwm_chip(chip);
> + struct pwm_state cstate;
> +
> + pwm_get_state(pwm, &cstate);
> + if (!cstate.enabled) {
> + ret = clk_prepare_enable(sun8i_pwm->clk);
> + if (ret) {
> + dev_err(chip->dev, "Failed to enable PWM clock\n");
> + return ret;
> + }
> + }
> +
> + spin_lock(&sun8i_pwm->ctrl_lock);

What do you need that spinlock for? Can you use a mutex instead?

Thanks!
Maxime

--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-02-26 10:03    [W:0.103 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site