* [PATCH v2] HID: hiddev: fix potential Spectre v1
From: Breno Leitao @ 2018-10-19 20:01 UTC (permalink / raw)
To: linux-usb, linux-input; +Cc: gustavo, jkosina, Breno Leitao, stable
uref->usage_index can be indirectly controlled by userspace, hence leading
to a potential exploitation of the Spectre variant 1 vulnerability.
This field is used as an array index by the hiddev_ioctl_usage() function,
when 'cmd' is either HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or
HIDIOCSUSAGES.
For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
field->maxusage and then used as an index to dereference field->usage
array. The same thing happens to the cmd == HIDIOC{G,S}USAGES cases, where
uref->usage_index is checked against an array maximum value and then it is
used as an index in an array.
This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
traditional Spectre V1 first load:
copy_from_user(uref, user_arg, sizeof(*uref))
if (uref->usage_index >= field->maxusage)
goto inval;
i = field->usage[uref->usage_index].collection_index;
return i;
This patch fixes this by sanitizing field uref->usage_index before using it
to index field->usage (HIDIOCGCOLLECTIONINDEX) or field->value in
HIDIOC{G,S}USAGES arrays, thus, avoiding speculation in the first load.
Signed-off-by: Breno Leitao <leitao@debian.org>
Cc: <stable@vger.kernel.org>
--
v2: Contemplate cmd == HIDIOC{G,S}USAGES case
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 23872d08308c..a746017fac17 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -512,14 +512,24 @@ static noinline int hiddev_ioctl_usage(struct hiddev *hiddev, unsigned int cmd,
if (cmd == HIDIOCGCOLLECTIONINDEX) {
if (uref->usage_index >= field->maxusage)
goto inval;
+ uref->usage_index =
+ array_index_nospec(uref->usage_index,
+ field->maxusage);
} else if (uref->usage_index >= field->report_count)
goto inval;
}
- if ((cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) &&
- (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
- uref->usage_index + uref_multi->num_values > field->report_count))
- goto inval;
+ if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
+ if (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
+ uref->usage_index + uref_multi->num_values >
+ field->report_count)
+ goto inval;
+
+ uref->usage_index =
+ array_index_nospec(uref->usage_index,
+ field->report_count -
+ uref_multi->num_values);
+ }
switch (cmd) {
case HIDIOCGUSAGE:
--
2.17.1
^ permalink raw reply related
* Re: [git pull] Input updates for v4.19-rc8
From: Greg Kroah-Hartman @ 2018-10-20 7:08 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-kernel, linux-input, Linus Torvalds
In-Reply-To: <20181019174508.GA47209@dtor-ws>
On Fri, Oct 19, 2018 at 10:45:08AM -0700, Dmitry Torokhov wrote:
> Hi Greg,
>
> Please pull from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
>
> to receive updates for the input subsystem. Just an addition to elan
> touchpad driver ACPI table.
Now merged, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH] Input: uinput - fix Spectre v1 vulnerability
From: Pavel Machek @ 2018-10-22 14:14 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <ead3e95a-1acb-362a-0ee2-d02dbc4f378c@embeddedor.com>
[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]
On Tue 2018-10-16 19:52:58, Gustavo A. R. Silva wrote:
> Hi Dmitry,
>
> On 10/16/18 7:21 PM, Dmitry Torokhov wrote:
> > Hi Gustavo,
> >
> > On Tue, Oct 16, 2018 at 01:13:13PM +0200, Gustavo A. R. Silva wrote:
> >> setup.code can be indirectly controlled by user-space, hence leading to
> >> a potential exploitation of the Spectre variant 1 vulnerability.
> >>
> >> This issue was detected with the help of Smatch:
> >>
> >> drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential
> >> spectre issue 'dev->absinfo' [w] (local cap)
> >>
> >> Fix this by sanitizing setup.code before using it to index dev->absinfo.
> >
> > So we are saying that attacker, by repeatedly calling ioctl(...,
> > UI_ABS_SETUP, ...) will be able to poison branch predictor and discover
> > another program or kernel secrets? But uinput is a privileged interface
> > open to root only, as it allows injecting arbitrary keystrokes into the
> > kernel. And since only root can use uinput, meh?
> >
>
> Oh I see... in that case this is a false positive.
No, please lets fix it.
Some people are trying to make sure kernel is "more priviledged" than
root -- its called secure boot etc.
And "if you givean attacker possibility to generate keystrokes, he can
read kernel memory as well"... is very unexpected.
Unexpected is bad when talking about security.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570
From: Henrique de Moraes Holschuh @ 2018-10-23 3:47 UTC (permalink / raw)
To: Jaak Ristioja
Cc: Alexander Kappner, Jouke Witteveen,
linux-input-u79uwXL29TY76Z2rM5mHXA,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Pali Rohár,
Andy Shevchenko
In-Reply-To: <36111ed4-621c-47a8-bc6a-66cf756a0ac6-89mTbI93R4uuvFJfX82//w@public.gmane.org>
On Sat, 13 Oct 2018, Jaak Ristioja wrote:
> On 27.08.2018 19:22, Jaak Ristioja wrote:
> > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > spurious input events, mostly right mouse button click events, but also
> > cursor jumps.
> >
> > I have not attempted to understand whether these events come from the
> > trackpoint or touchpad or some other driver, but I managed to bisect
> > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> >
> > Apparently the quirk mitigation is applied when the BIOS version begins
> > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > which is probably why this commit causes the described problems on the
> > ThinkPad L570. How exactly? - I don't know.
> >
> > The issue did not reproduce when running some stable 4.17 and 4.18
> > kernels with commit a986c75a7df0 reverted.
> >
> > Please fix this for future kernels. Thanks! :)
>
> Ping. Do you need any additional information?
It looks like it is failing to detect N1X and N1 as different models,
the bug is likely on this commit:
commit 846a416b4630fc960c10500b4194ad504bc22d4b
platform/x86: thinkpad_acpi: Proper model/release matching
Real life has been **extremely** busy, so I'd appreciate a patch.
Otherwise I will eventually get to it, but it might take a few
weeks(!!).
--
Henrique Holschuh
^ permalink raw reply
* Re: 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570
From: Henrique de Moraes Holschuh @ 2018-10-23 3:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: agk-mA9ux5SlULTR7s880joybQ, Pali Rohár, Platform Driver,
ibm-acpi-devel,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
jaak-89mTbI93R4uuvFJfX82//w, Andy Shevchenko
In-Reply-To: <CAKdAkRTbQ+-+h8SMGf9Khc4+wE+P3_UCbYR0DDi1MoBwfSOWZQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 15 Oct 2018, Dmitry Torokhov wrote:
> On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja <jaak-89mTbI93R4uuvFJfX82//w@public.gmane.org> wrote:
> > On 27.08.2018 19:22, Jaak Ristioja wrote:
> > > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > > spurious input events, mostly right mouse button click events, but also
> > > cursor jumps.
> > >
> > > I have not attempted to understand whether these events come from the
> > > trackpoint or touchpad or some other driver, but I managed to bisect
> > > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> > >
> > > Apparently the quirk mitigation is applied when the BIOS version begins
> > > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > > which is probably why this commit causes the described problems on the
> > > ThinkPad L570. How exactly? - I don't know.
> > >
> > > The issue did not reproduce when running some stable 4.17 and 4.18
> > > kernels with commit a986c75a7df0 reverted.
> > >
> > > Please fix this for future kernels. Thanks! :)
> >
> > Ping. Do you need any additional information?
>
> Sounds like we need tighter check for the quirk, maybe based on
> DMI/Board name? Can we revert the offending commit for now?
Apparently it has to be fixed properly, because if I understood the
issue correctly, every two-character quirk will trigger on *any*
three-character model that starts with those two characters.
We could revert the three-character quirk support, but this would cause
regressions as well.
--
Henrique Holschuh
^ permalink raw reply
* Re: 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570
From: Jouke Witteveen @ 2018-10-23 8:16 UTC (permalink / raw)
To: hmh-N3TV7GIv+o9fyO9Q7EP/yw
Cc: agk-mA9ux5SlULTR7s880joybQ, pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-input-u79uwXL29TY76Z2rM5mHXA, jaak-89mTbI93R4uuvFJfX82//w,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <20181023035007.iy5vy2z3vuto2d6s-ZGHd14iZgfaRjzvQDGKj+xxZW9W5cXbT@public.gmane.org>
On Tue, Oct 23, 2018 at 6:06 AM Henrique de Moraes Holschuh
<hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org> wrote:
>
> On Mon, 15 Oct 2018, Dmitry Torokhov wrote:
> > On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja <jaak-89mTbI93R4uuvFJfX82//w@public.gmane.org> wrote:
> > > On 27.08.2018 19:22, Jaak Ristioja wrote:
> > > > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > > > spurious input events, mostly right mouse button click events, but also
> > > > cursor jumps.
> > > >
> > > > I have not attempted to understand whether these events come from the
> > > > trackpoint or touchpad or some other driver, but I managed to bisect
> > > > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > > > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> > > >
> > > > Apparently the quirk mitigation is applied when the BIOS version begins
> > > > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > > > which is probably why this commit causes the described problems on the
> > > > ThinkPad L570. How exactly? - I don't know.
> > > >
> > > > The issue did not reproduce when running some stable 4.17 and 4.18
> > > > kernels with commit a986c75a7df0 reverted.
> > > >
> > > > Please fix this for future kernels. Thanks! :)
> > >
> > > Ping. Do you need any additional information?
> >
> > Sounds like we need tighter check for the quirk, maybe based on
> > DMI/Board name? Can we revert the offending commit for now?
>
> Apparently it has to be fixed properly, because if I understood the
> issue correctly, every two-character quirk will trigger on *any*
> three-character model that starts with those two characters.
This should be (and hopefully is) incorrect.
In the tpacpi_check_quirks function, equality between the quirk bios
and the actual model bios is tested. For two-character models there
are three nonzero bytes, for two character models there are only two.
Therefore, a two-character quirk should not trigger on any
three-character model.
> We could revert the three-character quirk support, but this would cause
> regressions as well.
I doubt this would fix anything. Maybe the bios id for the L570 that
is used by the driver is not the one Jaak Ristioja reported? The
detected string is reported in the dmesg output.
That said, I do wonder why the fan quirks redefine all matching macros
(TPACPI_FAN_Q*) and don't use the generic ones (TPACPI_Q*). This looks
like an easy thing to clean up.
Regards,
- Jouke
> --
> Henrique Holschuh
>
>
> _______________________________________________
> ibm-acpi-devel mailing list
> ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
^ permalink raw reply
* Re: 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570
From: Henrique de Moraes Holschuh @ 2018-10-24 0:47 UTC (permalink / raw)
To: Jouke Witteveen
Cc: agk-mA9ux5SlULTR7s880joybQ, pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-input-u79uwXL29TY76Z2rM5mHXA, jaak-89mTbI93R4uuvFJfX82//w,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <CAJ2ouawfddrf8K22auR4OaJkO_n5f-rme=+XrbQYNo0eJfCtQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 23 Oct 2018, Jouke Witteveen wrote:
> > Apparently it has to be fixed properly, because if I understood the
> > issue correctly, every two-character quirk will trigger on *any*
> > three-character model that starts with those two characters.
>
> This should be (and hopefully is) incorrect.
> In the tpacpi_check_quirks function, equality between the quirk bios
> and the actual model bios is tested. For two-character models there
> are three nonzero bytes, for two character models there are only two.
Hmm, good. Need to check the parser that is feeding it, though.
One actually has to parse a DMI string like aaETxxWW (old models) and
aaaET???? (need to actually verify how the ending goes on three-char
models). This for BIOS. For EC, it used to be HT instead of ET for
H8S-based embedded controllers, maybe it changed for the new ARC-based
embedded controllers.
> That said, I do wonder why the fan quirks redefine all matching macros
> (TPACPI_FAN_Q*) and don't use the generic ones (TPACPI_Q*). This looks
> like an easy thing to clean up.
They used to have some minor differences. A cleanup patch is likely
fine, as long as it is tested.
--
Henrique Holschuh
^ permalink raw reply
* Re: [PATCH v4 4/8] regulator: stpmic1: add stpmic1 regulator driver
From: Pascal PAILLET-LME @ 2018-10-24 12:54 UTC (permalink / raw)
To: Mark Brown
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lee.jones@linaro.org, lgirdwood@gmail.com,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <20181019115015.GC5895@sirena.org.uk>
Hello Mark,
Le 10/19/2018 01:50 PM, Mark Brown a écrit :
> On Thu, Oct 18, 2018 at 09:02:12AM +0000, Pascal PAILLET-LME wrote:
>
>> + for (i = 0; i < ARRAY_SIZE(stpmic1_regulator_cfgs); i++) {
>> + /* Parse DT & find regulators to register */
>> + init_data = stpmic1_regulators_matches[i].init_data;
>> + if (init_data)
>> + init_data->regulator_init = &stpmic1_regulator_parse_dt;
>> +
>> + rdev = stpmic1_regulator_register(pdev, i, init_data, regul);
>> + if (IS_ERR(rdev))
>> + return PTR_ERR(rdev);
> This looks mostly good, the only big thing is this - the default is to
> just unconditionally register all the regulators that exist rather than
> only those that are configured on that particular platform. This is a
> bit simpler and means that all the readback of the configuration for the
> unconfigured regulators is available for diagnostics. Is there a reason
> not to do that?
I'm sorry, I'm not sure to understand. Would you prefer to not register
regulators that
are not described in the device-tree ?
In that case I could add:
if (!init_data)
continue;
This would permit to keep some regulators unmodified by the kernel. This
can be useful
because we have some regulators configured by boot loaders (for the DDR
at least) and
it would be more simple to not handle them in the kernel.
best regards,
pascal
^ permalink raw reply
* Re: [PATCH v4 4/8] regulator: stpmic1: add stpmic1 regulator driver
From: Mark Brown @ 2018-10-24 13:17 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lee.jones@linaro.org, lgirdwood@gmail.com,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <5BD06B96.3080809@st.com>
[-- Attachment #1: Type: text/plain, Size: 404 bytes --]
On Wed, Oct 24, 2018 at 12:54:46PM +0000, Pascal PAILLET-LME wrote:
> I'm sorry, I'm not sure to understand. Would you prefer to not register
> regulators that
> are not described in the device-tree ?
No, I'm saying register all regulators regardless of if they are in the
device tree - you shouldn't be looking at the init data at all here,
just let the framework match them using of_match.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v3 0/3] ARM: qcom: add vibrator support for various MSM SOCs
From: Brian Masney @ 2018-10-25 1:29 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross, david.brown
Cc: robh+dt, mark.rutland, masneyb, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
This patch set adds support for the vibrator found on various Qualcomm
MSM SOCs. This is based on work from:
Jonathan Marek from qcom,pwm-vibrator.c in the PostmarketOS repo:
https://gitlab.com/postmarketOS/linux-postmarketos/commit/7647fb36cb1cbd060f8b52087a68ab93583292b5
Jongrak Kwon and Devin Kim from msm_pwm_vibrator.c in the downstream
Android 3.4.0 sources:
https://android.googlesource.com/kernel/msm/+/android-msm-lenok-3.10-lollipop-wear-release/drivers/misc/msm_pwm_vibrator.c
Driver was tested on a LG Nexus 5 (hammerhead) phone using rumble-test:
https://git.collabora.com/cgit/user/sre/rumble-test.git/plain/rumble-test.c
Changes since v2
- Moved from pwm to input subsystem based on feedback from
https://lore.kernel.org/lkml/20181012114749.GC31561@ulmo/. I
previously wired this into the input system via pwm-vibra.
Changes since v1
- Update device tree binding document based on feedback from Rob
Herring.
- Changed the driver description to: Qualcomm PWM driver for the MSM
vibrator.
Brian Masney (3):
dt-bindings: Input: new bindings for MSM vibrator
Input: add new vibrator driver for various MSM SOCs
ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for
vibrator
.../bindings/input/msm-vibrator.txt | 36 +++
.../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 ++
drivers/input/misc/Kconfig | 10 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/msm-vibrator.c | 276 ++++++++++++++++++
5 files changed, 354 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/msm-vibrator.txt
create mode 100644 drivers/input/misc/msm-vibrator.c
--
2.17.2
^ permalink raw reply
* [PATCH v3 1/3] dt-bindings: Input: new bindings for MSM vibrator
From: Brian Masney @ 2018-10-25 1:29 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross, david.brown
Cc: robh+dt, mark.rutland, masneyb, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20181025012937.2154-1-masneyb@onstation.org>
This patch adds the device tree bindings for the vibrator found on
various Qualcomm MSM SOCs.
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
.../bindings/input/msm-vibrator.txt | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/msm-vibrator.txt
diff --git a/Documentation/devicetree/bindings/input/msm-vibrator.txt b/Documentation/devicetree/bindings/input/msm-vibrator.txt
new file mode 100644
index 000000000000..8dcf014ef2e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/msm-vibrator.txt
@@ -0,0 +1,36 @@
+* Device tree bindings for the Qualcomm MSM vibrator
+
+Required properties:
+
+ - compatible: Should be one of
+ "qcom,msm8226-vibrator"
+ "qcom,msm8974-vibrator"
+ - reg: the base address and length of the IO memory for the registers.
+ - pinctrl-names: set to default.
+ - pinctrl-0: phandles pointing to pin configuration nodes. See
+ Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+ - clock-names: set to pwm
+ - clocks: phandle of the clock. See
+ Documentation/devicetree/bindings/clock/clock-bindings.txt
+ - enable-gpios: GPIO that enables the vibrator.
+
+Optional properties:
+
+ - vcc-supply: phandle to the regulator that provides power to the sensor.
+
+Example from a LG Nexus 5 (hammerhead) phone:
+
+vibrator@fd8c3450 {
+ reg = <0xfd8c3450 0x400>;
+ compatible = "qcom,msm8974-vibrator";
+
+ vcc-supply = <&pm8941_l19>;
+
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+ clock-names = "pwm";
+
+ enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&vibrator_pin>;
+};
--
2.17.2
^ permalink raw reply related
* [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs
From: Brian Masney @ 2018-10-25 1:29 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross, david.brown
Cc: robh+dt, mark.rutland, masneyb, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20181025012937.2154-1-masneyb@onstation.org>
This patch adds a new vibrator driver that supports various Qualcomm
MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
drivers/input/misc/Kconfig | 10 ++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/msm-vibrator.c | 276 ++++++++++++++++++++++++++++++
3 files changed, 287 insertions(+)
create mode 100644 drivers/input/misc/msm-vibrator.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index ca59a2be9bc5..e39aef84f357 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -117,6 +117,16 @@ config INPUT_E3X0_BUTTON
To compile this driver as a module, choose M here: the
module will be called e3x0_button.
+config INPUT_MSM_VIBRATOR
+ tristate "Qualcomm MSM vibrator driver"
+ select INPUT_FF_MEMLESS
+ help
+ Support for the vibrator that is found on various Qualcomm MSM
+ SOCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called msm_vibrator.
+
config INPUT_PCSPKR
tristate "PC Speaker support"
depends on PCSPKR_PLATFORM
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 9d0f9d1ff68f..96a6419cb1f2 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o
obj-$(CONFIG_INPUT_MC13783_PWRBUTTON) += mc13783-pwrbutton.o
obj-$(CONFIG_INPUT_MMA8450) += mma8450.o
+obj-$(CONFIG_INPUT_MSM_VIBRATOR) += msm-vibrator.o
obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
diff --git a/drivers/input/misc/msm-vibrator.c b/drivers/input/misc/msm-vibrator.c
new file mode 100644
index 000000000000..bb862f30a780
--- /dev/null
+++ b/drivers/input/misc/msm-vibrator.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Qualcomm MSM vibrator driver
+ *
+ * Copyright (c) 2018 Brian Masney <masneyb@onstation.org>
+ *
+ * Based on qcom,pwm-vibrator.c from:
+ * Copyright (c) 2018 Jonathan Marek <jonathan@marek.ca>
+ *
+ * Based on msm_pwm_vibrator.c from downstream Android sources:
+ * Copyright (C) 2009-2014 LGE, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#define REG_CMD_RCGR 0x00
+#define REG_CFG_RCGR 0x04
+#define REG_M 0x08
+#define REG_N 0x0C
+#define REG_D 0x10
+#define REG_CBCR 0x24
+#define MMSS_CC_M_DEFAULT 1
+
+struct msm_vibrator {
+ struct input_dev *input;
+ struct mutex mutex;
+ struct work_struct worker;
+ void __iomem *base;
+ struct regulator *vcc;
+ struct clk *clk;
+ struct gpio_desc *enable_gpio;
+ u16 magnitude;
+ bool enabled;
+};
+
+#define msm_vibrator_write(msm_vibrator, offset, value) \
+ writel((value), (void __iomem *)((msm_vibrator)->base + (offset)))
+
+static int msm_vibrator_start(struct msm_vibrator *vibrator)
+{
+ int d_reg_val, ret = 0;
+
+ mutex_lock(&vibrator->mutex);
+
+ if (!vibrator->enabled) {
+ ret = clk_set_rate(vibrator->clk, 24000);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to set clock rate: %d\n", ret);
+ goto unlock;
+ }
+
+ ret = clk_prepare_enable(vibrator->clk);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to enable clock: %d\n", ret);
+ goto unlock;
+ }
+
+ ret = regulator_enable(vibrator->vcc);
+ if (ret) {
+ dev_err(&vibrator->input->dev,
+ "Failed to enable regulator: %d\n", ret);
+ clk_disable(vibrator->clk);
+ goto unlock;
+ }
+
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
+
+ vibrator->enabled = true;
+ }
+
+ d_reg_val = 127 - ((126 * vibrator->magnitude) / 0xffff);
+ msm_vibrator_write(vibrator, REG_CFG_RCGR,
+ (2 << 12) | /* dual edge mode */
+ (0 << 8) | /* cxo */
+ (7 << 0));
+ msm_vibrator_write(vibrator, REG_M, 1);
+ msm_vibrator_write(vibrator, REG_N, 128);
+ msm_vibrator_write(vibrator, REG_D, d_reg_val);
+ msm_vibrator_write(vibrator, REG_CMD_RCGR, 1);
+ msm_vibrator_write(vibrator, REG_CBCR, 1);
+
+unlock:
+ mutex_unlock(&vibrator->mutex);
+
+ return ret;
+}
+
+static void msm_vibrator_stop(struct msm_vibrator *vibrator)
+{
+ mutex_lock(&vibrator->mutex);
+
+ if (vibrator->enabled) {
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
+ regulator_disable(vibrator->vcc);
+ clk_disable(vibrator->clk);
+ vibrator->enabled = false;
+ }
+
+ mutex_unlock(&vibrator->mutex);
+}
+
+static void msm_vibrator_worker(struct work_struct *work)
+{
+ struct msm_vibrator *vibrator = container_of(work,
+ struct msm_vibrator,
+ worker);
+
+ if (vibrator->magnitude)
+ msm_vibrator_start(vibrator);
+ else
+ msm_vibrator_stop(vibrator);
+}
+
+static int msm_vibrator_play_effect(struct input_dev *dev, void *data,
+ struct ff_effect *effect)
+{
+ struct msm_vibrator *vibrator = input_get_drvdata(dev);
+
+ mutex_lock(&vibrator->mutex);
+
+ if (effect->u.rumble.strong_magnitude > 0)
+ vibrator->magnitude = effect->u.rumble.strong_magnitude;
+ else
+ vibrator->magnitude = effect->u.rumble.weak_magnitude;
+
+ mutex_unlock(&vibrator->mutex);
+
+ schedule_work(&vibrator->worker);
+
+ return 0;
+}
+
+static void msm_vibrator_close(struct input_dev *input)
+{
+ struct msm_vibrator *vibrator = input_get_drvdata(input);
+
+ cancel_work_sync(&vibrator->worker);
+ msm_vibrator_stop(vibrator);
+}
+
+static int msm_vibrator_probe(struct platform_device *pdev)
+{
+ struct msm_vibrator *vibrator;
+ struct resource *res;
+ int ret;
+
+ vibrator = devm_kzalloc(&pdev->dev, sizeof(*vibrator), GFP_KERNEL);
+ if (!vibrator)
+ return -ENOMEM;
+
+ vibrator->input = devm_input_allocate_device(&pdev->dev);
+ if (!vibrator->input)
+ return -ENOMEM;
+
+ vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
+ if (IS_ERR(vibrator->vcc)) {
+ if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
+ PTR_ERR(vibrator->vcc));
+ return PTR_ERR(vibrator->vcc);
+ }
+
+ vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(vibrator->enable_gpio)) {
+ dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
+ PTR_ERR(vibrator->enable_gpio));
+ return PTR_ERR(vibrator->enable_gpio);
+ }
+
+ vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
+ if (IS_ERR(vibrator->clk)) {
+ dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
+ PTR_ERR(vibrator->clk));
+ return PTR_ERR(vibrator->clk);
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get platform resource\n");
+ return -ENODEV;
+ }
+
+ vibrator->base = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
+ if (IS_ERR(vibrator->base)) {
+ dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
+ PTR_ERR(vibrator->base));
+ return PTR_ERR(vibrator->base);
+ }
+
+ vibrator->enabled = false;
+ mutex_init(&vibrator->mutex);
+ INIT_WORK(&vibrator->worker, msm_vibrator_worker);
+
+ vibrator->input->name = "msm-vibrator";
+ vibrator->input->id.bustype = BUS_HOST;
+ vibrator->input->dev.parent = &pdev->dev;
+ vibrator->input->close = msm_vibrator_close;
+
+ input_set_drvdata(vibrator->input, vibrator);
+ input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
+
+ ret = input_ff_create_memless(vibrator->input, NULL,
+ msm_vibrator_play_effect);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
+ return ret;
+ }
+
+ ret = input_register_device(vibrator->input);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register input device: %d", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, vibrator);
+
+ return 0;
+}
+
+static int __maybe_unused msm_vibrator_suspend(struct device *dev)
+{
+ struct msm_vibrator *vibrator = dev_get_drvdata(dev);
+
+ cancel_work_sync(&vibrator->worker);
+
+ if (vibrator->enabled)
+ msm_vibrator_stop(vibrator);
+
+ return 0;
+}
+
+static int __maybe_unused msm_vibrator_resume(struct device *dev)
+{
+ struct msm_vibrator *vibrator = dev_get_drvdata(dev);
+
+ if (vibrator->enabled)
+ msm_vibrator_start(vibrator);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(msm_vibrator_pm_ops, msm_vibrator_suspend,
+ msm_vibrator_resume);
+
+static const struct of_device_id msm_vibrator_of_match[] = {
+ { .compatible = "qcom,msm8226-vibrator" },
+ { .compatible = "qcom,msm8974-vibrator" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, msm_vibrator_of_match);
+
+static struct platform_driver msm_vibrator_driver = {
+ .probe = msm_vibrator_probe,
+ .driver = {
+ .name = "msm-vibrator",
+ .pm = &msm_vibrator_pm_ops,
+ .of_match_table = of_match_ptr(msm_vibrator_of_match),
+ },
+};
+module_platform_driver(msm_vibrator_driver);
+
+MODULE_AUTHOR("Brian Masney <masneyb@onstation.org>");
+MODULE_DESCRIPTION("Qualcomm MSM vibrator driver");
+MODULE_LICENSE("GPL");
--
2.17.2
^ permalink raw reply related
* [PATCH v3 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Brian Masney @ 2018-10-25 1:29 UTC (permalink / raw)
To: dmitry.torokhov, andy.gross, david.brown
Cc: robh+dt, mark.rutland, masneyb, linux-input, linux-kernel,
linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20181025012937.2154-1-masneyb@onstation.org>
This patch adds device device tree bindings for the vibrator found on
the LG Nexus 5 (hammerhead) phone.
Signed-off-by: Brian Masney <masneyb@onstation.org>
---
.../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index ed8f064d0895..de956eea31ac 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -5,6 +5,7 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/input/input.h>
#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/clock/qcom,mmcc-msm8974.h>
/ {
model = "LGE MSM 8974 HAMMERHEAD";
@@ -268,6 +269,36 @@
input-enable;
};
};
+
+ vibrator_pin: vibrator {
+ pwm {
+ pins = "gpio27";
+ function = "gp1_clk";
+
+ drive-strength = <6>;
+ bias-disable;
+ };
+
+ enable {
+ pins = "gpio60";
+ function = "gpio";
+ };
+ };
+ };
+
+ vibrator@fd8c3450 {
+ compatible = "qcom,msm8974-vibrator";
+ reg = <0xfd8c3450 0x400>;
+
+ vcc-supply = <&pm8941_l19>;
+
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+ clock-names = "pwm";
+
+ enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&vibrator_pin>;
};
sdhci@f9824900 {
--
2.17.2
^ permalink raw reply related
* [regression, bisected] Keyboard not responding after resuming from suspend/hibernate
From: Pavel Machek @ 2018-10-25 7:49 UTC (permalink / raw)
To: Numan Demirdöğen, jason.low2, Waiman.Long, paulmck,
tglx
Cc: dmitry.torokhov, peterz, mingo, linux-kernel, linux-input
In-Reply-To: <20181019102031.77015d44@korsan.localdomain>
[-- Attachment #1: Type: text/plain, Size: 6682 bytes --]
Hi!
Here's problem bisected down to:
commit 9d659ae14b545c4296e812c70493bfdc999b5c1c
Author: Peter Zijlstra <peterz@infradead.org>
Date: Tue Aug 23 14:40:16 2016 +0200
locking/mutex: Add lock handoff to avoid starvation
Implement lock handoff to avoid lock starvation.
Numan, I assume revert of that patch on the 4.18 kernel still makes it
work?
Peter, any ideas?
Pavel
On Fri 2018-10-19 10:20:31, Numan Demirdöğen wrote:
> On Fri, 31 Aug 2018 21:53:11 +0300
> Numan Demirdöğen <if.gnu.linux@gmail.com> wrote:
>
> >If I put laptop to suspend or hibernate by closing lid, power
> >manager or any other method and then I resume/wake up laptop, keyboard
> >is not responding. My laptop is a Sony Vaio VPCEH2F1E.
> >
> >Steps to produce bug:
> >1. Boot
> >2. Put laptop to sleep
> >3. Resume
> >
> >What I expect to happen: Keyboard responds to key press.
> >What happens: Keyboard does not respond but mouse and trackball are
> >working.
> >
> >git bisect point 9d659ae14b545c4296e812c70493bfdc999b5c1c as the first
> >bad commit.
> >
> >Bad commit link:
> >https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=9d659ae14b545c4296e812c70493bfdc999b5c1c
> >
> > Link to actual bug report:
> >https://bugzilla.kernel.org/show_bug.cgi?id=195471
> >
> >awk -f ver_linux
> >Linux korsan 4.18.5-arch1-1-ARCH #1 SMP PREEMPT Fri Aug 24 12:48:58
> >UTC 2018 x86_64 GNU/Linux GNU C 8.2.0
> >GNU Make 4.2.1
> >Binutils 2.31.1
> >Util-linux 2.32.1
> >Mount 2.32.1
> >Module-init-tools 25
> >E2fsprogs 1.44.4
> >Jfsutils 1.1.15
> >Reiserfsprogs 3.6.27
> >Xfsprogs 4.17.0
> >Pcmciautils 018
> >Linux C Library 2.28
> >Dynamic linker (ldd) 2.28
> >Linux C++ Library 6.0.25
> >Procps 3.3.15
> >Kbd 2.0.4
> >Console-tools 2.0.4
> >Sh-utils 8.29
> >Udev 239
> >Wireless-tools 30
> >Modules Loaded ac agpgart ahci arc4 ath ath3k ath9k
> >ath9k_common ath9k_hw atkbd battery bluetooth bpfilter ccm cdrom
> >cfg80211 coretemp crc16 crc32c_generic crc32c_intel crc32_pclmul
> >crct10dif_pclmul cryptd drm drm_kms_helper ecdh_generic ehci_hcd
> >ehci_pci evdev ext4 fb_sys_fops fscrypto fuse ghash_clmulni_intel
> >gpio_ich hid hid_generic i2c_algo_bit i2c_i801 i8042 i915 input_leds
> >intel_cstate intel_gtt intel_powerclamp intel_rapl intel_rapl_perf
> >intel_uncore iptable_filter iptable_mangle iptable_nat ip_tables
> >irqbypass iTCO_vendor_support iTCO_wdt jbd2 kvm kvm_intel led_class
> >libahci libata libcrc32c libps2 lpc_ich mac80211 mac_hid mbcache mei
> >mei_me mousedev msr nf_conntrack nf_conntrack_ipv4 nf_defrag_ipv4
> >nf_nat nf_nat_ipv4 pcc_cpufreq psmouse rfkill rtc_cmos scsi_mod sd_mod
> >serio serio_raw snd snd_hda_codec snd_hda_codec_conexant
> >snd_hda_codec_generic snd_hda_codec_hdmi snd_hda_core snd_hda_intel
> >snd_hwdep snd_pcm snd_timer sony_laptop soundcore sr_mod syscopyarea
> >sysfillrect sysimgblt usb_common usbcore usbhid x86_pkg_temp_thermal
> >x_tables
> >
> >dmesg | grep i8042
> >[ 0.574078] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M]
> >at 0x60,0x64 irq 1,12 [ 0.575936] serio: i8042 KBD port at
> >0x60,0x64 irq 1 [ 0.576143] serio: i8042 AUX port at 0x60,0x64 irq
> >12 [ 0.618880] input: AT Translated Set 2 keyboard
> >as /devices/platform/i8042/serio0/input/input3 [ 11.248435] input:
> >AlpsPS/2 ALPS GlidePoint as /devices/platform/i8042/serio1/input/input5
> >
> >cat /proc/interrupts
> > CPU0 CPU1 CPU2 CPU3
> > 0: 8 0 0 0 IO-APIC
> > 2-edge timer 1: 0 0 0 10286
> > IO-APIC 1-edge i8042 8: 0 0
> > 0 1 IO-APIC 8-edge rtc0 9: 0
> > 9479 0 0 IO-APIC 9-fasteoi acpi 12:
> > 0 0 370114 0 IO-APIC 12-edge i8042
> > 16: 197457 0 0 0 IO-APIC
> > 16-fasteoi ehci_hcd:usb1, ath9k 19: 0 0
> > 0 0 IO-APIC 19-fasteoi i801_smbus 23:
> > 0 0 71055 0 IO-APIC 23-fasteoi
> > ehci_hcd:usb2 24: 0 74549 0 0
> > PCI-MSI 512000-edge ahci[0000:00:1f.2] 25: 0
> > 14 0 0 PCI-MSI 360448-edge mei_me
> > 26: 0 0 138870 0 PCI-MSI
> > 32768-edge i915 27: 0 0 0
> > 330 PCI-MSI 442368-edge snd_hda_intel:card0
> >NMI: 24 22 25 21 Non-maskable
> >interrupts LOC: 534734 446934 603337 440320 Local
> >timer interrupts SPU: 0 0 0 0
> >Spurious interrupts PMI: 24 22 25 21
> >Performance monitoring interrupts IWI: 26 12
> >43115 22 IRQ work interrupts RTR: 0
> >0 0 0 APIC ICR read retries RES: 95282
> >76970 60242 46409 Rescheduling interrupts CAL:
> >66746 75300 70554 69119 Function call interrupts
> >TLB: 50929 54528 51413 48017 TLB shootdowns
> >TRM: 0 0 0 0 Thermal event
> >interrupts THR: 0 0 0 0
> >Threshold APIC interrupts DFR: 0 0
> >0 0 Deferred Error APIC interrupts MCE: 0
> >0 0 0 Machine check exceptions MCP:
> >18 19 19 19 Machine check polls
> >HYP: 0 0 0 0 Hypervisor callback
> >interrupts HRE: 0 0 0 0 Hyper-V
> >reenlightenment interrupts HVS: 0 0
> >0 0 Hyper-V stimer0 interrupts ERR: 0
> >MIS: 0 PIN: 0 0 0 0
> >Posted-interrupt notification event NPI: 0
> >0 0 0 Nested posted-interrupt event PIW:
> >0 0 0 0 Posted-interrupt wakeup event
> >
> >P.S: My native language is not English.
> >
>
> Hi,
>
> I am forwarding this e-mail which describes a problem related to
> suspend/hibernate at the advise of Pavel Machek.
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/8] dt-bindings: mfd: document stpmic1
From: Lee Jones @ 2018-10-25 9:44 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lgirdwood@gmail.com, broonie@kernel.org,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <1539853324-29051-2-git-send-email-p.paillet@st.com>
Rob: please grep your name for some feedback.
On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
Please use `git send-email` to send patches.
Please capitalise your name: Pascal Paillet
> stpmic1 is a pmic from STMicroelectronics. The STPMIC1 integrates 10
"STPMIC1"
"PMIC"
> regulators , 3 switches, a watchdog and an input for a power on key.
Whitespace.
What is a switch?
> Signed-off-by: pascal paillet <p.paillet@st.com>
> ---
> changes in v4:
> * remove interrupt-parent description
> * pmic1@33 renamed to pmic@33
> * fix indentation
>
> .../devicetree/bindings/mfd/st,stpmic1.txt | 131 +++++++++++++++++++++
> include/dt-bindings/mfd/st,stpmic1.h | 46 ++++++++
> 2 files changed, 177 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> create mode 100644 include/dt-bindings/mfd/st,stpmic1.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/st,stpmic1.txt b/Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> new file mode 100644
> index 0000000..bb19cc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> @@ -0,0 +1,131 @@
> +* STMicroelectronics STPMIC1 Power Management IC
> +
> +Required parent device properties:
> +- compatible: "st,stpmic1"
> +- reg: The I2C slave address for the STPMIC1 chip.
> +- interrupts: The interrupt line the device is connected to.
> +- #interrupt-cells: Should be 1.
> +- interrupt-controller: Describes the STPMIC1 as an interrupt
Tabbing (this could just be a patch quirk).
Either use full-stops or don't - please be consistent.
> + controller (has its own domain). Interrupt number are the following:
"numbers"
Actually consider rewording - sounds funny.
> + /* Interrupt Register 1 (0x50 for latch) */
> + IT_SWOUT_R=0
> + IT_SWOUT_F=1
> + IT_VBUS_OTG_R=2
> + IT_VBUS_OTG_F=3
> + IT_WAKEUP_R=4
> + IT_WAKEUP_F=5
> + IT_PONKEY_R=6
> + IT_PONKEY_F=7
> + /* Interrupt Register 2 (0x51 for latch) */
> + IT_OVP_BOOST=8
> + IT_OCP_BOOST=9
> + IT_OCP_SWOUT=10
> + IT_OCP_OTG=11
> + IT_CURLIM_BUCK4=12
> + IT_CURLIM_BUCK3=13
> + IT_CURLIM_BUCK2=14
> + IT_CURLIM_BUCK1=15
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SHORT_SWOUT=16
> + IT_SHORT_SWOTG=17
> + IT_CURLIM_LDO6=18
> + IT_CURLIM_LDO5=19
> + IT_CURLIM_LDO4=20
> + IT_CURLIM_LDO3=21
> + IT_CURLIM_LDO2=22
> + IT_CURLIM_LDO1=23
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SWIN_R=24
> + IT_SWIN_F=25
> + IT_RESERVED_1=26
> + IT_RESERVED_2=27
> + IT_VINLOW_R=28
> + IT_VINLOW_F=29
> + IT_TWARN_R=30
> + IT_TWARN_F=31
Do you really need these in here? I suspect not.
> +Optional parent device properties:
> +- st,main-control-register:
> + -bit 1: Power cycling will be performed on turn OFF condition
> + -bit 2: PWRCTRL is functional
> + -bit 3: PWRCTRL active high
> +- st,pads-pull-register:
> + -bit 1: WAKEUP pull down is not active
> + -bit 2: PWRCTRL pull up is active
> + -bit 3: PWRCTRL pull down is active
> + -bit 4: WAKEUP detector is disabled
> +- st,vin-control-register:
> + -bit 0: VINLOW monitoring is enabled
> + -bit [1...3]: VINLOW rising threshold
> + 000 VINOK_f + 50mV
> + 001 VINOK_f + 100mV
> + 010 VINOK_f + 150mV
> + 011 VINOK_f + 200mV
> + 100 VINOK_f + 250mV
> + 101 VINOK_f + 300mV
> + 110 VINOK_f + 350mV
> + 111 VINOK_f + 400mV
> + -bit [4...5]: VINLOW hyst
> + 00 100mV
> + 01 200mV
> + 10 300mV
> + 11 400mV
> + -bit 6: SW_OUT detector is disabled
> + -bit 7: SW_IN detector is enabled.
> +- st,usb-control-register:
> + -bit 3: SW_OUT current limit
> + 0: 600mA
> + 1: 1.1A
> + -bit 4: VBUS_OTG discharge is enabled
> + -bit 5: SW_OUT discharge is enabled
> + -bit 6: VBUS_OTG detection is enabled
> + -bit 7: BOOST_OVP is disabled
I'm surprised Rob allowed you to add register bits in a DT property?
> +STPMIC1 consists in a varied group of sub-devices.
> +Each sub-device binding is be described in own documentation file.
> +
> +Device Description
> +------ ------------
> +st,stpmic1-onkey : Power on key, see ../input/st,stpmic1-onkey.txt
> +st,stpmic1-regulators : Regulators, see ../regulator/st,stpmic1-regulator.txt
> +st,stpmic1-wdt : Watchdog, see ../watchdog/st,stpmic1-wdt.txt
> +
> +Example:
> +
> +pmic: pmic@33 {
> + compatible = "st,stpmic1";
> + reg = <0x33>;
> + interrupt-parent = <&gpioa>;
> + interrupts = <0 2>;
> + st,main-control-register=<0x0c>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> +
> + onkey {
> + compatible = "st,stpmic1-onkey";
> + interrupts = <IT_PONKEY_F 0>,<IT_PONKEY_R 1>;
> + interrupt-names = "onkey-falling", "onkey-rising";
> + power-off-time-sec = <10>;
> + };
> +
> + watchdog {
> + compatible = "st,stpmic1-wdt";
> + };
> +
> + regulators {
> + compatible = "st,stpmic1-regulators";
> +
> + vdd_core: buck1 {
> + regulator-name = "vdd_core";
> + regulator-boot-on;
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1200000>;
> + };
> + vdd: buck3 {
> + regulator-name = "vdd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-pull-down;
> + };
> + };
> diff --git a/include/dt-bindings/mfd/st,stpmic1.h b/include/dt-bindings/mfd/st,stpmic1.h
> new file mode 100644
> index 0000000..b2d6c83
> --- /dev/null
> +++ b/include/dt-bindings/mfd/st,stpmic1.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
> + * Author: Philippe Peurichard <philippe.peurichard@st.com>,
> + * Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
> + */
> +
> +#ifndef __DT_BINDINGS_STPMIC1_H__
> +#define __DT_BINDINGS_STPMIC1_H__
> +
> +/* IRQ definitions */
> +#define IT_PONKEY_F 0
> +#define IT_PONKEY_R 1
> +#define IT_WAKEUP_F 2
> +#define IT_WAKEUP_R 3
> +#define IT_VBUS_OTG_F 4
> +#define IT_VBUS_OTG_R 5
> +#define IT_SWOUT_F 6
> +#define IT_SWOUT_R 7
> +
> +#define IT_CURLIM_BUCK1 8
> +#define IT_CURLIM_BUCK2 9
> +#define IT_CURLIM_BUCK3 10
> +#define IT_CURLIM_BUCK4 11
> +#define IT_OCP_OTG 12
> +#define IT_OCP_SWOUT 13
> +#define IT_OCP_BOOST 14
> +#define IT_OVP_BOOST 15
> +
> +#define IT_CURLIM_LDO1 16
> +#define IT_CURLIM_LDO2 17
> +#define IT_CURLIM_LDO3 18
> +#define IT_CURLIM_LDO4 19
> +#define IT_CURLIM_LDO5 20
> +#define IT_CURLIM_LDO6 21
> +#define IT_SHORT_SWOTG 22
> +#define IT_SHORT_SWOUT 23
> +
> +#define IT_TWARN_F 24
> +#define IT_TWARN_R 25
> +#define IT_VINLOW_F 26
> +#define IT_VINLOW_R 27
> +#define IT_SWIN_F 30
> +#define IT_SWIN_R 31
> +
> +#endif /* __DT_BINDINGS_STPMIC1_H__ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 2/8] mfd: stpmic1: add stpmic1 driver
From: Lee Jones @ 2018-10-25 11:21 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lgirdwood@gmail.com, broonie@kernel.org,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <1539853324-29051-3-git-send-email-p.paillet@st.com>
On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
>
> stpmic1 is a pmic from STMicroelectronics. The STPMIC1 integrates 10
> regulators , 3 switches, a watchdog and an input for a power on key.
Same comments as for the DT binding patch.
> Signed-off-by: pascal paillet <p.paillet@st.com>
> ---
> changes in v4:
> * rename PONKEY_PU_ACTIVE to PONKEY_PU_INACTIVE
>
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/stpmic1.c | 401 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/stpmic1.h | 212 +++++++++++++++++++++++
> 4 files changed, 627 insertions(+)
> create mode 100644 drivers/mfd/stpmic1.c
> create mode 100644 include/linux/mfd/stpmic1.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 11841f4..b8dabc7 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1855,6 +1855,19 @@ config MFD_STM32_TIMERS
> for PWM and IIO Timer. This driver allow to share the
> registers between the others drivers.
>
> +config MFD_STPMIC1
> + tristate "Support for STPMIC1 PMIC"
> + depends on (I2C=y && OF)
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Support for ST Microelectronics STPMIC1 PMIC. STPMIC1 MFD driver is
Remove 'MFD' and replace with something else.
MFD is not a real thing. It's a Linuxisum.
> + the core driver for STPMIC1 component that mainly handles interrupts.
You need to document what the child devices are.
> + To compile this driver as a module, choose M here: the
> + module will be called stpmic1.
> +
> menu "Multimedia Capabilities Port drivers"
> depends on ARCH_SA1100
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5856a94..76fff14 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -232,6 +232,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
>
> obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
> +obj-$(CONFIG_MFD_STPMIC1) += stpmic1.o
> obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
>
> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
> diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c
> new file mode 100644
> index 0000000..90dfee4
> --- /dev/null
> +++ b/drivers/mfd/stpmic1.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
'\n' here.
> +// Author: Pascal Paillet <p.paillet@st.com>
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/stpmic1.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/mfd/st,stpmic1.h>
> +
> +#define STPMIC1_MAIN_IRQ 0
> +#define STPMIC1_WAKEUP_IRQ 1
> +
> +static bool stpmic1_reg_readable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case TURN_ON_SR:
> + case TURN_OFF_SR:
> + case ICC_LDO_TURN_OFF_SR:
> + case ICC_BUCK_TURN_OFF_SR:
> + case RREQ_STATE_SR:
> + case VERSION_SR:
> + case SWOFF_PWRCTRL_CR:
> + case PADS_PULL_CR:
> + case BUCKS_PD_CR:
> + case LDO14_PD_CR:
> + case LDO56_VREF_PD_CR:
> + case VBUS_DET_VIN_CR:
> + case PKEY_TURNOFF_CR:
> + case BUCKS_MASK_RANK_CR:
> + case BUCKS_MASK_RESET_CR:
> + case LDOS_MASK_RANK_CR:
> + case LDOS_MASK_RESET_CR:
> + case WCHDG_CR:
> + case WCHDG_TIMER_CR:
> + case BUCKS_ICCTO_CR:
> + case LDOS_ICCTO_CR:
> + case BUCK1_ACTIVE_CR:
> + case BUCK2_ACTIVE_CR:
> + case BUCK3_ACTIVE_CR:
> + case BUCK4_ACTIVE_CR:
> + case VREF_DDR_ACTIVE_CR:
> + case LDO1_ACTIVE_CR:
> + case LDO2_ACTIVE_CR:
> + case LDO3_ACTIVE_CR:
> + case LDO4_ACTIVE_CR:
> + case LDO5_ACTIVE_CR:
> + case LDO6_ACTIVE_CR:
> + case BUCK1_STDBY_CR:
> + case BUCK2_STDBY_CR:
> + case BUCK3_STDBY_CR:
> + case BUCK4_STDBY_CR:
> + case VREF_DDR_STDBY_CR:
> + case LDO1_STDBY_CR:
> + case LDO2_STDBY_CR:
> + case LDO3_STDBY_CR:
> + case LDO4_STDBY_CR:
> + case LDO5_STDBY_CR:
> + case LDO6_STDBY_CR:
> + case BST_SW_CR:
> + case INT_PENDING_R1:
> + case INT_PENDING_R2:
> + case INT_PENDING_R3:
> + case INT_PENDING_R4:
> + case INT_DBG_LATCH_R1:
> + case INT_DBG_LATCH_R2:
> + case INT_DBG_LATCH_R3:
> + case INT_DBG_LATCH_R4:
> + case INT_CLEAR_R1:
> + case INT_CLEAR_R2:
> + case INT_CLEAR_R3:
> + case INT_CLEAR_R4:
> + case INT_MASK_R1:
> + case INT_MASK_R2:
> + case INT_MASK_R3:
> + case INT_MASK_R4:
> + case INT_SET_MASK_R1:
> + case INT_SET_MASK_R2:
> + case INT_SET_MASK_R3:
> + case INT_SET_MASK_R4:
> + case INT_CLEAR_MASK_R1:
> + case INT_CLEAR_MASK_R2:
> + case INT_CLEAR_MASK_R3:
> + case INT_CLEAR_MASK_R4:
> + case INT_SRC_R1:
> + case INT_SRC_R2:
> + case INT_SRC_R3:
> + case INT_SRC_R4:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool stpmic1_reg_writeable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case SWOFF_PWRCTRL_CR:
> + case PADS_PULL_CR:
> + case BUCKS_PD_CR:
> + case LDO14_PD_CR:
> + case LDO56_VREF_PD_CR:
> + case VBUS_DET_VIN_CR:
> + case PKEY_TURNOFF_CR:
> + case BUCKS_MASK_RANK_CR:
> + case BUCKS_MASK_RESET_CR:
> + case LDOS_MASK_RANK_CR:
> + case LDOS_MASK_RESET_CR:
> + case WCHDG_CR:
> + case WCHDG_TIMER_CR:
> + case BUCKS_ICCTO_CR:
> + case LDOS_ICCTO_CR:
> + case BUCK1_ACTIVE_CR:
> + case BUCK2_ACTIVE_CR:
> + case BUCK3_ACTIVE_CR:
> + case BUCK4_ACTIVE_CR:
> + case VREF_DDR_ACTIVE_CR:
> + case LDO1_ACTIVE_CR:
> + case LDO2_ACTIVE_CR:
> + case LDO3_ACTIVE_CR:
> + case LDO4_ACTIVE_CR:
> + case LDO5_ACTIVE_CR:
> + case LDO6_ACTIVE_CR:
> + case BUCK1_STDBY_CR:
> + case BUCK2_STDBY_CR:
> + case BUCK3_STDBY_CR:
> + case BUCK4_STDBY_CR:
> + case VREF_DDR_STDBY_CR:
> + case LDO1_STDBY_CR:
> + case LDO2_STDBY_CR:
> + case LDO3_STDBY_CR:
> + case LDO4_STDBY_CR:
> + case LDO5_STDBY_CR:
> + case LDO6_STDBY_CR:
> + case BST_SW_CR:
> + case INT_DBG_LATCH_R1:
> + case INT_DBG_LATCH_R2:
> + case INT_DBG_LATCH_R3:
> + case INT_DBG_LATCH_R4:
> + case INT_CLEAR_R1:
> + case INT_CLEAR_R2:
> + case INT_CLEAR_R3:
> + case INT_CLEAR_R4:
> + case INT_SET_MASK_R1:
> + case INT_SET_MASK_R2:
> + case INT_SET_MASK_R3:
> + case INT_SET_MASK_R4:
> + case INT_CLEAR_MASK_R1:
> + case INT_CLEAR_MASK_R2:
> + case INT_CLEAR_MASK_R3:
> + case INT_CLEAR_MASK_R4:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool stpmic1_reg_volatile(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case TURN_ON_SR:
> + case TURN_OFF_SR:
> + case ICC_LDO_TURN_OFF_SR:
> + case ICC_BUCK_TURN_OFF_SR:
> + case RREQ_STATE_SR:
> + case INT_PENDING_R1:
> + case INT_PENDING_R2:
> + case INT_PENDING_R3:
> + case INT_PENDING_R4:
> + case INT_SRC_R1:
> + case INT_SRC_R2:
> + case INT_SRC_R3:
> + case INT_SRC_R4:
> + case WCHDG_CR:
> + return true;
> + default:
> + return false;
> + }
> +}
Can you use ranges for all of these?
> +const struct regmap_config stpmic1_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .max_register = PMIC_MAX_REGISTER_ADDRESS,
> + .readable_reg = stpmic1_reg_readable,
> + .writeable_reg = stpmic1_reg_writeable,
> + .volatile_reg = stpmic1_reg_volatile,
> +};
> +
> +static const struct regmap_irq stpmic1_irqs[] = {
> + [IT_PONKEY_F] = { .mask = 0x01 },
> + [IT_PONKEY_R] = { .mask = 0x02 },
> + [IT_WAKEUP_F] = { .mask = 0x04 },
> + [IT_WAKEUP_R] = { .mask = 0x08 },
> + [IT_VBUS_OTG_F] = { .mask = 0x10 },
> + [IT_VBUS_OTG_R] = { .mask = 0x20 },
> + [IT_SWOUT_F] = { .mask = 0x40 },
> + [IT_SWOUT_R] = { .mask = 0x80 },
> +
> + [IT_CURLIM_BUCK1] = { .reg_offset = 1, .mask = 0x01 },
> + [IT_CURLIM_BUCK2] = { .reg_offset = 1, .mask = 0x02 },
> + [IT_CURLIM_BUCK3] = { .reg_offset = 1, .mask = 0x04 },
> + [IT_CURLIM_BUCK4] = { .reg_offset = 1, .mask = 0x08 },
> + [IT_OCP_OTG] = { .reg_offset = 1, .mask = 0x10 },
> + [IT_OCP_SWOUT] = { .reg_offset = 1, .mask = 0x20 },
> + [IT_OCP_BOOST] = { .reg_offset = 1, .mask = 0x40 },
> + [IT_OVP_BOOST] = { .reg_offset = 1, .mask = 0x80 },
> +
> + [IT_CURLIM_LDO1] = { .reg_offset = 2, .mask = 0x01 },
> + [IT_CURLIM_LDO2] = { .reg_offset = 2, .mask = 0x02 },
> + [IT_CURLIM_LDO3] = { .reg_offset = 2, .mask = 0x04 },
> + [IT_CURLIM_LDO4] = { .reg_offset = 2, .mask = 0x08 },
> + [IT_CURLIM_LDO5] = { .reg_offset = 2, .mask = 0x10 },
> + [IT_CURLIM_LDO6] = { .reg_offset = 2, .mask = 0x20 },
> + [IT_SHORT_SWOTG] = { .reg_offset = 2, .mask = 0x40 },
> + [IT_SHORT_SWOUT] = { .reg_offset = 2, .mask = 0x80 },
> +
> + [IT_TWARN_F] = { .reg_offset = 3, .mask = 0x01 },
> + [IT_TWARN_R] = { .reg_offset = 3, .mask = 0x02 },
> + [IT_VINLOW_F] = { .reg_offset = 3, .mask = 0x04 },
> + [IT_VINLOW_R] = { .reg_offset = 3, .mask = 0x08 },
> + [IT_SWIN_F] = { .reg_offset = 3, .mask = 0x40 },
> + [IT_SWIN_R] = { .reg_offset = 3, .mask = 0x80 },
> +};
There should be a MACRO for doing this.
If there isn't, you should author one and put it in the Regmap header.
> +static const struct regmap_irq_chip stpmic1_regmap_irq_chip = {
> + .name = "pmic_irq",
> + .status_base = INT_PENDING_R1,
> + .mask_base = INT_CLEAR_MASK_R1,
> + .unmask_base = INT_SET_MASK_R1,
> + .ack_base = INT_CLEAR_R1,
> + .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS,
> + .irqs = stpmic1_irqs,
> + .num_irqs = ARRAY_SIZE(stpmic1_irqs),
> +};
> +
> +static int stpmic1_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct stpmic1 *ddata;
> + struct device *dev = &i2c->dev;
> + int ret;
> + struct device_node *np = dev->of_node;
> + u32 reg;
> +
> + ddata = devm_kzalloc(dev, sizeof(struct stpmic1), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, ddata);
> + ddata->dev = dev;
> +
> + ddata->regmap = devm_regmap_init_i2c(i2c, &stpmic1_regmap_config);
> + if (IS_ERR(ddata->regmap))
> + return PTR_ERR(ddata->regmap);
> +
> + ddata->irq = of_irq_get(np, STPMIC1_MAIN_IRQ);
> + if (ddata->irq < 0) {
> + dev_err(dev, "Failed to get main IRQ: %d\n", ddata->irq);
> + return ddata->irq;
> + }
> +
> + if (!of_property_read_u32(np, "st,main-control-register", ®)) {
I'm still waiting on feedback from Rob as to whether this is
acceptable. I suggest that it isn't.
> + ret = regmap_update_bits(ddata->regmap,
> + SWOFF_PWRCTRL_CR,
> + PWRCTRL_POLARITY_HIGH |
> + PWRCTRL_PIN_VALID |
> + RESTART_REQUEST_ENABLED,
> + reg);
> + if (ret) {
> + dev_err(dev,
> + "Failed to update main control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + /* Read Version ID */
> + ret = regmap_read(ddata->regmap, VERSION_SR, ®);
> + if (ret) {
> + dev_err(dev, "Unable to read pmic version\n");
"PMIC"
> + return ret;
> + }
> + dev_info(dev, "PMIC Chip Version: 0x%x\n", reg);
[...]
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 2/8] mfd: stpmic1: add stpmic1 driver
From: Benjamin Gaignard @ 2018-10-25 12:40 UTC (permalink / raw)
To: Lee Jones
Cc: pascal paillet, dmitry.torokhov, Rob Herring, Mark Rutland,
Liam Girdwood, Mark Brown, wim, Guenter Roeck, linux-input,
devicetree, Linux Kernel Mailing List, linux-watchdog, eballetbo
In-Reply-To: <20181025112156.GB4870@dell>
Le jeu. 25 oct. 2018 à 13:21, Lee Jones <lee.jones@linaro.org> a écrit :
>
> On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
>
> > From: pascal paillet <p.paillet@st.com>
> >
> > stpmic1 is a pmic from STMicroelectronics. The STPMIC1 integrates 10
> > regulators , 3 switches, a watchdog and an input for a power on key.
>
> Same comments as for the DT binding patch.
>
> > Signed-off-by: pascal paillet <p.paillet@st.com>
> > ---
> > changes in v4:
> > * rename PONKEY_PU_ACTIVE to PONKEY_PU_INACTIVE
> >
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/stpmic1.c | 401 ++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/stpmic1.h | 212 +++++++++++++++++++++++
> > 4 files changed, 627 insertions(+)
> > create mode 100644 drivers/mfd/stpmic1.c
> > create mode 100644 include/linux/mfd/stpmic1.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 11841f4..b8dabc7 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1855,6 +1855,19 @@ config MFD_STM32_TIMERS
> > for PWM and IIO Timer. This driver allow to share the
> > registers between the others drivers.
> >
> > +config MFD_STPMIC1
> > + tristate "Support for STPMIC1 PMIC"
> > + depends on (I2C=y && OF)
> > + select REGMAP_I2C
> > + select REGMAP_IRQ
> > + select MFD_CORE
> > + help
> > + Support for ST Microelectronics STPMIC1 PMIC. STPMIC1 MFD driver is
>
> Remove 'MFD' and replace with something else.
>
> MFD is not a real thing. It's a Linuxisum.
>
> > + the core driver for STPMIC1 component that mainly handles interrupts.
>
> You need to document what the child devices are.
>
> > + To compile this driver as a module, choose M here: the
> > + module will be called stpmic1.
> > +
> > menu "Multimedia Capabilities Port drivers"
> > depends on ARCH_SA1100
> >
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5856a94..76fff14 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -232,6 +232,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
> > obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
> >
> > obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
> > +obj-$(CONFIG_MFD_STPMIC1) += stpmic1.o
> > obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
> >
> > obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
> > diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c
> > new file mode 100644
> > index 0000000..90dfee4
> > --- /dev/null
> > +++ b/drivers/mfd/stpmic1.c
> > @@ -0,0 +1,401 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) STMicroelectronics 2018
>
> '\n' here.
>
> > +// Author: Pascal Paillet <p.paillet@st.com>
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/stpmic1.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm_wakeirq.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <dt-bindings/mfd/st,stpmic1.h>
> > +
> > +#define STPMIC1_MAIN_IRQ 0
> > +#define STPMIC1_WAKEUP_IRQ 1
> > +
> > +static bool stpmic1_reg_readable(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case TURN_ON_SR:
> > + case TURN_OFF_SR:
> > + case ICC_LDO_TURN_OFF_SR:
> > + case ICC_BUCK_TURN_OFF_SR:
> > + case RREQ_STATE_SR:
> > + case VERSION_SR:
> > + case SWOFF_PWRCTRL_CR:
> > + case PADS_PULL_CR:
> > + case BUCKS_PD_CR:
> > + case LDO14_PD_CR:
> > + case LDO56_VREF_PD_CR:
> > + case VBUS_DET_VIN_CR:
> > + case PKEY_TURNOFF_CR:
> > + case BUCKS_MASK_RANK_CR:
> > + case BUCKS_MASK_RESET_CR:
> > + case LDOS_MASK_RANK_CR:
> > + case LDOS_MASK_RESET_CR:
> > + case WCHDG_CR:
> > + case WCHDG_TIMER_CR:
> > + case BUCKS_ICCTO_CR:
> > + case LDOS_ICCTO_CR:
> > + case BUCK1_ACTIVE_CR:
> > + case BUCK2_ACTIVE_CR:
> > + case BUCK3_ACTIVE_CR:
> > + case BUCK4_ACTIVE_CR:
> > + case VREF_DDR_ACTIVE_CR:
> > + case LDO1_ACTIVE_CR:
> > + case LDO2_ACTIVE_CR:
> > + case LDO3_ACTIVE_CR:
> > + case LDO4_ACTIVE_CR:
> > + case LDO5_ACTIVE_CR:
> > + case LDO6_ACTIVE_CR:
> > + case BUCK1_STDBY_CR:
> > + case BUCK2_STDBY_CR:
> > + case BUCK3_STDBY_CR:
> > + case BUCK4_STDBY_CR:
> > + case VREF_DDR_STDBY_CR:
> > + case LDO1_STDBY_CR:
> > + case LDO2_STDBY_CR:
> > + case LDO3_STDBY_CR:
> > + case LDO4_STDBY_CR:
> > + case LDO5_STDBY_CR:
> > + case LDO6_STDBY_CR:
> > + case BST_SW_CR:
> > + case INT_PENDING_R1:
> > + case INT_PENDING_R2:
> > + case INT_PENDING_R3:
> > + case INT_PENDING_R4:
> > + case INT_DBG_LATCH_R1:
> > + case INT_DBG_LATCH_R2:
> > + case INT_DBG_LATCH_R3:
> > + case INT_DBG_LATCH_R4:
> > + case INT_CLEAR_R1:
> > + case INT_CLEAR_R2:
> > + case INT_CLEAR_R3:
> > + case INT_CLEAR_R4:
> > + case INT_MASK_R1:
> > + case INT_MASK_R2:
> > + case INT_MASK_R3:
> > + case INT_MASK_R4:
> > + case INT_SET_MASK_R1:
> > + case INT_SET_MASK_R2:
> > + case INT_SET_MASK_R3:
> > + case INT_SET_MASK_R4:
> > + case INT_CLEAR_MASK_R1:
> > + case INT_CLEAR_MASK_R2:
> > + case INT_CLEAR_MASK_R3:
> > + case INT_CLEAR_MASK_R4:
> > + case INT_SRC_R1:
> > + case INT_SRC_R2:
> > + case INT_SRC_R3:
> > + case INT_SRC_R4:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static bool stpmic1_reg_writeable(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case SWOFF_PWRCTRL_CR:
> > + case PADS_PULL_CR:
> > + case BUCKS_PD_CR:
> > + case LDO14_PD_CR:
> > + case LDO56_VREF_PD_CR:
> > + case VBUS_DET_VIN_CR:
> > + case PKEY_TURNOFF_CR:
> > + case BUCKS_MASK_RANK_CR:
> > + case BUCKS_MASK_RESET_CR:
> > + case LDOS_MASK_RANK_CR:
> > + case LDOS_MASK_RESET_CR:
> > + case WCHDG_CR:
> > + case WCHDG_TIMER_CR:
> > + case BUCKS_ICCTO_CR:
> > + case LDOS_ICCTO_CR:
> > + case BUCK1_ACTIVE_CR:
> > + case BUCK2_ACTIVE_CR:
> > + case BUCK3_ACTIVE_CR:
> > + case BUCK4_ACTIVE_CR:
> > + case VREF_DDR_ACTIVE_CR:
> > + case LDO1_ACTIVE_CR:
> > + case LDO2_ACTIVE_CR:
> > + case LDO3_ACTIVE_CR:
> > + case LDO4_ACTIVE_CR:
> > + case LDO5_ACTIVE_CR:
> > + case LDO6_ACTIVE_CR:
> > + case BUCK1_STDBY_CR:
> > + case BUCK2_STDBY_CR:
> > + case BUCK3_STDBY_CR:
> > + case BUCK4_STDBY_CR:
> > + case VREF_DDR_STDBY_CR:
> > + case LDO1_STDBY_CR:
> > + case LDO2_STDBY_CR:
> > + case LDO3_STDBY_CR:
> > + case LDO4_STDBY_CR:
> > + case LDO5_STDBY_CR:
> > + case LDO6_STDBY_CR:
> > + case BST_SW_CR:
> > + case INT_DBG_LATCH_R1:
> > + case INT_DBG_LATCH_R2:
> > + case INT_DBG_LATCH_R3:
> > + case INT_DBG_LATCH_R4:
> > + case INT_CLEAR_R1:
> > + case INT_CLEAR_R2:
> > + case INT_CLEAR_R3:
> > + case INT_CLEAR_R4:
> > + case INT_SET_MASK_R1:
> > + case INT_SET_MASK_R2:
> > + case INT_SET_MASK_R3:
> > + case INT_SET_MASK_R4:
> > + case INT_CLEAR_MASK_R1:
> > + case INT_CLEAR_MASK_R2:
> > + case INT_CLEAR_MASK_R3:
> > + case INT_CLEAR_MASK_R4:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static bool stpmic1_reg_volatile(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case TURN_ON_SR:
> > + case TURN_OFF_SR:
> > + case ICC_LDO_TURN_OFF_SR:
> > + case ICC_BUCK_TURN_OFF_SR:
> > + case RREQ_STATE_SR:
> > + case INT_PENDING_R1:
> > + case INT_PENDING_R2:
> > + case INT_PENDING_R3:
> > + case INT_PENDING_R4:
> > + case INT_SRC_R1:
> > + case INT_SRC_R2:
> > + case INT_SRC_R3:
> > + case INT_SRC_R4:
> > + case WCHDG_CR:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
>
> Can you use ranges for all of these?
>
> > +const struct regmap_config stpmic1_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .cache_type = REGCACHE_RBTREE,
> > + .max_register = PMIC_MAX_REGISTER_ADDRESS,
> > + .readable_reg = stpmic1_reg_readable,
> > + .writeable_reg = stpmic1_reg_writeable,
> > + .volatile_reg = stpmic1_reg_volatile,
> > +};
> > +
> > +static const struct regmap_irq stpmic1_irqs[] = {
> > + [IT_PONKEY_F] = { .mask = 0x01 },
> > + [IT_PONKEY_R] = { .mask = 0x02 },
> > + [IT_WAKEUP_F] = { .mask = 0x04 },
> > + [IT_WAKEUP_R] = { .mask = 0x08 },
> > + [IT_VBUS_OTG_F] = { .mask = 0x10 },
> > + [IT_VBUS_OTG_R] = { .mask = 0x20 },
> > + [IT_SWOUT_F] = { .mask = 0x40 },
> > + [IT_SWOUT_R] = { .mask = 0x80 },
> > +
> > + [IT_CURLIM_BUCK1] = { .reg_offset = 1, .mask = 0x01 },
> > + [IT_CURLIM_BUCK2] = { .reg_offset = 1, .mask = 0x02 },
> > + [IT_CURLIM_BUCK3] = { .reg_offset = 1, .mask = 0x04 },
> > + [IT_CURLIM_BUCK4] = { .reg_offset = 1, .mask = 0x08 },
> > + [IT_OCP_OTG] = { .reg_offset = 1, .mask = 0x10 },
> > + [IT_OCP_SWOUT] = { .reg_offset = 1, .mask = 0x20 },
> > + [IT_OCP_BOOST] = { .reg_offset = 1, .mask = 0x40 },
> > + [IT_OVP_BOOST] = { .reg_offset = 1, .mask = 0x80 },
> > +
> > + [IT_CURLIM_LDO1] = { .reg_offset = 2, .mask = 0x01 },
> > + [IT_CURLIM_LDO2] = { .reg_offset = 2, .mask = 0x02 },
> > + [IT_CURLIM_LDO3] = { .reg_offset = 2, .mask = 0x04 },
> > + [IT_CURLIM_LDO4] = { .reg_offset = 2, .mask = 0x08 },
> > + [IT_CURLIM_LDO5] = { .reg_offset = 2, .mask = 0x10 },
> > + [IT_CURLIM_LDO6] = { .reg_offset = 2, .mask = 0x20 },
> > + [IT_SHORT_SWOTG] = { .reg_offset = 2, .mask = 0x40 },
> > + [IT_SHORT_SWOUT] = { .reg_offset = 2, .mask = 0x80 },
> > +
> > + [IT_TWARN_F] = { .reg_offset = 3, .mask = 0x01 },
> > + [IT_TWARN_R] = { .reg_offset = 3, .mask = 0x02 },
> > + [IT_VINLOW_F] = { .reg_offset = 3, .mask = 0x04 },
> > + [IT_VINLOW_R] = { .reg_offset = 3, .mask = 0x08 },
> > + [IT_SWIN_F] = { .reg_offset = 3, .mask = 0x40 },
> > + [IT_SWIN_R] = { .reg_offset = 3, .mask = 0x80 },
> > +};
>
> There should be a MACRO for doing this.
>
> If there isn't, you should author one and put it in the Regmap header.
Hi Lee,
I don't understand why you want to put this MACRO in regmap header.
Offsets and masks are custom from this hardware block.
How can this become generic enough to be put in regmap ?
Regards,
Benjamin
>
> > +static const struct regmap_irq_chip stpmic1_regmap_irq_chip = {
> > + .name = "pmic_irq",
> > + .status_base = INT_PENDING_R1,
> > + .mask_base = INT_CLEAR_MASK_R1,
> > + .unmask_base = INT_SET_MASK_R1,
> > + .ack_base = INT_CLEAR_R1,
> > + .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS,
> > + .irqs = stpmic1_irqs,
> > + .num_irqs = ARRAY_SIZE(stpmic1_irqs),
> > +};
> > +
> > +static int stpmic1_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct stpmic1 *ddata;
> > + struct device *dev = &i2c->dev;
> > + int ret;
> > + struct device_node *np = dev->of_node;
> > + u32 reg;
> > +
> > + ddata = devm_kzalloc(dev, sizeof(struct stpmic1), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, ddata);
> > + ddata->dev = dev;
> > +
> > + ddata->regmap = devm_regmap_init_i2c(i2c, &stpmic1_regmap_config);
> > + if (IS_ERR(ddata->regmap))
> > + return PTR_ERR(ddata->regmap);
> > +
> > + ddata->irq = of_irq_get(np, STPMIC1_MAIN_IRQ);
> > + if (ddata->irq < 0) {
> > + dev_err(dev, "Failed to get main IRQ: %d\n", ddata->irq);
> > + return ddata->irq;
> > + }
> > +
> > + if (!of_property_read_u32(np, "st,main-control-register", ®)) {
>
> I'm still waiting on feedback from Rob as to whether this is
> acceptable. I suggest that it isn't.
>
> > + ret = regmap_update_bits(ddata->regmap,
> > + SWOFF_PWRCTRL_CR,
> > + PWRCTRL_POLARITY_HIGH |
> > + PWRCTRL_PIN_VALID |
> > + RESTART_REQUEST_ENABLED,
> > + reg);
> > + if (ret) {
> > + dev_err(dev,
> > + "Failed to update main control register: %d\n",
> > + ret);
> > + return ret;
> > + }
> > + }
> > +
> > + /* Read Version ID */
> > + ret = regmap_read(ddata->regmap, VERSION_SR, ®);
> > + if (ret) {
> > + dev_err(dev, "Unable to read pmic version\n");
>
> "PMIC"
>
> > + return ret;
> > + }
> > + dev_info(dev, "PMIC Chip Version: 0x%x\n", reg);
>
> [...]
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 1/8] dt-bindings: mfd: document stpmic1
From: Rob Herring @ 2018-10-25 12:57 UTC (permalink / raw)
To: Lee Jones
Cc: Pascal PAILLET-LME, Dmitry Torokhov, Mark Rutland, Liam Girdwood,
Mark Brown, Wim Van Sebroeck, Guenter Roeck, linux-input,
devicetree, linux-kernel@vger.kernel.org, LINUX-WATCHDOG,
Benjamin Gaignard, Enric Balletbo i Serra
In-Reply-To: <20181025094430.GQ4939@dell>
On Thu, Oct 25, 2018 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> Rob: please grep your name for some feedback.
>
> On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
>
> > From: pascal paillet <p.paillet@st.com>
> > +Optional parent device properties:
> > +- st,main-control-register:
> > + -bit 1: Power cycling will be performed on turn OFF condition
> > + -bit 2: PWRCTRL is functional
> > + -bit 3: PWRCTRL active high
> > +- st,pads-pull-register:
> > + -bit 1: WAKEUP pull down is not active
> > + -bit 2: PWRCTRL pull up is active
> > + -bit 3: PWRCTRL pull down is active
> > + -bit 4: WAKEUP detector is disabled
> > +- st,vin-control-register:
> > + -bit 0: VINLOW monitoring is enabled
> > + -bit [1...3]: VINLOW rising threshold
> > + 000 VINOK_f + 50mV
> > + 001 VINOK_f + 100mV
> > + 010 VINOK_f + 150mV
> > + 011 VINOK_f + 200mV
> > + 100 VINOK_f + 250mV
> > + 101 VINOK_f + 300mV
> > + 110 VINOK_f + 350mV
> > + 111 VINOK_f + 400mV
> > + -bit [4...5]: VINLOW hyst
> > + 00 100mV
> > + 01 200mV
> > + 10 300mV
> > + 11 400mV
> > + -bit 6: SW_OUT detector is disabled
> > + -bit 7: SW_IN detector is enabled.
> > +- st,usb-control-register:
> > + -bit 3: SW_OUT current limit
> > + 0: 600mA
> > + 1: 1.1A
> > + -bit 4: VBUS_OTG discharge is enabled
> > + -bit 5: SW_OUT discharge is enabled
> > + -bit 6: VBUS_OTG detection is enabled
> > + -bit 7: BOOST_OVP is disabled
>
> I'm surprised Rob allowed you to add register bits in a DT property?
Yeah, they are certainly questionable and not something we encourage,
but the alternative would be dozens of properties which only apply for
this one device and just get translated back to register values.
Rob
^ permalink raw reply
* Re: [PATCH v4 4/8] regulator: stpmic1: add stpmic1 regulator driver
From: Pascal PAILLET-LME @ 2018-10-25 13:23 UTC (permalink / raw)
To: Mark Brown
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lee.jones@linaro.org, lgirdwood@gmail.com,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <20181024131716.GQ2103@sirena.org.uk>
Hello Mark,
Le 10/24/2018 03:17 PM, Mark Brown a écrit :
> On Wed, Oct 24, 2018 at 12:54:46PM +0000, Pascal PAILLET-LME wrote:
>
>> I'm sorry, I'm not sure to understand. Would you prefer to not register
>> regulators that
>> are not described in the device-tree ?
> No, I'm saying register all regulators regardless of if they are in the
> device tree - you shouldn't be looking at the init data at all here,
> just let the framework match them using of_match.
I have reworked the code so that we don't touch any more to the init_data.
the new loop to register the regulators is below:
for (i = 0; i < ARRAY_SIZE(stpmic1_regulator_cfgs); i++) {
ret = stpmic1_regulator_register(pdev, i, &stpmic1_matches[i],
regul);
if (ret < 0)
return ret;
regul++;
}
Each regulator is registered, even is it is not described in the
device-tree.
stpmic1_regulator_parse_dt() and stpmic1_regulator_init() are now merged
inside
stpmic1_regulator_register() function.
Thank You,
pascal
^ permalink raw reply
* Re: [PATCH v3 1/3] dt-bindings: Input: new bindings for MSM vibrator
From: Rob Herring @ 2018-10-25 21:44 UTC (permalink / raw)
Cc: dmitry.torokhov, andy.gross, david.brown, robh+dt, mark.rutland,
masneyb, linux-input, linux-kernel, linux-arm-msm, linux-soc,
devicetree, jonathan
In-Reply-To: <20181025012937.2154-2-masneyb@onstation.org>
On Wed, 24 Oct 2018 21:29:35 -0400, Brian Masney wrote:
> This patch adds the device tree bindings for the vibrator found on
> various Qualcomm MSM SOCs.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
> .../bindings/input/msm-vibrator.txt | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/msm-vibrator.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 1/8] dt-bindings: mfd: document stpmic1
From: Lee Jones @ 2018-10-26 6:46 UTC (permalink / raw)
To: Rob Herring
Cc: Pascal PAILLET-LME, Dmitry Torokhov, Mark Rutland, Liam Girdwood,
Mark Brown, Wim Van Sebroeck, Guenter Roeck, linux-input,
devicetree, linux-kernel@vger.kernel.org, LINUX-WATCHDOG,
Benjamin Gaignard, Enric Balletbo i Serra
In-Reply-To: <CAL_JsqKsp8kTK2v_cBrNm+enzieQSEh2G7KMxsBdidr+XhDwTg@mail.gmail.com>
On Thu, 25 Oct 2018, Rob Herring wrote:
> On Thu, Oct 25, 2018 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > Rob: please grep your name for some feedback.
[Moved to the top]
> > I'm surprised Rob allowed you to add register bits in a DT property?
>
> Yeah, they are certainly questionable and not something we encourage,
> but the alternative would be dozens of properties which only apply for
> this one device and just get translated back to register values.
I don't see how this device is any different to the 100's of devices
which are already supported in DT.
> > On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
> >
> > > From: pascal paillet <p.paillet@st.com>
>
> > > +Optional parent device properties:
> > > +- st,main-control-register:
> > > + -bit 1: Power cycling will be performed on turn OFF condition
> > > + -bit 2: PWRCTRL is functional
> > > + -bit 3: PWRCTRL active high
> > > +- st,pads-pull-register:
> > > + -bit 1: WAKEUP pull down is not active
> > > + -bit 2: PWRCTRL pull up is active
> > > + -bit 3: PWRCTRL pull down is active
> > > + -bit 4: WAKEUP detector is disabled
This should be part of the Pinctrl configuration/driver.
And Pinctrl probably already has properties for this?
> > > +- st,vin-control-register:
> > > + -bit 0: VINLOW monitoring is enabled
> > > + -bit [1...3]: VINLOW rising threshold
> > > + 000 VINOK_f + 50mV
> > > + 001 VINOK_f + 100mV
> > > + 010 VINOK_f + 150mV
> > > + 011 VINOK_f + 200mV
> > > + 100 VINOK_f + 250mV
> > > + 101 VINOK_f + 300mV
> > > + 110 VINOK_f + 350mV
> > > + 111 VINOK_f + 400mV
> > > + -bit [4...5]: VINLOW hyst
> > > + 00 100mV
> > > + 01 200mV
> > > + 10 300mV
> > > + 11 400mV
> > > + -bit 6: SW_OUT detector is disabled
> > > + -bit 7: SW_IN detector is enabled.
This should be part of the Regulator configuration/driver?
> > > +- st,usb-control-register:
> > > + -bit 3: SW_OUT current limit
> > > + 0: 600mA
> > > + 1: 1.1A
> > > + -bit 4: VBUS_OTG discharge is enabled
> > > + -bit 5: SW_OUT discharge is enabled
> > > + -bit 6: VBUS_OTG detection is enabled
> > > + -bit 7: BOOST_OVP is disabled
This should be part of the USB configuration/driver?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 2/8] mfd: stpmic1: add stpmic1 driver
From: Lee Jones @ 2018-10-26 7:17 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: pascal paillet, dmitry.torokhov, Rob Herring, Mark Rutland,
Liam Girdwood, Mark Brown, wim, Guenter Roeck, linux-input,
devicetree, Linux Kernel Mailing List, linux-watchdog, eballetbo
In-Reply-To: <CA+M3ks6s=mhLr0Hzcym_vmPVKjsuk8AFnQQe0qj-doVaXo9VcA@mail.gmail.com>
On Thu, 25 Oct 2018, Benjamin Gaignard wrote:
> Le jeu. 25 oct. 2018 à 13:21, Lee Jones <lee.jones@linaro.org> a écrit :
> >
> > On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
> >
> > > From: pascal paillet <p.paillet@st.com>
> > >
> > > stpmic1 is a pmic from STMicroelectronics. The STPMIC1 integrates 10
> > > regulators , 3 switches, a watchdog and an input for a power on key.
> >
> > Same comments as for the DT binding patch.
> >
> > > Signed-off-by: pascal paillet <p.paillet@st.com>
> > > ---
> > > changes in v4:
> > > * rename PONKEY_PU_ACTIVE to PONKEY_PU_INACTIVE
> > >
> > > drivers/mfd/Kconfig | 13 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/stpmic1.c | 401 ++++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/mfd/stpmic1.h | 212 +++++++++++++++++++++++
> > > 4 files changed, 627 insertions(+)
> > > create mode 100644 drivers/mfd/stpmic1.c
> > > create mode 100644 include/linux/mfd/stpmic1.h
[...]
> > > +static const struct regmap_irq stpmic1_irqs[] = {
> > > + [IT_PONKEY_F] = { .mask = 0x01 },
> > > + [IT_PONKEY_R] = { .mask = 0x02 },
> > > + [IT_WAKEUP_F] = { .mask = 0x04 },
> > > + [IT_WAKEUP_R] = { .mask = 0x08 },
> > > + [IT_VBUS_OTG_F] = { .mask = 0x10 },
> > > + [IT_VBUS_OTG_R] = { .mask = 0x20 },
> > > + [IT_SWOUT_F] = { .mask = 0x40 },
> > > + [IT_SWOUT_R] = { .mask = 0x80 },
> > > +
> > > + [IT_CURLIM_BUCK1] = { .reg_offset = 1, .mask = 0x01 },
> > > + [IT_CURLIM_BUCK2] = { .reg_offset = 1, .mask = 0x02 },
> > > + [IT_CURLIM_BUCK3] = { .reg_offset = 1, .mask = 0x04 },
> > > + [IT_CURLIM_BUCK4] = { .reg_offset = 1, .mask = 0x08 },
> > > + [IT_OCP_OTG] = { .reg_offset = 1, .mask = 0x10 },
> > > + [IT_OCP_SWOUT] = { .reg_offset = 1, .mask = 0x20 },
> > > + [IT_OCP_BOOST] = { .reg_offset = 1, .mask = 0x40 },
> > > + [IT_OVP_BOOST] = { .reg_offset = 1, .mask = 0x80 },
> > > +
> > > + [IT_CURLIM_LDO1] = { .reg_offset = 2, .mask = 0x01 },
> > > + [IT_CURLIM_LDO2] = { .reg_offset = 2, .mask = 0x02 },
> > > + [IT_CURLIM_LDO3] = { .reg_offset = 2, .mask = 0x04 },
> > > + [IT_CURLIM_LDO4] = { .reg_offset = 2, .mask = 0x08 },
> > > + [IT_CURLIM_LDO5] = { .reg_offset = 2, .mask = 0x10 },
> > > + [IT_CURLIM_LDO6] = { .reg_offset = 2, .mask = 0x20 },
> > > + [IT_SHORT_SWOTG] = { .reg_offset = 2, .mask = 0x40 },
> > > + [IT_SHORT_SWOUT] = { .reg_offset = 2, .mask = 0x80 },
> > > +
> > > + [IT_TWARN_F] = { .reg_offset = 3, .mask = 0x01 },
> > > + [IT_TWARN_R] = { .reg_offset = 3, .mask = 0x02 },
> > > + [IT_VINLOW_F] = { .reg_offset = 3, .mask = 0x04 },
> > > + [IT_VINLOW_R] = { .reg_offset = 3, .mask = 0x08 },
> > > + [IT_SWIN_F] = { .reg_offset = 3, .mask = 0x40 },
> > > + [IT_SWIN_R] = { .reg_offset = 3, .mask = 0x80 },
> > > +};
> >
> > There should be a MACRO for doing this.
> >
> > If there isn't, you should author one and put it in the Regmap header.
> I don't understand why you want to put this MACRO in regmap header.
REGMAP_IRQ_REG()
> Offsets and masks are custom from this hardware block.
> How can this become generic enough to be put in regmap ?
When replying to only a small section of driver/review like this,
would you mind trimming any unrelated quoting (as I have now done),
please?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 1/8] dt-bindings: mfd: document stpmic1
From: Pascal PAILLET-LME @ 2018-10-26 9:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring
Cc: Dmitry Torokhov, Mark Rutland, Liam Girdwood, Mark Brown,
Wim Van Sebroeck, Guenter Roeck, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
LINUX-WATCHDOG, Benjamin Gaignard, Enric Balletbo i Serra
In-Reply-To: <20181026064614.GG4870@dell>
Hello Lee,
Le 10/26/2018 08:46 AM, Lee Jones a écrit :
> On Thu, 25 Oct 2018, Rob Herring wrote:
>
>> On Thu, Oct 25, 2018 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
>>> Rob: please grep your name for some feedback.
> [Moved to the top]
>
>>> I'm surprised Rob allowed you to add register bits in a DT property?
>> Yeah, they are certainly questionable and not something we encourage,
>> but the alternative would be dozens of properties which only apply for
>> this one device and just get translated back to register values.
> I don't see how this device is any different to the 100's of devices
> which are already supported in DT.
>
>>> On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
>>>
>>>> From: pascal paillet <p.paillet@st.com>
>>>> +Optional parent device properties:
>>>> +- st,main-control-register:
>>>> + -bit 1: Power cycling will be performed on turn OFF condition
>>>> + -bit 2: PWRCTRL is functional
>>>> + -bit 3: PWRCTRL active high
>>>> +- st,pads-pull-register:
>>>> + -bit 1: WAKEUP pull down is not active
>>>> + -bit 2: PWRCTRL pull up is active
>>>> + -bit 3: PWRCTRL pull down is active
>>>> + -bit 4: WAKEUP detector is disabled
> This should be part of the Pinctrl configuration/driver.
>
> And Pinctrl probably already has properties for this?
I will remove this in the next version.
>>>> +- st,vin-control-register:
>>>> + -bit 0: VINLOW monitoring is enabled
>>>> + -bit [1...3]: VINLOW rising threshold
>>>> + 000 VINOK_f + 50mV
>>>> + 001 VINOK_f + 100mV
>>>> + 010 VINOK_f + 150mV
>>>> + 011 VINOK_f + 200mV
>>>> + 100 VINOK_f + 250mV
>>>> + 101 VINOK_f + 300mV
>>>> + 110 VINOK_f + 350mV
>>>> + 111 VINOK_f + 400mV
>>>> + -bit [4...5]: VINLOW hyst
>>>> + 00 100mV
>>>> + 01 200mV
>>>> + 10 300mV
>>>> + 11 400mV
>>>> + -bit 6: SW_OUT detector is disabled
>>>> + -bit 7: SW_IN detector is enabled.
> This should be part of the Regulator configuration/driver?
I will also remove this in the next version. I think this could be
handled by an hardware monitor future driver.
>>>> +- st,usb-control-register:
>>>> + -bit 3: SW_OUT current limit
>>>> + 0: 600mA
>>>> + 1: 1.1A
>>>> + -bit 4: VBUS_OTG discharge is enabled
>>>> + -bit 5: SW_OUT discharge is enabled
>>>> + -bit 6: VBUS_OTG detection is enabled
>>>> + -bit 7: BOOST_OVP is disabled
> This should be part of the USB configuration/driver?
>
I will also remove this in the next version. And I'm going to add active
discharge ops in the regulator driver.
Thank you !
pascal
^ permalink raw reply
* Re: [PATCH v4 1/8] dt-bindings: mfd: document stpmic1
From: Lee Jones @ 2018-10-26 11:46 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: Rob Herring, Dmitry Torokhov, Mark Rutland, Liam Girdwood,
Mark Brown, Wim Van Sebroeck, Guenter Roeck,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, LINUX-WATCHDOG, Benjamin Gaignard,
Enric Balletbo i Serra
In-Reply-To: <5BD2E2DB.5090305@st.com>
On Fri, 26 Oct 2018, Pascal PAILLET-LME wrote:
> Hello Lee,
>
> Le 10/26/2018 08:46 AM, Lee Jones a écrit :
> > On Thu, 25 Oct 2018, Rob Herring wrote:
> >
> >> On Thu, Oct 25, 2018 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> >>> Rob: please grep your name for some feedback.
> > [Moved to the top]
> >
> >>> I'm surprised Rob allowed you to add register bits in a DT property?
> >> Yeah, they are certainly questionable and not something we encourage,
> >> but the alternative would be dozens of properties which only apply for
> >> this one device and just get translated back to register values.
> > I don't see how this device is any different to the 100's of devices
> > which are already supported in DT.
> >
> >>> On Thu, 18 Oct 2018, Pascal PAILLET-LME wrote:
> >>>
> >>>> From: pascal paillet <p.paillet@st.com>
> >>>> +Optional parent device properties:
> >>>> +- st,main-control-register:
> >>>> + -bit 1: Power cycling will be performed on turn OFF condition
> >>>> + -bit 2: PWRCTRL is functional
> >>>> + -bit 3: PWRCTRL active high
> >>>> +- st,pads-pull-register:
> >>>> + -bit 1: WAKEUP pull down is not active
> >>>> + -bit 2: PWRCTRL pull up is active
> >>>> + -bit 3: PWRCTRL pull down is active
> >>>> + -bit 4: WAKEUP detector is disabled
> > This should be part of the Pinctrl configuration/driver.
> >
> > And Pinctrl probably already has properties for this?
> I will remove this in the next version.
> >>>> +- st,vin-control-register:
> >>>> + -bit 0: VINLOW monitoring is enabled
> >>>> + -bit [1...3]: VINLOW rising threshold
> >>>> + 000 VINOK_f + 50mV
> >>>> + 001 VINOK_f + 100mV
> >>>> + 010 VINOK_f + 150mV
> >>>> + 011 VINOK_f + 200mV
> >>>> + 100 VINOK_f + 250mV
> >>>> + 101 VINOK_f + 300mV
> >>>> + 110 VINOK_f + 350mV
> >>>> + 111 VINOK_f + 400mV
> >>>> + -bit [4...5]: VINLOW hyst
> >>>> + 00 100mV
> >>>> + 01 200mV
> >>>> + 10 300mV
> >>>> + 11 400mV
> >>>> + -bit 6: SW_OUT detector is disabled
> >>>> + -bit 7: SW_IN detector is enabled.
> > This should be part of the Regulator configuration/driver?
> I will also remove this in the next version. I think this could be
> handled by an hardware monitor future driver.
> >>>> +- st,usb-control-register:
> >>>> + -bit 3: SW_OUT current limit
> >>>> + 0: 600mA
> >>>> + 1: 1.1A
> >>>> + -bit 4: VBUS_OTG discharge is enabled
> >>>> + -bit 5: SW_OUT discharge is enabled
> >>>> + -bit 6: VBUS_OTG detection is enabled
> >>>> + -bit 7: BOOST_OVP is disabled
> > This should be part of the USB configuration/driver?
> >
> I will also remove this in the next version. And I'm going to add active
> discharge ops in the regulator driver.
And unless this device is in any way special, these subsystems should
already have generic bindings to set these hardware configuration
options.
I would say that passing raw register values is not the way to go.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2] HID: hiddev: fix potential Spectre v1
From: Jiri Kosina @ 2018-10-26 15:07 UTC (permalink / raw)
To: Breno Leitao; +Cc: linux-usb, linux-input, gustavo, stable
In-Reply-To: <1539979293-16102-1-git-send-email-leitao@debian.org>
On Fri, 19 Oct 2018, Breno Leitao wrote:
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
>
> This field is used as an array index by the hiddev_ioctl_usage() function,
> when 'cmd' is either HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or
> HIDIOCSUSAGES.
>
> For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
> field->maxusage and then used as an index to dereference field->usage
> array. The same thing happens to the cmd == HIDIOC{G,S}USAGES cases, where
> uref->usage_index is checked against an array maximum value and then it is
> used as an index in an array.
>
> This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
> traditional Spectre V1 first load:
>
> copy_from_user(uref, user_arg, sizeof(*uref))
> if (uref->usage_index >= field->maxusage)
> goto inval;
> i = field->usage[uref->usage_index].collection_index;
> return i;
>
> This patch fixes this by sanitizing field uref->usage_index before using it
> to index field->usage (HIDIOCGCOLLECTIONINDEX) or field->value in
> HIDIOC{G,S}USAGES arrays, thus, avoiding speculation in the first load.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Cc: <stable@vger.kernel.org>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ 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