linux-kernel - Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 20 Aug 2014 12:04:49 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Heiko Stuebner <heiko@...ech.de>,
	Caesar Wang <caesar.wang@...k-chips.com>,
	Sonny Rao <sonnyrao@...omium.org>, olof@...om.net,
	Eddie Cai <eddie.cai@...k-chips.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288

On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
> The rk3288 has the ability to invert the polarity of the PWM.  Let's
> enable that ability.
> 
> To do this we increase the number of pwm_cells to 3 to allow using the
> PWM_POLARITY_INVERTED flag.  Since the PWM driver on rk3288 is very
> new, I thought this was OK.

I don't see any files in arch/arm/boot/dts using either of the
rockchip,vop-pwm or rockchip,rk3288-pwm compatible strings, so there's
no reason to consider this stable ABI yet. As far as I'm concerned the
last sentence can just as well be dropped.

Besides, patches have been posted to support #pwm-cells = <2> and
#pwm-cells = <3> at the same time which should give you backwards-
compatibility for free.

A couple more comments inline.

> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> +int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      enum pwm_polarity polarity)

This should be static.

> +{
> +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> +
> +	if (!pc->data->has_invert)
> +		return -ENOSYS;
> +
> +	/*
> +	 * No action needed here because pwm->polarity will be set by the core
> +	 * and the core will only change polarity when the PWM is not enabled.
> +	 * We'll handle things in set_enable().
> +	 */
> +
> +	return 0;
> +}

An alternative here would be to provide a separate pwm_ops with
.set_polarity = NULL for the versions of the IP block that don't support
polarity inversion yet, but this works for me too.

> @@ -173,6 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>  		.ctrl = 0x0c,
>  	},
>  	.prescaler = 1,
> +	.has_invert = true,
>  	.set_enable = rockchip_pwm_set_enable_v2,
>  };
>  
> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
>  		.ctrl = 0x00,
>  	},
>  	.prescaler = 1,
> +	.has_invert = true,
>  	.set_enable = rockchip_pwm_set_enable_v2,
>  };

Can you please add a '.has_invert = false,' line to pwm_data_v1? I know
it's not strictly necessary but I like it when things are explicitly
stated.

>  
> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	pc->chip.ops = &rockchip_pwm_ops;
>  	pc->chip.base = -1;
>  	pc->chip.npwm = 1;
> +	if (pc->data->has_invert) {

There should be a blank line between the above two.

Thierry

Content of type "application/pgp-signature" skipped