* Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
From: Jarkko Sakkinen @ 2018-06-25 10:24 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Peter Huewe, 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, Crt Mori,
Jonathan Cameron
In-Reply-To: <20180620051803.12206-2-peda@axentia.se>
On Wed, Jun 20, 2018 at 07:17:54AM +0200, 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>
Studied enough so that I can give
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Do not have hardware to test this, however.
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 0/2] Add another regulator to i2c-hid
From: Jiri Kosina @ 2018-06-25 13:14 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-kernel, linux-input, Benjamin Tissoires, Hans de Goede,
Andy Shevchenko, Dmitry Torokhov, Doug Anderson
In-Reply-To: <20180622022717.134300-1-swboyd@chromium.org>
Applied both to for-4.19/i2c-hid. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] HID: hid-ntrig: add error handling for sysfs_create_group
From: Jiri Kosina @ 2018-06-25 13:16 UTC (permalink / raw)
To: Zhouyang Jia; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <1528983437-52903-1-git-send-email-jiazhouyang09@gmail.com>
On Thu, 14 Jun 2018, Zhouyang Jia wrote:
> When sysfs_create_group fails, the lack of error-handling code may
> cause unexpected results.
>
> This patch adds error-handling code after calling sysfs_create_group.
>
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
> v1->v2:
> - Remove goto statement.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] Patch enabling Fn function keys for Laptop Asus G730 (keyboard 0x1869)
From: Jiri Kosina @ 2018-06-25 13:20 UTC (permalink / raw)
To: vereecke.jan; +Cc: linux-kernel, linux-input, benjamin.tissoires
In-Reply-To: <alpine.LNX.2.21.1806141112440.11126@Hermes.vereecke.mooo.com>
On Thu, 14 Jun 2018, vereecke.jan@skynet.be wrote:
> Attached is a patch enabling (most of) the Fn function keys for the Asus ROG
> Strix GL730 Laptop. The keyboard identifies as a USB device 0x1869.
>
> This patch enables:
> volume/mute keys, ROG key, rew/play/ff,
> monitor and keyboard brightness keys,
> Touchpad toggle key,
> Calculator key
>
> 'Touchpad on/off' Key returns keycode 33, which is mapped to the 'p' key.
> 'Display on/off' returns keycode 253, which is mapped in X11 to NoSymbol
> 'Airplane Key' returns keycode 255, which is mapped in X11 to NoSymbol
>
> The following keys are not activated yet:
> Microphone Mute key, Fan key, Display Switch key
Thanks a lot for the patch.
> This patch is against the 4.17-rc7 kernel, for the x86 architecture
This usually doesn't go to the changelog, it'd be unnecessary polution of
the set-in-stone git changelog.
> diffstat for this patch is:
> hid-asus.c | 2 ++
> hid-ids.h | 1 +
> hid-quirks.c | 1 +
> 3 files changed, 4 insertions(+)
>
> To apply the patch, in the root of the kernel tree use:
> patch -p1 < AsusGL703-Keyboard-Fn-keys.patch
Neither does any of this.
What is OTOH missing is your signoff.
Could you please fix these up and resubmit (ideally with Subject: line
also tailored to fit the subsystem customs, IOW something like "HID: asus:
add support for ....").
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: redragon: fix num lock and caps lock LEDs
From: Jiri Kosina @ 2018-06-25 13:23 UTC (permalink / raw)
Cc: Benjamin Tissoires, linux-input, linux-kernel, Robert Munteanu
In-Reply-To: <20180619082040.30825-1-rombert@apache.org>
On Tue, 19 Jun 2018, Robert@suse.de wrote:
> From: Robert Munteanu <rombert@apache.org>
>
> The redragon asura keyboard registers two input devices. The initial
> commit d44e2fadf940 ("HID: redragon: Fix modifier keys for Redragon Asura Keyboard")
This should actually have been 85455dd906d.
I have fixed that, added Fixes: tag, and applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: core: allow concurrent registration of drivers
From: Jiri Kosina @ 2018-06-25 13:30 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Mario.Limonciello, linux-input, linux-kernel
In-Reply-To: <20180531114929.30545-1-benjamin.tissoires@redhat.com>
On Thu, 31 May 2018, Benjamin Tissoires wrote:
> Detected on the Dell XPS 9365.
> The laptop has 2 devices that benefit from the hid-generic auto-unbinding.
> When those 2 devices are presented to the userspace, udev loads both
> wacom and hid-multitouch. When this happens, the code in
> __hid_bus_reprobe_drivers() is called concurrently and the second device
> gets reprobed twice.
> An other bug in the power_supply subsystem prevent to remove the wacom
> driver if it just finished its initialization, which basically kills
> the wacom node.
>
> Fixes c17a7476e4c4 ("HID: core: rewrite the hid-generic automatic unbind")
>
> Cc: stable@vger.kernel.org # v4.17
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Now staged in for-4.18/upstream-fixes_v2, with the plan to send it to
Linus this week.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/7] Input: psmouse - clean up code
From: Jiri Slaby @ 2018-06-25 18:35 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires, Hans de Goede, Lyude Paul
Cc: linux-input, linux-kernel
In-Reply-To: <20180119230629.49428-3-dmitry.torokhov@gmail.com>
On 01/20/2018, 12:06 AM, Dmitry Torokhov wrote:
> - switch to using BIT() macros
> - use u8 instead of unsigned char for byte data
> - use input_set_capability() instead of manipulating capabilities bits
> directly
> - use sign_extend32() when extracting wheel data.
> - do not abuse -1 as error code, propagate errors from various calls.
…
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
…
> @@ -157,39 +159,42 @@ psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
…
> case 0x00:
> case 0xC0:
> - input_report_rel(dev, REL_WHEEL, (int) (packet[3] & 8) - (int) (packet[3] & 7));
> - input_report_key(dev, BTN_SIDE, (packet[3] >> 4) & 1);
> - input_report_key(dev, BTN_EXTRA, (packet[3] >> 5) & 1);
> + input_report_rel(dev, REL_WHEEL,
> + -sign_extend32(packet[3], 3));
> + input_report_key(dev, BTN_SIDE, BIT(4));
> + input_report_key(dev, BTN_EXTRA, BIT(5));
> break;
> }
> break;
>
> case PSMOUSE_GENPS:
> /* Report scroll buttons on NetMice */
> - input_report_rel(dev, REL_WHEEL, -(signed char) packet[3]);
> + input_report_rel(dev, REL_WHEEL, -(s8) packet[3]);
>
> /* Extra buttons on Genius NewNet 3D */
> - input_report_key(dev, BTN_SIDE, (packet[0] >> 6) & 1);
> - input_report_key(dev, BTN_EXTRA, (packet[0] >> 7) & 1);
> + input_report_key(dev, BTN_SIDE, BIT(6));
> + input_report_key(dev, BTN_EXTRA, BIT(7));
> break;
>
> case PSMOUSE_THINKPS:
> /* Extra button on ThinkingMouse */
> - input_report_key(dev, BTN_EXTRA, (packet[0] >> 3) & 1);
> + input_report_key(dev, BTN_EXTRA, BIT(3));
While hunting a 4.17 bug where some openSUSE users lost their ability to
mouse-click, I came across this commit. Putting aside it's a total mess
of multiple changes, were these changes above intentional at all? I mean
changing
(packet[0] >> 3) & 1
to hardwired
BIT(3)
does not look quite right. And if it is for some to me unknown reason,
it should have been properly documented in the commit log. I believe
your intent was:
packet[0] & BIT(3)
?
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH 2/7] Input: psmouse - clean up code
From: Dmitry Torokhov @ 2018-06-25 18:52 UTC (permalink / raw)
To: Jiri Slaby
Cc: Benjamin Tissoires, Hans de Goede, Lyude Paul, linux-input,
linux-kernel
In-Reply-To: <e8ad7e61-342a-8d53-7de1-f30a58c91975@suse.cz>
On Mon, Jun 25, 2018 at 08:35:14PM +0200, Jiri Slaby wrote:
> On 01/20/2018, 12:06 AM, Dmitry Torokhov wrote:
> > - switch to using BIT() macros
> > - use u8 instead of unsigned char for byte data
> > - use input_set_capability() instead of manipulating capabilities bits
> > directly
> > - use sign_extend32() when extracting wheel data.
> > - do not abuse -1 as error code, propagate errors from various calls.
> …
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> …
> > @@ -157,39 +159,42 @@ psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
> …
> > case 0x00:
> > case 0xC0:
> > - input_report_rel(dev, REL_WHEEL, (int) (packet[3] & 8) - (int) (packet[3] & 7));
> > - input_report_key(dev, BTN_SIDE, (packet[3] >> 4) & 1);
> > - input_report_key(dev, BTN_EXTRA, (packet[3] >> 5) & 1);
> > + input_report_rel(dev, REL_WHEEL,
> > + -sign_extend32(packet[3], 3));
> > + input_report_key(dev, BTN_SIDE, BIT(4));
> > + input_report_key(dev, BTN_EXTRA, BIT(5));
> > break;
> > }
> > break;
> >
> > case PSMOUSE_GENPS:
> > /* Report scroll buttons on NetMice */
> > - input_report_rel(dev, REL_WHEEL, -(signed char) packet[3]);
> > + input_report_rel(dev, REL_WHEEL, -(s8) packet[3]);
> >
> > /* Extra buttons on Genius NewNet 3D */
> > - input_report_key(dev, BTN_SIDE, (packet[0] >> 6) & 1);
> > - input_report_key(dev, BTN_EXTRA, (packet[0] >> 7) & 1);
> > + input_report_key(dev, BTN_SIDE, BIT(6));
> > + input_report_key(dev, BTN_EXTRA, BIT(7));
> > break;
> >
> > case PSMOUSE_THINKPS:
> > /* Extra button on ThinkingMouse */
> > - input_report_key(dev, BTN_EXTRA, (packet[0] >> 3) & 1);
> > + input_report_key(dev, BTN_EXTRA, BIT(3));
>
> While hunting a 4.17 bug where some openSUSE users lost their ability to
> mouse-click, I came across this commit. Putting aside it's a total mess
> of multiple changes, were these changes above intentional at all? I mean
> changing
> (packet[0] >> 3) & 1
> to hardwired
> BIT(3)
> does not look quite right. And if it is for some to me unknown reason,
> it should have been properly documented in the commit log. I believe
> your intent was:
> packet[0] & BIT(3)
> ?
Yes, that was, I am not sure what I was thinking; I'll fix that up.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: psmouse - fix button reporting for basic protocols
From: Dmitry Torokhov @ 2018-06-25 19:09 UTC (permalink / raw)
To: linux-input, Jiri Slaby; +Cc: Benjamin Tissoires, linux-kernel
The commit ba667650c568 ("Input: psmouse - clean up code") was pretty
brain-dead and broke extra buttons reporting for variety of PS/2 mice:
Genius, Thinkmouse and Intellimouse Explorer. We need to actually inspect
the data coming from the device when reporting events.
Fixes: ba667650c568 ("Input: psmouse - clean up code")
Reported-by: Jiri Slaby <jslaby@suse.cz>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/mouse/psmouse-base.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index 5ff5b1952be0c..d3ff1fc09af71 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -192,8 +192,8 @@ psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
else
input_report_rel(dev, REL_WHEEL, -wheel);
- input_report_key(dev, BTN_SIDE, BIT(4));
- input_report_key(dev, BTN_EXTRA, BIT(5));
+ input_report_key(dev, BTN_SIDE, packet[3] & BIT(4));
+ input_report_key(dev, BTN_EXTRA, packet[3] & BIT(5));
break;
}
break;
@@ -203,13 +203,13 @@ psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
input_report_rel(dev, REL_WHEEL, -(s8) packet[3]);
/* Extra buttons on Genius NewNet 3D */
- input_report_key(dev, BTN_SIDE, BIT(6));
- input_report_key(dev, BTN_EXTRA, BIT(7));
+ input_report_key(dev, BTN_SIDE, packet[0] & BIT(6));
+ input_report_key(dev, BTN_EXTRA, packet[0] & BIT(7));
break;
case PSMOUSE_THINKPS:
/* Extra button on ThinkingMouse */
- input_report_key(dev, BTN_EXTRA, BIT(3));
+ input_report_key(dev, BTN_EXTRA, packet[0] & BIT(3));
/*
* Without this bit of weirdness moving up gives wildly
@@ -223,7 +223,7 @@ psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
* Cortron PS2 Trackball reports SIDE button in the
* 4th bit of the first byte.
*/
- input_report_key(dev, BTN_SIDE, BIT(3));
+ input_report_key(dev, BTN_SIDE, packet[0] & BIT(3));
packet[0] |= BIT(3);
break;
--
2.18.0.rc2.346.g013aa6912e-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code
From: Joe Perches @ 2018-06-26 2:10 UTC (permalink / raw)
To: Chengguang Xu, jakub.kicinski, davem, LKML, Julia Lawall, cocci
Cc: oss-drivers, netdev, Dmitry Torokhov, linux-input, linux-s390
In-Reply-To: <20180626011631.22717-1-cgxu519@gmx.com>
On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> sizeof() will return unsigned value so in the error check
> negative error code will be always larger than sizeof().
This looks like a general class of error in the kernel
where a signed result that could be returning a -errno
is tested against < or <= sizeof()
A couple examples:
drivers/input/mouse/elan_i2c_smbus.c:
len = i2c_smbus_read_block_data(client,
ETP_SMBUS_IAP_PASSWORD_READ,
val);
if (len < sizeof(u16)) {
i2c_smbus_read_block_data can return a negative errno
net/smc/smc_clc.c:
len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
sizeof(struct smc_clc_msg_decline));
if (len < sizeof(struct smc_clc_msg_decline))
where kernel_sendmsg can return a negative errno
There are probably others, I didn't look hard.
Perhaps a cocci script to find these could be generated?
^ permalink raw reply
* Re: [PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment
From: Wolfram Sang @ 2018-06-26 2:37 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, Guenter Roeck, Crt Mori
In-Reply-To: <20180620051803.12206-1-peda@axentia.se>
On Wed, Jun 20, 2018 at 07:17:53AM +0200, Peter Rosin wrote:
> Hi!
>
> With the introduction of mux-locked I2C muxes, the concept of
> locking only a segment of the I2C adapter tree was added. At the
> time, I did not want to cause a lot of extra churn, so left most
> users of i2c_lock_adapter alone and apparently didn't think enough
> about it; they simply continued to lock the whole adapter tree.
> However, i2c_lock_adapter is in fact wrong for almost every caller
> (there is naturally an exception) that is itself not a driver for
> a root adapter. What normal drivers generally want is to only
> lock the segment of the adapter tree that their device sits on.
>
> In fact, if a device sits behind a mux-locked I2C mux, and its
> driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
> things will deadlock (since even a mux-locked I2C adapter will lock
> its parent at some point). If the device is not sitting behind a
> mux-locked I2C mux (i.e. either directly on the root adapter or
> behind a (chain of) parent-locked I2C muxes) the root/segment
> distinction is of no consequence; the root adapter is locked either
> way.
>
> Mux-locked I2C muxes are probably not that common, and putting any
> of the affected devices behind one is probably even rarer, which
> is why we have not seen any deadlocks. At least not that I know
> of...
>
> Since silently changing the semantics of i2c_lock_adapter might
> be quite a surprise, especially for out-of-tree users, this series
> instead removes the function and forces all users to explicitly
> name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
> i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
> bit more wordy, but open-coding I2C locking from random drivers
> should be avoided, so it's perhaps a good thing if it doesn't look
> too neat?
>
> I suggest that Wolfram takes this series through the I2C tree and
> creates an immutable branch for the other subsystems. The series
> is based on v4.18-r1.
Applied to a seperate branch named "i2c/precise-locking-names" which I
will merge into for-next, so it will get proper testing already. Once we
get the missing acks from media, MFD, and IIO maintainers, I will merge
it into for-4.19.
Thank you, Peter!
^ permalink raw reply
* Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code
From: Julia Lawall @ 2018-06-26 7:21 UTC (permalink / raw)
To: Joe Perches
Cc: Chengguang Xu, jakub.kicinski, davem, LKML, cocci, oss-drivers,
netdev, Dmitry Torokhov, linux-input, linux-s390
In-Reply-To: <ff0b2834b771a467246b9f7c1b4600c38f5bb463.camel@perches.com>
On Mon, 25 Jun 2018, Joe Perches wrote:
> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> > sizeof() will return unsigned value so in the error check
> > negative error code will be always larger than sizeof().
>
> This looks like a general class of error in the kernel
> where a signed result that could be returning a -errno
> is tested against < or <= sizeof()
>
> A couple examples:
>
> drivers/input/mouse/elan_i2c_smbus.c:
>
> len = i2c_smbus_read_block_data(client,
> ETP_SMBUS_IAP_PASSWORD_READ,
> val);
> if (len < sizeof(u16)) {
>
> i2c_smbus_read_block_data can return a negative errno
>
>
> net/smc/smc_clc.c:
>
> len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> sizeof(struct smc_clc_msg_decline));
> if (len < sizeof(struct smc_clc_msg_decline))
>
> where kernel_sendmsg can return a negative errno
>
> There are probably others, I didn't look hard.
>
> Perhaps a cocci script to find these could be generated?
Currently there is a rule for comparison of unsigneds to 0. It would be
reasonable to extend it for sizes. I will see what it gives.
julia
^ permalink raw reply
* Re: [PATCH v6 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Linus Walleij @ 2018-06-26 7:54 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
Lee Jones, Liam Girdwood, Mark Brown, mazziesaccount,
Arnd Bergmann, Dmitry Torokhov, Sebastian Reichel, Jianhong Chen,
Andrey Smirnov, Kate Stewart, Heiko Stübner, Greg KH,
linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <e6b215db77c8203704c9d1216a9e8747de1b5779.1529067662.git.matti.vaittinen@fi.rohmeurope.com>
On Fri, Jun 15, 2018 at 3:29 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for two subsystems:
> - clk
> - Regulators
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
This looks like a clean and nice MFD core driver to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code
From: Julia Lawall @ 2018-06-26 8:06 UTC (permalink / raw)
To: Joe Perches
Cc: linux-s390, jakub.kicinski, Chengguang Xu, netdev,
Dmitry Torokhov, LKML, davem, oss-drivers, linux-input, cocci
In-Reply-To: <ff0b2834b771a467246b9f7c1b4600c38f5bb463.camel@perches.com>
On Mon, 25 Jun 2018, Joe Perches wrote:
> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> > sizeof() will return unsigned value so in the error check
> > negative error code will be always larger than sizeof().
>
> This looks like a general class of error in the kernel
> where a signed result that could be returning a -errno
> is tested against < or <= sizeof()
>
> A couple examples:
>
> drivers/input/mouse/elan_i2c_smbus.c:
>
> len = i2c_smbus_read_block_data(client,
> ETP_SMBUS_IAP_PASSWORD_READ,
> val);
> if (len < sizeof(u16)) {
>
> i2c_smbus_read_block_data can return a negative errno
>
>
> net/smc/smc_clc.c:
>
> len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> sizeof(struct smc_clc_msg_decline));
> if (len < sizeof(struct smc_clc_msg_decline))
>
> where kernel_sendmsg can return a negative errno
>
> There are probably others, I didn't look hard.
>
> Perhaps a cocci script to find these could be generated?
Here's another one:
drivers/usb/serial/ir-usb.c
@@ -126,13 +126,8 @@ irda_usb_find_class_desc(struct usb_seri
if (!desc)
return NULL;
- ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
- USB_REQ_CS_IRDA_GET_CLASS_DESC,
- USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
- 0, ifnum, desc, sizeof(*desc), 1000);
dev_dbg(&serial->dev->dev, "%s - ret=%d\n", __func__, ret);
- if (ret < sizeof(*desc)) {
dev_dbg(&serial->dev->dev,
"%s - class descriptor read %s (%d)\n", __func__,
(ret < 0) ? "failed" : "too short", ret);
There are other results, but I haven't checked all of them.
julia
^ permalink raw reply
* Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code
From: cgxu519 @ 2018-06-26 8:21 UTC (permalink / raw)
To: Julia Lawall
Cc: Joe Perches, jakub.kicinski, davem, LKML, cocci, oss-drivers,
netdev, Dmitry Torokhov, linux-input, linux-s390
In-Reply-To: <alpine.DEB.2.20.1806261005190.4512@hadrien>
On 06/26/2018 04:06 PM, Julia Lawall wrote:
>
> On Mon, 25 Jun 2018, Joe Perches wrote:
>
>> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
>>> sizeof() will return unsigned value so in the error check
>>> negative error code will be always larger than sizeof().
>> This looks like a general class of error in the kernel
>> where a signed result that could be returning a -errno
>> is tested against < or <= sizeof()
>>
>> A couple examples:
>>
>> drivers/input/mouse/elan_i2c_smbus.c:
>>
>> len = i2c_smbus_read_block_data(client,
>> ETP_SMBUS_IAP_PASSWORD_READ,
>> val);
>> if (len < sizeof(u16)) {
>>
>> i2c_smbus_read_block_data can return a negative errno
>>
>>
>> net/smc/smc_clc.c:
>>
>> len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
>> sizeof(struct smc_clc_msg_decline));
>> if (len < sizeof(struct smc_clc_msg_decline))
>>
>> where kernel_sendmsg can return a negative errno
>>
>> There are probably others, I didn't look hard.
>>
>> Perhaps a cocci script to find these could be generated?
> Here's another one:
>
> drivers/usb/serial/ir-usb.c
> @@ -126,13 +126,8 @@ irda_usb_find_class_desc(struct usb_seri
> if (!desc)
> return NULL;
>
> - ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
> - USB_REQ_CS_IRDA_GET_CLASS_DESC,
> - USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> - 0, ifnum, desc, sizeof(*desc), 1000);
>
> dev_dbg(&serial->dev->dev, "%s - ret=%d\n", __func__, ret);
> - if (ret < sizeof(*desc)) {
> dev_dbg(&serial->dev->dev,
> "%s - class descriptor read %s (%d)\n", __func__,
> (ret < 0) ? "failed" : "too short", ret);
>
> There are other results, but I haven't checked all of them.
Hi Julia,
Thanks for your check. I posted a patch yesterday to fix three places in
usb subsystem
and the patch is just in queue now, so you can skip these places.
The detail of patch.
---
diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 24b06c7e5e2d..7643716b5299 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -132,7 +132,7 @@ irda_usb_find_class_desc(struct usb_serial *serial, unsigned int ifnum)
0, ifnum, desc, sizeof(*desc), 1000);
dev_dbg(&serial->dev->dev, "%s - ret=%d\n", __func__, ret);
- if (ret < sizeof(*desc)) {
+ if (ret < (int)sizeof(*desc)) {
dev_dbg(&serial->dev->dev,
"%s - class descriptor read %s (%d)\n", __func__,
(ret < 0) ? "failed" : "too short", ret);
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index 958e12e1e7c7..ff2322ea5e14 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -194,7 +194,7 @@ static inline int qt2_getregister(struct usb_device *dev,
ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
QT_SET_GET_REGISTER, 0xc0, reg,
uart, data, sizeof(*data), QT2_USB_TIMEOUT);
- if (ret < sizeof(*data)) {
+ if (ret < (int)sizeof(*data)) {
if (ret >= 0)
ret = -EIO;
}
diff --git a/drivers/usb/serial/ssu100.c b/drivers/usb/serial/ssu100.c
index 2083c267787b..0900b47b5f57 100644
--- a/drivers/usb/serial/ssu100.c
+++ b/drivers/usb/serial/ssu100.c
@@ -104,7 +104,7 @@ static inline int ssu100_getregister(struct usb_device *dev,
ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
QT_SET_GET_REGISTER, 0xc0, reg,
uart, data, sizeof(*data), 300);
- if (ret < sizeof(*data)) {
+ if (ret < (int)sizeof(*data)) {
if (ret >= 0)
ret = -EIO;
}
---
Thanks,
Chengguang.
^ permalink raw reply related
* Re: [PATCH v2 10/10] i2c: remove i2c_lock_adapter and use i2c_lock_bus directly
From: Jonathan Cameron @ 2018-06-26 8:28 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, Crt
In-Reply-To: <20180620051803.12206-11-peda@axentia.se>
On Wed, 20 Jun 2018 07:18:03 +0200
Peter Rosin <peda@axentia.se> wrote:
> The i2c_lock_adapter name is ambiguous since it is unclear if it
> refers to the root adapter or the adapter you name in the argument.
> The natural interpretation is the adapter you name in the argument,
> but there are historical reasons for that not being the case; it
> in fact locks the root adapter. Just remove the function and force
> users to spell out the I2C_LOCK_ROOT_ADAPTER name to indicate what
> is really going on. Also remove i2c_unlock_adapter, of course.
>
> This patch was generated with
>
> git grep -l 'i2c_\(un\)\?lock_adapter' \
> | xargs sed -i 's/i2c_\(un\)\?lock_adapter(\([^)]*\))/'\
> 'i2c_\1lock_bus(\2, I2C_LOCK_ROOT_ADAPTER)/g'
>
> followed by white-space touch-up.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
For IIO: Acked-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> drivers/i2c/busses/i2c-brcmstb.c | 8 ++++----
> drivers/i2c/busses/i2c-davinci.c | 4 ++--
> drivers/i2c/busses/i2c-gpio.c | 40 +++++++++++++++++++-------------------
> drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> drivers/i2c/busses/i2c-sprd.c | 8 ++++----
> drivers/i2c/i2c-core-slave.c | 8 ++++----
> drivers/iio/temperature/mlx90614.c | 4 ++--
> include/linux/i2c.h | 12 ------------
> 8 files changed, 38 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
> index 78792b4d6437..826d32049996 100644
> --- a/drivers/i2c/busses/i2c-brcmstb.c
> +++ b/drivers/i2c/busses/i2c-brcmstb.c
> @@ -689,9 +689,9 @@ static int brcmstb_i2c_suspend(struct device *dev)
> {
> struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_adapter(&i2c_dev->adapter);
> + i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = true;
> - i2c_unlock_adapter(&i2c_dev->adapter);
> + i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> @@ -700,10 +700,10 @@ static int brcmstb_i2c_resume(struct device *dev)
> {
> struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_adapter(&i2c_dev->adapter);
> + i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
> i2c_dev->is_suspended = false;
> - i2c_unlock_adapter(&i2c_dev->adapter);
> + i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 75d6ab177055..d945a2654c2f 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -714,14 +714,14 @@ static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
>
> dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
>
> - i2c_lock_adapter(&dev->adapter);
> + i2c_lock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> if (val == CPUFREQ_PRECHANGE) {
> davinci_i2c_reset_ctrl(dev, 0);
> } else if (val == CPUFREQ_POSTCHANGE) {
> i2c_davinci_calc_clk_dividers(dev);
> davinci_i2c_reset_ctrl(dev, 1);
> }
> - i2c_unlock_adapter(&dev->adapter);
> + i2c_unlock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index 005e6e0330c2..9d63337efa82 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -78,24 +78,24 @@ static struct dentry *i2c_gpio_debug_dir;
> #define getscl(bd) ((bd)->getscl((bd)->data))
>
> #define WIRE_ATTRIBUTE(wire) \
> -static int fops_##wire##_get(void *data, u64 *val) \
> -{ \
> - struct i2c_gpio_private_data *priv = data; \
> - \
> - i2c_lock_adapter(&priv->adap); \
> - *val = get##wire(&priv->bit_data); \
> - i2c_unlock_adapter(&priv->adap); \
> - return 0; \
> -} \
> -static int fops_##wire##_set(void *data, u64 val) \
> -{ \
> - struct i2c_gpio_private_data *priv = data; \
> - \
> - i2c_lock_adapter(&priv->adap); \
> - set##wire(&priv->bit_data, val); \
> - i2c_unlock_adapter(&priv->adap); \
> - return 0; \
> -} \
> +static int fops_##wire##_get(void *data, u64 *val) \
> +{ \
> + struct i2c_gpio_private_data *priv = data; \
> + \
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + *val = get##wire(&priv->bit_data); \
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + return 0; \
> +} \
> +static int fops_##wire##_set(void *data, u64 val) \
> +{ \
> + struct i2c_gpio_private_data *priv = data; \
> + \
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + set##wire(&priv->bit_data, val); \
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + return 0; \
> +} \
> DEFINE_DEBUGFS_ATTRIBUTE(fops_##wire, fops_##wire##_get, fops_##wire##_set, "%llu\n")
>
> WIRE_ATTRIBUTE(scl);
> @@ -113,7 +113,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
> /* ADDR (7 bit) + RD (1 bit) + SDA hi (1 bit) */
> pattern = (addr << 2) | 3;
>
> - i2c_lock_adapter(&priv->adap);
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
>
> /* START condition */
> setsda(bit_data, 0);
> @@ -129,7 +129,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
> udelay(bit_data->udelay);
> }
>
> - i2c_unlock_adapter(&priv->adap);
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 9fe2b6951895..2f2e28d60ef5 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -919,9 +919,9 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
>
> if ((val == CPUFREQ_POSTCHANGE && delta_f < 0) ||
> (val == CPUFREQ_PRECHANGE && delta_f > 0)) {
> - i2c_lock_adapter(&i2c->adap);
> + i2c_lock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
> ret = s3c24xx_i2c_clockrate(i2c, &got);
> - i2c_unlock_adapter(&i2c->adap);
> + i2c_unlock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret < 0)
> dev_err(i2c->dev, "cannot find frequency (%d)\n", ret);
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 4053259bccb8..a94e724f51dc 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -590,9 +590,9 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
>
> - i2c_lock_adapter(&i2c_dev->adap);
> + i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = true;
> - i2c_unlock_adapter(&i2c_dev->adap);
> + i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return pm_runtime_force_suspend(pdev);
> }
> @@ -601,9 +601,9 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
>
> - i2c_lock_adapter(&i2c_dev->adap);
> + i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = false;
> - i2c_unlock_adapter(&i2c_dev->adap);
> + i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return pm_runtime_force_resume(pdev);
> }
> diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
> index 4a78c65e9971..47a9f70a24a9 100644
> --- a/drivers/i2c/i2c-core-slave.c
> +++ b/drivers/i2c/i2c-core-slave.c
> @@ -47,9 +47,9 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
>
> client->slave_cb = slave_cb;
>
> - i2c_lock_adapter(client->adapter);
> + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
> ret = client->adapter->algo->reg_slave(client);
> - i2c_unlock_adapter(client->adapter);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret) {
> client->slave_cb = NULL;
> @@ -69,9 +69,9 @@ int i2c_slave_unregister(struct i2c_client *client)
> return -EOPNOTSUPP;
> }
>
> - i2c_lock_adapter(client->adapter);
> + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
> ret = client->adapter->algo->unreg_slave(client);
> - i2c_unlock_adapter(client->adapter);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret == 0)
> client->slave_cb = NULL;
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index d619e8634a00..13a4cec64ea8 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -433,11 +433,11 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
>
> dev_dbg(&data->client->dev, "Requesting wake-up");
>
> - i2c_lock_adapter(data->client->adapter);
> + i2c_lock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
> gpiod_direction_output(data->wakeup_gpio, 0);
> msleep(MLX90614_TIMING_WAKEUP);
> gpiod_direction_input(data->wakeup_gpio);
> - i2c_unlock_adapter(data->client->adapter);
> + i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> data->ready_timestamp = jiffies +
> msecs_to_jiffies(MLX90614_TIMING_STARTUP);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 254cd34eeae2..795e3a860afe 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -754,18 +754,6 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
> adapter->lock_ops->unlock_bus(adapter, flags);
> }
>
> -static inline void
> -i2c_lock_adapter(struct i2c_adapter *adapter)
> -{
> - i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
> -}
> -
> -static inline void
> -i2c_unlock_adapter(struct i2c_adapter *adapter)
> -{
> - i2c_unlock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
> -}
> -
> /*flags for the client struct: */
> #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */
> #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Enric Balletbo Serra @ 2018-06-26 9:06 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, 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: <7e1888375c979accc402e9ddd7316e528b2ac52c.1529404894.git.matti.vaittinen@fi.rohmeurope.com>
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:
>
> ROHM BD71837 PMIC MFD driver providing interrupts and support
> for two subsystems:
> - clk
> - Regulators
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 602 insertions(+)
> create mode 100644 drivers/mfd/bd71837.c
> create mode 100644 include/linux/mfd/bd71837.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..7aa05fc9ed8e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1787,6 +1787,19 @@ config MFD_STW481X
> in various ST Microelectronics and ST-Ericsson embedded
> Nomadik series.
>
> +config MFD_BD71837
> + bool "BD71837 Power Management chip"
I know that some drivers need to be built-in, is this really a
requirement for this driver? Or should work as a module too.
> + depends on I2C=y
> + depends on OF
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Select this option to get support for the ROHM BD71837
> + Power Management chips. BD71837 is designed to power processors like
> + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
> + and emergency shut down as well as 32,768KHz clock output.
> +
> config MFD_STM32_LPTIMER
> tristate "Support for STM32 Low-Power Timer"
> depends on (ARCH_STM32 && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e9fd20dba18d..09dc9eb3782c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> +obj-$(CONFIG_MFD_BD71837) += bd71837.o
>
> diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> new file mode 100644
> index 000000000000..0f0361d6cad6
> --- /dev/null
> +++ b/drivers/mfd/bd71837.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
There is a mismatch between what SPDX says and MODULE_LICENSE says.
GPL-2.0 = GPL v2 only
MODULE_LICENSE(GPL) = GPL v2 or later.
You'd like to change the SPDX identifier to GPL-2.0-or-later or set
module license to "GPL v2".
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c -- ROHM BD71837MWV mfd driver
> +//
> +// Datasheet available from
> +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
This include is not required.
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
ditto
> +#include <linux/irqdomain.h>
ditto
> +#include <linux/gpio.h>
ditto
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
ditto
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/bd71837.h>
> +
Please review the needed includes.
> +static const struct resource irqs[] = {
> + {
> + .start = BD71837_INT_PWRBTN,
> + .end = BD71837_INT_PWRBTN,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn",
> + }, {
> + .start = BD71837_INT_PWRBTN_L,
> + .end = BD71837_INT_PWRBTN_L,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-l",
> + }, {
> + .start = BD71837_INT_PWRBTN_S,
> + .end = BD71837_INT_PWRBTN_S,
> + .flags = IORESOURCE_IRQ,
> + .name = "pwr-btn-s",
> + },
nit: no comma at the end
> +};
> +
> +/* 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
> +};
> +
> +static const struct regmap_irq bd71837_irqs[] = {
> + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> +};
> +
> +static struct regmap_irq_chip bd71837_irq_chip = {
> + .name = "bd71837-irq",
> + .irqs = bd71837_irqs,
> + .num_irqs = ARRAY_SIZE(bd71837_irqs),
> + .num_regs = 1,
> + .irq_reg_stride = 1,
> + .status_base = BD71837_REG_IRQ,
> + .mask_base = BD71837_REG_MIRQ,
> + .ack_base = BD71837_REG_IRQ,
> + .init_ack_masked = true,
> + .mask_invert = false,
> +};
> +
> +static int bd71837_irq_exit(struct bd71837 *bd71837)
> +{
> + if (bd71837->chip_irq > 0)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> + return 0;
> +}
> +
> +static const struct regmap_range pmic_status_range = {
> + .range_min = BD71837_REG_IRQ,
> + .range_max = BD71837_REG_POW_STATE,
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = &pmic_status_range,
> + .n_yes_ranges = 1,
> +};
> +
> +static const struct regmap_config bd71837_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .max_register = BD71837_MAX_REGISTER - 1,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +#ifdef CONFIG_OF
The driver is DT-only and depends on OF so you don't need to protect this.
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { },
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> +#endif //CONFIG_OF
> +
> +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);
> +
> + if (!board_info) {
> + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> + GFP_KERNEL);
> + if (!board_info) {
> + ret = -ENOMEM;
> + goto err_out;
> + } else if (i2c->irq) {
> + board_info->gpio_intr = i2c->irq;
> + } else {
> + ret = -ENOENT;
> + goto err_out;
> + }
> + }
> +
> + if (!board_info)
> + goto err_out;
> +
> + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> + if (bd71837 == NULL)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, bd71837);
> + bd71837->dev = &i2c->dev;
> + bd71837->i2c_client = i2c;
> + bd71837->chip_irq = board_info->gpio_intr;
> +
> + 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;
> + }
> +
> + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> + if (ret < 0) {
> + dev_err(bd71837->dev,
> + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
> + goto err_out;
> + }
> +
> + ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> + IRQF_ONESHOT, 0,
> + &bd71837_irq_chip, &bd71837->irq_data);
I think that you can use 'devm_regmap_add_irq_chip' here and remove
the code to delete the irq chip
> + if (ret < 0) {
> + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> + goto err_out;
> + }
> +
> + ret = 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));
Same here, I think you can use the devm_mfd_add_devices.
> + if (ret)
> + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
If you use devm_ functions you can remove this.
> +err_out:
> +
> + return ret;
> +}
> +
> +static int bd71837_i2c_remove(struct i2c_client *i2c)
> +{
> + struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
> +
> + bd71837_irq_exit(bd71837);
> + mfd_remove_devices(bd71837->dev);
All this can go away if you use the devm_ calls.
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id bd71837_i2c_id[] = {
> + { .name = "bd71837", },
> + { }
nit: { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> +
> +static struct i2c_driver bd71837_i2c_driver = {
> + .driver = {
> + .name = "bd71837-mfd",
> + .owner = THIS_MODULE,
Remove this, it is not needed, the core does it for you.
> + .of_match_table = of_match_ptr(bd71837_of_match),
The driver is DT-only, don't need to call of_match_ptr.
> + },
> + .probe = bd71837_i2c_probe,
> + .remove = bd71837_i2c_remove,
> + .id_table = bd71837_i2c_id,
> +};
> +
> +static int __init bd71837_i2c_init(void)
> +{
> + return i2c_add_driver(&bd71837_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(bd71837_i2c_init);
> +
> +static void __exit bd71837_i2c_exit(void)
> +{
> + i2c_del_driver(&bd71837_i2c_driver);
> +}
> +module_exit(bd71837_i2c_exit);
> +
Can't you use module_i2c_driver here and get rid of init/exit calls?
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> +MODULE_LICENSE("GPL");
License mismatch.
> diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> new file mode 100644
> index 000000000000..125c7478ec29
> --- /dev/null
> +++ b/include/linux/mfd/bd71837.h
> @@ -0,0 +1,367 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +
> +/*
> + * ROHM BD71837MWV header file
> + */
> +
> +#ifndef __LINUX_MFD_BD71837_H__
> +#define __LINUX_MFD_BD71837_H__
> +
> +#include <linux/regmap.h>
> +
> +enum {
> + BD71837_BUCK1 = 0,
> + BD71837_BUCK2,
> + BD71837_BUCK3,
> + BD71837_BUCK4,
> + BD71837_BUCK5,
> + BD71837_BUCK6,
> + BD71837_BUCK7,
> + BD71837_BUCK8,
> + BD71837_LDO1,
> + BD71837_LDO2,
> + BD71837_LDO3,
> + BD71837_LDO4,
> + BD71837_LDO5,
> + BD71837_LDO6,
> + BD71837_LDO7,
> + BD71837_REGULATOR_CNT,
> +};
> +
> +#define BD71837_BUCK1_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK2_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK3_VOLTAGE_NUM 0x40
> +#define BD71837_BUCK4_VOLTAGE_NUM 0x40
> +
> +#define BD71837_BUCK5_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK6_VOLTAGE_NUM 0x04
> +#define BD71837_BUCK7_VOLTAGE_NUM 0x08
> +#define BD71837_BUCK8_VOLTAGE_NUM 0x40
> +
> +#define BD71837_LDO1_VOLTAGE_NUM 0x04
> +#define BD71837_LDO2_VOLTAGE_NUM 0x02
> +#define BD71837_LDO3_VOLTAGE_NUM 0x10
> +#define BD71837_LDO4_VOLTAGE_NUM 0x10
> +#define BD71837_LDO5_VOLTAGE_NUM 0x10
> +#define BD71837_LDO6_VOLTAGE_NUM 0x10
> +#define BD71837_LDO7_VOLTAGE_NUM 0x10
> +
> +enum {
> + BD71837_REG_REV = 0x00,
> + BD71837_REG_SWRESET = 0x01,
> + BD71837_REG_I2C_DEV = 0x02,
> + BD71837_REG_PWRCTRL0 = 0x03,
> + BD71837_REG_PWRCTRL1 = 0x04,
> + BD71837_REG_BUCK1_CTRL = 0x05,
> + BD71837_REG_BUCK2_CTRL = 0x06,
> + BD71837_REG_BUCK3_CTRL = 0x07,
> + BD71837_REG_BUCK4_CTRL = 0x08,
> + BD71837_REG_BUCK5_CTRL = 0x09,
> + BD71837_REG_BUCK6_CTRL = 0x0A,
> + BD71837_REG_BUCK7_CTRL = 0x0B,
> + BD71837_REG_BUCK8_CTRL = 0x0C,
> + BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
> + BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
> + BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
> + BD71837_REG_BUCK2_VOLT_RUN = 0x10,
> + BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
> + BD71837_REG_BUCK3_VOLT_RUN = 0x12,
> + BD71837_REG_BUCK4_VOLT_RUN = 0x13,
> + BD71837_REG_BUCK5_VOLT = 0x14,
> + BD71837_REG_BUCK6_VOLT = 0x15,
> + BD71837_REG_BUCK7_VOLT = 0x16,
> + BD71837_REG_BUCK8_VOLT = 0x17,
> + BD71837_REG_LDO1_VOLT = 0x18,
> + BD71837_REG_LDO2_VOLT = 0x19,
> + BD71837_REG_LDO3_VOLT = 0x1A,
> + BD71837_REG_LDO4_VOLT = 0x1B,
> + BD71837_REG_LDO5_VOLT = 0x1C,
> + BD71837_REG_LDO6_VOLT = 0x1D,
> + BD71837_REG_LDO7_VOLT = 0x1E,
> + BD71837_REG_TRANS_COND0 = 0x1F,
> + BD71837_REG_TRANS_COND1 = 0x20,
> + BD71837_REG_VRFAULTEN = 0x21,
> + BD71837_REG_MVRFLTMASK0 = 0x22,
> + BD71837_REG_MVRFLTMASK1 = 0x23,
> + BD71837_REG_MVRFLTMASK2 = 0x24,
> + BD71837_REG_RCVCFG = 0x25,
> + BD71837_REG_RCVNUM = 0x26,
> + BD71837_REG_PWRONCONFIG0 = 0x27,
> + BD71837_REG_PWRONCONFIG1 = 0x28,
> + BD71837_REG_RESETSRC = 0x29,
> + BD71837_REG_MIRQ = 0x2A,
> + BD71837_REG_IRQ = 0x2B,
> + BD71837_REG_IN_MON = 0x2C,
> + BD71837_REG_POW_STATE = 0x2D,
> + BD71837_REG_OUT32K = 0x2E,
> + BD71837_REG_REGLOCK = 0x2F,
> + BD71837_REG_OTPVER = 0xFF,
> + BD71837_MAX_REGISTER = 0x100,
> +};
> +
> +#define REGLOCK_PWRSEQ 0x1
> +#define REGLOCK_VREG 0x10
> +
> +/* Generic BUCK control masks */
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_BUCK_RUN_ON 0x04
> +
> +/* Generic LDO masks */
> +#define BD71837_LDO_SEL 0x80
> +#define BD71837_LDO_EN 0x40
> +
> +/* BD71837 BUCK ramp rate CTRL reg bits */
> +#define BUCK_RAMPRATE_MASK 0xC0
> +#define BUCK_RAMPRATE_10P00MV 0x0
> +#define BUCK_RAMPRATE_5P00MV 0x1
> +#define BUCK_RAMPRATE_2P50MV 0x2
> +#define BUCK_RAMPRATE_1P25MV 0x3
> +
> +/* BD71837_REG_BUCK1_VOLT_RUN bits */
> +#define BUCK1_RUN_MASK 0x3F
> +#define BUCK1_RUN_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_SUSP bits */
> +#define BUCK1_SUSP_MASK 0x3F
> +#define BUCK1_SUSP_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK1_VOLT_IDLE bits */
> +#define BUCK1_IDLE_MASK 0x3F
> +#define BUCK1_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK2_VOLT_RUN bits */
> +#define BUCK2_RUN_MASK 0x3F
> +#define BUCK2_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK2_VOLT_IDLE bits */
> +#define BUCK2_IDLE_MASK 0x3F
> +#define BUCK2_IDLE_DEFAULT 0x14
> +
> +/* BD71837_REG_BUCK3_VOLT_RUN bits */
> +#define BUCK3_RUN_MASK 0x3F
> +#define BUCK3_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK4_VOLT_RUN bits */
> +#define BUCK4_RUN_MASK 0x3F
> +#define BUCK4_RUN_DEFAULT 0x1E
> +
> +/* BD71837_REG_BUCK5_VOLT bits */
> +#define BUCK5_MASK 0x07
> +#define BUCK5_DEFAULT 0x02
> +
> +/* BD71837_REG_BUCK6_VOLT bits */
> +#define BUCK6_MASK 0x03
> +#define BUCK6_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK7_VOLT bits */
> +#define BUCK7_MASK 0x07
> +#define BUCK7_DEFAULT 0x03
> +
> +/* BD71837_REG_BUCK8_VOLT bits */
> +#define BUCK8_MASK 0x3F
> +#define BUCK8_DEFAULT 0x1E
> +
> +/* BD71837_REG_IRQ bits */
> +#define IRQ_SWRST 0x40
> +#define IRQ_PWRON_S 0x20
> +#define IRQ_PWRON_L 0x10
> +#define IRQ_PWRON 0x08
> +#define IRQ_WDOG 0x04
> +#define IRQ_ON_REQ 0x02
> +#define IRQ_STBY_REQ 0x01
> +
> +/* BD71837_REG_OUT32K bits */
> +#define BD71837_OUT32K_EN 0x01
> +
> +/* BD71837 gated clock rate */
> +#define BD71837_CLK_RATE 32768
> +
> +/* BD71837 irqs */
> +enum {
> + BD71837_INT_STBY_REQ,
> + BD71837_INT_ON_REQ,
> + BD71837_INT_WDOG,
> + BD71837_INT_PWRBTN,
> + BD71837_INT_PWRBTN_L,
> + BD71837_INT_PWRBTN_S,
> + BD71837_INT_SWRST
> +};
> +
> +/* BD71837 interrupt masks */
> +#define BD71837_INT_SWRST_MASK 0x40
> +#define BD71837_INT_PWRBTN_S_MASK 0x20
> +#define BD71837_INT_PWRBTN_L_MASK 0x10
> +#define BD71837_INT_PWRBTN_MASK 0x8
> +#define BD71837_INT_WDOG_MASK 0x4
> +#define BD71837_INT_ON_REQ_MASK 0x2
> +#define BD71837_INT_STBY_REQ_MASK 0x1
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO1_MASK 0x03
> +
> +/* BD71837_REG_LDO1_VOLT bits */
> +#define LDO2_MASK 0x20
> +
> +/* BD71837_REG_LDO3_VOLT bits */
> +#define LDO3_MASK 0x0F
> +
> +/* BD71837_REG_LDO4_VOLT bits */
> +#define LDO4_MASK 0x0F
> +
> +/* BD71837_REG_LDO5_VOLT bits */
> +#define LDO5_MASK 0x0F
> +
> +/* BD71837_REG_LDO6_VOLT bits */
> +#define LDO6_MASK 0x0F
> +
> +/* BD71837_REG_LDO7_VOLT bits */
> +#define LDO7_MASK 0x0F
> +
> +/* register write induced reset settings */
/* Register ...
> +
> +/* even though the bit zero is not SWRESET type we still want to write zero
/*
* Even
> + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
s/Biz/Bit/
> + * write 1 to it we will trigger the action. So always write 0 to it when
> + * changning SWRESET action - no matter what we read from it.
> + */
> +#define BD71837_SWRESET_TYPE_MASK 7
> +#define BD71837_SWRESET_TYPE_DISABLED 0
> +#define BD71837_SWRESET_TYPE_COLD 4
> +#define BD71837_SWRESET_TYPE_WARM 6
> +
> +#define BD71837_SWRESET_RESET_MASK 1
> +#define BD71837_SWRESET_RESET 1
> +
> +/* Poweroff state transition conditions */
> +
> +#define BD718XX_ON_REQ_POWEROFF_MASK 1
> +#define BD718XX_SWRESET_POWEROFF_MASK 2
> +#define BD718XX_WDOG_POWEROFF_MASK 4
> +#define BD718XX_KEY_L_POWEROFF_MASK 8
> +
> +#define BD718XX_POWOFF_TO_SNVS 0
> +#define BD718XX_POWOFF_TO_RDY 0xF
> +
> +#define BD718XX_POWOFF_TIME_MASK 0xF0
> +enum {
> + BD718XX_POWOFF_TIME_5MS = 0,
> + BD718XX_POWOFF_TIME_10MS,
> + BD718XX_POWOFF_TIME_15MS,
> + BD718XX_POWOFF_TIME_20MS,
> + BD718XX_POWOFF_TIME_25MS,
> + BD718XX_POWOFF_TIME_30MS,
> + BD718XX_POWOFF_TIME_35MS,
> + BD718XX_POWOFF_TIME_40MS,
> + BD718XX_POWOFF_TIME_45MS,
> + BD718XX_POWOFF_TIME_50MS,
> + BD718XX_POWOFF_TIME_75MS,
> + BD718XX_POWOFF_TIME_100MS,
> + BD718XX_POWOFF_TIME_250MS,
> + BD718XX_POWOFF_TIME_500MS,
> + BD718XX_POWOFF_TIME_750MS,
> + BD718XX_POWOFF_TIME_1500MS
> +};
> +
> +/* Poweron sequence state transition conditions */
> +
> +#define BD718XX_RDY_TO_SNVS_MASK 0xF
> +#define BD718XX_SNVS_TO_RUN_MASK 0xF0
> +
> +#define BD718XX_PWR_TRIG_KEY_L 1
> +#define BD718XX_PWR_TRIG_KEY_S 2
> +#define BD718XX_PWR_TRIG_PMIC_ON 4
> +#define BD718XX_PWR_TRIG_VSYS_UVLO 8
> +#define BD718XX_RDY_TO_SNVS_SIFT 0
> +#define BD718XX_SNVS_TO_RUN_SIFT 4
> +
> +/* Timeout value for detecting short press */
> +
> +#define BD718XX_PWRBTN_SHORT_PRESS_MASK 0xF
> +
> +enum {
> + BD718XX_PWRBTN_SHORT_PRESS_10MS = 0,
> + BD718XX_PWRBTN_SHORT_PRESS_500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_1500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_2500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_3500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_4500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_5500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_6500MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7000MS,
> + BD718XX_PWRBTN_SHORT_PRESS_7500MS
> +};
> +
> +struct bd71837;
> +struct bd71837_pmic;
> +struct bd71837_clk;
> +struct bd718xx_pwrkey;
> +
> +/*
> + * Board platform data may be used to initialize regulators.
> + */
> +
> +struct bd71837_board {
> + struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> + int gpio_intr;
> +};
> +
> +struct bd71837 {
> + struct device *dev;
> + struct i2c_client *i2c_client;
> + struct regmap *regmap;
> + unsigned long int id;
> +
> + int chip_irq;
> + struct regmap_irq_chip_data *irq_data;
> +
> + struct bd71837_pmic *pmic;
> + struct bd71837_clk *clk;
> + struct bd718xx_pwrkey *pwrkey;
> +};
> +
> +/*
> + * bd71837 sub-driver chip access routines
> + */
> +
All these access routines only hide the regmap_* calls, so why not use
the regmap calls directly in the driver and remove all this?
> +static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
> +{
> + int r, val;
> +
> + r = regmap_read(bd71837->regmap, reg, &val);
> + if (r < 0)
> + return r;
> + return val;
> +}
> +
> +static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
> + unsigned int val)
> +{
> + return regmap_write(bd71837->regmap, reg, val);
> +}
> +
> +static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, mask);
> +}
> +
> +static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, 0);
> +}
> +
> +static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
> + u8 mask, u8 val)
> +{
> + return regmap_update_bits(bd71837->regmap, reg, mask, val);
> +}
> +
> +#endif /* __LINUX_MFD_BD71837_H__ */
> --
> 2.14.3
>
^ permalink raw reply
* [PATCH 0/9] Fix references for some missing documentation files
From: Mauro Carvalho Chehab @ 2018-06-26 9:49 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Jacek Anaszewski, devicetree, Ingo Molnar,
linux-kernel, Andrew Morton, linux-leds, intel-wired-lan,
Mark Rutland, linux-gpio, David S. Miller, James Morris,
Jeff Kirsher, Changbin Du, Masami Hiramatsu, netdev,
Steven Rostedt, linux-input
Having nothing to do while waiting for my plane to arrive while
returning back from Japan, I ended by writing a small series of
patches meant to reduce the number of bad Documentation/*
links that are detected by:
./scripts/documentation-file-ref-check
I ended by rebasing this patch series against linux-next, because
of those two patches:
3b0c3ebe2a42 Documentation: e100: Fix docs build error
805f16a5f12f Documentation: e1000: Fix docs build error
They basically fix documentation builds with upstream Kernel. Both
got merged on -rc2.
The first two patches in this series makes the script to ignore some
false positives.
Patches 3 to 6 corrects the location of some documentation files.
Patches 7 and 8 were actually two patches meant to fix the build
error. I ended by rebasing them over linux-next, as they fix some
troubles with the ReST syntax with causes warnings.
Patch 9 converts Documentation/trace/histogram.txt to ReST
syntax. It also had to be rebased against linux-next, due to some minor
conflicts with:
064f35a95224 ("tracing: Fix some errors in histogram documentation")
After this series, the script still produces 16 warnings:
Documentation/devicetree/bindings/input/mtk-pmic-keys.txt: Documentation/devicetree/bindings/input/keys.txt
Documentation/devicetree/bindings/input/mtk-pmic-keys.txt: Documentation/devicetree/bindings/input/keys.txt
Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt: Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
Documentation/devicetree/dynamic-resolution-notes.txt: Documentation/devicetree/dt-object-internal.txt
Documentation/scsi/scsi_mid_low_api.txt: Documentation/Configure.help
Documentation/translations/zh_CN/HOWTO: Documentation/DocBook/
Documentation/translations/zh_CN/basic_profiling.txt: Documentation/basic_profiling
Documentation/translations/zh_CN/basic_profiling.txt: Documentation/basic_profiling
MAINTAINERS: Documentation/fpga/
MAINTAINERS: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
arch/powerpc/Kconfig: Documentation/vm/protection-keys.rst
drivers/isdn/mISDN/dsp_core.c: Documentation/isdn/mISDN.cert
drivers/scsi/Kconfig: file:Documentation/scsi/tmscsim.txt
drivers/vhost/vhost.c: Documentation/virtual/lguest/lguest.c
include/linux/fs_context.h: Documentation/filesystems/mounting.txt
include/linux/lsm_hooks.h: Documentation/filesystems/mounting.txt
IMHO, the above should be fixed by the corresponding maintainers.
The ones that scarry me most are the DT binding documentation, as
the binding documentation for some stuff are likely broken.
Btw, two of the above are new on linux-next (include/linux/fs_context.h
and include/linux/lsm_hooks.h) . That makes me wander that we should
likely add some logic (or run the detect script) at checkpatch.pl or make
it to call ./scripts/documentation-file-ref-check.
Mauro Carvalho Chehab (9):
scripts/documentation-file-ref-check: remove some false positives
scripts/documentation-file-ref-check: ignore sched-pelt false positive
docs: zh_CN: fix location of oops-tracing.txt
devicectree: bindings: fix location of leds common file
MAINTAINERS: fix location of ina2xx.txt device tree file
gpio.h: fix location of gpio legacy documentation
networking: e100.rst: Get rid of Sphinx warnings
networking: e1000.rst: Get rid of Sphinx warnings
docs: histogram.txt: convert it to ReST file format
.../devicetree/bindings/leds/common.txt | 2 +-
Documentation/networking/e100.rst | 27 +-
Documentation/networking/e1000.rst | 187 ++-
Documentation/trace/events.rst | 2 +-
.../trace/{histogram.txt => histogram.rst} | 1242 +++++++++--------
Documentation/trace/index.rst | 1 +
.../translations/zh_CN/oops-tracing.txt | 4 +-
MAINTAINERS | 2 +-
include/linux/gpio.h | 2 +-
kernel/trace/Kconfig | 2 +-
scripts/documentation-file-ref-check | 6 +
11 files changed, 767 insertions(+), 710 deletions(-)
rename Documentation/trace/{histogram.txt => histogram.rst} (73%)
--
2.17.1
^ permalink raw reply
* Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
From: Alexander Steffen @ 2018-06-26 10:07 UTC (permalink / raw)
To: Jarkko Sakkinen, Peter Rosin
Cc: Crt Mori, Wolfram Sang, linux-iio, Sekhar Nori, linux-i2c,
Peter Meerwald-Stadler, Peter Huewe, Michael Krufky, Lee Jones,
Antti Palosaari, linux-samsung-soc, Florian Fainelli,
Kevin Hilman, Chunyan Zhang, Krzysztof Kozlowski, Jason Gunthorpe,
Kukjin Kim, bcm-kernel-feedback-list, linux-input, Orson Zhai,
Guenter Roeck, Haavard Skinnemoen, Arnd Bergmann, linux-media,
Lars-Peter Clausen, Gregory Fong <gregory.0xf0>
In-Reply-To: <20180625102454.GA3845@linux.intel.com>
On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 07:17:54AM +0200, 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>
>
> Studied enough so that I can give
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> Do not have hardware to test this, however.
I don't have a mux-locked I2C mux either, but at least I can confirm
that this change did not break my existing test setup (SLB9635/SLB9645
on Raspberry Pi 2B).
Tested-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Alexander
^ permalink raw reply
* Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
From: Andy Shevchenko @ 2018-06-26 11:13 UTC (permalink / raw)
To: Dmitry Torokhov, Andy Shevchenko
Cc: Andrew Morton, agk, Mike Snitzer, device-mapper development, shli,
linux-raid, linux-input@vger.kernel.org, Yury Norov, lkml,
Mika Westerberg, Joe Perches
In-Reply-To: <20180622184613.GC92912@dtor-ws>
On Fri, 2018-06-22 at 11:46 -0700, Dmitry Torokhov wrote:
> On Thu, Jun 21, 2018 at 05:13:39AM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> > > On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokho
> > > v@gmail.com> wrote:
> > > > We can't as we end up including bitmap.h (by the way of
> > > > cpumask.h)
> > > > form slab.h, so we gen circular dependency.
> >
> > It's not just so easy. See below.
> >
> > > That info should have been in the changelog, and probably a code
> > > comment.
> > >
> > > > Maybe if we removed memcg
> > > > stuff from slab.h so we do not need to include workqueue.h...
> > >
> > > Or move the basic slab API stuff out of slab.h into a new
> > > header. Or
> > > create a new, standalone work_struct.h - that looks pretty simple.
> >
> > I tried to move out work_struct, it didn't help. There are actually
> > several circular dependencies that ends in bitmap.h either way or
> > another.
> >
> > First one is
> >
> > slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h
> >
> > And so on...
> >
> > Splitting out kXalloc stuff to a separate header won't help, I
> > think,
> > because of the above.
> > Splitting out struct work_struct is just a tip of an iceberg.
> > Splitting out memcg stuff won't help in the similar way.
> >
> > I'm all ears for (a better) solution.
>
> I think ultimately we'd want to untangle this, but allocating bitmaps
> is
> not in any hot paths so having them as non-inlined functions should
> not
> hurt us that much for time being.
Perhaps I can elaborate a bit in a commit message.
Thanks for review!
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-06-26 11:24 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, 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>
Hello Eric,
Thanks for the comments! I'll be addressing these in patch series v8
- except the regmap wrapper one which will be taken care of by another
patch set.
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:
> >
> > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > for two subsystems:
> > - clk
> > - Regulators
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 602 insertions(+)
> > create mode 100644 drivers/mfd/bd71837.c
> > create mode 100644 include/linux/mfd/bd71837.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b860eb5aa194..7aa05fc9ed8e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > in various ST Microelectronics and ST-Ericsson embedded
> > Nomadik series.
> >
> > +config MFD_BD71837
> > + bool "BD71837 Power Management chip"
>
> I know that some drivers need to be built-in, is this really a
> requirement for this driver? Or should work as a module too.
>
I have been writing power/reset driver for this PMIC. I thought that the
modules would be unload before reset hook is ran - which seems to be
not true on platform where I tested this. So you are correct, at least
on cases where reset is not used via PMIC - or where the platform is not
unloading modules prior reset - these drivers can indeed be modules. So
it is fair to allow building them as modules. Users who want to build
this in kernel can still do so. => I'll change this.
> > + depends on I2C=y
> > + depends on OF
> > + select REGMAP_I2C
> > + select REGMAP_IRQ
> > + select MFD_CORE
> > + help
> > + Select this option to get support for the ROHM BD71837
> > + Power Management chips. BD71837 is designed to power processors like
> > + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
> > + and emergency shut down as well as 32,768KHz clock output.
> > +
> > config MFD_STM32_LPTIMER
> > tristate "Support for STM32 Low-Power Timer"
> > depends on (ARCH_STM32 && OF) || COMPILE_TEST
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e9fd20dba18d..09dc9eb3782c 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> > obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> > +obj-$(CONFIG_MFD_BD71837) += bd71837.o
> >
> > diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> > new file mode 100644
> > index 000000000000..0f0361d6cad6
> > --- /dev/null
> > +++ b/drivers/mfd/bd71837.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> There is a mismatch between what SPDX says and MODULE_LICENSE says.
>
> GPL-2.0 = GPL v2 only
> MODULE_LICENSE(GPL) = GPL v2 or later.
>
> You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> module license to "GPL v2".
Right. Thanks for pointing that out.
>
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > +//
> > +// Datasheet available from
> > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> This include is not required.
>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> ditto
>
> > +#include <linux/irqdomain.h>
> ditto
>
> > +#include <linux/gpio.h>
> ditto
>
> > +#include <linux/regmap.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> ditto
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/bd71837.h>
> > +
>
> Please review the needed includes.
I'll do that, thanks.
> > +static const struct resource irqs[] = {
> > + {
> > + .start = BD71837_INT_PWRBTN,
> > + .end = BD71837_INT_PWRBTN,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_L,
> > + .end = BD71837_INT_PWRBTN_L,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-l",
> > + }, {
> > + .start = BD71837_INT_PWRBTN_S,
> > + .end = BD71837_INT_PWRBTN_S,
> > + .flags = IORESOURCE_IRQ,
> > + .name = "pwr-btn-s",
> > + },
>
> nit: no comma at the end
Whole struct will be removed. I will use gpio-keys and remove the
bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
> > +};
> > +
> > +/* 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
Ok.
> > +};
> > +
> > +static const struct regmap_irq bd71837_irqs[] = {
> > + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> > + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> > +};
> > +
> > +static struct regmap_irq_chip bd71837_irq_chip = {
> > + .name = "bd71837-irq",
> > + .irqs = bd71837_irqs,
> > + .num_irqs = ARRAY_SIZE(bd71837_irqs),
> > + .num_regs = 1,
> > + .irq_reg_stride = 1,
> > + .status_base = BD71837_REG_IRQ,
> > + .mask_base = BD71837_REG_MIRQ,
> > + .ack_base = BD71837_REG_IRQ,
> > + .init_ack_masked = true,
> > + .mask_invert = false,
> > +};
> > +
> > +static int bd71837_irq_exit(struct bd71837 *bd71837)
> > +{
> > + if (bd71837->chip_irq > 0)
> > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > + return 0;
> > +}
> > +
> > +static const struct regmap_range pmic_status_range = {
> > + .range_min = BD71837_REG_IRQ,
> > + .range_max = BD71837_REG_POW_STATE,
> > +};
> > +
> > +static const struct regmap_access_table volatile_regs = {
> > + .yes_ranges = &pmic_status_range,
> > + .n_yes_ranges = 1,
> > +};
> > +
> > +static const struct regmap_config bd71837_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .volatile_table = &volatile_regs,
> > + .max_register = BD71837_MAX_REGISTER - 1,
> > + .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +#ifdef CONFIG_OF
> The driver is DT-only and depends on OF so you don't need to protect this.
True. I'll remove this
> > +static const struct of_device_id bd71837_of_match[] = {
> > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > + { },
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > +#endif //CONFIG_OF
> > +
> > +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);
> > +
> > + if (!board_info) {
> > + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> > + GFP_KERNEL);
> > + if (!board_info) {
> > + ret = -ENOMEM;
> > + goto err_out;
> > + } else if (i2c->irq) {
> > + board_info->gpio_intr = i2c->irq;
> > + } else {
> > + ret = -ENOENT;
> > + goto err_out;
> > + }
> > + }
> > +
> > + if (!board_info)
> > + goto err_out;
> > +
> > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > + if (bd71837 == NULL)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, bd71837);
> > + bd71837->dev = &i2c->dev;
> > + bd71837->i2c_client = i2c;
> > + bd71837->chip_irq = board_info->gpio_intr;
> > +
> > + 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;
> > + }
> > +
> > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > + if (ret < 0) {
> > + dev_err(bd71837->dev,
> > + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
> > + goto err_out;
> > + }
> > +
> > + ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > + IRQF_ONESHOT, 0,
> > + &bd71837_irq_chip, &bd71837->irq_data);
>
> I think that you can use 'devm_regmap_add_irq_chip' here and remove
> the code to delete the irq chip
Right. Good point. Makes this lot leaner and cleaner.
> > + if (ret < 0) {
> > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > + goto err_out;
> > + }
> > +
> > + ret = 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));
>
> Same here, I think you can use the devm_mfd_add_devices.
Right. Good point. Makes this lot leaner and cleaner.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > + { .name = "bd71837", },
> > + { }
>
> nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
> > +};
> > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > +
> > +static struct i2c_driver bd71837_i2c_driver = {
> > + .driver = {
> > + .name = "bd71837-mfd",
> > + .owner = THIS_MODULE,
>
> Remove this, it is not needed, the core does it for you.
True. Thanks.
> > + .of_match_table = of_match_ptr(bd71837_of_match),
>
> The driver is DT-only, don't need to call of_match_ptr.
Ok.
> > + },
> > + .probe = bd71837_i2c_probe,
> > + .remove = bd71837_i2c_remove,
> > + .id_table = bd71837_i2c_id,
> > +};
> > +
> > +static int __init bd71837_i2c_init(void)
> > +{
> > + return i2c_add_driver(&bd71837_i2c_driver);
> > +}
> > +/* init early so consumer devices can complete system boot */
> > +subsys_initcall(bd71837_i2c_init);
> > +
> > +static void __exit bd71837_i2c_exit(void)
> > +{
> > + i2c_del_driver(&bd71837_i2c_driver);
> > +}
> > +module_exit(bd71837_i2c_exit);
> > +
>
> Can't you use module_i2c_driver here and get rid of init/exit calls?
I did this because of subsys_initcall() and how other drivers had used
that.
> > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > +MODULE_LICENSE("GPL");
>
> License mismatch.
Thanks again
> > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > new file mode 100644
> > index 000000000000..125c7478ec29
> > --- /dev/null
> > +++ b/include/linux/mfd/bd71837.h
> > @@ -0,0 +1,367 @@
> > +
> > +/* register write induced reset settings */
>
> /* Register ...
Ok.
> > +
> > +/* even though the bit zero is not SWRESET type we still want to write zero
>
> /*
> * Even
Ok.
> > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
>
> s/Biz/Bit/
Ok.
> > +/*
> > + * bd71837 sub-driver chip access routines
> > + */
> > +
>
> All these access routines only hide the regmap_* calls, so why not use
> the regmap calls directly in the driver and remove all this?
This was also discussed with Stephen during the review for clk patches:
https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
I will later send a new patch set which only removes these wrappers from
this MFD and also the submodules. I like to introduce that as a separate
change set after the whole driver is applied as it impacts to some already
applied parts. I don't want any mismatches which jump out as compile errors
when MFD gets applied and config dependencies get solved.
But yes, you are correct - the write/read to BD71837 is plain regmap access
and same simple access seems to be working with the BD71847 when it comes.
So these are not needed and will be removed.
Br,
Matti Vaittinen
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Enric Balletbo Serra @ 2018-06-26 11:40 UTC (permalink / raw)
To: matti.vaittinen
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, 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: <20180626112455.GC2118@localhost.localdomain>
Hi Matti,
Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
dia dt., 26 de juny 2018 a les 13:25:
>
> Hello Eric,
>
> Thanks for the comments! I'll be addressing these in patch series v8
> - except the regmap wrapper one which will be taken care of by another
> patch set.
>
> 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:
> > >
> > > ROHM BD71837 PMIC MFD driver providing interrupts and support
> > > for two subsystems:
> > > - clk
> > > - Regulators
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ---
> > > drivers/mfd/Kconfig | 13 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/bd71837.c | 221 ++++++++++++++++++++++++++
> > > include/linux/mfd/bd71837.h | 367 ++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 602 insertions(+)
> > > create mode 100644 drivers/mfd/bd71837.c
> > > create mode 100644 include/linux/mfd/bd71837.h
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index b860eb5aa194..7aa05fc9ed8e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1787,6 +1787,19 @@ config MFD_STW481X
> > > in various ST Microelectronics and ST-Ericsson embedded
> > > Nomadik series.
> > >
> > > +config MFD_BD71837
> > > + bool "BD71837 Power Management chip"
> >
> > I know that some drivers need to be built-in, is this really a
> > requirement for this driver? Or should work as a module too.
> >
>
> I have been writing power/reset driver for this PMIC. I thought that the
> modules would be unload before reset hook is ran - which seems to be
> not true on platform where I tested this. So you are correct, at least
> on cases where reset is not used via PMIC - or where the platform is not
> unloading modules prior reset - these drivers can indeed be modules. So
> it is fair to allow building them as modules. Users who want to build
> this in kernel can still do so. => I'll change this.
>
> > > + depends on I2C=y
> > > + depends on OF
> > > + select REGMAP_I2C
> > > + select REGMAP_IRQ
> > > + select MFD_CORE
> > > + help
> > > + Select this option to get support for the ROHM BD71837
> > > + Power Management chips. BD71837 is designed to power processors like
> > > + NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
> > > + and emergency shut down as well as 32,768KHz clock output.
> > > +
> > > config MFD_STM32_LPTIMER
> > > tristate "Support for STM32 Low-Power Timer"
> > > depends on (ARCH_STM32 && OF) || COMPILE_TEST
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index e9fd20dba18d..09dc9eb3782c 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> > > obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> > > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> > > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> > > +obj-$(CONFIG_MFD_BD71837) += bd71837.o
> > >
> > > diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
> > > new file mode 100644
> > > index 000000000000..0f0361d6cad6
> > > --- /dev/null
> > > +++ b/drivers/mfd/bd71837.c
> > > @@ -0,0 +1,221 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> >
> > There is a mismatch between what SPDX says and MODULE_LICENSE says.
> >
> > GPL-2.0 = GPL v2 only
> > MODULE_LICENSE(GPL) = GPL v2 or later.
> >
> > You'd like to change the SPDX identifier to GPL-2.0-or-later or set
> > module license to "GPL v2".
>
> Right. Thanks for pointing that out.
>
> >
> > > +// Copyright (C) 2018 ROHM Semiconductors
> > > +// bd71837.c -- ROHM BD71837MWV mfd driver
> > > +//
> > > +// Datasheet available from
> > > +// https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > This include is not required.
> >
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > ditto
> >
> > > +#include <linux/irqdomain.h>
> > ditto
> >
> > > +#include <linux/gpio.h>
> > ditto
> >
> > > +#include <linux/regmap.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_gpio.h>
> > ditto
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/bd71837.h>
> > > +
> >
> > Please review the needed includes.
>
> I'll do that, thanks.
>
> > > +static const struct resource irqs[] = {
> > > + {
> > > + .start = BD71837_INT_PWRBTN,
> > > + .end = BD71837_INT_PWRBTN,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_L,
> > > + .end = BD71837_INT_PWRBTN_L,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-l",
> > > + }, {
> > > + .start = BD71837_INT_PWRBTN_S,
> > > + .end = BD71837_INT_PWRBTN_S,
> > > + .flags = IORESOURCE_IRQ,
> > > + .name = "pwr-btn-s",
> > > + },
> >
> > nit: no comma at the end
>
> Whole struct will be removed. I will use gpio-keys and remove the
> bd718xx-pwrkey driver as suggested by Dmitry Torokhov.
>
> > > +};
> > > +
> > > +/* 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
>
> Ok.
>
> > > +};
> > > +
> > > +static const struct regmap_irq bd71837_irqs[] = {
> > > + REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
> > > + REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
> > > +};
> > > +
> > > +static struct regmap_irq_chip bd71837_irq_chip = {
> > > + .name = "bd71837-irq",
> > > + .irqs = bd71837_irqs,
> > > + .num_irqs = ARRAY_SIZE(bd71837_irqs),
> > > + .num_regs = 1,
> > > + .irq_reg_stride = 1,
> > > + .status_base = BD71837_REG_IRQ,
> > > + .mask_base = BD71837_REG_MIRQ,
> > > + .ack_base = BD71837_REG_IRQ,
> > > + .init_ack_masked = true,
> > > + .mask_invert = false,
> > > +};
> > > +
> > > +static int bd71837_irq_exit(struct bd71837 *bd71837)
> > > +{
> > > + if (bd71837->chip_irq > 0)
> > > + regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
> > > + return 0;
> > > +}
> > > +
> > > +static const struct regmap_range pmic_status_range = {
> > > + .range_min = BD71837_REG_IRQ,
> > > + .range_max = BD71837_REG_POW_STATE,
> > > +};
> > > +
> > > +static const struct regmap_access_table volatile_regs = {
> > > + .yes_ranges = &pmic_status_range,
> > > + .n_yes_ranges = 1,
> > > +};
> > > +
> > > +static const struct regmap_config bd71837_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .volatile_table = &volatile_regs,
> > > + .max_register = BD71837_MAX_REGISTER - 1,
> > > + .cache_type = REGCACHE_RBTREE,
> > > +};
> > > +
> > > +#ifdef CONFIG_OF
>
> > The driver is DT-only and depends on OF so you don't need to protect this.
> True. I'll remove this
>
> > > +static const struct of_device_id bd71837_of_match[] = {
> > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > + { },
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
It's a nit but It is a good practice to specify that the last entry is
a sentinel. Just this.
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { /* sentinel */ }
+};
And just noticed, is .data = (void *)0 really required?
> > > +};
> > > +MODULE_DEVICE_TABLE(of, bd71837_of_match);
> > > +#endif //CONFIG_OF
> > > +
> > > +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);
> > > +
> > > + if (!board_info) {
> > > + board_info = devm_kzalloc(&i2c->dev, sizeof(*board_info),
> > > + GFP_KERNEL);
> > > + if (!board_info) {
> > > + ret = -ENOMEM;
> > > + goto err_out;
> > > + } else if (i2c->irq) {
> > > + board_info->gpio_intr = i2c->irq;
> > > + } else {
> > > + ret = -ENOENT;
> > > + goto err_out;
> > > + }
> > > + }
> > > +
> > > + if (!board_info)
> > > + goto err_out;
> > > +
> > > + bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
> > > + if (bd71837 == NULL)
> > > + return -ENOMEM;
> > > +
> > > + i2c_set_clientdata(i2c, bd71837);
> > > + bd71837->dev = &i2c->dev;
> > > + bd71837->i2c_client = i2c;
> > > + bd71837->chip_irq = board_info->gpio_intr;
> > > +
> > > + 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;
> > > + }
> > > +
> > > + ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
> > > + if (ret < 0) {
> > > + dev_err(bd71837->dev,
> > > + "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
> > > + goto err_out;
> > > + }
> > > +
> > > + ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
> > > + IRQF_ONESHOT, 0,
> > > + &bd71837_irq_chip, &bd71837->irq_data);
> >
> > I think that you can use 'devm_regmap_add_irq_chip' here and remove
> > the code to delete the irq chip
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > + if (ret < 0) {
> > > + dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
> > > + goto err_out;
> > > + }
> > > +
> > > + ret = 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));
> >
> > Same here, I think you can use the devm_mfd_add_devices.
>
> Right. Good point. Makes this lot leaner and cleaner.
>
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id bd71837_i2c_id[] = {
> > > + { .name = "bd71837", },
> > > + { }
> >
> > nit: { /* sentinel */ }
>
> I am sorry but I didn't quite get the point here. Could you please
> explain what did you expect to be added here?
>
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
> > > +
> > > +static struct i2c_driver bd71837_i2c_driver = {
> > > + .driver = {
> > > + .name = "bd71837-mfd",
> > > + .owner = THIS_MODULE,
> >
> > Remove this, it is not needed, the core does it for you.
>
> True. Thanks.
>
> > > + .of_match_table = of_match_ptr(bd71837_of_match),
> >
> > The driver is DT-only, don't need to call of_match_ptr.
>
> Ok.
>
> > > + },
> > > + .probe = bd71837_i2c_probe,
> > > + .remove = bd71837_i2c_remove,
> > > + .id_table = bd71837_i2c_id,
> > > +};
> > > +
> > > +static int __init bd71837_i2c_init(void)
> > > +{
> > > + return i2c_add_driver(&bd71837_i2c_driver);
> > > +}
> > > +/* init early so consumer devices can complete system boot */
> > > +subsys_initcall(bd71837_i2c_init);
> > > +
> > > +static void __exit bd71837_i2c_exit(void)
> > > +{
> > > + i2c_del_driver(&bd71837_i2c_driver);
> > > +}
> > > +module_exit(bd71837_i2c_exit);
> > > +
> >
> > Can't you use module_i2c_driver here and get rid of init/exit calls?
>
> I did this because of subsys_initcall() and how other drivers had used
> that.
>
> > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> > > +MODULE_DESCRIPTION("BD71837 chip multi-function driver");
> > > +MODULE_LICENSE("GPL");
> >
> > License mismatch.
>
> Thanks again
>
> > > diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
> > > new file mode 100644
> > > index 000000000000..125c7478ec29
> > > --- /dev/null
> > > +++ b/include/linux/mfd/bd71837.h
> > > @@ -0,0 +1,367 @@
> > > +
> > > +/* register write induced reset settings */
> >
> > /* Register ...
>
> Ok.
>
> > > +
> > > +/* even though the bit zero is not SWRESET type we still want to write zero
> >
> > /*
> > * Even
>
> Ok.
>
> > > + * to it when changing type. Biz zero is 'SWRESET' trigger bit and if we
> >
> > s/Biz/Bit/
>
> Ok.
>
> > > +/*
> > > + * bd71837 sub-driver chip access routines
> > > + */
> > > +
> >
> > All these access routines only hide the regmap_* calls, so why not use
> > the regmap calls directly in the driver and remove all this?
>
> This was also discussed with Stephen during the review for clk patches:
> https://lore.kernel.org/lkml/152997029783.143105.16692843405899913246@swboyd.mtv.corp.google.com/
>
> I will later send a new patch set which only removes these wrappers from
> this MFD and also the submodules. I like to introduce that as a separate
> change set after the whole driver is applied as it impacts to some already
> applied parts. I don't want any mismatches which jump out as compile errors
> when MFD gets applied and config dependencies get solved.
>
> But yes, you are correct - the write/read to BD71837 is plain regmap access
> and same simple access seems to be working with the BD71847 when it comes.
> So these are not needed and will be removed.
>
> Br,
> Matti Vaittinen
^ permalink raw reply
* Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-06-26 12:03 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Lee Jones,
Liam Girdwood, Mark Brown, mazziesaccount, Arnd Bergmann,
Dmitry Torokhov, 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_52NERfORwSkBh0stW+mvzRpw0to9CiWPwLB9mFo_TsW_w@mail.gmail.com>
Hello Again Eric,
On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
> Hi Matti,
> Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> dia dt., 26 de juny 2018 a les 13:25:
> > On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
> > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > dia dt., 19 de juny 2018 a les 12:57:
> >
> > > > +static const struct of_device_id bd71837_of_match[] = {
> > > > + { .compatible = "rohm,bd71837", .data = (void *)0},
> > > > + { },
> > >
> > > nit: { /* sentinel */ }
> >
> > I am sorry but I didn't quite get the point here. Could you please
> > explain what did you expect to be added here?
> >
>
> It's a nit but It is a good practice to specify that the last entry is
> a sentinel. Just this.
>
> +static const struct of_device_id bd71837_of_match[] = {
> + { .compatible = "rohm,bd71837", .data = (void *)0},
> + { /* sentinel */ }
> +};
Oh, I see. Finally something I can disagree =) I quickly opened few
random drivers which declare match table. None of them practiced this
good practice. So I guess it is not such a standard after all. And I
guess the meaning of last entry in match table should be quite obvious.
Adding the comment /* sentinel */ sounds like stating the obvious at
best - at worst it gets one just to wonder what the "sentinel" means =)
>
> And just noticed, is .data = (void *)0 really required?
As static structs should be initialized to zero I'd say it is not
required. Will remove this. Thanks for pointing this out.
Br,
Matti Vaittinen
^ permalink raw reply
* Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
From: Jarkko Sakkinen @ 2018-06-26 12:05 UTC (permalink / raw)
To: Alexander Steffen, Peter Rosin
Cc: linux-kernel, Peter Huewe, 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, Crt Mori,
Jonathan Cameron
In-Reply-To: <7703d6a2-b22c-104c-7390-b5143a504725@infineon.com>
On Tue, 2018-06-26 at 12:07 +0200, Alexander Steffen wrote:
> On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> > On Wed, Jun 20, 2018 at 07:17:54AM +0200, 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>
> >
> > Studied enough so that I can give
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > Do not have hardware to test this, however.
>
> I don't have a mux-locked I2C mux either, but at least I can confirm
> that this change did not break my existing test setup (SLB9635/SLB9645
> on Raspberry Pi 2B).
>
> Tested-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>
> Alexander
Given the scope of the change and since analogous change works for
every other subsystem, this should be enough! Thank you.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
From: Jarkko Sakkinen @ 2018-06-26 12:07 UTC (permalink / raw)
To: Alexander Steffen, Peter Rosin
Cc: linux-kernel, Peter Huewe, 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, Crt Mori,
Jonathan Cameron
In-Reply-To: <6169da633eab7e68d645e6b933b605dcc9ae09f1.camel@linux.intel.com>
On Tue, 2018-06-26 at 15:05 +0300, Jarkko Sakkinen wrote:
> On Tue, 2018-06-26 at 12:07 +0200, Alexander Steffen wrote:
> > On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> > > On Wed, Jun 20, 2018 at 07:17:54AM +0200, 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>
> > >
> > > Studied enough so that I can give
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > Do not have hardware to test this, however.
> >
> > I don't have a mux-locked I2C mux either, but at least I can confirm
> > that this change did not break my existing test setup (SLB9635/SLB9645
> > on Raspberry Pi 2B).
> >
> > Tested-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> >
> > Alexander
>
> Given the scope of the change and since analogous change works for
> every other subsystem, this should be enough! Thank you.
It is now applied to my tree (master). I will move it to the
next branch once James has updated security/next-general.
/Jarkko
^ 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