linux-kernel - Re: [PATCH v2 4/4] ARM: PWM: add allwinner sun8i pwm support.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180226090038.etk5q4pd4rl5dvf6@flea.lan>
Date:   Mon, 26 Feb 2018 10:00:38 +0100
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     hao_zhang <hao5781286@...il.com>
Cc:     thierry.reding@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        linux@...linux.org.uk, wens@...e.org, Claudiu.Beznea@...rochip.com,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-pwm@...r.kernel.org, linux-sunxi@...glegroups.com
Subject: Re: [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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)