Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 29 Nov 2020 18:48:43 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        Mark Brown <broonie@kernel.org>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Heiko Stuebner <heiko@sntech.de>, devicetree@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>, linux-rtc@vger.kernel.org,
        Arnd Bergmann <arnd@arndb.de>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Fabio Estevam <festevam@gmail.com>,
        Daniel Palmer <daniel@0x0f.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Andreas Kemnade <andreas@kemnade.info>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-pwm@vger.kernel.org, Stephan Gerhold <stephan@gerhold.net>,
        allen <allen.chen@ite.com.tw>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Lubomir Rintel <lkundrak@v3.sk>,
        Rob Herring <robh+dt@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        linux-arm-kernel@lists.infradead.org,
        Alessandro Zummo <a.zummo@towertech.it>,
        linux-kernel@vger.kernel.org,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Heiko Stuebner <heiko.stuebner@theobroma-systems.com>,
        Josua Mayer <josua.mayer@jm0.eu>,
        Shawn Guo <shawnguo@kernel.org>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in
 Netronix EC
Message-ID: <20201129174843.GF456020@latitude>
References: <20201122222739.1455132-1-j.neuschaefer@gmx.net>
 <20201122222739.1455132-5-j.neuschaefer@gmx.net>
 <20201124082019.vpkr3xnp55arjpnp@pengutronix.de>
 <20201126231931.GE456020@latitude>
 <20201127071105.k2rb4iykeqevbao5@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="o0ZfoUVt4BxPQnbU"
Content-Disposition: inline
In-Reply-To: <20201127071105.k2rb4iykeqevbao5@pengutronix.de>
Precedence: bulk


--o0ZfoUVt4BxPQnbU
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-K=C3=B6nig wrote:
> Hello Jonathan,
>=20
> On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neusch=C3=A4fer wrote:
> > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-K=C3=B6nig wrote:
[...]
> > > state->duty_cycle and state->period are u64, so you're losing
> > > information here. Consider state->duty_cycle =3D 0x100000001 and
> > > state->period =3D 0x200000001.
> >=20
> > Oh, good point, I didn't notice the truncation.
> >=20
> > The reason I picked unsigned int was to avoid a 64-bit division;
> > I suppose I can do something like this:
> >=20
> >     period =3D (u32)period / TIME_BASE_NS;
> >     duty =3D (u32)duty / TIME_BASE_NS;
>=20
> You can do that after you checked period > MAX_PERIOD_NS below, yes.
> Something like:
>=20
> 	if (state->polarity !=3D PWM_POLARITY_NORMAL)
> 		return -EINVAL;
>=20
> 	if (state->period > MAX_PERIOD_NS) {
> 		period =3D MAX_PERIOD_NS;
> 	else
> 		period =3D state->period;
>=20
> 	if (state->duty_cycle > period)
> 		duty_cycle =3D period;
> 	else
> 		duty_cycle =3D state->duty_cycle;
>=20
> should work with even keeping the local variables as unsigned int.

With the min_t() macro, this becomes nice and short:

	 period =3D min_t(u64, state->period, MAX_PERIOD_NS);
	 duty   =3D min_t(u64, state->duty_cycle, period);

	 period /=3D TIME_BASE_NS;
	 duty   /=3D TIME_BASE_NS;


> > > I think I already asked, but I don't remember the reply: What happens=
 to
> > > the output between these writes? A comment here about this would be
> > > suitable.
> >=20
> > I will add something like the following:
> >=20
> > /*
> >  * Changes to the period and duty cycle take effect as soon as the
> >  * corresponding low byte is written, so the hardware may be configured
> >  * to an inconsistent state after the period is written and before the
> >  * duty cycle is fully written. If, in such a case, the old duty cycle
> >  * is longer than the new period, the EC will output 100% for a moment.
> >  */
>=20
> Is the value pair taken over by hardware atomically? That is, is it
> really "will" in your last line, or only "might". (E.g. when changing
> from duty_cycle, period =3D 1000, 2000 to 500, 800 and a new cycle begins
> after reducing period, the new duty_cycle is probably written before the
> counter reaches 500. Do we get a 100% cycle here?)

I am not sure when exactly a new period or duty cycle value is applied,
and I don't have the test equipment to measure it. I'll change the text
to "may output 100%".

> Other than that the info is fine. Make sure to point this out in the
> Limitations paragraph at the top of the driver please, too.

Okay.


> > > /*
> > >  * The current state cannot be read out, so there is no .get_state
> > >  * callback.
> > >  */
> > >=20
> > > Hmm, at least you could provice a .get_state() callback that reports =
the
> > > setting that was actually implemented for in the last call to .apply(=
)?
> >=20
> > Yes... I see two options:
> >=20
> > 1. Caching the state in the driver's private struct. I'm not completely
> >    convinced of the value, given that the information is mostly
> >    available in the PWM core already (except for the adjustments that
> >    the driver makes).
> >=20
> > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> >    This seems a bit dirty.
>=20
> 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> can be made doing that)?

With regmap caching, I'd be concerned that a read operation may slip
through and reach the device, producing a bogus result. Not sure if
write-only/write-through caching can be configured in regmap.


Thanks,
Jonathan

--o0ZfoUVt4BxPQnbU
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl/D3vAACgkQCDBEmo7z
X9t1kA//Rr2iUNp6uHCpqNEZM8RTiPQXN3QbeqMwR17sfoIUcaVl5lT0gD949Ea8
VbzZAp+3xPIlkGjs6JvjpDm9VVEM1jIV78+SVdvbAf7jW6g4WoM+xsSpCz1oD3QK
MU2M6v/hkW51EaTB05SKxYWVNkHrEYcxjLv2pjuBE/WP7MATjX1CzksJ8y3QSrYc
4j4os/8/ulznBElr1epcNr+IpMEotlV6JT7W+CQzW1Zj2ASOJjUjy9uUGBcOUUfH
v7Sfe/SFS0Dniy6GDaBQj0r13rRGyevR+TQBRUcrrIogEn8rc51KDMZW1FvAp1Sw
+a/ZURqwce2YP03FuFxMoJhePJRfqxXIQ82/BG51sE6MwnVwNls4dDIZGlHMzfPD
crmjBo0Tk3G2WMQIiPtJoRbKNLqQ3J+RoU4+P0ch0eQcvrOqzn54wJpVAPE1/aHl
lWFa5bzXUTdSxVoHzBu5iNcUnlVp4J8USdo59u/l0Sj7ZLnXRtrJQL58Xkf0yh9m
VtyK02UJ7m9YfwtCo6j8yqk1/uJ1PlqyABrYgMOMtUDcUXKBLgnjB+vu+x/NLtEu
Mwd8vA6dJYL6rATyv4l90Aagh97wjnTy9WjdgydXv/5kyEPzbGmihsGcX07/tcCc
qiLeT9MG0S0a2/TKef6MfDMOEBszXiQ9jbCkHIBTQW3nCBGYGRE=
=eiSy
-----END PGP SIGNATURE-----

--o0ZfoUVt4BxPQnbU--