From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98559C433E2 for ; Thu, 10 Sep 2020 12:14:40 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 28DEF2076D for ; Thu, 10 Sep 2020 12:14:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="lt8i2BuH"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="D132hh1t"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=gmx.net header.i=@gmx.net header.b="dfpDNAsr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 28DEF2076D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gmx.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=7z0+NTVzlHvA/96XHOdGtUOyI0c+Fh7T7xqbG1Pvy+M=; b=lt8i2BuH0LBZI8sa/HKcBAFHe ae6lRNwmM0dpk8iErtIsLmt11MruaFefiY1dCczvVRHiCMLC57ckRKWWNqtYxI3V5bU09BXJ/6Pf6 apWa41EolGa9CId8PV2HRBVn8RSkRnN99pgOSQkgKi454XflbONEfNz2xzqHeYNX1GHhj/LwDjNQT T2Fhfzwh6rE7r/Cf99OxQSABTaKWbc8arRyNvC9VnzRDdeZfaxElU7//kpDAtABcQjyhnAKsG1uxA dvEQL3FfSk8EXfpbRtjQJJRvKgKg86h6Orbs9RHlx74QFKyX7KtYlXadFfvzxCuC+GNQ8u+05i+6f rBawxbunA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kGLS9-0002Cx-Qh; Thu, 10 Sep 2020 12:13:25 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kGLS7-0002C6-Bv for linux-arm-kernel@merlin.infradead.org; Thu, 10 Sep 2020 12:13:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=qICE4gszz8AorcJmZwNU0/DmDlZZLYZRrRtrpUBNX6g=; b=D132hh1t3KCZPcS2pOQla3isC+ 5WICcLvAh3TxpAfk5bFpbHRmNYgEmC0sMjJnVANHtICQy2/j0vQt0yRrahc/BzvtPG4VQ/z3zG3v7 VwMH9qTpNCnczx9bykJ680D1AnLt88B1mpBzo9OIfHyDowCi94HinvG8njU9JJCi6Q1a3mQtHr70B DeHVZOY9Fb92ziCQVaqItyQmlbIhmjoigW0vFiMtaP3O0u9yOCocRLJsgwYa6hgRIYj0HsELzcnhk 457+ag2FVdWoH1Vkzuzh47Wc8K9O+zjyhr5u2SI2lRWk2CGLOEgI443PP5dkmY9nQ8WRk8/wVY6AO 3XqNRTIA==; Received: from mout.gmx.net ([212.227.15.18]) by casper.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kGLS2-0006JL-KA for linux-arm-kernel@lists.infradead.org; Thu, 10 Sep 2020 12:13:21 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1599739973; bh=qzVY6yzsPrrf2h1jDkFWfRG1ddzeALKdqUvY8f8kxa0=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:References:In-Reply-To; b=dfpDNAsrRhWKwswAZ0E5jHWvcfnqc7tTiRyQGgfRrEPLbN+EdOicJUN+rkHoklZDh ZbMqDbrAqAr3Iz47xDS+k4I57xPrHvqxWN8oDi9pwXhW7jMH2PdkZDq0X2dUMAqgon 8U8e6t3ZflteCb7JGRvHBXiBabkw6YpXu7KzcXs0= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from longitude ([5.146.195.151]) by mail.gmx.com (mrgmx005 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MuUnA-1kXb1F1EZO-00rWJE; Thu, 10 Sep 2020 14:04:43 +0200 Date: Thu, 10 Sep 2020 14:04:28 +0200 From: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= To: Lee Jones Subject: Re: [PATCH v2 03/10] mfd: Add base driver for Netronix embedded controller Message-ID: <20200910120428.GA3306@latitude> References: <20200905133230.1014581-1-j.neuschaefer@gmx.net> <20200905133230.1014581-4-j.neuschaefer@gmx.net> <20200908132934.GT4400@dell> MIME-Version: 1.0 In-Reply-To: <20200908132934.GT4400@dell> X-Provags-ID: V03:K1:x4RJpktdT7JqtxxiLTo52FsNglQaWRz741nH+QILas+w8FVRcxR 27KV3AYLrQ49I1+1TR+bS9tBtkWHrt+q8ONM5fw4l6U1A/Z3e4g4EHu58UuKkwp5JRwwl+O 2zn7mGlCAtYpOxJJuGOChMvjvJeNk4XH6elgua7y9AbMuiNAGJv+Gpj2bqY/AhM1Ld4atm5 0sSPE/ntwgudtH1KF9KQw== X-UI-Out-Filterresults: notjunk:1;V03:K0:Amffg/DM7dE=:LjDEIOIY8DllmwnuoLjJF/ tEP2YHF391W8gS0qh2SacuMfWjH3oiYj9agjHBSApg2sjgohm2+dQe3lQuHEaSoYmBKU869LH NiFEouNJC/ojl+9iXc1I7s1ixTmiItmvtUhtobP3P3kt1jnJJ8JW4vyGaKJJDJKdg+Ev34x0H MQX+nFq1kAWFfiVDwkXJh1UgwKNClxb/O/eKSVDofzCULi5RNSzRZIIWeDrX7elRFukwSU81k dIkjEDI+NKj/vrc2yNCyWmtWfE5bPrsBNctlSCpa+PabWrXOYzEhAbTYTo1s2yQ2cH0hmVkaN Kx49KI0FJhjXZWx0lXWrvGol4uQJwhsoYhaf7fnB87McnDaWCUVqZnt7CPqqEebna2ah0jKWk jPVVn2tuqBXq7q0I1lK4cIGBbr/CIcx/BXsk9Nrtehx112dpWBLADZChoIdVgYBWIIMs3POS5 /HBUFgg0cP45+jw6qwL9qcoXo2N/ZdJ403ktYezVteDI9s4j7mSLACKXSWB6PMYjn0yH+a8pU qmEcpMOFyCDUF2KtXtDfePhUVihg1+jzS3XQAf4BQDPH6v+p7VR+A1R/WYgneI6uRsgXd3gh8 z4OvkzQVk29sgvHPshOzhOimkuHf85mt7Wu9Gk+jc7mkJqOlQIn3WdrlQUPdzpxCNYXToB1g8 5kw2m/gpJQEF748UG7VBamr/leOLEBQBIwiFspVQi78GhloGuhYu2IzF8FgrkquGJtnXfuydX UeOeSvQ0zBuIGezv1QeiXWKThXWzTpeaQn0CDgw+JCwPXomH621CKXcwQLEkbau3E4fTfbOX1 kstGGb48V+KmuObjlUP3eqQ4V/pkCL6/5/+uIeniQHpFWdemE9HCQ3q2PD+ImYbcz98cBeOeU cQP3m/f6IG73qI7b/0WTGUP59nySThRveghv4AdBiZVBnA8ZOx5V3gQEnNjjnAv/NybHfElud m64SBS6hNY+8tOBhkhKWMd8C6YS4CZjp7O+2T5zvmEDZjjgnXfiq0tb5uH5GcqUwfUMELPfEQ 7FsJHSRXhhPiJv4ES8s96WO19Wk6R12P/WT9BNGHDO/TJodBkm9U1kkiqxIXSPGrNKSpbIAe5 bPZZ7TGCGvIhvCkL8wDEfkQwMQbPJUN94jFFlf9R9pyJxmqCy0Nvx36S1lFzkeNSxqVAmoCht ZNE+4SxKAxpy+S+Gto9s5uEn0dc9aW2/pTPytG2B1Hcv5DlU7aoyPH1gehZz/b0vvQQhvlOij 3PF0tnrCdp2CRrtRv5Bdbg7v57fzpyJepFk347A== X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200910_131318_826626_1B764B08 X-CRM114-Status: GOOD ( 45.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , Heiko Stuebner , linux-pwm@vger.kernel.org, Linus Walleij , Thierry Reding , Sam Ravnborg , linux-rtc@vger.kernel.org, Arnd Bergmann , Mauro Carvalho Chehab , Fabio Estevam , Daniel Palmer , Andreas Kemnade , NXP Linux Team , Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , devicetree@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Jonathan =?utf-8?Q?Neusch=C3=A4fer?= , Lubomir Rintel , Mark Brown , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , linux-kernel@vger.kernel.org, Rob Herring , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Content-Type: multipart/mixed; boundary="===============6642917379128561743==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============6642917379128561743== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="wRRV7LY7NUeQGEoC" Content-Disposition: inline --wRRV7LY7NUeQGEoC Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 08, 2020 at 02:29:34PM +0100, Lee Jones wrote: > On Sat, 05 Sep 2020, Jonathan Neusch=C3=A4fer wrote: [...] > > +config MFD_NTXEC > > + tristate "Netronix embedded controller" >=20 > Nit: "Embedded Controller (EC)" I intentionally left it lowercase, because the term 'embedded controller' is not used in code coming from Netronix. It's just a label that I found fitting to assign to the device. (In the vendor kernels it's either called 'msp430', named after the TI MSP430 microcontroller family, or 'uc', presumably for 'microcontroller') Adding '(EC)' seems somewhat useful. >=20 > > + depends on I2C && OF >=20 > depends on (I2C && OF) || COMPILE_TEST Okay > > +++ b/drivers/mfd/ntxec.c > > @@ -0,0 +1,217 @@ > > +// SPDX-License-Identifier: GPL-2.0-only >=20 > Why 2 only? No particular reason. If you prefer 2-or-later, I'll change it. > > +/* > > + * The Netronix embedded controller is a microcontroller found in some > > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, > > + * battery monitoring, system power management, and PWM functionality. > > + * > > + * This driver implements register access, version detection, and syst= em > > + * power-off/reset. > > + * > > + * Copyright 2020 Jonathan Neusch=C3=A4fer >=20 > Email? Alright, I'll add it > > +static void ntxec_poweroff(void) > > +{ > > + int res; > > + >=20 > Remove this line please. >=20 > > + u8 buf[] =3D { > > + NTXEC_REG_POWEROFF, > > + (NTXEC_POWEROFF_VALUE >> 8) & 0xff, > > + NTXEC_POWEROFF_VALUE & 0xff, > > + }; > > + >=20 > And this one. >=20 > > + struct i2c_msg msgs[] =3D { > > + { > > + .addr =3D poweroff_restart_client->addr, > > + .flags =3D 0, > > + .len =3D sizeof(buf), > > + .buf =3D buf > > + } > > + }; > > + > > + res =3D i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SI= ZE(msgs)); > > + >=20 > And this one. Okay >=20 > > + if (res < 0) > > + dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = =3D %d)\n", res); >=20 > This looks way over 80 chars. Okay, I'll break it up. > Did you run checkpatch.pl? Yes, but it didn't complain about this line. - propabably because the line length threshold in checkpatch has recently been increased to 100. > > + /* > > + * The time from the register write until the host CPU is powered off > > + * has been observed to be about 2.5 to 3 seconds. Sleep long enough = to > > + * safely avoid returning from the poweroff handler. > > + */ > > + msleep(5000); > > +} > > + > > +static int ntxec_restart(struct notifier_block *nb, > > + unsigned long action, void *data) [...] >=20 > All as above for this function. Alright >=20 > > +static struct notifier_block ntxec_restart_handler =3D { > > + .notifier_call =3D ntxec_restart, > > + .priority =3D 128 > > +}; > > + > > +static const struct regmap_config regmap_config =3D { > > + .name =3D "ntxec", > > + .reg_bits =3D 8, > > + .val_bits =3D 16, > > + .cache_type =3D REGCACHE_NONE, > > + .val_format_endian =3D REGMAP_ENDIAN_BIG, > > +}; > > + > > +static const struct ntxec_info ntxec_known_versions[] =3D { > > + { 0xd726 }, /* found in Kobo Aura */ > > +}; > > + > > +static const struct ntxec_info *ntxec_lookup_info(u16 version) > > +{ > > + int i; > > + > > + for (i =3D 0; i < ARRAY_SIZE(ntxec_known_versions); i++) { > > + const struct ntxec_info *info =3D &ntxec_known_versions[i]; > > + > > + if (info->version =3D=3D version) > > + return info; > > + } > > + > > + return NULL; > > +} >=20 > Wait, what? This is over-engineered. I thought it would be useful when we want to attach additional information to specific versions.... okay, it is over-engineered. > Just have a look-up table (switch) of supported devices. Will do. >=20 > > +static int ntxec_probe(struct i2c_client *client) > > +{ > > + struct ntxec *ec; > > + unsigned int version; > > + int res; > > + > > + ec =3D devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > > + if (!ec) > > + return -ENOMEM; > > + > > + ec->dev =3D &client->dev; > > + > > + ec->regmap =3D devm_regmap_init_i2c(client, ®map_config); > > + if (IS_ERR(ec->regmap)) { > > + dev_err(ec->dev, "Failed to set up regmap for device\n"); > > + return res; > > + } > > + > > + /* Determine the firmware version */ > > + res =3D regmap_read(ec->regmap, NTXEC_REG_VERSION, &version); > > + if (res < 0) { > > + dev_err(ec->dev, "Failed to read firmware version number\n"); > > + return res; > > + } > > + dev_info(ec->dev, > > + "Netronix embedded controller version %04x detected.\n", > > + version); > > + > > + /* Bail out if we encounter an unknown firmware version */ > > + ec->info =3D ntxec_lookup_info(version); > > + if (!ec->info) > > + return -EOPNOTSUPP; >=20 > #define EOPNOTSUPP 95 /* Operation not supported on transport e= ndpoint */ >=20 > I think you want: >=20 > #define ENODEV 19 /* No such device */ Indeed, EOPNOTSUPP seems quite wrong here. I think I used ENOTSUPP at some earlier point but moved away from it because it's one of the internal error codes (=E2=89=A5512). ENODEV makes sense when I read it as "Not a device that this driver can deal with". >=20 > > + if (of_device_is_system_power_controller(ec->dev->of_node)) { > > + /* > > + * Set the 'powerkeep' bit. This is necessary on some boards > > + * in order to keep the system running. > > + */ > > + res =3D regmap_write(ec->regmap, NTXEC_REG_POWERKEEP, > > + NTXEC_POWERKEEP_VALUE); > > + if (res < 0) > > + return res; > > + > > + /* Install poweroff handler */ >=20 > Don't think this comment is required. >=20 > > + WARN_ON(poweroff_restart_client); > > + poweroff_restart_client =3D client; > > + if (pm_power_off) > > + dev_err(ec->dev, "pm_power_off already assigned\n"); > > + else > > + pm_power_off =3D ntxec_poweroff; > > + > > + /* Install board reset handler */ >=20 > Nor this. Alright, I'll remove them. > > + res =3D register_restart_handler(&ntxec_restart_handler); > > + if (res) > > + dev_err(ec->dev, > > + "Failed to register restart handler: %d\n", res); > > + } > > + > > + i2c_set_clientdata(client, ec); > > + > > + return devm_of_platform_populate(ec->dev); > > +} > > + > > +static int ntxec_remove(struct i2c_client *i2c) >=20 > Why do you call it 'client' in .probe, but 'i2c' in .remove? No particular reason. I'll make them consistent. > > +struct ntxec_info { > > + u16 version; > > +}; > > + > > +struct ntxec { > > + struct device *dev; > > + struct regmap *regmap; >=20 > > + const struct ntxec_info *info; >=20 > What is this used for? At this point, nothing. I'll remove it. Thanks, Jonathan Neusch=C3=A4fer --wRRV7LY7NUeQGEoC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl9aFjsACgkQCDBEmo7z X9tAGQ//SrW5PtjGv5XZfjK/Y51oKvi2VtoRlg6p03T3vUyB9j7OPeoadeFgsJae RFlyDcecCt3TFA8hG9L16YBQ34+xA/W0Sa+fN9QM9JBtGpCitFNHYEyar6ONOmAl tfWwjnj6s5ddT9/7+Vq8I0PN5hU5IlpsNSmQQr1W/NtcWrywctorAn4S4RVyMKPs 9invE5HDBx7DZEOOHf0F0zOnIw3ilayCe5aAAhjMo0Q/rbDtrX54M1yr7KkyZxyh 6Kn4gQPLvw3rneXDKmsvMj4bPi/NNrs4WWqwLFsaLkCX9sXwdOssKoLRNO2zj6AJ wUJB2BEZa/DCbCP/oOGrZzpf80sOW+ELpCc4lKkasywI3FEOOC0DhythD342SYwz GBC7MOiwHd3fXn2owynnyj+3QUWd3hg+Rbl6ueaLMXY0Mb1ojmi8GfYHZTORlcpZ 3iXS9CazhgPD2ZTDka3B0H8C2IzpUV5ku7r8jVNxeUHCrV200asvpwZNojewXF// lCGucwAB8tSwVNPlPLuVUnqQLSm1RJoY0zsWb9YGJj4WtQK9crmK2GEfFPTSEetK n0JfMJYlbO1oa69tVDnw1guPNFPYRiKzpE3rS8KVVgaF7LtjouAJX+Fj+0414/Z6 iQiwfoMawFirtow8DvdHt0Y+OSazNz4Y3wn1UkI1V1ikYTRi86c= =ckCY -----END PGP SIGNATURE----- --wRRV7LY7NUeQGEoC-- --===============6642917379128561743== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============6642917379128561743==--