Re: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller

From: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
To: Andreas Kemnade <andreas@kemnade.info>
Cc: "Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	linux-pwm@vger.kernel.org,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Fabio Estevam" <festevam@gmail.com>,
	linux-rtc@vger.kernel.org,
	"Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>,
	"Sam Ravnborg" <sam@ravnborg.org>,
	"NXP Linux Team" <linux-imx@nxp.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	devicetree@vger.kernel.org,
	"Stephan Gerhold" <stephan@gerhold.net>,
	allen <allen.chen@ite.com.tw>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	"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, "Mark Brown" <broonie@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: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
Date: Sun, 28 Jun 2020 10:29:06 +0200	[thread overview]
Message-ID: <20200628082906.GB2561@latitude> (raw)
In-Reply-To: <20200627101738.2fe4abc3@aktux>

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

On Sat, Jun 27, 2020 at 10:17:38AM +0200, Andreas Kemnade wrote:
> On Sun, 21 Jun 2020 00:42:15 +0200
> Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 
> > Third-party hardware documentation is available at
> > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> > 
> > The EC supports interrupts, but the driver doesn't make use of them so
> > far.
> > 
> > Known problems:
> > - The reboot handler is installed in such a way that it directly calls
> >   into the i2c subsystem to send the reboot command to the EC. This
> >   means that the reboot handler may sleep, which is not allowed.
> > 
> see
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20190415213432.8972-3-contact@stefanchrist.eu/
> 
> for a fix of such problems. 

So far, regmap isn't involved here, but I'll remember it when I switch
to regmap.

Between when I first wrote this driver and now, the I2C has added
support for transfers in atomic contexts very late in the system's life
(exactly what happens when you reset a system via PMIC/EC), so this
problem seems to be gone from my driver, for now.
(See commit 63b96983a5ddf ("i2c: core: introduce callbacks for atomic transfers"))

[...]
> > +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> > +{
> > +	return ntxec_write16(ec, addr, value << 8);
> > +}
> > +EXPORT_SYMBOL(ntxec_write8);
> > +
> 
> do we really need both 16bit and 8bit accessors?

No, the hardware/firmware doesn't care.

> If not, then simply use regmap_i2c_init and set val_bits accordingly.
> Maybe just doing the << 8 in the constants?

Thanks, I'll try this approach.

The values are not always constants, for example in the PWM driver:

	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);

Jonathan

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

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

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