* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Lee Jones @ 2018-07-05 5:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Dmitry Torokhov, matti.vaittinen, Michael Turquette, sboyd,
Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <CAFqH_50UshTTc0Cv0wBhUoPdm0jZVgcvRKr3qzb=M-jCcMhdtA@mail.gmail.com>
On Wed, 04 Jul 2018, Enric Balletbo Serra wrote:
> Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
> de jul. 2018 a les 17:10:
> >
> > Hi Enric,
> >
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Hi Matti,
> > >
> > > Thanks for the patch, a few comments below, some are feedback I
> > > received when I sent some patches to this subsystem.
> > >
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> > > > +};
> > > > +
> > > > +/* bd71837 multi function cells */
> > > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > > + {
> > > > + .name = "bd71837-clk",
> > > > + }, {
> > > > + .name = "bd718xx-pwrkey",
> > > > + .resources = &irqs[0],
> > > > + .num_resources = ARRAY_SIZE(irqs),
> > > > + }, {
> > > > + .name = "bd71837-pmic",
> > > > + },
> > > nit: no comma at the end
> >
> > Actually, trailing comma is preferred on structures/arrays without
> > sentinels, because if one needs to add a new entry/new member, then in
> > the diff there will have only one new line added, instead of one line
> > being changed (adding now necessary comma) and one added.
[...]
> Dmitry, I really appreciate this kind of comments, especially when
> there is a justification, so I can learn more and more to do better
> patches.
+1
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [RESEND PATCH] dt-bindings: input: Add common keyboard document bindings
From: Chen Zhong @ 2018-07-05 1:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Mark Rutland, Matthias Brugger, Lee Jones,
linux-input, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
In-Reply-To: <20180704152514.33h52ujqp34hpj2o@penguin>
On Wed, 2018-07-04 at 15:25 +0000, Dmitry Torokhov wrote:
> On Fri, Jun 22, 2018 at 02:39:55PM +0800, Chen Zhong wrote:
> > Hi Dmitry,
> >
> > The other patches in the series to add mediatek pmic keys support are
> > now available in 4.18-rc1, but this patch seems not being merged.
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
> > ib-mfd-input-rtc-v4.18
> >
> > I have resend the patch, and rebase to 4.18-rc1.
> > Could you help to see if it is good enough to be merged into your tree?
>
> I thought Rob would pick it up, but since he hasn't I'll do it.
>
> Thanks.
>
That would be great helpful.
Thank you.
^ permalink raw reply
* Re: [PATCH] HID: debug: check length before copy_to_user()
From: Al Viro @ 2018-07-05 0:47 UTC (permalink / raw)
To: Daniel Rosenberg
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
kernel-team, stable
In-Reply-To: <20180702235937.111619-1-drosen@google.com>
On Mon, Jul 02, 2018 at 04:59:37PM -0700, Daniel Rosenberg wrote:
> If our length is greater than the size of the buffer, we
> overflow the buffer
Hmm...
How about this:
buf = list->hid_debug_buf;
if (list->tail < list->head) {
ret = simple_read_from_buffer(buffer, count, &list->head,
buf, HID_DEBUG_BUFSIZE);
if (ret < 0)
break;
if (list->head != buf + HID_DEBUG_BUFSIZE)
break;
list->head = 0;
}
n = simple_read_from_buffer(buffer + ret, count - ret, &list->head,
buf, list->tail);
if (n >= 0)
ret += n;
if (list->head == buf + HID_DEBUG_BUFSIZE)
list->head = 0;
instead?
^ permalink raw reply
* Re: [PATCH v4 0/5] bitmap: Introduce alloc/free helpers
From: Andy Shevchenko @ 2018-07-04 21:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alasdair Kergon, Mike Snitzer, device-mapper development,
Shaohua Li, linux-raid, Dmitry Torokhov, linux-input,
Andrew Morton, Yury Norov, Linux Kernel Mailing List,
Mika Westerberg, Joe Perches
In-Reply-To: <20180630201750.2588-1-andriy.shevchenko@linux.intel.com>
On Sat, Jun 30, 2018 at 11:17 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> A lot of code is using allocation of bitmaps using BITS_PER_LONG() macro and
> sizeof(unsigned long) operator. The readability suffers because of this.
>
> The series introduces three helpers, i.e. bitmap_alloc(), bitmap_zalloc() and
> bitmap_free(), to make it more cleaner.
>
Dmitry, it seems we are all set now and if you have no objections
against this version, it would be nice to see it in your tree followed
by upstream.
> Patches 1 and 2 are preparatory to avoid namespace collisions between
> bitmap API and DM / MD bitmap. No functional changes intended.
>
> Patch 3 introduces new helpers.
>
> Patches 4 and 5 is just an example how to use new helpers. Locally I have
> like dozen of them against different subsystems and drivers.
>
> Taking above into consideration it might make sense to have
> an immutable branch for this cycle.
>
> Ideally it would go through Input subsystem, thus, needs an Ack
> from MD / DM maintainer(s).
>
> Since v3:
> - split DM part and do rename accordingly (Mike)
> - explain better in the commit message why we can't make helpers as
> inliners (Dmitry, Andrew)
> - drop applied orphaned patch
>
> Since v2:
> - fix compilation issue in MD bitmap code
> - elaborate changes in commit message of patch 5
>
> Since v1:
> - added namespace fix patch against MD bitmap API
> - moved functions to lib/bitmap.c to avoid circular dependencies
> - appended Dmitry's tags
>
> Andy Shevchenko (5):
> dm: Avoid namespace collision with bitmap API
> md: Avoid namespace collision with bitmap API
> bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
> Input: gpio-keys - Switch to bitmap_zalloc()
> Input: evdev - Switch to bitmap API
>
> drivers/input/evdev.c | 16 +-
> drivers/input/keyboard/gpio_keys.c | 8 +-
> drivers/md/dm-raid.c | 6 +-
> drivers/md/md-bitmap.c | 305 +++++++++---------
> drivers/md/md-bitmap.h | 60 ++--
> drivers/md/md-cluster.c | 18 +-
> drivers/md/md.c | 44 +--
> .../md/persistent-data/dm-space-map-common.c | 20 +-
> drivers/md/raid1.c | 35 +-
> drivers/md/raid10.c | 52 ++-
> drivers/md/raid5-cache.c | 8 +-
> drivers/md/raid5.c | 44 +--
> include/linux/bitmap.h | 8 +
> lib/bitmap.c | 19 ++
> 14 files changed, 326 insertions(+), 317 deletions(-)
>
> --
> 2.18.0
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v6 2/4] resource: Use list_head to link sibling resource
From: kbuild test robot @ 2018-07-04 17:00 UTC (permalink / raw)
Cc: kbuild-all, linux-kernel, akpm, robh+dt, dan.j.williams,
nicolas.pitre, josh, fengguang.wu, bp, andy.shevchenko,
brijesh.singh, devicetree, airlied, linux-pci, richard.weiyang,
keith.busch, jcmvbkbc, baiyaowei, frowand.list, lorenzo.pieralisi,
sthemmin, Baoquan He, linux-nvdimm, patrik.r.jakobsson,
linux-input, gustavo, dyoung, vgoyal, thomas.lendacky, haiyangz,
maarten.lankhorst, jglisse
In-Reply-To: <20180704041038.8190-3-bhe@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2540 bytes --]
Hi Baoquan,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc3 next-20180704]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Baoquan-He/resource-Use-list_head-to-link-sibling-resource/20180704-121402
config: mips-rb532_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All error/warnings (new ones prefixed by >>):
>> arch/mips/pci/pci-rc32434.c:57:11: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.child = &rc32434_res_pci_mem2
^
arch/mips/pci/pci-rc32434.c:57:11: note: (near initialization for 'rc32434_res_pci_mem1.child.next')
>> arch/mips/pci/pci-rc32434.c:51:47: warning: missing braces around initializer [-Wmissing-braces]
static struct resource rc32434_res_pci_mem1 = {
^
arch/mips/pci/pci-rc32434.c:60:47: warning: missing braces around initializer [-Wmissing-braces]
static struct resource rc32434_res_pci_mem2 = {
^
cc1: some warnings being treated as errors
vim +57 arch/mips/pci/pci-rc32434.c
73b4390f Ralf Baechle 2008-07-16 50
73b4390f Ralf Baechle 2008-07-16 @51 static struct resource rc32434_res_pci_mem1 = {
73b4390f Ralf Baechle 2008-07-16 52 .name = "PCI MEM1",
73b4390f Ralf Baechle 2008-07-16 53 .start = 0x50000000,
73b4390f Ralf Baechle 2008-07-16 54 .end = 0x5FFFFFFF,
73b4390f Ralf Baechle 2008-07-16 55 .flags = IORESOURCE_MEM,
73b4390f Ralf Baechle 2008-07-16 56 .sibling = NULL,
73b4390f Ralf Baechle 2008-07-16 @57 .child = &rc32434_res_pci_mem2
73b4390f Ralf Baechle 2008-07-16 58 };
73b4390f Ralf Baechle 2008-07-16 59
:::::: The code at line 57 was first introduced by commit
:::::: 73b4390fb23456964201abda79f1210fe337d01a [MIPS] Routerboard 532: Support for base system
:::::: TO: Ralf Baechle <ralf@linux-mips.org>
:::::: CC: Ralf Baechle <ralf@linux-mips.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14130 bytes --]
^ permalink raw reply
* Re: [PATCH v6 2/4] resource: Use list_head to link sibling resource
From: kbuild test robot @ 2018-07-04 17:00 UTC (permalink / raw)
Cc: nicolas.pitre-QSEj5FYQhm4dnm+yROfE0A, brijesh.singh-5C7GfCeVMHo,
thomas.lendacky-5C7GfCeVMHo, airlied-cv59FeDIM0c,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w,
jcmvbkbc-Re5JQEeQqe8AvxtiuMwx3w,
baiyaowei-0p4V/sDNsUmm0O/7XYngnFaTQe2KTcn/,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w, tglx-hfZtesqFncYOwBW4kG4KsQ,
lorenzo.pieralisi-5wv7dgnIgG8, sthemmin-0li6OtcxBFHby3iVrkZq2A,
Baoquan He, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
patrik.r.jakobsson-Re5JQEeQqe8AvxtiuMwx3w,
andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w,
linux-input-u79uwXL29TY76Z2rM5mHXA,
gustavo-THi1TnShQwVAfugRpC6u6w, bp-l3A5Bk7waGM,
dyoung-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
haiyangz-0li6OtcxBFHby3iVrkZq2A,
maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA,
josh-iaAMLnmF4UmaiuxdJuQwMA, jglisse-H+wXaHxf7aLQT0dZR+AlfA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, seanpaul-F7+t8E8rja9g9hUCZPvPmw,
bhelgaas-hpIqsD4AKlfQT0dZR+AlfA, yinghai-DgEjT+Ai2ygdnm+yROfE0A,
jonathan.derrick-ral2JQCrhuEAvxtiuMwx3w,
chris-YvXeqwSYzG2sTnJN9+BGXg, monstr-pSz03upnqPeHXe+LvDLADg,
linux-parisc-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, kexec
In-Reply-To: <20180704041038.8190-3-bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi Baoquan,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc3 next-20180704]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Baoquan-He/resource-Use-list_head-to-link-sibling-resource/20180704-121402
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64
All errors (new ones prefixed by >>):
arch/ia64/sn/kernel/io_init.c: In function 'sn_io_slot_fixup':
>> arch/ia64/sn/kernel/io_init.c:195:19: error: invalid operands to binary && (have 'int' and 'struct list_head')
if (res->parent && res->parent->child)
~~~~~~~~~~~ ^~ ~~~~~~~~~~~~~~~~~~
vim +195 arch/ia64/sn/kernel/io_init.c
^1da177e Linus Torvalds 2005-04-16 142
3ec829b6 John Keller 2005-11-29 143 /*
6f09a925 John Keller 2007-01-30 144 * sn_io_slot_fixup() - We are not running with an ACPI capable PROM,
8ea6091f John Keller 2006-10-04 145 * and need to convert the pci_dev->resource
8ea6091f John Keller 2006-10-04 146 * 'start' and 'end' addresses to mapped addresses,
8ea6091f John Keller 2006-10-04 147 * and setup the pci_controller->window array entries.
^1da177e Linus Torvalds 2005-04-16 148 */
8ea6091f John Keller 2006-10-04 149 void
6f09a925 John Keller 2007-01-30 150 sn_io_slot_fixup(struct pci_dev *dev)
^1da177e Linus Torvalds 2005-04-16 151 {
^1da177e Linus Torvalds 2005-04-16 152 int idx;
ab97b8cc Bjorn Helgaas 2016-03-02 153 struct resource *res;
18c25526 Matt Fleming 2016-05-04 154 unsigned long size;
6f09a925 John Keller 2007-01-30 155 struct pcidev_info *pcidev_info;
6f09a925 John Keller 2007-01-30 156 struct sn_irq_info *sn_irq_info;
6f09a925 John Keller 2007-01-30 157 int status;
6f09a925 John Keller 2007-01-30 158
6f09a925 John Keller 2007-01-30 159 pcidev_info = kzalloc(sizeof(struct pcidev_info), GFP_KERNEL);
6f09a925 John Keller 2007-01-30 160 if (!pcidev_info)
d4ed8084 Harvey Harrison 2008-03-04 161 panic("%s: Unable to alloc memory for pcidev_info", __func__);
6f09a925 John Keller 2007-01-30 162
6f09a925 John Keller 2007-01-30 163 sn_irq_info = kzalloc(sizeof(struct sn_irq_info), GFP_KERNEL);
6f09a925 John Keller 2007-01-30 164 if (!sn_irq_info)
d4ed8084 Harvey Harrison 2008-03-04 165 panic("%s: Unable to alloc memory for sn_irq_info", __func__);
6f09a925 John Keller 2007-01-30 166
6f09a925 John Keller 2007-01-30 167 /* Call to retrieve pci device information needed by kernel. */
6f09a925 John Keller 2007-01-30 168 status = sal_get_pcidev_info((u64) pci_domain_nr(dev),
6f09a925 John Keller 2007-01-30 169 (u64) dev->bus->number,
6f09a925 John Keller 2007-01-30 170 dev->devfn,
6f09a925 John Keller 2007-01-30 171 (u64) __pa(pcidev_info),
6f09a925 John Keller 2007-01-30 172 (u64) __pa(sn_irq_info));
6f09a925 John Keller 2007-01-30 173
80a03e29 Stoyan Gaydarov 2009-03-10 174 BUG_ON(status); /* Cannot get platform pci device information */
6f09a925 John Keller 2007-01-30 175
3ec829b6 John Keller 2005-11-29 176
^1da177e Linus Torvalds 2005-04-16 177 /* Copy over PIO Mapped Addresses */
^1da177e Linus Torvalds 2005-04-16 178 for (idx = 0; idx <= PCI_ROM_RESOURCE; idx++) {
ab97b8cc Bjorn Helgaas 2016-03-02 179 if (!pcidev_info->pdi_pio_mapped_addr[idx])
^1da177e Linus Torvalds 2005-04-16 180 continue;
^1da177e Linus Torvalds 2005-04-16 181
ab97b8cc Bjorn Helgaas 2016-03-02 182 res = &dev->resource[idx];
ab97b8cc Bjorn Helgaas 2016-03-02 183
ab97b8cc Bjorn Helgaas 2016-03-02 184 size = res->end - res->start;
ab97b8cc Bjorn Helgaas 2016-03-02 185 if (size == 0)
3ec829b6 John Keller 2005-11-29 186 continue;
ab97b8cc Bjorn Helgaas 2016-03-02 187
240504ad Bjorn Helgaas 2016-03-02 188 res->start = pcidev_info->pdi_pio_mapped_addr[idx];
18c25526 Matt Fleming 2016-05-04 189 res->end = res->start + size;
64715725 Bernhard Walle 2007-03-18 190
64715725 Bernhard Walle 2007-03-18 191 /*
64715725 Bernhard Walle 2007-03-18 192 * if it's already in the device structure, remove it before
64715725 Bernhard Walle 2007-03-18 193 * inserting
64715725 Bernhard Walle 2007-03-18 194 */
ab97b8cc Bjorn Helgaas 2016-03-02 @195 if (res->parent && res->parent->child)
ab97b8cc Bjorn Helgaas 2016-03-02 196 release_resource(res);
64715725 Bernhard Walle 2007-03-18 197
ab97b8cc Bjorn Helgaas 2016-03-02 198 if (res->flags & IORESOURCE_IO)
ab97b8cc Bjorn Helgaas 2016-03-02 199 insert_resource(&ioport_resource, res);
^1da177e Linus Torvalds 2005-04-16 200 else
ab97b8cc Bjorn Helgaas 2016-03-02 201 insert_resource(&iomem_resource, res);
d7ad2254 John Keller 2007-07-09 202 /*
240504ad Bjorn Helgaas 2016-03-02 203 * If ROM, mark as shadowed in PROM.
d7ad2254 John Keller 2007-07-09 204 */
d7ad2254 John Keller 2007-07-09 205 if (idx == PCI_ROM_RESOURCE) {
240504ad Bjorn Helgaas 2016-03-02 206 pci_disable_rom(dev);
240504ad Bjorn Helgaas 2016-03-02 207 res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
240504ad Bjorn Helgaas 2016-03-02 208 IORESOURCE_PCI_FIXED;
d7ad2254 John Keller 2007-07-09 209 }
^1da177e Linus Torvalds 2005-04-16 210 }
6f09a925 John Keller 2007-01-30 211
6f09a925 John Keller 2007-01-30 212 sn_pci_fixup_slot(dev, pcidev_info, sn_irq_info);
^1da177e Linus Torvalds 2005-04-16 213 }
6f09a925 John Keller 2007-01-30 214 EXPORT_SYMBOL(sn_io_slot_fixup);
6f09a925 John Keller 2007-01-30 215
:::::: The code at line 195 was first introduced by commit
:::::: ab97b8cc560eabfd8139dd97924a09e46a3c9632 ia64/PCI: Use temporary struct resource * to avoid repetition
:::::: TO: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
:::::: CC: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Enric Balletbo Serra @ 2018-07-04 16:57 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
Matti Vaittinen, Arnd Bergmann, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree@vger.kernel.org,
linux-kernel
In-Reply-To: <20180704145623.zgei3ehrwdpcu26r@penguin>
Missatge de Dmitry Torokhov <dmitry.torokhov@gmail.com> del dia dc., 4
de jul. 2018 a les 17:10:
>
> Hi Enric,
>
> On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > Hi Matti,
> >
> > Thanks for the patch, a few comments below, some are feedback I
> > received when I sent some patches to this subsystem.
> >
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dt., 19 de juny 2018 a les 12:57:
> > > +};
> > > +
> > > +/* bd71837 multi function cells */
> > > +static struct mfd_cell bd71837_mfd_cells[] = {
> > > + {
> > > + .name = "bd71837-clk",
> > > + }, {
> > > + .name = "bd718xx-pwrkey",
> > > + .resources = &irqs[0],
> > > + .num_resources = ARRAY_SIZE(irqs),
> > > + }, {
> > > + .name = "bd71837-pmic",
> > > + },
> > nit: no comma at the end
>
> Actually, trailing comma is preferred on structures/arrays without
> sentinels, because if one needs to add a new entry/new member, then in
> the diff there will have only one new line added, instead of one line
> being changed (adding now necessary comma) and one added.
>
Many thanks for sharing your knowledge! That looks to me a good
reason. I did this comment because at some point I received this
comment. I try to mark this kind of things as nitpicks because
sometimes there are differences between maintainers. E.g: some
maintainers prefer 'if (something == NULL)', others 'if (!something)';
others are fine using goto even there is nothing to unwind, other
prefer plain returns, etc.
Matti, I don't want to beat about the bush with these nitpicks. It is
not my intention. So I'd say, do what the maintainer wants :)
Dmitry, I really appreciate this kind of comments, especially when
there is a justification, so I can learn more and more to do better
patches.
Thanks,
Enric
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH v6 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
From: Andy Shevchenko @ 2018-07-04 16:46 UTC (permalink / raw)
To: Baoquan He
Cc: Nicolas Pitre, brijesh.singh-5C7GfCeVMHo, devicetree,
David Airlie, linux-pci-u79uwXL29TY76Z2rM5mHXA,
richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w, Max Filippov,
baiyaowei-0p4V/sDNsUmm0O/7XYngnFaTQe2KTcn/, KY Srinivasan,
Frank Rowand, Lorenzo Pieralisi, Stephen Hemminger,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Patrik Jakobsson,
linux-input, Gustavo Padovan, Borislav Petkov, Dave Young,
Tom Lendacky, Haiyang Zhang, Maarten Lankhorst, Josh Triplett,
Jérôme Glisse
In-Reply-To: <20180704041038.8190-2-bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Jul 4, 2018 at 7:10 AM, Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.
With couple of comments below,
Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
P.S. In some commit message in this series you used 'likt' instead of 'like'.
>
> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> arch/microblaze/pci/pci-common.c | 37 -------------------------------------
> arch/powerpc/kernel/pci-common.c | 35 -----------------------------------
> include/linux/ioport.h | 1 +
> kernel/resource.c | 39 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 40 insertions(+), 72 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
> index f34346d56095..7899bafab064 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -619,43 +619,6 @@ int pcibios_add_device(struct pci_dev *dev)
> EXPORT_SYMBOL(pcibios_add_device);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int __init reparent_resources(struct resource *parent,
> - struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s [%llx..%llx] under %s\n",
> - p->name,
> - (unsigned long long)p->start,
> - (unsigned long long)p->end, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
> * just allocate all the resource regions and do nothing more. It isn't.
> * On the other hand, we cannot just re-allocate all devices, as it would
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index fe9733ffffaa..926035bb378d 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1088,41 +1088,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> EXPORT_SYMBOL(pcibios_align_resource);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int reparent_resources(struct resource *parent,
> - struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s %pR under %s\n",
> - p->name, p, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
> * just allocate all the resource regions and do nothing more. It isn't.
> * On the other hand, we cannot just re-allocate all devices, as it would
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index da0ebaec25f0..dfdcd0bfe54e 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -192,6 +192,7 @@ extern int allocate_resource(struct resource *root, struct resource *new,
> struct resource *lookup_resource(struct resource *root, resource_size_t start);
> int adjust_resource(struct resource *res, resource_size_t start,
> resource_size_t size);
> +int reparent_resources(struct resource *parent, struct resource *res);
> resource_size_t resource_alignment(struct resource *res);
> static inline resource_size_t resource_size(const struct resource *res)
> {
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 30e1bc68503b..d1cbf4b50e17 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -983,6 +983,45 @@ int adjust_resource(struct resource *res, resource_size_t start,
> }
> EXPORT_SYMBOL(adjust_resource);
>
> +/*
/** ?
> + * reparent_resources - reparent resource children of parent that res covers
> + * @parent: parent resource descriptor
> + * @res: resource descriptor desired by caller
> + *
> + * Reparent resource children of 'parent' that conflict with 'res'
> + * under 'res', and make 'res' replace those children.
Describe error codes, please.
> + */
> +int reparent_resources(struct resource *parent, struct resource *res)
> +{
> + struct resource *p, **pp;
> + struct resource **firstpp = NULL;
> +
> + for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> + if (p->end < res->start)
> + continue;
> + if (res->end < p->start)
> + break;
> + if (p->start < res->start || p->end > res->end)
> + return -ENOTSUPP; /* not completely contained */
> + if (firstpp == NULL)
> + firstpp = pp;
> + }
> + if (firstpp == NULL)
> + return -ECANCELED; /* didn't find any conflicting entries? */
> + res->parent = parent;
> + res->child = *firstpp;
> + res->sibling = *pp;
> + *firstpp = res;
> + *pp = NULL;
> + for (p = res->child; p != NULL; p = p->sibling) {
> + p->parent = res;
> + pr_debug("PCI: Reparented %s %pR under %s\n",
> + p->name, p, res->name);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(reparent_resources);
> +
> static void __init __reserve_region_with_split(struct resource *root,
> resource_size_t start, resource_size_t end,
> const char *name)
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] Input: cros_ec_keyb: remove redundant variable num_cols
From: Dmitry Torokhov @ 2018-07-04 15:55 UTC (permalink / raw)
To: Colin King
Cc: Ravi Chandra Sadineni, linux-input, kernel-janitors, linux-kernel
In-Reply-To: <20180702162841.12553-1-colin.king@canonical.com>
On Mon, Jul 02, 2018 at 05:28:41PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Variable num_cols is being assigned but is never used hence it is
> redundant and can be removed.
>
> Cleans up clang warning:
> warning: variable ‘num_cols’ set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied, thank you.
> ---
> drivers/input/keyboard/cros_ec_keyb.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 820bebad519a..81be6f781f0b 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -160,9 +160,6 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> int col, row;
> int new_state;
> int old_state;
> - int num_cols;
> -
> - num_cols = len;
>
> if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev, kb_state)) {
> /*
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4] MAINTAINERS: Add file patterns for serio device tree bindings
From: Dmitry Torokhov @ 2018-07-04 15:27 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rob Herring, Mark Rutland, linux-input, devicetree, linux-kernel
In-Reply-To: <20180622100818.29420-1-geert@linux-m68k.org>
On Fri, Jun 22, 2018 at 12:08:18PM +0200, Geert Uytterhoeven wrote:
> Submitters of device tree binding documentation may forget to CC
> the subsystem maintainer if this is missing.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Since nobody else did I'll pick it up, thank you Geert.
> ---
> v3:
> - Update for next-20180622,
>
> v2:
> - Add Acked-by.
>
> Impact on "scripts/get_maintainer.pl -f Documentation/devicetree/bindings/serio/":
>
> -Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,commit_signer:3/3=100%,authored:1/3=33%)
> -Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,commit_signer:1/3=33%)
> -Dmitry Torokhov <dmitry.torokhov@gmail.com> (commit_signer:1/3=33%)
> -Danilo Krummrich <danilokrummrich@dk-develop.de> (commit_signer:1/3=33%,authored:1/3=33%)
> -Linus Walleij <linus.walleij@linaro.org> (commit_signer:1/3=33%)
> -Mathieu Malaterre <malat@debian.org> (authored:1/3=33%)
> +Dmitry Torokhov <dmitry.torokhov@gmail.com> (maintainer:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...)
> +Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> +Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> +linux-input@vger.kernel.org (open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...)
> devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> linux-kernel@vger.kernel.org (open list)
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 464d91879df89fce..df246daab5a27af2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7100,6 +7100,7 @@ F: include/uapi/linux/input.h
> F: include/uapi/linux/input-event-codes.h
> F: include/linux/input/
> F: Documentation/devicetree/bindings/input/
> +F: Documentation/devicetree/bindings/serio/
> F: Documentation/input/
>
> INPUT MULTITOUCH (MT) PROTOCOL
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH] dt-bindings: input: Add common keyboard document bindings
From: Dmitry Torokhov @ 2018-07-04 15:25 UTC (permalink / raw)
To: Chen Zhong
Cc: Rob Herring, Mark Rutland, Matthias Brugger, Lee Jones,
linux-input, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
In-Reply-To: <1529649595.14396.18.camel@mhfsdcap03>
On Fri, Jun 22, 2018 at 02:39:55PM +0800, Chen Zhong wrote:
> Hi Dmitry,
>
> The other patches in the series to add mediatek pmic keys support are
> now available in 4.18-rc1, but this patch seems not being merged.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
> ib-mfd-input-rtc-v4.18
>
> I have resend the patch, and rebase to 4.18-rc1.
> Could you help to see if it is good enough to be merged into your tree?
I thought Rob would pick it up, but since he hasn't I'll do it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 10/12] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-07-04 15:18 UTC (permalink / raw)
To: Peter Hutterer
Cc: Benjamin Tissoires, Jiri Kosina, Mario.Limonciello, linux-input,
linux-kernel
In-Reply-To: <20180627010858.GB25847@jelly>
On Wed, Jun 27, 2018 at 11:08:58AM +1000, Peter Hutterer wrote:
> On Thu, Jun 21, 2018 at 02:09:06PM +0200, Benjamin Tissoires wrote:
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > According to Microsoft specification [1] for Precision Touchpads (and
> > Touchscreens) the devices use "confidence" reports to signal accidental
> > touches, or contacts that are "too large to be a finger". Instead of
> > simply marking contact inactive in this case (which causes issues if
> > contact was originally proper and we lost confidence in it later, as
> > this results in accidental clicks, drags, etc), let's report such
> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> > Additionally, let's report contact size for such touches as maximum
> > allowed for major/minor, which should help userspace that is not yet
> > aware of MT_TOOL_PALM to still perform palm rejection.
>
> tbh. I have a queasy feeling about this last bit. MT_TOOL_PALM has shown to
> not be reliable on some devices (X1 Carbon 6th) and we mask it out on those.
> Changing the major/minor means it's impossible to rely on the touch size for
> palm detection once a device supports MT_TOOL_PALM.
OK, then let's drop this bit.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Dmitry Torokhov @ 2018-07-04 14:56 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: matti.vaittinen, Michael Turquette, sboyd, Rob Herring,
Mark Rutland, Lee Jones, Liam Girdwood, Mark Brown,
mazziesaccount, Arnd Bergmann, Sebastian Reichel, chenjh,
andrew.smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, linux-clk, devicetree@vger.kernel.org,
linux-kernel, linux-input
In-Reply-To: <CAFqH_51izuVgHukAORvwjZqGhxy85tdNdaHL9mOUnPnbCQmQjA@mail.gmail.com>
Hi Enric,
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
>
> Thanks for the patch, a few comments below, some are feedback I
> received when I sent some patches to this subsystem.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 19 de juny 2018 a les 12:57:
> > +};
> > +
> > +/* bd71837 multi function cells */
> > +static struct mfd_cell bd71837_mfd_cells[] = {
> > + {
> > + .name = "bd71837-clk",
> > + }, {
> > + .name = "bd718xx-pwrkey",
> > + .resources = &irqs[0],
> > + .num_resources = ARRAY_SIZE(irqs),
> > + }, {
> > + .name = "bd71837-pmic",
> > + },
> nit: no comma at the end
Actually, trailing comma is preferred on structures/arrays without
sentinels, because if one needs to add a new entry/new member, then in
the diff there will have only one new line added, instead of one line
being changed (adding now necessary comma) and one added.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/5] iio: adc: ti_am335x_adc: Disable ADC during suspend unconditionally
From: Vignesh R @ 2018-07-04 14:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Lee Jones, linux-iio@vger.kernel.org,
linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-input@vger.kernel.org
In-Reply-To: <20180630161848.1e72deea@archlinux>
On Saturday 30 June 2018 08:48 PM, Jonathan Cameron wrote:
> On Sat, 30 Jun 2018 16:03:17 +0530
> Vignesh R <vigneshr@ti.com> wrote:
>
>> Parent MFD device takes care of enabling ADC interface whenever
>> touchscreen is marked wakeup capable. Therefore, unconditionally disable
>> ADC interface during system suspend to save power in case of system with
>> just ADC and no TSC.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>
> This sounds like a sensible change to me. Is it applicable without the
> rest of the series? If so I'll take it through IIO. If the answer
> is that it needs to go with the rest then
>
This patch as such does not have any compile time dependencies on other
patches, but I would like Dmitry's opinion on the series before being
merged.
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
Thanks!
Regards
Vignesh
> Thanks,
>
> Jonathan
>> ---
>> drivers/iio/adc/ti_am335x_adc.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 80df5a377d30..cafb1dcadc48 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -693,16 +693,12 @@ static int __maybe_unused tiadc_suspend(struct device *dev)
>> {
>> struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> - struct ti_tscadc_dev *tscadc_dev;
>> unsigned int idle;
>>
>> - tscadc_dev = ti_tscadc_dev_get(to_platform_device(dev));
>> - if (!device_may_wakeup(tscadc_dev->dev)) {
>> - idle = tiadc_readl(adc_dev, REG_CTRL);
>> - idle &= ~(CNTRLREG_TSCSSENB);
>> - tiadc_writel(adc_dev, REG_CTRL, (idle |
>> - CNTRLREG_POWERDOWN));
>> - }
>> + idle = tiadc_readl(adc_dev, REG_CTRL);
>> + idle &= ~(CNTRLREG_TSCSSENB);
>> + tiadc_writel(adc_dev, REG_CTRL, (idle |
>> + CNTRLREG_POWERDOWN));
>>
>> return 0;
>> }
>
--
Regards
Vignesh
^ permalink raw reply
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Lee Jones @ 2018-07-04 10:53 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <20180704103430.GA13087@localhost.localdomain>
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> >
> > > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dv., 29 de juny 2018 a les 11:47:
> > > > >
> > > > > Now that you use devm calls and you don't need to unwind things I
> > > > > think is better to use plain returns. So,
> > > > >
> > > > > return -ENOMEM;
> > > >
> > > > I have never really understood why use of gotos in error handling is
> > > > discouraged.
> >
> > They're not.
> >
> > > > Personally I would always choose single point of exit from
> > > > a function when it is simple enough to achieve (like in this case). I've
> > > > written and fixed way too many functions which leak resources or
> > > > accidentally keep a lock when exiting from error branches. But I know
> > > > many colleagues like you who prefer not to have gotos but in place returns
> > > > instead. So I guess I'll leave the final call on this to the one who is
> > > > maintainer for this code. And it is true there is no things to unwind
> > > > now - which does not mean that next updater won't add such. But as I
> > > > said, I know plenty of people share your view - and even though I rather
> > > > maintain code with only one exit the final call is on subsystem maintainer
> > > > here.
> >
> > Please use gotos in the error path.
> >
> > IMO, it's the nicest way to unwind (as you call it).
>
> I'll keep the gotos but clean other stuff for patch v9 then.
Sounds good.
> > > Actually, If it was completely my call the probe would look something
> > > like this:
> > >
> > > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > > + const struct i2c_device_id *id)
> > > +{
[...]
> > > +}
> > >
> > > What do you think of this? To my eye it is nice. It keeps single point of
> > > exit and introduces only simple if-statements without the need of curly
> > > brackets. And finally the error prints string works as a comment too.
> > > I've seen bunch of constructs like this on the networking side but I
> > > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > > is just to illustrate the idea, I did not even try compiling it yet.
> >
> > That is horrible. I nearly vomited on my keyboard.
>
> Note to self: Never to buy second hand keyboard from Lee =)
That is sound advice.
Not sure I would buy 2nd-hand keyboard from anyone - ewe! :\
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-07-04 10:34 UTC (permalink / raw)
To: Lee Jones
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <20180704101102.GA496@dell>
On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> On Wed, 04 Jul 2018, Matti Vaittinen wrote:
>
> > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > dia dv., 29 de juny 2018 a les 11:47:
> > > >
> > > > Now that you use devm calls and you don't need to unwind things I
> > > > think is better to use plain returns. So,
> > > >
> > > > return -ENOMEM;
> > >
> > > I have never really understood why use of gotos in error handling is
> > > discouraged.
>
> They're not.
>
> > > Personally I would always choose single point of exit from
> > > a function when it is simple enough to achieve (like in this case). I've
> > > written and fixed way too many functions which leak resources or
> > > accidentally keep a lock when exiting from error branches. But I know
> > > many colleagues like you who prefer not to have gotos but in place returns
> > > instead. So I guess I'll leave the final call on this to the one who is
> > > maintainer for this code. And it is true there is no things to unwind
> > > now - which does not mean that next updater won't add such. But as I
> > > said, I know plenty of people share your view - and even though I rather
> > > maintain code with only one exit the final call is on subsystem maintainer
> > > here.
>
> Please use gotos in the error path.
>
> IMO, it's the nicest way to unwind (as you call it).
I'll keep the gotos but clean other stuff for patch v9 then.
>
> > Actually, If it was completely my call the probe would look something
> > like this:
> >
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int gpio_intr = 0;
> > +
> > + const char *errstr = "No IRQ configured";
> > + int ret = -EINVAL;
> > +
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > +
> > + if (bd71837 == NULL)
> > + return -ENOMEM;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
> > +
> > + if (!board_info)
> > + gpio_intr = i2c->irq;
> > + else
> > + gpio_intr = board_info->gpio_intr;
> > +
> > + if (!gpio_intr)
> > + goto err_out;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = gpio_intr;
> > +
> > + errstr = "regmap initialization failed";
> > +
> > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> > + ret = PTR_ERR(bd71837->regmap);
> > + if (IS_ERR(bd71837->regmap))
> > + goto err_out;
> > +
> > + errstr = "Read BD71837_REG_DEVICE failed";
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to add irq_chip";
> > + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> > + bd71837->chip_irq, IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to configure button short press timeout";
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG0,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + /* According to BD71847 datasheet the HW default for long press
> > + * detection is 10ms. So lets change it to 10 sec so we can actually
> > + * get the short push and allow gracefull shut down
> > + */
> > + ret = regmap_update_bits(bd71837->regmap,
> > + BD71837_REG_PWRONCONFIG1,
> > + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> > + BD718XX_PWRBTN_LONG_PRESS_10S);
> > +
> > + errstr = "Failed to configure button long press timeout";
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> > + BD71837_INT_PWRBTN_S);
> > +
> > + errstr = "Failed to get the IRQ";
> > + ret = btns[0].irq;
> > + if (btns[0].irq < 0)
> > + goto err_out;
> > +
> > + errstr = "Failed to create subdevices";
> > + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> > + bd71837_mfd_cells,
> > + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> > + regmap_irq_get_domain(bd71837->irq_data));
> > + if (ret) {
> > +err_out:
> > + if (errstr)
> > + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> > + }
> > +
> > + return ret;
> > +}
> >
> > What do you think of this? To my eye it is nice. It keeps single point of
> > exit and introduces only simple if-statements without the need of curly
> > brackets. And finally the error prints string works as a comment too.
> > I've seen bunch of constructs like this on the networking side but I
> > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > is just to illustrate the idea, I did not even try compiling it yet.
>
> That is horrible. I nearly vomited on my keyboard.
Note to self: Never to buy second hand keyboard from Lee =)
> It doesn't flow
> anywhere nearly as nicely has sorting out all of the error handling
> *after* it has been detected. You're sacrificing readability to save
> a single line and do not save any *actual* lines of code, only a brace.
I was expecting something like this comment =) But the truth is that one
gets used to reading this quickly. Well, this still sounds like I should
not try convincing you - so you can stay heretic ;)
>
> Landing a goto in the middle of a statement is messy and unsightly.
This is another thing one gets used to. I've actually seen plenty of
code using
if (0) {
error_label:
....
}
for error handling. But again - you can keep your view and I'll adopt to
it here =)
> What happens when you have some resources to free? The last few lines
> will become very messy, very quickly.
One can just build the usual clean-up sequence inside the last if (ret)
using different goto lables. Eg:
if (ret) {
err_unwind_X:
undo_x();
err_unwind_Y:
undo_y();
err_out:
dev_err(...);
}
>
> Nit: "something == NULL" is better written as "!something".
Oh, I personally liked the !foo more as Enric - but I will write the
NULL in case it won't make line too long. This is not a big deal to me.
> Nit: Please use proper multi-line comments as per the Coding Style.
Will do.
Thanks for quick reply! I will send new version today or tomorrow.
Best Regards
Matti Vaittinen
^ permalink raw reply
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Lee Jones @ 2018-07-04 10:11 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Enric Balletbo Serra, Michael Turquette, Rob Herring,
Mark Rutland, Liam Girdwood, Mark Brown, Matti Vaittinen,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, chenjh,
Andrey Smirnov, Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <20180704095257.GJ2118@localhost.localdomain>
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dv., 29 de juny 2018 a les 11:47:
> > >
> > > Now that you use devm calls and you don't need to unwind things I
> > > think is better to use plain returns. So,
> > >
> > > return -ENOMEM;
> >
> > I have never really understood why use of gotos in error handling is
> > discouraged.
They're not.
> > Personally I would always choose single point of exit from
> > a function when it is simple enough to achieve (like in this case). I've
> > written and fixed way too many functions which leak resources or
> > accidentally keep a lock when exiting from error branches. But I know
> > many colleagues like you who prefer not to have gotos but in place returns
> > instead. So I guess I'll leave the final call on this to the one who is
> > maintainer for this code. And it is true there is no things to unwind
> > now - which does not mean that next updater won't add such. But as I
> > said, I know plenty of people share your view - and even though I rather
> > maintain code with only one exit the final call is on subsystem maintainer
> > here.
Please use gotos in the error path.
IMO, it's the nicest way to unwind (as you call it).
> Actually, If it was completely my call the probe would look something
> like this:
>
> +static int bd71837_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct bd71837 *bd71837;
> + struct bd71837_board *board_info;
> + int gpio_intr = 0;
> +
> + const char *errstr = "No IRQ configured";
> + int ret = -EINVAL;
> +
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> +
> + if (bd71837 == NULL)
> + return -ENOMEM;
> +
> + board_info = dev_get_platdata(&i2c->dev);
> +
> + if (!board_info)
> + gpio_intr = i2c->irq;
> + else
> + gpio_intr = board_info->gpio_intr;
> +
> + if (!gpio_intr)
> + goto err_out;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = gpio_intr;
> +
> + errstr = "regmap initialization failed";
> +
> + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> + ret = PTR_ERR(bd71837->regmap);
> + if (IS_ERR(bd71837->regmap))
> + goto err_out;
> +
> + errstr = "Read BD71837_REG_DEVICE failed";
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to add irq_chip";
> + ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
> + bd71837->chip_irq, IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
> + if (ret < 0)
> + goto err_out;
> +
> + errstr = "Failed to configure button short press timeout";
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG0,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_SHORT_PRESS_10MS);
> + if (ret < 0)
> + goto err_out;
> +
> + /* According to BD71847 datasheet the HW default for long press
> + * detection is 10ms. So lets change it to 10 sec so we can actually
> + * get the short push and allow gracefull shut down
> + */
> + ret = regmap_update_bits(bd71837->regmap,
> + BD71837_REG_PWRONCONFIG1,
> + BD718XX_PWRBTN_PRESS_DURATION_MASK,
> + BD718XX_PWRBTN_LONG_PRESS_10S);
> +
> + errstr = "Failed to configure button long press timeout";
> + if (ret < 0)
> + goto err_out;
> +
> + btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
> + BD71837_INT_PWRBTN_S);
> +
> + errstr = "Failed to get the IRQ";
> + ret = btns[0].irq;
> + if (btns[0].irq < 0)
> + goto err_out;
> +
> + errstr = "Failed to create subdevices";
> + ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
> + bd71837_mfd_cells,
> + ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
> + regmap_irq_get_domain(bd71837->irq_data));
> + if (ret) {
> +err_out:
> + if (errstr)
> + dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
> + }
> +
> + return ret;
> +}
>
> What do you think of this? To my eye it is nice. It keeps single point of
> exit and introduces only simple if-statements without the need of curly
> brackets. And finally the error prints string works as a comment too.
> I've seen bunch of constructs like this on the networking side but I
> have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> is just to illustrate the idea, I did not even try compiling it yet.
That is horrible. I nearly vomited on my keyboard. It doesn't flow
anywhere nearly as nicely has sorting out all of the error handling
*after* it has been detected. You're sacrificing readability to save
a single line and do not save any *actual* lines of code, only a brace.
Landing a goto in the middle of a statement is messy and unsightly.
What happens when you have some resources to free? The last few lines
will become very messy, very quickly.
Nit: "something == NULL" is better written as "!something".
Nit: Please use proper multi-line comments as per the Coding Style.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-07-04 9:52 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <20180704083911.GH2118@localhost.localdomain>
On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > dia dv., 29 de juny 2018 a les 11:47:
> >
> > Now that you use devm calls and you don't need to unwind things I
> > think is better to use plain returns. So,
> >
> > return -ENOMEM;
>
> I have never really understood why use of gotos in error handling is
> discouraged. Personally I would always choose single point of exit from
> a function when it is simple enough to achieve (like in this case). I've
> written and fixed way too many functions which leak resources or
> accidentally keep a lock when exiting from error branches. But I know
> many colleagues like you who prefer not to have gotos but in place returns
> instead. So I guess I'll leave the final call on this to the one who is
> maintainer for this code. And it is true there is no things to unwind
> now - which does not mean that next updater won't add such. But as I
> said, I know plenty of people share your view - and even though I rather
> maintain code with only one exit the final call is on subsystem maintainer
> here.
Actually, If it was completely my call the probe would look something
like this:
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *board_info;
+ int gpio_intr = 0;
+
+ const char *errstr = "No IRQ configured";
+ int ret = -EINVAL;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ board_info = dev_get_platdata(&i2c->dev);
+
+ if (!board_info)
+ gpio_intr = i2c->irq;
+ else
+ gpio_intr = board_info->gpio_intr;
+
+ if (!gpio_intr)
+ goto err_out;
+
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = gpio_intr;
+
+ errstr = "regmap initialization failed";
+
+ bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+ ret = PTR_ERR(bd71837->regmap);
+ if (IS_ERR(bd71837->regmap))
+ goto err_out;
+
+ errstr = "Read BD71837_REG_DEVICE failed";
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to add irq_chip";
+ ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+ bd71837->chip_irq, IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to configure button short press timeout";
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (ret < 0)
+ goto err_out;
+
+ /* According to BD71847 datasheet the HW default for long press
+ * detection is 10ms. So lets change it to 10 sec so we can actually
+ * get the short push and allow gracefull shut down
+ */
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG1,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_LONG_PRESS_10S);
+
+ errstr = "Failed to configure button long press timeout";
+ if (ret < 0)
+ goto err_out;
+
+ btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+ BD71837_INT_PWRBTN_S);
+
+ errstr = "Failed to get the IRQ";
+ ret = btns[0].irq;
+ if (btns[0].irq < 0)
+ goto err_out;
+
+ errstr = "Failed to create subdevices";
+ ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells,
+ ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret) {
+err_out:
+ if (errstr)
+ dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
+ }
+
+ return ret;
+}
What do you think of this? To my eye it is nice. It keeps single point of
exit and introduces only simple if-statements without the need of curly
brackets. And finally the error prints string works as a comment too.
I've seen bunch of constructs like this on the networking side but I
have no idea if this is frowned on this subsystem =) Oh, and probe abowe
is just to illustrate the idea, I did not even try compiling it yet.
Best Regards
Matti Vaittinen
^ permalink raw reply
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
From: Lee Jones @ 2018-07-04 9:21 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh, linux-clk,
devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
In-Reply-To: <20180704084715.GI2118@localhost.localdomain>
On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 08:02:00AM +0100, Lee Jones wrote:
> > On Thu, 21 Jun 2018, Matti Vaittinen wrote:
> >
> > > On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> > > > Patch series adding support for ROHM BD71837 PMIC.
> > > >
> > > What is the preferred way when I send updated patches:
> > >
> > > 1. always resend _all_ unapplied patches even if there is no changes to
> > > some of them. (patch-vN mail thread contains _all_ unapplied patches)
> > > 2. only resend changed patches (patch-vN mail thread contains only
> > > patches that were changed from patch-vN-1)
> > >
> > > I have currently used approach 1 - so that no patches would be
> > > accidentally forgotten - but downside is that people need to check if
> > > they have already reviewed some of the patches. I'd rather not caused
> > > any extra work. What is the most convenient way for you guys?
> >
> > Option 1 is preferred.
> >
> > Just ensure you apply any tags you have collected so reviewers can see
> > which patches are pending a review. It's also a good idea to keep a
> > succinct change log between the "--" marker and the diff stat where
> > you can state "v4: No change" or the like.
>
> Right. Thanks. Just one question - what if I get reviewed-by for a
> patch which I later rework? Like this MFD patch where I got reviewed-by
> from Linus Walleij for v6 - but which I reworked due to comments from
> Enric and Dmitry. I have not kept the reviewed-by as the patch is not
> exactly the same Linus was originally reviewing. I guess the tags should
> be only kept for patches which are unchanged, right?
That is the $64,000 question. The answer is "it depends". You should
use your common sense. Did your re-work taint the code that your
reviewer provided his tag for? If so, drop it. If not, keep it.
There are no hard and fast rules about these kinds of things.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v7 0/4] mfd/regulator/clk/input: bd71837: ROHM BD71837 PMIC driver
From: Matti Vaittinen @ 2018-07-04 8:47 UTC (permalink / raw)
To: Lee Jones
Cc: mturquette, sboyd, robh+dt, mark.rutland, lgirdwood, broonie,
mazziesaccount, arnd, dmitry.torokhov, sre, chenjh,
andrew.smirnov, linus.walleij, kstewart, heiko, gregkh, linux-clk,
devicetree, linux-kernel, linux-input, mikko.mutanen,
heikki.haikola
In-Reply-To: <20180703070200.GC20176@dell>
On Tue, Jul 03, 2018 at 08:02:00AM +0100, Lee Jones wrote:
> On Thu, 21 Jun 2018, Matti Vaittinen wrote:
>
> > On Tue, Jun 19, 2018 at 01:55:31PM +0300, Matti Vaittinen wrote:
> > > Patch series adding support for ROHM BD71837 PMIC.
> > >
> > What is the preferred way when I send updated patches:
> >
> > 1. always resend _all_ unapplied patches even if there is no changes to
> > some of them. (patch-vN mail thread contains _all_ unapplied patches)
> > 2. only resend changed patches (patch-vN mail thread contains only
> > patches that were changed from patch-vN-1)
> >
> > I have currently used approach 1 - so that no patches would be
> > accidentally forgotten - but downside is that people need to check if
> > they have already reviewed some of the patches. I'd rather not caused
> > any extra work. What is the most convenient way for you guys?
>
> Option 1 is preferred.
>
> Just ensure you apply any tags you have collected so reviewers can see
> which patches are pending a review. It's also a good idea to keep a
> succinct change log between the "--" marker and the diff stat where
> you can state "v4: No change" or the like.
Right. Thanks. Just one question - what if I get reviewed-by for a
patch which I later rework? Like this MFD patch where I got reviewed-by
from Linus Walleij for v6 - but which I reworked due to comments from
Enric and Dmitry. I have not kept the reviewed-by as the patch is not
exactly the same Linus was originally reviewing. I guess the tags should
be only kept for patches which are unchanged, right?
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-07-04 8:39 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, Matti Vaittinen, Arnd Bergmann,
Dmitry Torokhov, Sebastian Reichel, chenjh, Andrey Smirnov,
Linus Walleij, Kate Stewart, Heiko Stübner,
Greg Kroah-Hartman, sboyd, linux-clk, devicetree@vger.kernel.org
In-Reply-To: <CAFqH_533te3UuY9V9U+Z0yxTkW==3wsK22uzB4G=aWGour7Quw@mail.gmail.com>
Hello Enric, Lee and others.
Thanks again for taking the time to review the patch! I do appreciate
the effort. Especially because I find reviewing to be quite hard work.
You spotted some obvious things to change but additionally commented on
things which I would rather not change. (Namely the platdata usage and
replacing gotos with in-pklace returns). I would like to get opinion
from Lee on these before implementing the changes. So I will cook new
åatch only after I know what changes are required. Please see my view on
suggested changes below.
On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> One doubt regarding the probe function and few comments.
>
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dv., 29 de juny 2018 a les 11:47:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for three subsystems:
> > - clk
> > - Regulators
> > - input/power-key
> >
> > fix too long lines
>
> I guess that this message is not intended to be here.
Right. That's a leftover from squash commit. Good catch!
>
> > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct bd71837 *bd71837;
> > + struct bd71837_board *board_info;
> > + int ret = -EINVAL;
> > +
> > + board_info = dev_get_platdata(&i2c->dev);
>
> Sorry in advance if I am missing something, but isn't this always NULL?
At the moment, yes. But the idea is that one using this PMIC could
relatively easily get rid of the "depends on OF" if the PMIC is controlled
for example using X86 family chips. So platdata is a mechanism that
could be used to bring in for example the irq information - or perhaps
the chip type when I add support to BD71847. I can remove the platdata
for now if it really bothers - but if it does not, then I would like to
keep it in.
>
> > +
> > + if (!board_info) {
>
> then this check is not required
Yes. But as I said, I would like to keep this so that platdata could be
used for giving the HW information to driver on certain architectures,
But as I said - if this is a problem it can be removed. Please let me
know if platdata usage is a "show stopper" here.
>
> > + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> > + GFP_KERNEL);
> > + if (!board_info) {
> > + ret = -ENOMEM;
> > + goto err_out;
>
> Now that you use devm calls and you don't need to unwind things I
> think is better to use plain returns. So,
>
> return -ENOMEM;
I have never really understood why use of gotos in error handling is
discouraged. Personally I would always choose single point of exit from
a function when it is simple enough to achieve (like in this case). I've
written and fixed way too many functions which leak resources or
accidentally keep a lock when exiting from error branches. But I know
many colleagues like you who prefer not to have gotos but in place returns
instead. So I guess I'll leave the final call on this to the one who is
maintainer for this code. And it is true there is no things to unwind
now - which does not mean that next updater won't add such. But as I
said, I know plenty of people share your view - and even though I rather
maintain code with only one exit the final call is on subsystem maintainer
here.
>
> > + } else if (i2c->irq) {
>
> IMO this else is confusing, also maybe you want to warn about the
> missing interrupt.
Right. The else is completely unnecessary as we have goto in previous
if. Nicely spotted-
>
> if (!i2c->irq) {
> dev_err(&i2c->dev, "No IRQ configured!);
> return -EINVAL;
> }
>
> > + board_info->gpio_intr = i2c->irq;
>
> Is board_info really necessary?
>
As explained before the idea of board info is to be able to provide the
HW description without device-tree. It could be used for example to provide
the regulator information to sub device(s). I have not tested/used this
mechanism as my development setup relies on DT - but I like to keep this
as an option for those who need to work on archs which do not have DT
support.
> > + } else {
> > + ret = -ENOENT;
> > + goto err_out;
> > + }
>
> and remove the else
>
> > + }
> > +
> > + if (!board_info)
> > + goto err_out;
> > +
>
> This is redundant.
Right. We have alloc check abowe and goto error there.
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > + if (bd71837 == NULL)
>
> if (!bd71837)
>
Right.
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = board_info->gpio_intr;
>
> bd71837->chip_irq = i2c->irq;
>
Maybe not if we want to keep the platdata support, right?
> > +
> > + bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
> > + if (IS_ERR(bd71837->regmap)) {
> > + ret = PTR_ERR(bd71837->regmap);
> > + dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
> > + goto err_out;
>
> dev_err(bd71837->dev, "regmap initialization failed: %d\n", ret);
> return PTR_ERR(bd71837->regmap);
>
This goes back to the discussion on whether to prefer single point of
exit or not.
> > + }
> > +
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0) {
> > + dev_err(bd71837->dev,
> > + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
>
> __func__ part is redundant.
>
Indeed. Thanks.
> > + goto err_out;
> return ret;
Rest of the comments can be fixed if we choose to add multpile exit
points. But as I said, I would prefer having only one so let's wait for
another opinion, Ok?
Best Regards,
Matti Vaittinen
^ permalink raw reply
* Re: [PATCH 3/5] mfd: ti_am335x_tscadc: Keep ADC interface on if child is wakeup capable
From: Lee Jones @ 2018-07-04 7:39 UTC (permalink / raw)
To: Vignesh R
Cc: Jonathan Cameron, Dmitry Torokhov, linux-iio, linux-omap,
linux-kernel, linux-input
In-Reply-To: <20180630103318.25355-4-vigneshr@ti.com>
On Sat, 30 Jun 2018, Vignesh R wrote:
> If a child device like touchscreen is wakeup capable, then keep ADC
> interface on, so that a touching resistive screen will generate wakeup
> event to the system.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> drivers/mfd/ti_am335x_tscadc.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 1/5] mfd: ti_am335x_tscadc: Don't mark TSCADC MFD as wakeup capable
From: Lee Jones @ 2018-07-04 7:38 UTC (permalink / raw)
To: Vignesh R
Cc: Jonathan Cameron, Dmitry Torokhov, linux-iio, linux-omap,
linux-kernel, linux-input
In-Reply-To: <20180630103318.25355-2-vigneshr@ti.com>
On Sat, 30 Jun 2018, Vignesh R wrote:
> Currently tscadc MFD is marked as wakeup capable which incorrect
> because, its actually touch event by child TSC device that wakes up the
> system. Therefore, remove device_init_wakeup() call that marks TSCADC
> device as wakeup capable in favor of moving to mark TSC input device as
> wakeup capable later.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> drivers/mfd/ti_am335x_tscadc.c | 1 -
> 1 file changed, 1 deletion(-)
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 09/10] mfd: 88pm860x-i2c: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
From: Lee Jones @ 2018-07-04 7:04 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
Arnd Bergmann, Greg Kroah-Hartman, Brian Norris, Gregory Fong,
Florian Fainelli, bcm-kernel-feedback-list, Sekhar Nori,
Kevin Hilman, Haavard Skinnemoen, Kukjin Kim, Krzysztof Kozlowski,
Orson Zhai, Baolin Wang, Chunyan Zhang, Wolfram Sang,
Guenter Roeck
In-Reply-To: <20180620051803.12206-10-peda@axentia.se>
On Wed, 20 Jun 2018, Peter Rosin wrote:
> Locking the root adapter for __i2c_transfer will deadlock if the
> device sits behind a mux-locked I2C mux. Switch to the finer-grained
> i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> sit behind a mux-locked mux, the two locking variants are equivalent.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/mfd/88pm860x-i2c.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
If Wolfram is happy with this, then I am.
Since this file sees few changes - please merge through the I2C tree.
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
From: Baoquan He @ 2018-07-04 4:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Nicolas Pitre, brijesh.singh-5C7GfCeVMHo, devicetree,
David Airlie, linux-pci-u79uwXL29TY76Z2rM5mHXA,
richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w, Max Filippov,
Paul Mackerras, baiyaowei-0p4V/sDNsUmm0O/7XYngnFaTQe2KTcn/,
KY Srinivasan, Frank Rowand, Lorenzo Pieralisi, Stephen Hemminger,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Michael Ellerman,
Patrik Jakobsson, linux-input, Gustavo Padovan, Borislav Petkov,
Dave Young, Tom Lendacky, Haiyang Zhang, Maarten Lankhorst,
Josh Triplett
In-Reply-To: <CAHp75VeEjhcYF4Fy4O_srdv_nEGGjGwAHhZufscB_tiLQVQ=FQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 07/03/18 at 11:57pm, Andy Shevchenko wrote:
> On Tue, Jul 3, 2018 at 5:55 PM, Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On 06/12/18 at 05:24pm, Andy Shevchenko wrote:
> >> On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko
> >> <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> >> > I briefly looked at the code and error codes we have, so, my proposal
> >> > is one of the following
> >>
> >> > - use -ECANCELED (not the best choice for first occurrence here,
> >> > though I can't find better)
> >>
> >> Actually -ENOTSUPP might suit the first case (although the actual
> >> would be something like -EOVERLAP, which we don't have)
> >
> > Sorry for late reply, and many thanks for your great suggestion.
> >
>
> > I am fine to use -ENOTSUPP as the first returned value, and -ECANCELED
> > for the 2nd one.
>
> I have no strong opinion, but I like (slightly better) this approach ^^^
Done, post v6 in this way, many thanks.
>
> > Or define an enum as you suggested inside the function
> > or in header file.
>
> >
> > Or use -EBUSY for the first case because existing resource is
> > overlapping but not fully contained by 'res'; and -EINVAL for
> > the 2nd case since didn't find any one resources which is contained by
> > 'res', means we passed in a invalid resource.
> >
> > All is fine to me, I can repost with each of them.
>
> >> > - use positive integers (or enum), like
> >> > #define RES_REPARENTED 0
> >> > #define RES_OVERLAPPED 1
> >> > #define RES_NOCONFLICT 2
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox