Re: [PATCH v4 4/4] pwm: meson: make full use of common clock framework

From: Thierry Reding <thierry.reding@gmail.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: "Heiner Kallweit" <hkallweit1@gmail.com>,
	"Jerome Brunet" <jbrunet@baylibre.com>,
	"Martin Blumenstingl" <martin.blumenstingl@googlemail.com>,
	"Neil Armstrong" <narmstrong@baylibre.com>,
	"Kevin Hilman" <khilman@baylibre.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"open list:ARM/Amlogic Meson..."
	<linux-amlogic@lists.infradead.org>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH v4 4/4] pwm: meson: make full use of common clock framework
Date: Mon, 17 Apr 2023 11:17:57 +0200	[thread overview]
Message-ID: <ZD0OxV_VOWcS7URd@orome> (raw)
In-Reply-To: <ca531c1a-3c62-5fb1-6765-68ec1e541483@linaro.org>

[-- Attachment #1.1: Type: text/plain, Size: 1595 bytes --]

On Mon, Apr 17, 2023 at 09:23:35AM +0200, Neil Armstrong wrote:
> On 13/04/2023 07:54, Heiner Kallweit wrote:
[...]
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
[...]
> > @@ -271,16 +255,16 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >   			/*
> >   			 * This IP block revision doesn't have an "always high"
> >   			 * setting which we can use for "inverted disabled".
> > -			 * Instead we achieve this using the same settings
> > -			 * that we use a pre_div of 0 (to get the shortest
> > -			 * possible duration for one "count") and
> > +			 * Instead we achieve this by setting an arbitrary,
> > +			 * very high frequency, resulting in the shortest
> > +			 * possible duration for one "count" and
> >   			 * "period == duty_cycle". This results in a signal
> >   			 * which is LOW for one "count", while being HIGH for
> >   			 * the rest of the (so the signal is HIGH for slightly
> >   			 * less than 100% of the period, but this is the best
> >   			 * we can achieve).
> >   			 */
> > -			channel->pre_div = 0;
> > +			channel->rate = 1000000000;
> >   			channel->hi = ~0;
> >   			channel->lo = 0;
> 
> This looks like a really bad idea... please don't do that and instead introduce
> some pinctrl states where we set the PWM pin as GPIO mode high/low state like we
> did for SPI:
> https://lore.kernel.org/r/20221004-up-aml-fix-spi-v4-2-0342d8e10c49@baylibre.com

Yeah, absolutely. If the pin controller can do that, that's the right
way to implement the desired behavior.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic