* Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Peter Rosin @ 2017-01-03 6:44 UTC (permalink / raw)
To: Jonathan Cameron, Jonathan Cameron,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <90A8A4B3-670B-4409-B7AB-47BF4B2ABDDA-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org>
On 2017-01-02 22:13, Jonathan Cameron wrote:
>
>
> On 2 January 2017 20:47:58 GMT+00:00, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>> On 2017-01-02 19:05, Jonathan Cameron wrote:
>>> On 02/01/17 16:01, Peter Rosin wrote:
>>>> On 2017-01-01 12:00, Jonathan Cameron wrote:
>>>>> On 30/11/16 08:17, Peter Rosin wrote:
>>>>>> Analog Devices ADG792A/G is a triple 4:1 mux.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>>>> Few comments inline. Worth adding anything about the gpio (output pins) to
>>>>> the binding at this stage as well? Would certainly be nice to support
>>>>> them.
>>>>
>>>> I'll add optional properties "gpio-controller;" and "#gpio-cells = <2>;"
>>>> with the usual interpretation in v7 (but no implementation...) Is that
>>>> enough?
>>>>
>>>>> Jonathan
>>>>>> ---
>>>>>> .../devicetree/bindings/misc/mux-adg792a.txt | 64 ++++++++++++++++++++++
>>>>>> 1 file changed, 64 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/misc/mux-adg792a.txt b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..4677f9ab1c55
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>>>> @@ -0,0 +1,64 @@
>>>>>> +Bindings for Analog Devices ADG792A/G Triple 4:1 Multiplexers
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible : "adi,adg792a" or "adi,adg792g"
>>>>>> +- #mux-control-cells : <0> if parallel, or <1> if not.
>>>>>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- adi,parallel : if present, the three muxes are bound together with a single
>>>>>> + mux controller, controlling all three muxes in parallel.
>>>>>> +- adi,idle-state : if present, array of states the three mux controllers will
>>>>>> + have when idle (or, if parallel, a single idle-state).
>>>>> Hmm. These are actually a policy decision. As only one policy will make
>>>>> sense for a given set of hardware probably fine to have it in here I guess.
>>>>> Might be worth adding a note to say this though.
>>>>
>>>> I don't really know what you want me to add, do you have a suggestion for the
>>>> wording?
>>>>
>>>>>> +
>>>>>> +Mux controller states 0 through 3 correspond to signals A through D in the
>>>>>> +datasheet. Mux controller states 4 and 5 are only available as possible idle
>>>>>> +states. State 4 represents that nothing is connected, and state 5 represents
>>>>>> +that the mux controller keeps the mux in its previously selected state during
>>>>>> +the idle period. State 5 is the default idle state.
>>>>> I'm never a great fan of magic numbers. Can we represent this more cleanly by
>>>>> breaking it into multiple properties?
>>>>> Optional:
>>>>> adi,idle-switch-to-channel : switch to this channel when idle.
>>>>> adi,idle-high-impedance : <boolean> the nothing connected state?
>>>>>
>>>>> If neither present leaves it in previous state?
>>>>
>>>> It's not that easy. adi,idle-state is an array when there are three single
>>>> pole quadruple throw muxes, so there really needs to be a number for each
>>>> desired idle-behavior. Unless you have a better idea for how to describe
>>>> that?
>>> The above with arrays for each of the two parameters?
>>> Though then you need a priority documented - I'd say high impedance overrides
>>> the channel selection if both are present.
>>
>> How would you specify that the first mux should idle in "state 5", the second
>> should idle in "state 4" and the third in "state 0"? (original state numbering)
>>
>> You'd still need a magic number for the default idle state (state 5) so that
>> you can skip entries in the arrays. Or am I missing something?
> Ah I had missed state 5. Hmm would need explicit control for that as well. Not nice...
>
> Perhaps 3 state control (magic number but with channel nums separate)
>
> Idle-state array of <switchtostate, currentstate, highimpedance>
>
> Idle-state array of states to switch to if so set?
>
> Slight nicer than a mess of the two things perhaps?
Perhaps making adi,idle-state an array of tuples <mux-number state> and
add adi,idle-high-impedance as an array of mux-numbers, so that my example
above would come out as:
adi,idle-high-impedance = <1>; /* mux 1 idles with high imp */
adi,idle-state = <2 0>; /* mux 2 idles in state 0 (signal A) */
mux 0 is not mentioned and idles in its previously selected state.
If you want mux 0 to idle with high impedance:
adi,idle-high-impedance = <0 1>;
adi,idle-state = <2 0>;
If you want mux 0 to idle with signal C:
adi,idle-high-impedance = <1>;
adi,idle-state = <0 3>, <2 0>;
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> + /* three independent mux controllers (of which one is used) */
>>>>>> + &i2c0 {
>>>>>> + mux: adg792a@50 {
>>>>>> + compatible = "adi,adg792a";
>>>>>> + reg = <0x50>;
>>>>>> + #mux-control-cells = <1>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + adc-mux {
>>>>>> + compatible = "iio-mux";
>>>>>> + io-channels = <&adc 0>;
>>>>>> + io-channel-names = "parent";
>>>>>> +
>>>>>> + mux-controls = <&mux 1>;
>>>>>> +
>>>>>> + channels = "sync-1", "", "out";
>>>>>> + };
>>>>>> +
>>>>>> +
>>>>>> + /*
>>>>>> + * Three parallel muxes with one mux controller, useful e.g. if
>>>>>> + * the adc is differential, thus needing two signals to be muxed
>>>>>> + * simultaneously for correct operation.
>>>>>> + */
>>>>>> + &i2c0 {
>>>>>> + pmux: adg792a@50 {
>>>>>> + compatible = "adi,adg792a";
>>>>>> + reg = <0x50>;
>>>>>> + #mux-control-cells = <0>;
>>>>>> + adi,parallel;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + diff-adc-mux {
>>>>>> + compatible = "iio-mux";
>>>>>> + io-channels = <&adc 0>;
>>>>>> + io-channel-names = "parent";
>>>>>> +
>>>>>> + mux-controls = <&pmux>;
>>>>>> +
>>>>>> + channels = "sync-1", "", "out";
>>>>>> + };
>>>>>>
>>>>>
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-03 9:06 UTC (permalink / raw)
To: Pali Rohár
Cc: Michał Kępień, Jean Delvare, Wolfram Sang,
Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86,
Dmitry Torokhov
In-Reply-To: <201612292228.18706@pali>
On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > > > Latitude machines have ST microelectronics accelerometer at
> > > > > > > i2c address 0x29. That i2c address is not specified in DMI
> > > > > > > or ACPI, so runtime detection without whitelist which is
> > > > > > > below is not possible.
> > > > > > >
> > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > represent that accelerometer. Unfortunately without i2c
> > > > > > > address.
> > > > > >
> > > > > > This part of the commit message sounded a bit confusing to me
> > > > > > at first because there is already an ACPI driver which
> > > > > > handles SMO88xx
> > > > > >
> > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > * the purpose of this patch is to expose a richer interface
> > > > > > (as
> > > > > >
> > > > > > provided by lis3lv02d) to these devices on some machines,
> > > > > >
> > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d can
> > > > > > work
> > > > > >
> > > > > > simultaneously (even though dell-smo8800 effectively
> > > > > > duplicates the work that lis3lv02d does).
> > > > >
> > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > /dev/freefall device which notify userspace about falls.
> > > > > lis3lv02d is i2c driver which exports axes of accelerometer.
> > > > > Additionaly lis3lv02d can export also /dev/freefall if
> > > > > registerer of i2c device provides irq number -- which is not
> > > > > case of this patch.
> > > > >
> > > > > So both drivers are doing different things and both are useful.
> > > > >
> > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > (that ST microelectronics accelerometer) but due to
> > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > handled by two drivers, one ACPI and one i2c.
> > > > >
> > > > > Yes, in ideal world irq number should be passed to lis3lv02d
> > > > > driver and that would export whole device (with /dev/freefall
> > > > > too), but due to HW abstraction it is too much complicated...
> > > >
> > > > Why? AFAICT, all that is required to pass that IRQ number all
> > > > the way down to lis3lv02d is to set the irq field of the struct
> > > > i2c_board_info you are passing to i2c_new_device(). And you can
> > > > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > > > However, you would then need to make sure dell-smo8800 does not
> > > > attempt to request the same IRQ on whitelisted machines. This
> > > > got me thinking about a way to somehow incorporate your changes
> > > > into dell-smo8800 using Wolfram's bus_notifier suggestion, but I
> > > > do not have a working solution for now. What is tempting about
> > > > this approach is that you would not have to scan the ACPI
> > > > namespace in search of SMO88xx devices, because smo8800_add() is
> > > > automatically called for them. However, I fear that the
> > > > resulting solution may be more complicated than the one you
> > > > submitted.
> > >
> > > Then we need to deal with lot of problems. Order of loading .ko
> > > modules is undefined. Binding devices to drivers registered by .ko
> > > module is also in "random" order. At any time any of those .ko
> > > module can be unloaded or at least device unbind (via sysfs) from
> > > driver... And there can be some pathological situation (thanks to
> > > adding ACPI layer as Andy pointed) that there will be more SMO88xx
> > > devices in ACPI. Plus you can compile kernel with and without
> > > those modules and also you can blacklist loading them (so compile
> > > time check is not enough). And still some correct message notifier
> > > must be used.
> > >
> > > I think such solution is much much more complicated, there are lot
> > > of combinations of kernel configuration and available dell
> > > devices...
> >
> > I tried a few more things, but ultimately failed to find a nice way
> > to implement this.
> >
> > Another issue popped up, though. Linus' master branch contains a
> > recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use
> > an IRQ to report Host Notify events, not alert") which breaks your
> > patch. The reason for that is that lis3lv02d relies on the i2c
> > client's IRQ being 0 to detect that it should not create
> > /dev/freefall. Benjamin's patch causes the Host Notify IRQ to be
> > assigned to the i2c client your patch creates, thus causing
> > lis3lv02d to create /dev/freefall, which in turn conflicts with
> > dell-smo8800 which is trying to create /dev/freefall itself.
>
> So 4d5538f5882a is breaking lis3lv02d driver...
Apologies for that.
I could easily fix this by adding a kernel API to know whether the
provided irq is from Host Notify or if it was coming from an actual
declaration. However, I have no idea how many other drivers would
require this (hopefully only this one).
One other solution would be to reserve the Host Notify IRQ and let the
actual drivers that need it to set it, but this was not the best
solution according to Dmitri. On my side, I am not entirely against this
given that it's a chip feature, so the driver should be able to know
that it's available.
Dmitri, Wolfram, Jean, any preferences?
>
> > Also, just to make sure we do not overthink this, I understand that
> > not every unit of the models from the whitelist has an
> > accelerometer, correct? In other words, could we perhaps skip the
> > part where we are making sure the SMO88xx ACPI device is there?
>
> Good question... At least for E6440 I'm did not thing it was possible to
> configure notebook without "3 axes free fall sensor".
>
> But! In BIOS SETUP it is possible to disable free fall sensor. I will
> try to disable it there and will check what happen. My guess is that it
> will be disabled in ACPI.
Just adding my 2 cents regarding the whitelist and interaction between
those 2 drivers. I find this very fragile to have only one available
/dev/freefall node and to rely on the fairness of each driver to not bind
one. It would have been much simpler to have /dev/freefallXX and a
proper misc class device for it. This way, we don't even need to
mutually exclude the drivers. But this is already 8 years old code, so I
guess userspace expects this... (why isn't that using the input subsystem
at all?).
Cheers,
Benjamin.
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-03 9:23 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Michał Kępień, Jean Delvare, Wolfram Sang,
Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86,
Dmitry Torokhov
In-Reply-To: <20170103090641.GH5767@mail.corp.redhat.com>
On Tuesday 03 January 2017 10:06:41 Benjamin Tissoires wrote:
> On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > Also, just to make sure we do not overthink this, I understand that
> > > not every unit of the models from the whitelist has an
> > > accelerometer, correct? In other words, could we perhaps skip the
> > > part where we are making sure the SMO88xx ACPI device is there?
> >
> > Good question... At least for E6440 I'm did not thing it was possible to
> > configure notebook without "3 axes free fall sensor".
> >
> > But! In BIOS SETUP it is possible to disable free fall sensor. I will
> > try to disable it there and will check what happen. My guess is that it
> > will be disabled in ACPI.
>
> Just adding my 2 cents regarding the whitelist and interaction between
> those 2 drivers. I find this very fragile to have only one available
> /dev/freefall node and to rely on the fairness of each driver to not bind
> one. It would have been much simpler to have /dev/freefallXX and a
> proper misc class device for it. This way, we don't even need to
> mutually exclude the drivers. But this is already 8 years old code, so I
> guess userspace expects this... (why isn't that using the input subsystem
> at all?).
>
> Cheers,
> Benjamin.
>
I think there is no problem with more /dev/freefall devices. With these
Dell drivers it should not happen as only one driver can request IRQ
which is associated with /dev/freefall. And /dev/freefal is registered
after acquiring IRQ.
But... there are other problems with it as wrote in previous emails.
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH v7 3/4] arm64: dts: marvell: Add I2C definitions for the Armada 3700
From: Gregory CLEMENT @ 2017-01-03 15:20 UTC (permalink / raw)
To: Romain Perier
Cc: Mark Rutland, devicetree, Yahuda Yitschak, Omri Itach,
Jason Cooper, Pawel Moll, Ian Campbell, Igal Liberman, Hanna Hawa,
Wolfram Sang, Neta Zur Hershkovits, Nadav Haklai, Rob Herring,
Andrew Lunn, linux-i2c, Kumar Gala, Shadi Ammouri, Marcin Wojtas,
Thomas Petazzoni, linux-arm-kernel, Sebastian Hesselbarth
In-Reply-To: <20161201110440.27530-4-romain.perier@free-electrons.com>
Hi Romain,
On jeu., déc. 01 2016, Romain Perier <romain.perier@free-electrons.com> wrote:
> The Armada 3700 has two i2c bus interface units, this commit adds the
> definitions of the corresponding device nodes. It also enables the node
> on the development board for this SoC.
>
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Applied on mvebu/dt64
Thanks,
Gregory
> ---
> arch/arm64/boot/dts/marvell/armada-3720-db.dts | 4 ++++
> arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 18 ++++++++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> index 1372e9a6..16d84af 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> @@ -62,6 +62,10 @@
> };
> };
>
> +&i2c0 {
> + status = "okay";
> +};
> +
> /* CON3 */
> &sata {
> status = "okay";
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index e9bd587..1b0fd21 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -98,6 +98,24 @@
> /* 32M internal register @ 0xd000_0000 */
> ranges = <0x0 0x0 0xd0000000 0x2000000>;
>
> + i2c0: i2c@11000 {
> + compatible = "marvell,armada-3700-i2c";
> + reg = <0x11000 0x24>;
> + clocks = <&nb_periph_clk 10>;
> + interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> + mrvl,i2c-fast-mode;
> + status = "disabled";
> + };
> +
> + i2c1: i2c@11080 {
> + compatible = "marvell,armada-3700-i2c";
> + reg = <0x11080 0x24>;
> + clocks = <&nb_periph_clk 9>;
> + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> + mrvl,i2c-fast-mode;
> + status = "disabled";
> + };
> +
> uart0: serial@12000 {
> compatible = "marvell,armada-3700-uart";
> reg = <0x12000 0x400>;
> --
> 2.9.3
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/5] mfd: axp20x: Use IRQF_TRIGGER_LOW on the axp288
From: Lee Jones @ 2017-01-03 17:53 UTC (permalink / raw)
To: Hans de Goede; +Cc: Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
On Wed, 14 Dec 2016, Hans de Goede wrote:
> The interrupt line of the entire family of axp2xx pmics is active-low,
> for devicetree enumerated irqs, this is dealt with in the devicetree.
>
> ACPI irq resources have a flag field for this too, I tried using this
> on my CUBE iwork8 Air tablet, but it does not contain the right data.
>
> The dstd shows the irq listed as either ActiveLow or ActiveHigh,
> depending on the OSID variable, which seems to be set by the
> "OS IMAGE ID" in the BIOS/EFI setup screen.
>
> Since the acpi-resource info is no good, simply pass in IRQF_TRIGGER_LOW
> on the axp288.
>
> Together with the other axp288 fixes in this series, this fixes the axp288
> irq contineously triggering.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/mfd/axp20x.c | 6 +++---
> include/linux/mfd/axp20x.h | 1 +
> 2 files changed, 4 insertions(+), 3 deletions(-)
Applied, thanks.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index ba130be..3d76941 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -800,6 +800,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> axp20x->regmap_cfg = &axp288_regmap_config;
> axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
> + axp20x->irq_flags = IRQF_TRIGGER_LOW;
> break;
> case AXP806_ID:
> axp20x->nr_cells = ARRAY_SIZE(axp806_cells);
> @@ -829,9 +830,8 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
> int ret;
>
> ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
> - IRQF_ONESHOT | IRQF_SHARED, -1,
> - axp20x->regmap_irq_chip,
> - &axp20x->regmap_irqc);
> + IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
> + -1, axp20x->regmap_irq_chip, &axp20x->regmap_irqc);
> if (ret) {
> dev_err(axp20x->dev, "failed to add irq chip: %d\n", ret);
> return ret;
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index fec597f..199cce3 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -517,6 +517,7 @@ enum axp809_irqs {
> struct axp20x_dev {
> struct device *dev;
> int irq;
> + unsigned long irq_flags;
> struct regmap *regmap;
> struct regmap_irq_chip_data *regmap_irqc;
> long variant;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 2/5] mfd: axp20x: Add missing axp288 irqs
From: Lee Jones @ 2017-01-03 17:54 UTC (permalink / raw)
To: Hans de Goede; +Cc: Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-2-hdegoede@redhat.com>
On Wed, 14 Dec 2016, Hans de Goede wrote:
> The axp288 has the following irqs 2 times: VBUS_FALL, VBUS_RISE,
> VBUS_OV. On boot / reset the enable flags for both the normal and alt
> version of these irqs is set.
>
> Since we were only listing the normal version in the axp288 regmap_irq
> struct, we were never disabling the alt versions of these irqs.
>
> Add the alt versions to the axp288 regmap_irq struct, so that these
> get properly disabled.
>
> Together with the other axp288 fixes in this series, this fixes the axp288
> irq contineously triggering.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/mfd/axp20x.c | 3 +++
> 1 file changed, 3 insertions(+)
Applied, thanks.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 3d76941..9a81659 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -405,6 +405,9 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
> INIT_REGMAP_IRQ(AXP288, VBUS_FALL, 0, 2),
> INIT_REGMAP_IRQ(AXP288, VBUS_RISE, 0, 3),
> INIT_REGMAP_IRQ(AXP288, OV, 0, 4),
> + INIT_REGMAP_IRQ(AXP288, FALLING_ALT, 0, 5),
> + INIT_REGMAP_IRQ(AXP288, RISING_ALT, 0, 6),
> + INIT_REGMAP_IRQ(AXP288, OV_ALT, 0, 7),
>
> INIT_REGMAP_IRQ(AXP288, DONE, 1, 2),
> INIT_REGMAP_IRQ(AXP288, CHARGING, 1, 3),
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 3/5] mfd: axp20x: Fix axp288 PEK_DBR and PEK_DBF irqs being swapped
From: Lee Jones @ 2017-01-03 17:54 UTC (permalink / raw)
To: Hans de Goede; +Cc: Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-3-hdegoede@redhat.com>
On Wed, 14 Dec 2016, Hans de Goede wrote:
> The R in PEK_DBR stands for rising, so it should be mapped to
> AXP288_IRQ_POKP where the last P stands for positive edge.
>
> Likewise PEK_DBF should be mapped to the falling edge, aka the
> _N_egative edge, so it should be mapped to AXP288_IRQ_POKN.
>
> This fixes the inverted powerbutton status reporting by the
> axp20x-pek driver.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/mfd/axp20x.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Applied, thanks.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 9a81659..a294121 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -205,14 +205,14 @@ static struct resource axp22x_pek_resources[] = {
> static struct resource axp288_power_button_resources[] = {
> {
> .name = "PEK_DBR",
> - .start = AXP288_IRQ_POKN,
> - .end = AXP288_IRQ_POKN,
> + .start = AXP288_IRQ_POKP,
> + .end = AXP288_IRQ_POKP,
> .flags = IORESOURCE_IRQ,
> },
> {
> .name = "PEK_DBF",
> - .start = AXP288_IRQ_POKP,
> - .end = AXP288_IRQ_POKP,
> + .start = AXP288_IRQ_POKN,
> + .end = AXP288_IRQ_POKN,
> .flags = IORESOURCE_IRQ,
> },
> };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 4/5] mfd: axp20x: Drop wrong AXP288_PMIC_ADC_EN define
From: Lee Jones @ 2017-01-03 17:54 UTC (permalink / raw)
To: Hans de Goede; +Cc: Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-4-hdegoede@redhat.com>
On Wed, 14 Dec 2016, Hans de Goede wrote:
> The adc-enable register for the axp288 is 0x82, not 0x84.
> 0x82 is already defined as AXP20X_ADC_EN1 and that is what the
> axp288_adc driver is actually using, so simply drop the wrong
> AXP288_PMIC_ADC_EN define.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> include/linux/mfd/axp20x.h | 1 -
> 1 file changed, 1 deletion(-)
Applied, thanks.
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 199cce3..fe93e00 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -232,7 +232,6 @@ enum {
> #define AXP288_PMIC_ADC_H 0x56
> #define AXP288_PMIC_ADC_L 0x57
> #define AXP288_ADC_TS_PIN_CTRL 0x84
> -#define AXP288_PMIC_ADC_EN 0x84
>
> /* Fuel Gauge */
> #define AXP288_FG_RDC1_REG 0xba
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Jacob Pan @ 2017-01-03 18:23 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Chen-Yu Tsai, Lars-Peter Clausen,
Peter Meerwald-Stadler, russianneuromancer @ ya . ru,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <054b52de-40fd-a879-7b8e-2d25fed18840-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Sun, 1 Jan 2017 12:19:40 +0100
Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
> On 30-12-16 19:15, Jacob Pan wrote:
> > On Fri, 30 Dec 2016 16:46:03 +0000
> > Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> >> On 14/12/16 13:55, Hans de Goede wrote:
> >>> For some reason the axp288_adc driver was modifying the
> >>> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
> >>> whether the GP_ADC channel or another channel was written.
> >>>
> >>> These bits control when a bias current is send to the TS_PIN, the
> >>> GP_ADC has its own pin and a separate bit in another register to
> >>> control the bias current.
> >>>
> > It has been a while. Just looked at the datasheet, reg 84h is for
> > ADC and TS pin control. IIRC, we had to set the TS bits in order to
> > make ADC read to work.
>
> The bits the code I'm removing are manipulating are bits 0 & 1 of
> reg 84h which are:
>
> 1-0 Current source from TS pin on/off enable bit [1:0]
>
> 00: off
> 01: on when charging battery, off when not charging
> 10: on in ADC phase and off when out of the ADC phase, for power
> saving 11: always on
>
> And they are being toggled between 10 and 11, so in both
> cases the current source is enabled when reading the adc
> value. Specifically the code this patch removes are setting
> these bits to 10 before reading and back to 11 after reading,
> which makes no sense.
>
> And to make things even funkier, this manipulation is only
> done when reading the GP_ADC, which is the ADC function of
> GPIO0, whereas these bits control the bias current source
> for the TS pin, which is a completely different pin.
>
> So all in all this entire bit of code seems to be big NOP.
>
It could have been a quirk we had to do on our platforms, I just
cannot recall the details. Are you testing this on x86 platforms?
> > What is the other register?
>
> The register to actually enable / disable the bias current
> source for GPIO0 / the GPADC pin is register 85h, where setting
> bit 2 enables the GPIO0 output current.
>
> Regards,
>
> Hans
>
>
>
> >>> Not only does changing when to enable the TS_PIN bias current
> >>> (always or only when sampling) when reading the GP_ADC make no
> >>> sense at all, the code is modifying these bits is writing the
> >>> entire register, assuming that all the other bits have their
> >>> default value.
> > Agreed, it would be better to do read-modify-write and not to
> > touch the other bits.
> >>> So if the firmware has configured a different bias-current for
> >>> either pin, then that change gets clobbered by the write, likewise
> >>> if the firmware has set bit 2 to indicate that the battery has no
> >>> thermal sensor, this will get clobbered by the write.
> >>>
> >>> This commit fixes all this, by simply removing all writes to the
> >>> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
> >>> GP_ADC pin, and can actually be harmful.
> >>>
> >>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> I guess you probably have more up to date contact details than I
> >> do, but seems worth trying to cc Jacob Pan on this to see if we
> >> can find out what the original reasoning behind this was. Seems a
> >> very odd thing to do with no purpose!
> >>
> >> If Jacob isn't contactable we'll fall back to guessing it was just
> >> an oddity of driver evolution.
> >>
> >> Jonathan
> >>> ---
> >>> drivers/iio/adc/axp288_adc.c | 32
> >>> +------------------------------- 1 file changed, 1 insertion(+),
> >>> 31 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/axp288_adc.c
> >>> b/drivers/iio/adc/axp288_adc.c index 7fd2494..64799ad 100644
> >>> --- a/drivers/iio/adc/axp288_adc.c
> >>> +++ b/drivers/iio/adc/axp288_adc.c
> >>> @@ -28,8 +28,6 @@
> >>> #include <linux/iio/driver.h>
> >>>
> >>> #define AXP288_ADC_EN_MASK 0xF1
> >>> -#define AXP288_ADC_TS_PIN_GPADC 0xF2
> >>> -#define AXP288_ADC_TS_PIN_ON 0xF3
> >>>
> >>> enum axp288_adc_id {
> >>> AXP288_ADC_TS,
> >>> @@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val,
> >>> unsigned long address, return IIO_VAL_INT;
> >>> }
> >>>
> >>> -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int
> >>> mode,
> >>> - unsigned long address)
> >>> -{
> >>> - /* channels other than GPADC do not need to switch TS pin
> >>> */
> >>> - if (address != AXP288_GP_ADC_H)
> >>> - return 0;
> >>> -
> >>> - return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
> >>> mode); -}
> >>> -
> >>> static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> >>> struct iio_chan_spec const *chan,
> >>> int *val, int *val2, long mask)
> >>> @@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev
> >>> *indio_dev, mutex_lock(&indio_dev->mlock);
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_RAW:
> >>> - if (axp288_adc_set_ts(info->regmap,
> >>> AXP288_ADC_TS_PIN_GPADC,
> >>> - chan->address)) {
> >>> - dev_err(&indio_dev->dev, "GPADC mode\n");
> >>> - ret = -EINVAL;
> >>> - break;
> >>> - }
> >>> ret = axp288_adc_read_channel(val, chan->address,
> >>> info->regmap);
> >>> - if (axp288_adc_set_ts(info->regmap,
> >>> AXP288_ADC_TS_PIN_ON,
> >>> - chan->address))
> >>> - dev_err(&indio_dev->dev, "TS pin
> >>> restore\n"); break;
> >>> default:
> >>> ret = -EINVAL;
> >>> @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev
> >>> *indio_dev, return ret;
> >>> }
> >>>
> >>> -static int axp288_adc_set_state(struct regmap *regmap)
> >>> -{
> >>> - /* ADC should be always enabled for internal FG to
> >>> function */
> >>> - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
> >>> AXP288_ADC_TS_PIN_ON))
> >>> - return -EIO;
> >>> -
> >>> - return regmap_write(regmap, AXP20X_ADC_EN1,
> >>> AXP288_ADC_EN_MASK); -}
> >>> -
> >>> static const struct iio_info axp288_adc_iio_info = {
> >>> .read_raw = &axp288_adc_read_raw,
> >>> .driver_module = THIS_MODULE,
> >>> @@ -199,7 +169,7 @@ static int axp288_adc_probe(struct
> >>> platform_device *pdev)
> >>> * Set ADC to enabled state at all time, including system
> >>> suspend.
> >>> * otherwise internal fuel gauge functionality may be
> >>> affected. */
> >>> - ret = axp288_adc_set_state(axp20x->regmap);
> >>> + ret = regmap_write(info->regmap, AXP20X_ADC_EN1,
> >>> AXP288_ADC_EN_MASK); if (ret) {
> >>> dev_err(&pdev->dev, "unable to enable ADC
> >>> device\n"); return ret;
> >>>
> >>
> >
> > [Jacob Pan]
> >
[Jacob Pan]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Dmitry Torokhov @ 2017-01-03 18:38 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pali Rohár, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103090641.GH5767@mail.corp.redhat.com>
On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > > > > Latitude machines have ST microelectronics accelerometer at
> > > > > > > > i2c address 0x29. That i2c address is not specified in DMI
> > > > > > > > or ACPI, so runtime detection without whitelist which is
> > > > > > > > below is not possible.
> > > > > > > >
> > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > represent that accelerometer. Unfortunately without i2c
> > > > > > > > address.
> > > > > > >
> > > > > > > This part of the commit message sounded a bit confusing to me
> > > > > > > at first because there is already an ACPI driver which
> > > > > > > handles SMO88xx
> > > > > > >
> > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > * the purpose of this patch is to expose a richer interface
> > > > > > > (as
> > > > > > >
> > > > > > > provided by lis3lv02d) to these devices on some machines,
> > > > > > >
> > > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d can
> > > > > > > work
> > > > > > >
> > > > > > > simultaneously (even though dell-smo8800 effectively
> > > > > > > duplicates the work that lis3lv02d does).
> > > > > >
> > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > lis3lv02d is i2c driver which exports axes of accelerometer.
> > > > > > Additionaly lis3lv02d can export also /dev/freefall if
> > > > > > registerer of i2c device provides irq number -- which is not
> > > > > > case of this patch.
> > > > > >
> > > > > > So both drivers are doing different things and both are useful.
> > > > > >
> > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > >
> > > > > > Yes, in ideal world irq number should be passed to lis3lv02d
> > > > > > driver and that would export whole device (with /dev/freefall
> > > > > > too), but due to HW abstraction it is too much complicated...
> > > > >
> > > > > Why? AFAICT, all that is required to pass that IRQ number all
> > > > > the way down to lis3lv02d is to set the irq field of the struct
> > > > > i2c_board_info you are passing to i2c_new_device(). And you can
> > > > > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > > > > However, you would then need to make sure dell-smo8800 does not
> > > > > attempt to request the same IRQ on whitelisted machines. This
> > > > > got me thinking about a way to somehow incorporate your changes
> > > > > into dell-smo8800 using Wolfram's bus_notifier suggestion, but I
> > > > > do not have a working solution for now. What is tempting about
> > > > > this approach is that you would not have to scan the ACPI
> > > > > namespace in search of SMO88xx devices, because smo8800_add() is
> > > > > automatically called for them. However, I fear that the
> > > > > resulting solution may be more complicated than the one you
> > > > > submitted.
> > > >
> > > > Then we need to deal with lot of problems. Order of loading .ko
> > > > modules is undefined. Binding devices to drivers registered by .ko
> > > > module is also in "random" order. At any time any of those .ko
> > > > module can be unloaded or at least device unbind (via sysfs) from
> > > > driver... And there can be some pathological situation (thanks to
> > > > adding ACPI layer as Andy pointed) that there will be more SMO88xx
> > > > devices in ACPI. Plus you can compile kernel with and without
> > > > those modules and also you can blacklist loading them (so compile
> > > > time check is not enough). And still some correct message notifier
> > > > must be used.
> > > >
> > > > I think such solution is much much more complicated, there are lot
> > > > of combinations of kernel configuration and available dell
> > > > devices...
> > >
> > > I tried a few more things, but ultimately failed to find a nice way
> > > to implement this.
> > >
> > > Another issue popped up, though. Linus' master branch contains a
> > > recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use
> > > an IRQ to report Host Notify events, not alert") which breaks your
> > > patch. The reason for that is that lis3lv02d relies on the i2c
> > > client's IRQ being 0 to detect that it should not create
> > > /dev/freefall. Benjamin's patch causes the Host Notify IRQ to be
> > > assigned to the i2c client your patch creates, thus causing
> > > lis3lv02d to create /dev/freefall, which in turn conflicts with
> > > dell-smo8800 which is trying to create /dev/freefall itself.
> >
> > So 4d5538f5882a is breaking lis3lv02d driver...
>
> Apologies for that.
>
> I could easily fix this by adding a kernel API to know whether the
> provided irq is from Host Notify or if it was coming from an actual
> declaration. However, I have no idea how many other drivers would
> require this (hopefully only this one).
>
> One other solution would be to reserve the Host Notify IRQ and let the
> actual drivers that need it to set it, but this was not the best
> solution according to Dmitri. On my side, I am not entirely against this
> given that it's a chip feature, so the driver should be able to know
> that it's available.
>
> Dmitri, Wolfram, Jean, any preferences?
I read this:
"IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST
microelectronics accelerometer) but due to complicated HW abstraction
and layers on Dell laptops it is handled by two drivers, one ACPI and
one i2c."
and that is the core of the issue. You have 2 drivers fighting over the
same device. Fix this and it will all work.
As far as I can see hp_accel instantiates lis3lv02d and accesses it via
ACPI methods, can the same be done for Dell?
>
> >
> > > Also, just to make sure we do not overthink this, I understand that
> > > not every unit of the models from the whitelist has an
> > > accelerometer, correct? In other words, could we perhaps skip the
> > > part where we are making sure the SMO88xx ACPI device is there?
> >
> > Good question... At least for E6440 I'm did not thing it was possible to
> > configure notebook without "3 axes free fall sensor".
> >
> > But! In BIOS SETUP it is possible to disable free fall sensor. I will
> > try to disable it there and will check what happen. My guess is that it
> > will be disabled in ACPI.
>
> Just adding my 2 cents regarding the whitelist and interaction between
> those 2 drivers. I find this very fragile to have only one available
> /dev/freefall node and to rely on the fairness of each driver to not bind
> one. It would have been much simpler to have /dev/freefallXX and a
> proper misc class device for it. This way, we don't even need to
> mutually exclude the drivers. But this is already 8 years old code, so I
> guess userspace expects this... (why isn't that using the input subsystem
> at all?).
I do not consider throwing a unit down an ordinary user interaction ;)
So there is no input event code for this.
Userspace should really use IIO accelerometer interface here. And kernel
could provide composite IIO->/dev/freefall bridge, like we did for
/dev/input/mice when all userspace wanted only PS/2.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-03 18:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103183843.GA16032@dtor-ws>
[-- Attachment #1: Type: Text/Plain, Size: 9073 bytes --]
On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > microelectronics accelerometer at i2c address 0x29.
> > > > > > > > > That i2c address is not specified in DMI or ACPI, so
> > > > > > > > > runtime detection without whitelist which is below
> > > > > > > > > is not possible.
> > > > > > > > >
> > > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > > represent that accelerometer. Unfortunately without
> > > > > > > > > i2c address.
> > > > > > > >
> > > > > > > > This part of the commit message sounded a bit confusing
> > > > > > > > to me at first because there is already an ACPI driver
> > > > > > > > which handles SMO88xx
> > > > > > > >
> > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > interface (as
> > > > > > > >
> > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > machines,
> > > > > > > >
> > > > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d
> > > > > > > > can work
> > > > > > > >
> > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > does).
> > > > > > >
> > > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > > lis3lv02d is i2c driver which exports axes of
> > > > > > > accelerometer. Additionaly lis3lv02d can export also
> > > > > > > /dev/freefall if registerer of i2c device provides irq
> > > > > > > number -- which is not case of this patch.
> > > > > > >
> > > > > > > So both drivers are doing different things and both are
> > > > > > > useful.
> > > > > > >
> > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > device (that ST microelectronics accelerometer) but due
> > > > > > > to complicated HW abstraction and layers on Dell laptops
> > > > > > > it is handled by two drivers, one ACPI and one i2c.
> > > > > > >
> > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > (with /dev/freefall too), but due to HW abstraction it
> > > > > > > is too much complicated...
> > > > > >
> > > > > > Why? AFAICT, all that is required to pass that IRQ number
> > > > > > all the way down to lis3lv02d is to set the irq field of
> > > > > > the struct i2c_board_info you are passing to
> > > > > > i2c_new_device(). And you can extract that IRQ number
> > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > request the same IRQ on whitelisted machines. This got me
> > > > > > thinking about a way to somehow incorporate your changes
> > > > > > into dell-smo8800 using Wolfram's bus_notifier suggestion,
> > > > > > but I do not have a working solution for now. What is
> > > > > > tempting about this approach is that you would not have to
> > > > > > scan the ACPI namespace in search of SMO88xx devices,
> > > > > > because smo8800_add() is automatically called for them.
> > > > > > However, I fear that the resulting solution may be more
> > > > > > complicated than the one you submitted.
> > > > >
> > > > > Then we need to deal with lot of problems. Order of loading
> > > > > .ko modules is undefined. Binding devices to drivers
> > > > > registered by .ko module is also in "random" order. At any
> > > > > time any of those .ko module can be unloaded or at least
> > > > > device unbind (via sysfs) from driver... And there can be
> > > > > some pathological situation (thanks to adding ACPI layer as
> > > > > Andy pointed) that there will be more SMO88xx devices in
> > > > > ACPI. Plus you can compile kernel with and without those
> > > > > modules and also you can blacklist loading them (so compile
> > > > > time check is not enough). And still some correct message
> > > > > notifier must be used.
> > > > >
> > > > > I think such solution is much much more complicated, there
> > > > > are lot of combinations of kernel configuration and
> > > > > available dell devices...
> > > >
> > > > I tried a few more things, but ultimately failed to find a nice
> > > > way to implement this.
> > > >
> > > > Another issue popped up, though. Linus' master branch contains
> > > > a recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a
> > > > ("i2c: use an IRQ to report Host Notify events, not alert")
> > > > which breaks your patch. The reason for that is that
> > > > lis3lv02d relies on the i2c client's IRQ being 0 to detect
> > > > that it should not create /dev/freefall. Benjamin's patch
> > > > causes the Host Notify IRQ to be assigned to the i2c client
> > > > your patch creates, thus causing lis3lv02d to create
> > > > /dev/freefall, which in turn conflicts with dell-smo8800 which
> > > > is trying to create /dev/freefall itself.
> > >
> > > So 4d5538f5882a is breaking lis3lv02d driver...
> >
> > Apologies for that.
> >
> > I could easily fix this by adding a kernel API to know whether the
> > provided irq is from Host Notify or if it was coming from an actual
> > declaration. However, I have no idea how many other drivers would
> > require this (hopefully only this one).
> >
> > One other solution would be to reserve the Host Notify IRQ and let
> > the actual drivers that need it to set it, but this was not the
> > best solution according to Dmitri. On my side, I am not entirely
> > against this given that it's a chip feature, so the driver should
> > be able to know that it's available.
> >
> > Dmitri, Wolfram, Jean, any preferences?
>
> I read this:
>
> "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> ST microelectronics accelerometer) but due to complicated HW
> abstraction and layers on Dell laptops it is handled by two drivers,
> one ACPI and one i2c."
>
> and that is the core of the issue. You have 2 drivers fighting over
> the same device. Fix this and it will all work.
With my current implementation (which I sent in this patch), they are
not fighting.
dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only
accelerometer device as lis3lv02d driver does not get IRQ number in
platform data.
> As far as I can see hp_accel instantiates lis3lv02d and accesses it
> via ACPI methods, can the same be done for Dell?
No, Dell does not have any ACPI methods. And as I wrote in ACPI or DMI
is even not i2c address of device, so it needs to be specified in code
itself.
Really there is no other way... :-(
> > > > Also, just to make sure we do not overthink this, I understand
> > > > that not every unit of the models from the whitelist has an
> > > > accelerometer, correct? In other words, could we perhaps skip
> > > > the part where we are making sure the SMO88xx ACPI device is
> > > > there?
> > >
> > > Good question... At least for E6440 I'm did not thing it was
> > > possible to configure notebook without "3 axes free fall
> > > sensor".
> > >
> > > But! In BIOS SETUP it is possible to disable free fall sensor. I
> > > will try to disable it there and will check what happen. My
> > > guess is that it will be disabled in ACPI.
> >
> > Just adding my 2 cents regarding the whitelist and interaction
> > between those 2 drivers. I find this very fragile to have only one
> > available /dev/freefall node and to rely on the fairness of each
> > driver to not bind one. It would have been much simpler to have
> > /dev/freefallXX and a proper misc class device for it. This way,
> > we don't even need to mutually exclude the drivers. But this is
> > already 8 years old code, so I guess userspace expects this...
> > (why isn't that using the input subsystem at all?).
>
> I do not consider throwing a unit down an ordinary user interaction
> ;) So there is no input event code for this.
>
> Userspace should really use IIO accelerometer interface here. And
> kernel could provide composite IIO->/dev/freefall bridge, like we
Such "generic" bridge is probably not possible, as /dev/freefall is
connected to specific lis3lv02d IRQ.
> did for /dev/input/mice when all userspace wanted only PS/2.
>
> Thanks.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* RE: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Mario.Limonciello @ 2017-01-03 18:58 UTC (permalink / raw)
To: pali.rohar, dmitry.torokhov
Cc: benjamin.tissoires, kernel, jdelvare, wsa, stevenhoneyman,
Valdis.Kletnieks, jochen, gabriele.mzt, luto, alex.hung, tiwai,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201701031950.17389@pali>
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Tuesday, January 3, 2017 12:50 PM
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>; Michał Kępień
> <kernel@kempniu.pl>; Jean Delvare <jdelvare@suse.com>; Wolfram Sang
> <wsa@the-dreams.de>; Steven Honeyman <stevenhoneyman@gmail.com>;
> Valdis.Kletnieks@vt.edu; Jochen Eisinger <jochen@penguin-breeder.org>;
> Gabriele Mazzotta <gabriele.mzt@gmail.com>; Andy Lutomirski
> <luto@kernel.org>; Limonciello, Mario <Mario_Limonciello@Dell.com>; Alex
> Hung <alex.hung@canonical.com>; Takashi Iwai <tiwai@suse.de>; linux-
> i2c@vger.kernel.org; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org
> Subject: Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell
> machines
>
> On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > microelectronics accelerometer at i2c address 0x29.
> > > > > > > > > > That i2c address is not specified in DMI or ACPI, so
> > > > > > > > > > runtime detection without whitelist which is below is
> > > > > > > > > > not possible.
> > > > > > > > > >
> > > > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > > > represent that accelerometer. Unfortunately without
> > > > > > > > > > i2c address.
> > > > > > > > >
> > > > > > > > > This part of the commit message sounded a bit confusing
> > > > > > > > > to me at first because there is already an ACPI driver
> > > > > > > > > which handles SMO88xx
> > > > > > > > >
> > > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > > interface (as
> > > > > > > > >
> > > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > > machines,
> > > > > > > > >
> > > > > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d
> > > > > > > > > can work
> > > > > > > > >
> > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > > does).
> > > > > > > >
> > > > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > > > lis3lv02d is i2c driver which exports axes of
> > > > > > > > accelerometer. Additionaly lis3lv02d can export also
> > > > > > > > /dev/freefall if registerer of i2c device provides irq
> > > > > > > > number -- which is not case of this patch.
> > > > > > > >
> > > > > > > > So both drivers are doing different things and both are
> > > > > > > > useful.
> > > > > > > >
> > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > device (that ST microelectronics accelerometer) but due to
> > > > > > > > complicated HW abstraction and layers on Dell laptops it
> > > > > > > > is handled by two drivers, one ACPI and one i2c.
> > > > > > > >
> > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > lis3lv02d driver and that would export whole device (with
> > > > > > > > /dev/freefall too), but due to HW abstraction it is too
> > > > > > > > much complicated...
> > > > > > >
> > > > > > > Why? AFAICT, all that is required to pass that IRQ number
> > > > > > > all the way down to lis3lv02d is to set the irq field of the
> > > > > > > struct i2c_board_info you are passing to i2c_new_device().
> > > > > > > And you can extract that IRQ number e.g. in
> > > > > > > check_acpi_smo88xx_device(). However, you would then need to
> > > > > > > make sure dell-smo8800 does not attempt to request the same
> > > > > > > IRQ on whitelisted machines. This got me thinking about a
> > > > > > > way to somehow incorporate your changes into dell-smo8800
> > > > > > > using Wolfram's bus_notifier suggestion, but I do not have a
> > > > > > > working solution for now. What is tempting about this
> > > > > > > approach is that you would not have to scan the ACPI
> > > > > > > namespace in search of SMO88xx devices, because
> > > > > > > smo8800_add() is automatically called for them.
> > > > > > > However, I fear that the resulting solution may be more
> > > > > > > complicated than the one you submitted.
> > > > > >
> > > > > > Then we need to deal with lot of problems. Order of loading
> > > > > > .ko modules is undefined. Binding devices to drivers
> > > > > > registered by .ko module is also in "random" order. At any
> > > > > > time any of those .ko module can be unloaded or at least
> > > > > > device unbind (via sysfs) from driver... And there can be some
> > > > > > pathological situation (thanks to adding ACPI layer as Andy
> > > > > > pointed) that there will be more SMO88xx devices in ACPI. Plus
> > > > > > you can compile kernel with and without those modules and also
> > > > > > you can blacklist loading them (so compile time check is not
> > > > > > enough). And still some correct message notifier must be used.
> > > > > >
> > > > > > I think such solution is much much more complicated, there are
> > > > > > lot of combinations of kernel configuration and available dell
> > > > > > devices...
> > > > >
> > > > > I tried a few more things, but ultimately failed to find a nice
> > > > > way to implement this.
> > > > >
> > > > > Another issue popped up, though. Linus' master branch contains
> > > > > a recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a
> > > > > ("i2c: use an IRQ to report Host Notify events, not alert")
> > > > > which breaks your patch. The reason for that is that lis3lv02d
> > > > > relies on the i2c client's IRQ being 0 to detect that it should
> > > > > not create /dev/freefall. Benjamin's patch causes the Host
> > > > > Notify IRQ to be assigned to the i2c client your patch creates,
> > > > > thus causing lis3lv02d to create /dev/freefall, which in turn
> > > > > conflicts with dell-smo8800 which is trying to create
> > > > > /dev/freefall itself.
> > > >
> > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > >
> > > Apologies for that.
> > >
> > > I could easily fix this by adding a kernel API to know whether the
> > > provided irq is from Host Notify or if it was coming from an actual
> > > declaration. However, I have no idea how many other drivers would
> > > require this (hopefully only this one).
> > >
> > > One other solution would be to reserve the Host Notify IRQ and let
> > > the actual drivers that need it to set it, but this was not the best
> > > solution according to Dmitri. On my side, I am not entirely against
> > > this given that it's a chip feature, so the driver should be able to
> > > know that it's available.
> > >
> > > Dmitri, Wolfram, Jean, any preferences?
> >
> > I read this:
> >
> > "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST
> > microelectronics accelerometer) but due to complicated HW abstraction
> > and layers on Dell laptops it is handled by two drivers, one ACPI and
> > one i2c."
> >
> > and that is the core of the issue. You have 2 drivers fighting over
> > the same device. Fix this and it will all work.
>
> With my current implementation (which I sent in this patch), they are not
> fighting.
>
> dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only
> accelerometer device as lis3lv02d driver does not get IRQ number in platform
> data.
>
> > As far as I can see hp_accel instantiates lis3lv02d and accesses it
> > via ACPI methods, can the same be done for Dell?
>
> No, Dell does not have any ACPI methods. And as I wrote in ACPI or DMI is even
> not i2c address of device, so it needs to be specified in code itself.
>
> Really there is no other way... :-(
Dell doesn't export the general purpose accelerometer
data up to the OS.
Pali wanted it however and came up with a way to access it though
from the information we have shared.
That's the reason that there is no ACPI method for this and it needs
to be whitelisted in this driver on the platforms that have it wired
up this way.
>
> > > > > Also, just to make sure we do not overthink this, I understand
> > > > > that not every unit of the models from the whitelist has an
> > > > > accelerometer, correct? In other words, could we perhaps skip
> > > > > the part where we are making sure the SMO88xx ACPI device is
> > > > > there?
> > > >
> > > > Good question... At least for E6440 I'm did not thing it was
> > > > possible to configure notebook without "3 axes free fall sensor".
> > > >
> > > > But! In BIOS SETUP it is possible to disable free fall sensor. I
> > > > will try to disable it there and will check what happen. My guess
> > > > is that it will be disabled in ACPI.
> > >
> > > Just adding my 2 cents regarding the whitelist and interaction
> > > between those 2 drivers. I find this very fragile to have only one
> > > available /dev/freefall node and to rely on the fairness of each
> > > driver to not bind one. It would have been much simpler to have
> > > /dev/freefallXX and a proper misc class device for it. This way, we
> > > don't even need to mutually exclude the drivers. But this is already
> > > 8 years old code, so I guess userspace expects this...
> > > (why isn't that using the input subsystem at all?).
> >
> > I do not consider throwing a unit down an ordinary user interaction
> > ;) So there is no input event code for this.
> >
> > Userspace should really use IIO accelerometer interface here. And
> > kernel could provide composite IIO->/dev/freefall bridge, like we
>
> Such "generic" bridge is probably not possible, as /dev/freefall is connected to
> specific lis3lv02d IRQ.
>
> > did for /dev/input/mice when all userspace wanted only PS/2.
> >
> > Thanks.
>
> --
> Pali Rohár
> pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Dmitry Torokhov @ 2017-01-03 19:48 UTC (permalink / raw)
To: Pali Rohár
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201701031950.17389@pali>
On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > microelectronics accelerometer at i2c address 0x29.
> > > > > > > > > > That i2c address is not specified in DMI or ACPI, so
> > > > > > > > > > runtime detection without whitelist which is below
> > > > > > > > > > is not possible.
> > > > > > > > > >
> > > > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > > > represent that accelerometer. Unfortunately without
> > > > > > > > > > i2c address.
> > > > > > > > >
> > > > > > > > > This part of the commit message sounded a bit confusing
> > > > > > > > > to me at first because there is already an ACPI driver
> > > > > > > > > which handles SMO88xx
> > > > > > > > >
> > > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > > interface (as
> > > > > > > > >
> > > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > > machines,
> > > > > > > > >
> > > > > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d
> > > > > > > > > can work
> > > > > > > > >
> > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > > does).
> > > > > > > >
> > > > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > > > lis3lv02d is i2c driver which exports axes of
> > > > > > > > accelerometer. Additionaly lis3lv02d can export also
> > > > > > > > /dev/freefall if registerer of i2c device provides irq
> > > > > > > > number -- which is not case of this patch.
> > > > > > > >
> > > > > > > > So both drivers are doing different things and both are
> > > > > > > > useful.
> > > > > > > >
> > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > device (that ST microelectronics accelerometer) but due
> > > > > > > > to complicated HW abstraction and layers on Dell laptops
> > > > > > > > it is handled by two drivers, one ACPI and one i2c.
> > > > > > > >
> > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > > (with /dev/freefall too), but due to HW abstraction it
> > > > > > > > is too much complicated...
> > > > > > >
> > > > > > > Why? AFAICT, all that is required to pass that IRQ number
> > > > > > > all the way down to lis3lv02d is to set the irq field of
> > > > > > > the struct i2c_board_info you are passing to
> > > > > > > i2c_new_device(). And you can extract that IRQ number
> > > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > > request the same IRQ on whitelisted machines. This got me
> > > > > > > thinking about a way to somehow incorporate your changes
> > > > > > > into dell-smo8800 using Wolfram's bus_notifier suggestion,
> > > > > > > but I do not have a working solution for now. What is
> > > > > > > tempting about this approach is that you would not have to
> > > > > > > scan the ACPI namespace in search of SMO88xx devices,
> > > > > > > because smo8800_add() is automatically called for them.
> > > > > > > However, I fear that the resulting solution may be more
> > > > > > > complicated than the one you submitted.
> > > > > >
> > > > > > Then we need to deal with lot of problems. Order of loading
> > > > > > .ko modules is undefined. Binding devices to drivers
> > > > > > registered by .ko module is also in "random" order. At any
> > > > > > time any of those .ko module can be unloaded or at least
> > > > > > device unbind (via sysfs) from driver... And there can be
> > > > > > some pathological situation (thanks to adding ACPI layer as
> > > > > > Andy pointed) that there will be more SMO88xx devices in
> > > > > > ACPI. Plus you can compile kernel with and without those
> > > > > > modules and also you can blacklist loading them (so compile
> > > > > > time check is not enough). And still some correct message
> > > > > > notifier must be used.
> > > > > >
> > > > > > I think such solution is much much more complicated, there
> > > > > > are lot of combinations of kernel configuration and
> > > > > > available dell devices...
> > > > >
> > > > > I tried a few more things, but ultimately failed to find a nice
> > > > > way to implement this.
> > > > >
> > > > > Another issue popped up, though. Linus' master branch contains
> > > > > a recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a
> > > > > ("i2c: use an IRQ to report Host Notify events, not alert")
> > > > > which breaks your patch. The reason for that is that
> > > > > lis3lv02d relies on the i2c client's IRQ being 0 to detect
> > > > > that it should not create /dev/freefall. Benjamin's patch
> > > > > causes the Host Notify IRQ to be assigned to the i2c client
> > > > > your patch creates, thus causing lis3lv02d to create
> > > > > /dev/freefall, which in turn conflicts with dell-smo8800 which
> > > > > is trying to create /dev/freefall itself.
> > > >
> > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > >
> > > Apologies for that.
> > >
> > > I could easily fix this by adding a kernel API to know whether the
> > > provided irq is from Host Notify or if it was coming from an actual
> > > declaration. However, I have no idea how many other drivers would
> > > require this (hopefully only this one).
> > >
> > > One other solution would be to reserve the Host Notify IRQ and let
> > > the actual drivers that need it to set it, but this was not the
> > > best solution according to Dmitri. On my side, I am not entirely
> > > against this given that it's a chip feature, so the driver should
> > > be able to know that it's available.
> > >
> > > Dmitri, Wolfram, Jean, any preferences?
> >
> > I read this:
> >
> > "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > ST microelectronics accelerometer) but due to complicated HW
> > abstraction and layers on Dell laptops it is handled by two drivers,
> > one ACPI and one i2c."
> >
> > and that is the core of the issue. You have 2 drivers fighting over
> > the same device. Fix this and it will all work.
>
> With my current implementation (which I sent in this patch), they are
> not fighting.
>
> dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only
> accelerometer device as lis3lv02d driver does not get IRQ number in
> platform data.
>
> > As far as I can see hp_accel instantiates lis3lv02d and accesses it
> > via ACPI methods, can the same be done for Dell?
>
> No, Dell does not have any ACPI methods. And as I wrote in ACPI or DMI
> is even not i2c address of device, so it needs to be specified in code
> itself.
>
> Really there is no other way... :-(
Sure there is:
1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
2. dell-smo8800 provides read/write functions for lis3lv02d that simply
forward requests to dell-smo8800-accel i2c client.
3. dell-smo8800 instantiates lis3lv02d instance like hp_accel does.
Alternatively, can lis3lv02d be tasked to create /dev/freefall?
Yet another option: can we add a new flag to i2c_board_info controlling
whether we want to enable/disable wiring up host notify interrupt?
Benjamin, is there anything "special" in RMI SMbus ACPI descriptors we
could use?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-03 20:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103194812.GD16032@dtor-ws>
[-- Attachment #1: Type: Text/Plain, Size: 9473 bytes --]
On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > wrote:
> > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień
> > > > > > > > > wrote:
> > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > 0x29. That i2c address is not specified in DMI
> > > > > > > > > > > or ACPI, so runtime detection without whitelist
> > > > > > > > > > > which is below is not possible.
> > > > > > > > > > >
> > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > address.
> > > > > > > > > >
> > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > confusing to me at first because there is already
> > > > > > > > > > an ACPI driver which handles SMO88xx
> > > > > > > > > >
> > > > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > > > interface (as
> > > > > > > > > >
> > > > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > > > machines,
> > > > > > > > > >
> > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > lis3lv02d can work
> > > > > > > > > >
> > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > > > does).
> > > > > > > > >
> > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > exports /dev/freefall device which notify userspace
> > > > > > > > > about falls. lis3lv02d is i2c driver which exports
> > > > > > > > > axes of accelerometer. Additionaly lis3lv02d can
> > > > > > > > > export also /dev/freefall if registerer of i2c
> > > > > > > > > device provides irq number -- which is not case of
> > > > > > > > > this patch.
> > > > > > > > >
> > > > > > > > > So both drivers are doing different things and both
> > > > > > > > > are useful.
> > > > > > > > >
> > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > > device (that ST microelectronics accelerometer) but
> > > > > > > > > due to complicated HW abstraction and layers on Dell
> > > > > > > > > laptops it is handled by two drivers, one ACPI and
> > > > > > > > > one i2c.
> > > > > > > > >
> > > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > > > (with /dev/freefall too), but due to HW abstraction
> > > > > > > > > it is too much complicated...
> > > > > > > >
> > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > number all the way down to lis3lv02d is to set the irq
> > > > > > > > field of the struct i2c_board_info you are passing to
> > > > > > > > i2c_new_device(). And you can extract that IRQ number
> > > > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > > > request the same IRQ on whitelisted machines. This got
> > > > > > > > me thinking about a way to somehow incorporate your
> > > > > > > > changes into dell-smo8800 using Wolfram's bus_notifier
> > > > > > > > suggestion, but I do not have a working solution for
> > > > > > > > now. What is tempting about this approach is that you
> > > > > > > > would not have to scan the ACPI namespace in search of
> > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > automatically called for them. However, I fear that
> > > > > > > > the resulting solution may be more complicated than
> > > > > > > > the one you submitted.
> > > > > > >
> > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > order. At any time any of those .ko module can be
> > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > driver... And there can be some pathological situation
> > > > > > > (thanks to adding ACPI layer as Andy pointed) that there
> > > > > > > will be more SMO88xx devices in ACPI. Plus you can
> > > > > > > compile kernel with and without those modules and also
> > > > > > > you can blacklist loading them (so compile time check is
> > > > > > > not enough). And still some correct message notifier
> > > > > > > must be used.
> > > > > > >
> > > > > > > I think such solution is much much more complicated,
> > > > > > > there are lot of combinations of kernel configuration
> > > > > > > and available dell devices...
> > > > > >
> > > > > > I tried a few more things, but ultimately failed to find a
> > > > > > nice way to implement this.
> > > > > >
> > > > > > Another issue popped up, though. Linus' master branch
> > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > events, not alert") which breaks your patch. The reason
> > > > > > for that is that lis3lv02d relies on the i2c client's IRQ
> > > > > > being 0 to detect that it should not create /dev/freefall.
> > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > assigned to the i2c client your patch creates, thus
> > > > > > causing lis3lv02d to create /dev/freefall, which in turn
> > > > > > conflicts with dell-smo8800 which is trying to create
> > > > > > /dev/freefall itself.
> > > > >
> > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > >
> > > > Apologies for that.
> > > >
> > > > I could easily fix this by adding a kernel API to know whether
> > > > the provided irq is from Host Notify or if it was coming from
> > > > an actual declaration. However, I have no idea how many other
> > > > drivers would require this (hopefully only this one).
> > > >
> > > > One other solution would be to reserve the Host Notify IRQ and
> > > > let the actual drivers that need it to set it, but this was
> > > > not the best solution according to Dmitri. On my side, I am
> > > > not entirely against this given that it's a chip feature, so
> > > > the driver should be able to know that it's available.
> > > >
> > > > Dmitri, Wolfram, Jean, any preferences?
> > >
> > > I read this:
> > >
> > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > (that ST microelectronics accelerometer) but due to complicated
> > > HW abstraction and layers on Dell laptops it is handled by two
> > > drivers, one ACPI and one i2c."
> > >
> > > and that is the core of the issue. You have 2 drivers fighting
> > > over the same device. Fix this and it will all work.
> >
> > With my current implementation (which I sent in this patch), they
> > are not fighting.
> >
> > dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d
> > only accelerometer device as lis3lv02d driver does not get IRQ
> > number in platform data.
> >
> > > As far as I can see hp_accel instantiates lis3lv02d and accesses
> > > it via ACPI methods, can the same be done for Dell?
> >
> > No, Dell does not have any ACPI methods. And as I wrote in ACPI or
> > DMI is even not i2c address of device, so it needs to be specified
> > in code itself.
> >
> > Really there is no other way... :-(
>
> Sure there is:
>
> 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> 2. dell-smo8800 provides read/write functions for lis3lv02d that
> simply forward requests to dell-smo8800-accel i2c client.
> 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel does.
Sorry, but I do not understand how you mean it... Why to provides new
read/write i2c functions which are already implemented by i2c-i801 bus
and lis3lv02d i2c driver?
> Alternatively, can lis3lv02d be tasked to create /dev/freefall?
If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
device.
But... what is problem with current implementation? Accelerometer HW
provides two functions:
1) 3 axes reports
2) Disk freefall detection
And 1) is handled by i2c driver lis3lv02d and 2) is by dell-smo8800.
Both functions are independent here.
I think you just trying to complicate this situation even more to be
more complicated as currently is.
> Yet another option: can we add a new flag to i2c_board_info
> controlling whether we want to enable/disable wiring up host notify
> interrupt? Benjamin, is there anything "special" in RMI SMbus ACPI
> descriptors we could use?
>
> Thanks.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Andy Shevchenko @ 2017-01-03 20:20 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Benjamin Tissoires, Michał Kępień,
Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel@vger.kernel.org, Platform Driver
In-Reply-To: <201701031950.17389@pali>
On Tue, Jan 3, 2017 at 8:50 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
>> "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
>> ST microelectronics accelerometer) but due to complicated HW
>> abstraction and layers on Dell laptops it is handled by two drivers,
>> one ACPI and one i2c."
>>
>> and that is the core of the issue. You have 2 drivers fighting over
>> the same device. Fix this and it will all work.
>
> With my current implementation (which I sent in this patch), they are
> not fighting.
>
> dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only
> accelerometer device as lis3lv02d driver does not get IRQ number in
> platform data.
>
>> As far as I can see hp_accel instantiates lis3lv02d and accesses it
>> via ACPI methods, can the same be done for Dell?
>
> No, Dell does not have any ACPI methods.
> And as I wrote in ACPI or DMI
> is even not i2c address of device, so it needs to be specified in code
> itself.
And as I wrote there is still a way to provide it without hardcoding
on model basis.
> Really there is no other way... :-(
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Dmitry Torokhov @ 2017-01-03 20:24 UTC (permalink / raw)
To: Pali Rohár
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201701032105.51144@pali>
On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > wrote:
> > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień
> > > > > > > > > > wrote:
> > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > 0x29. That i2c address is not specified in DMI
> > > > > > > > > > > > or ACPI, so runtime detection without whitelist
> > > > > > > > > > > > which is below is not possible.
> > > > > > > > > > > >
> > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > address.
> > > > > > > > > > >
> > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > confusing to me at first because there is already
> > > > > > > > > > > an ACPI driver which handles SMO88xx
> > > > > > > > > > >
> > > > > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > > > > interface (as
> > > > > > > > > > >
> > > > > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > > > > machines,
> > > > > > > > > > >
> > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > >
> > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > > > > does).
> > > > > > > > > >
> > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > exports /dev/freefall device which notify userspace
> > > > > > > > > > about falls. lis3lv02d is i2c driver which exports
> > > > > > > > > > axes of accelerometer. Additionaly lis3lv02d can
> > > > > > > > > > export also /dev/freefall if registerer of i2c
> > > > > > > > > > device provides irq number -- which is not case of
> > > > > > > > > > this patch.
> > > > > > > > > >
> > > > > > > > > > So both drivers are doing different things and both
> > > > > > > > > > are useful.
> > > > > > > > > >
> > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > > > device (that ST microelectronics accelerometer) but
> > > > > > > > > > due to complicated HW abstraction and layers on Dell
> > > > > > > > > > laptops it is handled by two drivers, one ACPI and
> > > > > > > > > > one i2c.
> > > > > > > > > >
> > > > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > > > > (with /dev/freefall too), but due to HW abstraction
> > > > > > > > > > it is too much complicated...
> > > > > > > > >
> > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > number all the way down to lis3lv02d is to set the irq
> > > > > > > > > field of the struct i2c_board_info you are passing to
> > > > > > > > > i2c_new_device(). And you can extract that IRQ number
> > > > > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > > > > request the same IRQ on whitelisted machines. This got
> > > > > > > > > me thinking about a way to somehow incorporate your
> > > > > > > > > changes into dell-smo8800 using Wolfram's bus_notifier
> > > > > > > > > suggestion, but I do not have a working solution for
> > > > > > > > > now. What is tempting about this approach is that you
> > > > > > > > > would not have to scan the ACPI namespace in search of
> > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > the one you submitted.
> > > > > > > >
> > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > driver... And there can be some pathological situation
> > > > > > > > (thanks to adding ACPI layer as Andy pointed) that there
> > > > > > > > will be more SMO88xx devices in ACPI. Plus you can
> > > > > > > > compile kernel with and without those modules and also
> > > > > > > > you can blacklist loading them (so compile time check is
> > > > > > > > not enough). And still some correct message notifier
> > > > > > > > must be used.
> > > > > > > >
> > > > > > > > I think such solution is much much more complicated,
> > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > and available dell devices...
> > > > > > >
> > > > > > > I tried a few more things, but ultimately failed to find a
> > > > > > > nice way to implement this.
> > > > > > >
> > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > events, not alert") which breaks your patch. The reason
> > > > > > > for that is that lis3lv02d relies on the i2c client's IRQ
> > > > > > > being 0 to detect that it should not create /dev/freefall.
> > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > causing lis3lv02d to create /dev/freefall, which in turn
> > > > > > > conflicts with dell-smo8800 which is trying to create
> > > > > > > /dev/freefall itself.
> > > > > >
> > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > >
> > > > > Apologies for that.
> > > > >
> > > > > I could easily fix this by adding a kernel API to know whether
> > > > > the provided irq is from Host Notify or if it was coming from
> > > > > an actual declaration. However, I have no idea how many other
> > > > > drivers would require this (hopefully only this one).
> > > > >
> > > > > One other solution would be to reserve the Host Notify IRQ and
> > > > > let the actual drivers that need it to set it, but this was
> > > > > not the best solution according to Dmitri. On my side, I am
> > > > > not entirely against this given that it's a chip feature, so
> > > > > the driver should be able to know that it's available.
> > > > >
> > > > > Dmitri, Wolfram, Jean, any preferences?
> > > >
> > > > I read this:
> > > >
> > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > (that ST microelectronics accelerometer) but due to complicated
> > > > HW abstraction and layers on Dell laptops it is handled by two
> > > > drivers, one ACPI and one i2c."
> > > >
> > > > and that is the core of the issue. You have 2 drivers fighting
> > > > over the same device. Fix this and it will all work.
> > >
> > > With my current implementation (which I sent in this patch), they
> > > are not fighting.
> > >
> > > dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d
> > > only accelerometer device as lis3lv02d driver does not get IRQ
> > > number in platform data.
> > >
> > > > As far as I can see hp_accel instantiates lis3lv02d and accesses
> > > > it via ACPI methods, can the same be done for Dell?
> > >
> > > No, Dell does not have any ACPI methods. And as I wrote in ACPI or
> > > DMI is even not i2c address of device, so it needs to be specified
> > > in code itself.
> > >
> > > Really there is no other way... :-(
> >
> > Sure there is:
> >
> > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > simply forward requests to dell-smo8800-accel i2c client.
> > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel does.
>
> Sorry, but I do not understand how you mean it... Why to provides new
> read/write i2c functions which are already implemented by i2c-i801 bus
> and lis3lv02d i2c driver?
Because that would allow you to avoid clashes with i2c creating
interrupt mapping for client residing on host-notify-capable controller.
>
> > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
>
> If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> device.
>
> But... what is problem with current implementation? Accelerometer HW
> provides two functions:
>
> 1) 3 axes reports
> 2) Disk freefall detection
>
> And 1) is handled by i2c driver lis3lv02d and 2) is by dell-smo8800.
> Both functions are independent here.
>
> I think you just trying to complicate this situation even more to be
> more complicated as currently is.
Because this apparently does not work for you, does it? In general, if
you want the same hardware be handled by 2 different drivers you are
going to have bad time.
It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are the
same, right? So, instead of having 2 drivers split the functionality,
can you forego registering smo8800 ACPI driver on your whitelisted
boxes and instead instantiate full i2c client device with properly
assigned both address and IRQ and let lis3lv02d handle it (providing
both accelerometer data and /dev/freefall)?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-03 20:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103202418.GE16032@dtor-ws>
[-- Attachment #1: Type: Text/Plain, Size: 12177 bytes --]
On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > >
> > > > > wrote:
> > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > wrote:
> > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > Kępień
> > > > > > > > > > >
> > > > > > > > > > > wrote:
> > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > address.
> > > > > > > > > > > >
> > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > >
> > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > that:
> > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > richer interface (as
> > > > > > > > > > > >
> > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > some machines,
> > > > > > > > > > > >
> > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > >
> > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > >
> > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > >
> > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > both are useful.
> > > > > > > > > > >
> > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > >
> > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > >
> > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > the one you submitted.
> > > > > > > > >
> > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > driver... And there can be some pathological
> > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > still some correct message notifier must be used.
> > > > > > > > >
> > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > and available dell devices...
> > > > > > > >
> > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > find a nice way to implement this.
> > > > > > > >
> > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > create /dev/freefall.
> > > > > > > >
> > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > >
> > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > create /dev/freefall itself.
> > > > > > >
> > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > >
> > > > > > Apologies for that.
> > > > > >
> > > > > > I could easily fix this by adding a kernel API to know
> > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > coming from an actual declaration. However, I have no idea
> > > > > > how many other drivers would require this (hopefully only
> > > > > > this one).
> > > > > >
> > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > and let the actual drivers that need it to set it, but
> > > > > > this was not the best solution according to Dmitri. On my
> > > > > > side, I am not entirely against this given that it's a
> > > > > > chip feature, so the driver should be able to know that
> > > > > > it's available.
> > > > > >
> > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > >
> > > > > I read this:
> > > > >
> > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > (that ST microelectronics accelerometer) but due to
> > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > handled by two drivers, one ACPI and one i2c."
> > > > >
> > > > > and that is the core of the issue. You have 2 drivers
> > > > > fighting over the same device. Fix this and it will all
> > > > > work.
> > > >
> > > > With my current implementation (which I sent in this patch),
> > > > they are not fighting.
> > > >
> > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > not get IRQ number in platform data.
> > > >
> > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > >
> > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > or DMI is even not i2c address of device, so it needs to be
> > > > specified in code itself.
> > > >
> > > > Really there is no other way... :-(
> > >
> > > Sure there is:
> > >
> > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > simply forward requests to dell-smo8800-accel i2c client.
> > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > does.
> >
> > Sorry, but I do not understand how you mean it... Why to provides
> > new read/write i2c functions which are already implemented by
> > i2c-i801 bus and lis3lv02d i2c driver?
>
> Because that would allow you to avoid clashes with i2c creating
> interrupt mapping for client residing on host-notify-capable
> controller.
>
> > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> >
> > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > device.
> >
> > But... what is problem with current implementation? Accelerometer
> > HW provides two functions:
> >
> > 1) 3 axes reports
> > 2) Disk freefall detection
> >
> > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > dell-smo8800. Both functions are independent here.
> >
> > I think you just trying to complicate this situation even more to
> > be more complicated as currently is.
>
> Because this apparently does not work for you, does it?
It is working fine. I do not see any problem.
> In general,
> if you want the same hardware be handled by 2 different drivers you
> are going to have bad time.
Yes, but in this case half of device is ACPI based and other half i2c
based. This is problem of ACPI and Dell design.
> It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> the same, right?
Yes. I understand that clean solution is to have one driver which
provides everything.
But because half of data are ACPI and half i2c, you still needs to
create two drivers (one ACPI and one i2c). You can put both drivers into
one .ko module, but still these will be two drivers due to how ACPI and
i2c linux abstractions are different.
> So, instead of having 2 drivers split the
> functionality, can you forego registering smo8800 ACPI driver on
> your whitelisted boxes and instead instantiate full i2c client
> device with properly assigned both address and IRQ and let lis3lv02d
> handle it (providing both accelerometer data and /dev/freefall)?
With Michał we already discussed about it, see emails. Basically you can
enable/disable kernel modules at compile time or blacklist at runtime
(or even chose what will be compiled into vmlinux and what as external
.ko module). Some distributions blacklist i2c-i801.ko module... And
there can be also problem with initialization of i2c-i801 driver (fix is
in commit a7ae81952cda, but does not have to work at every time!). So
that move on whitelisted machines can potentially cause disappearance of
/dev/freefall and users will not have hdd protection which is currently
working.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Dmitry Torokhov @ 2017-01-03 20:59 UTC (permalink / raw)
To: Pali Rohár
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201701032139.13061@pali>
On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > >
> > > > > > wrote:
> > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > wrote:
> > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > Kępień
> > > > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > address.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > >
> > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > that:
> > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > >
> > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > some machines,
> > > > > > > > > > > > >
> > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > >
> > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > >
> > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > >
> > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > both are useful.
> > > > > > > > > > > >
> > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > >
> > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > the one you submitted.
> > > > > > > > > >
> > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > >
> > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > and available dell devices...
> > > > > > > > >
> > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > find a nice way to implement this.
> > > > > > > > >
> > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > create /dev/freefall.
> > > > > > > > >
> > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > >
> > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > create /dev/freefall itself.
> > > > > > > >
> > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > >
> > > > > > > Apologies for that.
> > > > > > >
> > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > how many other drivers would require this (hopefully only
> > > > > > > this one).
> > > > > > >
> > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > side, I am not entirely against this given that it's a
> > > > > > > chip feature, so the driver should be able to know that
> > > > > > > it's available.
> > > > > > >
> > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > >
> > > > > > I read this:
> > > > > >
> > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > >
> > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > fighting over the same device. Fix this and it will all
> > > > > > work.
> > > > >
> > > > > With my current implementation (which I sent in this patch),
> > > > > they are not fighting.
> > > > >
> > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > not get IRQ number in platform data.
> > > > >
> > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > >
> > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > specified in code itself.
> > > > >
> > > > > Really there is no other way... :-(
> > > >
> > > > Sure there is:
> > > >
> > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > does.
> > >
> > > Sorry, but I do not understand how you mean it... Why to provides
> > > new read/write i2c functions which are already implemented by
> > > i2c-i801 bus and lis3lv02d i2c driver?
> >
> > Because that would allow you to avoid clashes with i2c creating
> > interrupt mapping for client residing on host-notify-capable
> > controller.
> >
> > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > >
> > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > device.
> > >
> > > But... what is problem with current implementation? Accelerometer
> > > HW provides two functions:
> > >
> > > 1) 3 axes reports
> > > 2) Disk freefall detection
> > >
> > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > dell-smo8800. Both functions are independent here.
> > >
> > > I think you just trying to complicate this situation even more to
> > > be more complicated as currently is.
> >
> > Because this apparently does not work for you, does it?
>
> It is working fine. I do not see any problem.
>
> > In general,
> > if you want the same hardware be handled by 2 different drivers you
> > are going to have bad time.
>
> Yes, but in this case half of device is ACPI based and other half i2c
> based. This is problem of ACPI and Dell design.
>
> > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > the same, right?
>
> Yes. I understand that clean solution is to have one driver which
> provides everything.
>
> But because half of data are ACPI and half i2c, you still needs to
> create two drivers (one ACPI and one i2c). You can put both drivers into
> one .ko module, but still these will be two drivers due to how ACPI and
> i2c linux abstractions are different.
>
> > So, instead of having 2 drivers split the
> > functionality, can you forego registering smo8800 ACPI driver on
> > your whitelisted boxes and instead instantiate full i2c client
> > device with properly assigned both address and IRQ and let lis3lv02d
> > handle it (providing both accelerometer data and /dev/freefall)?
>
> With Michał we already discussed about it, see emails. Basically you can
> enable/disable kernel modules at compile time or blacklist at runtime
> (or even chose what will be compiled into vmlinux and what as external
> .ko module).
This can be solved with a bit of Kconfig/IS_ENABLED() code.
> Some distributions blacklist i2c-i801.ko module... And
Any particular reason for that?
> there can be also problem with initialization of i2c-i801 driver (fix is
> in commit a7ae81952cda, but does not have to work at every time!). So
> that move on whitelisted machines can potentially cause disappearance of
> /dev/freefall and users will not have hdd protection which is currently
> working.
Well, I gave you 2 possible solutions (roll your own i2c read/write,
forward them to i2c client) or have faith in your implementation and let
lis3lv02d handle it.
The 3rd one is to possibly add a flag to disable host notify to IRQ
mapping for given client (if Wolfram/Jean OK with it).
Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
i2c_client with board_info->irq = -1.
Pick whichever you prefer.
By the way, what do you need accelerometer for on these devices? They
don't appear to be tablets that could use one...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-03 21:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Pali Rohár, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103194812.GD16032@dtor-ws>
On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > microelectronics accelerometer at i2c address 0x29.
> > > > > > > > > > > That i2c address is not specified in DMI or ACPI, so
> > > > > > > > > > > runtime detection without whitelist which is below
> > > > > > > > > > > is not possible.
> > > > > > > > > > >
> > > > > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > > > > represent that accelerometer. Unfortunately without
> > > > > > > > > > > i2c address.
> > > > > > > > > >
> > > > > > > > > > This part of the commit message sounded a bit confusing
> > > > > > > > > > to me at first because there is already an ACPI driver
> > > > > > > > > > which handles SMO88xx
> > > > > > > > > >
> > > > > > > > > > devices (dell-smo8800). My understanding is that:
> > > > > > > > > > * the purpose of this patch is to expose a richer
> > > > > > > > > > interface (as
> > > > > > > > > >
> > > > > > > > > > provided by lis3lv02d) to these devices on some
> > > > > > > > > > machines,
> > > > > > > > > >
> > > > > > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d
> > > > > > > > > > can work
> > > > > > > > > >
> > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > effectively duplicates the work that lis3lv02d
> > > > > > > > > > does).
> > > > > > > > >
> > > > > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > > > > lis3lv02d is i2c driver which exports axes of
> > > > > > > > > accelerometer. Additionaly lis3lv02d can export also
> > > > > > > > > /dev/freefall if registerer of i2c device provides irq
> > > > > > > > > number -- which is not case of this patch.
> > > > > > > > >
> > > > > > > > > So both drivers are doing different things and both are
> > > > > > > > > useful.
> > > > > > > > >
> > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > > device (that ST microelectronics accelerometer) but due
> > > > > > > > > to complicated HW abstraction and layers on Dell laptops
> > > > > > > > > it is handled by two drivers, one ACPI and one i2c.
> > > > > > > > >
> > > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > > > (with /dev/freefall too), but due to HW abstraction it
> > > > > > > > > is too much complicated...
> > > > > > > >
> > > > > > > > Why? AFAICT, all that is required to pass that IRQ number
> > > > > > > > all the way down to lis3lv02d is to set the irq field of
> > > > > > > > the struct i2c_board_info you are passing to
> > > > > > > > i2c_new_device(). And you can extract that IRQ number
> > > > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > > > request the same IRQ on whitelisted machines. This got me
> > > > > > > > thinking about a way to somehow incorporate your changes
> > > > > > > > into dell-smo8800 using Wolfram's bus_notifier suggestion,
> > > > > > > > but I do not have a working solution for now. What is
> > > > > > > > tempting about this approach is that you would not have to
> > > > > > > > scan the ACPI namespace in search of SMO88xx devices,
> > > > > > > > because smo8800_add() is automatically called for them.
> > > > > > > > However, I fear that the resulting solution may be more
> > > > > > > > complicated than the one you submitted.
> > > > > > >
> > > > > > > Then we need to deal with lot of problems. Order of loading
> > > > > > > .ko modules is undefined. Binding devices to drivers
> > > > > > > registered by .ko module is also in "random" order. At any
> > > > > > > time any of those .ko module can be unloaded or at least
> > > > > > > device unbind (via sysfs) from driver... And there can be
> > > > > > > some pathological situation (thanks to adding ACPI layer as
> > > > > > > Andy pointed) that there will be more SMO88xx devices in
> > > > > > > ACPI. Plus you can compile kernel with and without those
> > > > > > > modules and also you can blacklist loading them (so compile
> > > > > > > time check is not enough). And still some correct message
> > > > > > > notifier must be used.
> > > > > > >
> > > > > > > I think such solution is much much more complicated, there
> > > > > > > are lot of combinations of kernel configuration and
> > > > > > > available dell devices...
> > > > > >
> > > > > > I tried a few more things, but ultimately failed to find a nice
> > > > > > way to implement this.
> > > > > >
> > > > > > Another issue popped up, though. Linus' master branch contains
> > > > > > a recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a
> > > > > > ("i2c: use an IRQ to report Host Notify events, not alert")
> > > > > > which breaks your patch. The reason for that is that
> > > > > > lis3lv02d relies on the i2c client's IRQ being 0 to detect
> > > > > > that it should not create /dev/freefall. Benjamin's patch
> > > > > > causes the Host Notify IRQ to be assigned to the i2c client
> > > > > > your patch creates, thus causing lis3lv02d to create
> > > > > > /dev/freefall, which in turn conflicts with dell-smo8800 which
> > > > > > is trying to create /dev/freefall itself.
> > > > >
> > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > >
> > > > Apologies for that.
> > > >
> > > > I could easily fix this by adding a kernel API to know whether the
> > > > provided irq is from Host Notify or if it was coming from an actual
> > > > declaration. However, I have no idea how many other drivers would
> > > > require this (hopefully only this one).
> > > >
> > > > One other solution would be to reserve the Host Notify IRQ and let
> > > > the actual drivers that need it to set it, but this was not the
> > > > best solution according to Dmitri. On my side, I am not entirely
> > > > against this given that it's a chip feature, so the driver should
> > > > be able to know that it's available.
> > > >
> > > > Dmitri, Wolfram, Jean, any preferences?
> > >
> > > I read this:
> > >
> > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > > ST microelectronics accelerometer) but due to complicated HW
> > > abstraction and layers on Dell laptops it is handled by two drivers,
> > > one ACPI and one i2c."
> > >
> > > and that is the core of the issue. You have 2 drivers fighting over
> > > the same device. Fix this and it will all work.
> >
> > With my current implementation (which I sent in this patch), they are
> > not fighting.
> >
> > dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only
> > accelerometer device as lis3lv02d driver does not get IRQ number in
> > platform data.
> >
> > > As far as I can see hp_accel instantiates lis3lv02d and accesses it
> > > via ACPI methods, can the same be done for Dell?
> >
> > No, Dell does not have any ACPI methods. And as I wrote in ACPI or DMI
> > is even not i2c address of device, so it needs to be specified in code
> > itself.
> >
> > Really there is no other way... :-(
>
> Sure there is:
>
> 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> 2. dell-smo8800 provides read/write functions for lis3lv02d that simply
> forward requests to dell-smo8800-accel i2c client.
> 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel does.
>
> Alternatively, can lis3lv02d be tasked to create /dev/freefall?
>
> Yet another option: can we add a new flag to i2c_board_info controlling
> whether we want to enable/disable wiring up host notify interrupt?
That should be fairly easy to implement. For now, given that only Elan
and Synaptics are the one in need for Host Notify, it could be better to
request the Host Notify IRQ instead of disabling it unconditionally
(which would make the current yet 8 years old lis3lv02d driver happy
again).
> Benjamin, is there anything "special" in RMI SMbus ACPI descriptors we
> could use?
>
No, there is nothing special. Same situation for Elan with their latest
touchpads over PS/2. There is just a knowledge from the driver that
there is a device connected on a Host Notify capable bus on a specific
address. To give you an idea, on Windows, the Synaptics (and Elan)
driver even ships the equivalent of i2c-i801 to be sure to have one
driver for it...
So the knowledge is all in the driver.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Hans de Goede @ 2017-01-03 22:10 UTC (permalink / raw)
To: Jacob Pan
Cc: Jonathan Cameron, Chen-Yu Tsai, Lars-Peter Clausen,
Peter Meerwald-Stadler, russianneuromancer @ ya . ru, linux-iio,
linux-i2c
In-Reply-To: <20170103102343.735dbdd1@jacob-builder>
Hi,
On 01/03/2017 07:23 PM, Jacob Pan wrote:
> On Sun, 1 Jan 2017 12:19:40 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>>
>> On 30-12-16 19:15, Jacob Pan wrote:
>>> On Fri, 30 Dec 2016 16:46:03 +0000
>>> Jonathan Cameron <jic23@kernel.org> wrote:
>>>
>>>> On 14/12/16 13:55, Hans de Goede wrote:
>>>>> For some reason the axp288_adc driver was modifying the
>>>>> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
>>>>> whether the GP_ADC channel or another channel was written.
>>>>>
>>>>> These bits control when a bias current is send to the TS_PIN, the
>>>>> GP_ADC has its own pin and a separate bit in another register to
>>>>> control the bias current.
>>>>>
>>> It has been a while. Just looked at the datasheet, reg 84h is for
>>> ADC and TS pin control. IIRC, we had to set the TS bits in order to
>>> make ADC read to work.
>>
>> The bits the code I'm removing are manipulating are bits 0 & 1 of
>> reg 84h which are:
>>
>> 1-0 Current source from TS pin on/off enable bit [1:0]
>>
>> 00: off
>> 01: on when charging battery, off when not charging
>> 10: on in ADC phase and off when out of the ADC phase, for power
>> saving 11: always on
>>
>> And they are being toggled between 10 and 11, so in both
>> cases the current source is enabled when reading the adc
>> value. Specifically the code this patch removes are setting
>> these bits to 10 before reading and back to 11 after reading,
>> which makes no sense.
>>
>> And to make things even funkier, this manipulation is only
>> done when reading the GP_ADC, which is the ADC function of
>> GPIO0, whereas these bits control the bias current source
>> for the TS pin, which is a completely different pin.
>>
>> So all in all this entire bit of code seems to be big NOP.
>>
> It could have been a quirk we had to do on our platforms, I just
> cannot recall the details. Are you testing this on x86 platforms?
Yes, I've successfully tested this on 2 different models cherrytrail
tablets.
Regards,
Hans
>
>>> What is the other register?
>>
>> The register to actually enable / disable the bias current
>> source for GPIO0 / the GPADC pin is register 85h, where setting
>> bit 2 enables the GPIO0 output current.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>>> Not only does changing when to enable the TS_PIN bias current
>>>>> (always or only when sampling) when reading the GP_ADC make no
>>>>> sense at all, the code is modifying these bits is writing the
>>>>> entire register, assuming that all the other bits have their
>>>>> default value.
>>> Agreed, it would be better to do read-modify-write and not to
>>> touch the other bits.
>>>>> So if the firmware has configured a different bias-current for
>>>>> either pin, then that change gets clobbered by the write, likewise
>>>>> if the firmware has set bit 2 to indicate that the battery has no
>>>>> thermal sensor, this will get clobbered by the write.
>>>>>
>>>>> This commit fixes all this, by simply removing all writes to the
>>>>> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
>>>>> GP_ADC pin, and can actually be harmful.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> I guess you probably have more up to date contact details than I
>>>> do, but seems worth trying to cc Jacob Pan on this to see if we
>>>> can find out what the original reasoning behind this was. Seems a
>>>> very odd thing to do with no purpose!
>>>>
>>>> If Jacob isn't contactable we'll fall back to guessing it was just
>>>> an oddity of driver evolution.
>>>>
>>>> Jonathan
>>>>> ---
>>>>> drivers/iio/adc/axp288_adc.c | 32
>>>>> +------------------------------- 1 file changed, 1 insertion(+),
>>>>> 31 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/axp288_adc.c
>>>>> b/drivers/iio/adc/axp288_adc.c index 7fd2494..64799ad 100644
>>>>> --- a/drivers/iio/adc/axp288_adc.c
>>>>> +++ b/drivers/iio/adc/axp288_adc.c
>>>>> @@ -28,8 +28,6 @@
>>>>> #include <linux/iio/driver.h>
>>>>>
>>>>> #define AXP288_ADC_EN_MASK 0xF1
>>>>> -#define AXP288_ADC_TS_PIN_GPADC 0xF2
>>>>> -#define AXP288_ADC_TS_PIN_ON 0xF3
>>>>>
>>>>> enum axp288_adc_id {
>>>>> AXP288_ADC_TS,
>>>>> @@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val,
>>>>> unsigned long address, return IIO_VAL_INT;
>>>>> }
>>>>>
>>>>> -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int
>>>>> mode,
>>>>> - unsigned long address)
>>>>> -{
>>>>> - /* channels other than GPADC do not need to switch TS pin
>>>>> */
>>>>> - if (address != AXP288_GP_ADC_H)
>>>>> - return 0;
>>>>> -
>>>>> - return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
>>>>> mode); -}
>>>>> -
>>>>> static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>>>>> struct iio_chan_spec const *chan,
>>>>> int *val, int *val2, long mask)
>>>>> @@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev
>>>>> *indio_dev, mutex_lock(&indio_dev->mlock);
>>>>> switch (mask) {
>>>>> case IIO_CHAN_INFO_RAW:
>>>>> - if (axp288_adc_set_ts(info->regmap,
>>>>> AXP288_ADC_TS_PIN_GPADC,
>>>>> - chan->address)) {
>>>>> - dev_err(&indio_dev->dev, "GPADC mode\n");
>>>>> - ret = -EINVAL;
>>>>> - break;
>>>>> - }
>>>>> ret = axp288_adc_read_channel(val, chan->address,
>>>>> info->regmap);
>>>>> - if (axp288_adc_set_ts(info->regmap,
>>>>> AXP288_ADC_TS_PIN_ON,
>>>>> - chan->address))
>>>>> - dev_err(&indio_dev->dev, "TS pin
>>>>> restore\n"); break;
>>>>> default:
>>>>> ret = -EINVAL;
>>>>> @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev
>>>>> *indio_dev, return ret;
>>>>> }
>>>>>
>>>>> -static int axp288_adc_set_state(struct regmap *regmap)
>>>>> -{
>>>>> - /* ADC should be always enabled for internal FG to
>>>>> function */
>>>>> - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
>>>>> AXP288_ADC_TS_PIN_ON))
>>>>> - return -EIO;
>>>>> -
>>>>> - return regmap_write(regmap, AXP20X_ADC_EN1,
>>>>> AXP288_ADC_EN_MASK); -}
>>>>> -
>>>>> static const struct iio_info axp288_adc_iio_info = {
>>>>> .read_raw = &axp288_adc_read_raw,
>>>>> .driver_module = THIS_MODULE,
>>>>> @@ -199,7 +169,7 @@ static int axp288_adc_probe(struct
>>>>> platform_device *pdev)
>>>>> * Set ADC to enabled state at all time, including system
>>>>> suspend.
>>>>> * otherwise internal fuel gauge functionality may be
>>>>> affected. */
>>>>> - ret = axp288_adc_set_state(axp20x->regmap);
>>>>> + ret = regmap_write(info->regmap, AXP20X_ADC_EN1,
>>>>> AXP288_ADC_EN_MASK); if (ret) {
>>>>> dev_err(&pdev->dev, "unable to enable ADC
>>>>> device\n"); return ret;
>>>>>
>>>>
>>>
>>> [Jacob Pan]
>>>
>
> [Jacob Pan]
>
^ permalink raw reply
* Re: [PATCH linux 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: Rob Herring @ 2017-01-03 22:45 UTC (permalink / raw)
To: eajames.ibm
Cc: linux, jdelvare, corbet, mark.rutland, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
In-Reply-To: <1483120568-21082-6-git-send-email-eajames.ibm@gmail.com>
On Fri, Dec 30, 2016 at 11:56:07AM -0600, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
> well as probe the entire driver from the I2C bus. I2C is the communication
> method between the BMC and the P8 OCC.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../devicetree/bindings/i2c/i2c-ibm-occ.txt | 13 ++
bindings/i2c/ is generally for host controllers. bindings/hwmon perhaps.
> drivers/hwmon/occ/Kconfig | 14 ++
> drivers/hwmon/occ/Makefile | 1 +
> drivers/hwmon/occ/p8_occ_i2c.c | 141 +++++++++++++++++++++
> 4 files changed, 169 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
> create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt b/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
> new file mode 100644
> index 0000000..b0d2b36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
> @@ -0,0 +1,13 @@
> +HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
> +
> +Required properties:
> + - compatible: must be "ibm,p8-occ-i2c"
> + - reg: physical address
> +
> +Example:
> +i2c3: i2c-bus@100 {
> + occ@50 {
> + compatible = "ibm,p8-occ-i2c";
> + reg = <0x50>;
> + };
> +};
> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> index cdb64a7..3a5188f 100644
> --- a/drivers/hwmon/occ/Kconfig
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
>
> This driver can also be built as a module. If so, the module
> will be called occ.
> +
> +if SENSORS_PPC_OCC
> +
> +config SENSORS_PPC_OCC_P8_I2C
> + tristate "POWER8 OCC hwmon support"
> + depends on I2C
> + help
> + Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
> + exposing temperature, frequency and power measurements.
> +
> + This driver can also be built as a module. If so, the module will be
> + called p8-occ-i2c.
> +
> +endif
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index a6881f9..9294b58 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> +obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
> diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
> new file mode 100644
> index 0000000..0c65894
> --- /dev/null
> +++ b/drivers/hwmon/occ/p8_occ_i2c.c
> @@ -0,0 +1,141 @@
> +/*
> + * p8_occ_i2c.c - hwmon OCC driver
> + *
> + * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +#include "scom.h"
> +#include "occ_scom_i2c.h"
> +#include "occ_p8.h"
> +#include "occ_sysfs.h"
> +
> +#define P8_OCC_I2C_NAME "p8-occ-i2c"
> +
> +static char *caps_sensor_names[] = {
> + "curr_powercap",
> + "curr_powerreading",
> + "norm_powercap",
> + "max_powercap",
> + "min_powercap",
> + "user_powerlimit"
> +};
> +
> +int p8_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> + /* P8 i2c slave requires address to be shifted by 1 */
> + address = address << 1;
> +
> + return occ_i2c_getscom(bus, address, data);
> +}
> +
> +int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
> +{
> + /* P8 i2c slave requires address to be shifted by 1 */
> + address = address << 1;
> +
> + return occ_i2c_putscom(bus, address, data0, data1);
> +}
> +
> +static struct occ_bus_ops p8_bus_ops = {
> + .getscom = p8_i2c_getscom,
> + .putscom = p8_i2c_putscom,
> +};
> +
> +static struct occ_sysfs_config p8_sysfs_config = {
> + .num_caps_fields = ARRAY_SIZE(caps_sensor_names),
> + .caps_names = caps_sensor_names,
> +};
> +
> +static int p8_occ_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct occ *occ;
> + struct occ_sysfs *hwmon;
> +
> + occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
> + if (IS_ERR(occ))
> + return PTR_ERR(occ);
> +
> + hwmon = occ_sysfs_start(&client->dev, occ, &p8_sysfs_config);
> + if (IS_ERR(hwmon))
> + return PTR_ERR(hwmon);
> +
> + i2c_set_clientdata(client, hwmon);
> +
> + return 0;
> +}
> +
> +static int p8_occ_remove(struct i2c_client *client)
> +{
> + int rc;
> + struct occ_sysfs *hwmon = i2c_get_clientdata(client);
> + struct occ *occ = hwmon->occ;
> +
> + rc = occ_sysfs_stop(&client->dev, hwmon);
> +
> + rc = rc || p8_occ_stop(occ);
> +
> + return rc;
> +}
> +
> +/* used by old-style board info. */
> +static const struct i2c_device_id occ_ids[] = {
> + { P8_OCC_I2C_NAME, 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, occ_ids);
> +
> +/* used by device table */
> +static const struct of_device_id occ_of_match[] = {
> + { .compatible = "ibm,p8-occ-i2c" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, occ_of_match);
> +
> +/*
> + * i2c-core uses i2c-detect() to detect device in below address list.
> + * If exists, address will be assigned to client.
> + * It is also possible to read address from device table.
> + */
> +static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
> +
> +static struct i2c_driver p8_occ_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = P8_OCC_I2C_NAME,
> + .pm = NULL,
> + .of_match_table = occ_of_match,
> + },
> + .probe = p8_occ_probe,
> + .remove = p8_occ_remove,
> + .id_table = occ_ids,
> + .address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(p8_occ_driver);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Dmitry Torokhov @ 2017-01-04 6:36 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pali Rohár, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103212934.GK5767@mail.corp.redhat.com>
On Tue, Jan 03, 2017 at 10:29:34PM +0100, Benjamin Tissoires wrote:
> On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
>
> > Yet another option: can we add a new flag to i2c_board_info controlling
> > whether we want to enable/disable wiring up host notify interrupt?
>
> That should be fairly easy to implement. For now, given that only Elan
> and Synaptics are the one in need for Host Notify, it could be better to
> request the Host Notify IRQ instead of disabling it unconditionally
> (which would make the current yet 8 years old lis3lv02d driver happy
> again).
I like that we have it done in i2c core instead of having drivers
implementing it individually. Since you are saying that handling host
notify is property of the slave/driver maybe we should be adding a flag
to the *i2c_driver* structure to let i2c core that we want to have host
notify mapped as interrupt if "native" interrupt is not supplied by the
platform code?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 8:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170103205937.GA2289@dtor-ws>
On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > >
> > > > > > > wrote:
> > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > Kępień
> > > > > > > > > > > > >
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > >
> > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > >
> > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > >
> > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > the one you submitted.
> > > > > > > > > > >
> > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > >
> > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > and available dell devices...
> > > > > > > > > >
> > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > find a nice way to implement this.
> > > > > > > > > >
> > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > create /dev/freefall.
> > > > > > > > > >
> > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > >
> > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > create /dev/freefall itself.
> > > > > > > > >
> > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > >
> > > > > > > > Apologies for that.
> > > > > > > >
> > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > this one).
> > > > > > > >
> > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > it's available.
> > > > > > > >
> > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > >
> > > > > > > I read this:
> > > > > > >
> > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > >
> > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > work.
> > > > > >
> > > > > > With my current implementation (which I sent in this patch),
> > > > > > they are not fighting.
> > > > > >
> > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > not get IRQ number in platform data.
> > > > > >
> > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > >
> > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > specified in code itself.
> > > > > >
> > > > > > Really there is no other way... :-(
> > > > >
> > > > > Sure there is:
> > > > >
> > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > does.
> > > >
> > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > new read/write i2c functions which are already implemented by
> > > > i2c-i801 bus and lis3lv02d i2c driver?
> > >
> > > Because that would allow you to avoid clashes with i2c creating
> > > interrupt mapping for client residing on host-notify-capable
> > > controller.
> > >
> > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > >
> > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > device.
> > > >
> > > > But... what is problem with current implementation? Accelerometer
> > > > HW provides two functions:
> > > >
> > > > 1) 3 axes reports
> > > > 2) Disk freefall detection
> > > >
> > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > dell-smo8800. Both functions are independent here.
> > > >
> > > > I think you just trying to complicate this situation even more to
> > > > be more complicated as currently is.
> > >
> > > Because this apparently does not work for you, does it?
> >
> > It is working fine. I do not see any problem.
> >
> > > In general,
> > > if you want the same hardware be handled by 2 different drivers you
> > > are going to have bad time.
> >
> > Yes, but in this case half of device is ACPI based and other half i2c
> > based. This is problem of ACPI and Dell design.
> >
> > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > the same, right?
> >
> > Yes. I understand that clean solution is to have one driver which
> > provides everything.
> >
> > But because half of data are ACPI and half i2c, you still needs to
> > create two drivers (one ACPI and one i2c). You can put both drivers into
> > one .ko module, but still these will be two drivers due to how ACPI and
> > i2c linux abstractions are different.
> >
> > > So, instead of having 2 drivers split the
> > > functionality, can you forego registering smo8800 ACPI driver on
> > > your whitelisted boxes and instead instantiate full i2c client
> > > device with properly assigned both address and IRQ and let lis3lv02d
> > > handle it (providing both accelerometer data and /dev/freefall)?
> >
> > With Michał we already discussed about it, see emails. Basically you can
> > enable/disable kernel modules at compile time or blacklist at runtime
> > (or even chose what will be compiled into vmlinux and what as external
> > .ko module).
>
> This can be solved with a bit of Kconfig/IS_ENABLED() code.
>
> > Some distributions blacklist i2c-i801.ko module... And
>
> Any particular reason for that?
>
> > there can be also problem with initialization of i2c-i801 driver (fix is
> > in commit a7ae81952cda, but does not have to work at every time!). So
> > that move on whitelisted machines can potentially cause disappearance of
> > /dev/freefall and users will not have hdd protection which is currently
> > working.
>
> Well, I gave you 2 possible solutions (roll your own i2c read/write,
> forward them to i2c client) or have faith in your implementation and let
> lis3lv02d handle it.
>
> The 3rd one is to possibly add a flag to disable host notify to IRQ
> mapping for given client (if Wolfram/Jean OK with it).
>
> Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> i2c_client with board_info->irq = -1.
>
> Pick whichever you prefer.
>
> By the way, what do you need accelerometer for on these devices? They
> don't appear to be tablets that could use one...
Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
not work... I thought that discussion is about different mechanism how
to implement bus registration notification to smo8800 driver (or
different solution to not have registration in i801).
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 9:05 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104081804.GA3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > >
> > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > >
> > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > and available dell devices...
> > > > > > > > > > >
> > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > >
> > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > >
> > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > >
> > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > >
> > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > >
> > > > > > > > > Apologies for that.
> > > > > > > > >
> > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > this one).
> > > > > > > > >
> > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > it's available.
> > > > > > > > >
> > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > >
> > > > > > > > I read this:
> > > > > > > >
> > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > >
> > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > work.
> > > > > > >
> > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > they are not fighting.
> > > > > > >
> > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > not get IRQ number in platform data.
> > > > > > >
> > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > >
> > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > specified in code itself.
> > > > > > >
> > > > > > > Really there is no other way... :-(
> > > > > >
> > > > > > Sure there is:
> > > > > >
> > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > does.
> > > > >
> > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > new read/write i2c functions which are already implemented by
> > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > >
> > > > Because that would allow you to avoid clashes with i2c creating
> > > > interrupt mapping for client residing on host-notify-capable
> > > > controller.
> > > >
> > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > >
> > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > device.
> > > > >
> > > > > But... what is problem with current implementation? Accelerometer
> > > > > HW provides two functions:
> > > > >
> > > > > 1) 3 axes reports
> > > > > 2) Disk freefall detection
> > > > >
> > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > dell-smo8800. Both functions are independent here.
> > > > >
> > > > > I think you just trying to complicate this situation even more to
> > > > > be more complicated as currently is.
> > > >
> > > > Because this apparently does not work for you, does it?
> > >
> > > It is working fine. I do not see any problem.
> > >
> > > > In general,
> > > > if you want the same hardware be handled by 2 different drivers you
> > > > are going to have bad time.
> > >
> > > Yes, but in this case half of device is ACPI based and other half i2c
> > > based. This is problem of ACPI and Dell design.
> > >
> > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > the same, right?
> > >
> > > Yes. I understand that clean solution is to have one driver which
> > > provides everything.
> > >
> > > But because half of data are ACPI and half i2c, you still needs to
> > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > one .ko module, but still these will be two drivers due to how ACPI and
> > > i2c linux abstractions are different.
> > >
> > > > So, instead of having 2 drivers split the
> > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > your whitelisted boxes and instead instantiate full i2c client
> > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > handle it (providing both accelerometer data and /dev/freefall)?
> > >
> > > With Michał we already discussed about it, see emails. Basically you can
> > > enable/disable kernel modules at compile time or blacklist at runtime
> > > (or even chose what will be compiled into vmlinux and what as external
> > > .ko module).
> >
> > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> >
> > > Some distributions blacklist i2c-i801.ko module... And
> >
> > Any particular reason for that?
> >
> > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > that move on whitelisted machines can potentially cause disappearance of
> > > /dev/freefall and users will not have hdd protection which is currently
> > > working.
> >
> > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > forward them to i2c client) or have faith in your implementation and let
> > lis3lv02d handle it.
> >
> > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > mapping for given client (if Wolfram/Jean OK with it).
> >
> > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > i2c_client with board_info->irq = -1.
> >
> > Pick whichever you prefer.
> >
> > By the way, what do you need accelerometer for on these devices? They
> > don't appear to be tablets that could use one...
>
> Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> not work... I thought that discussion is about different mechanism how
> to implement bus registration notification to smo8800 driver (or
> different solution to not have registration in i801).
>
Just because I am not sure I got everything right, could you confirm
that:
- in the current upstream tree, the dell-smo8800 driver is now broken
after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
alert)
- this series adds an extra lis3lv02d on some machines and you have
problem fighting for the irq (but this is not upstream yet). The extra
lis3lv02d node is added from dell-smo8800
If the first point is not correct (by default, dell-smo8800 will not be
loaded at the same time than lis3lv02d), then it's a design issue with
the interactions between those 2 drivers.
If the first point is correct because ACPI declares both devices, then
there is an urgent fix to propose to not enable Host Notify by default
on Host Notifier capable adapters. (even though the design between the
2 drivers is wrong, it's considered as a regression).
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 9:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Pali Rohár, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104063642.GA20545@dtor-ws>
On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 10:29:34PM +0100, Benjamin Tissoires wrote:
> > On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> >
> > > Yet another option: can we add a new flag to i2c_board_info controlling
> > > whether we want to enable/disable wiring up host notify interrupt?
> >
> > That should be fairly easy to implement. For now, given that only Elan
> > and Synaptics are the one in need for Host Notify, it could be better to
> > request the Host Notify IRQ instead of disabling it unconditionally
> > (which would make the current yet 8 years old lis3lv02d driver happy
> > again).
>
> I like that we have it done in i2c core instead of having drivers
> implementing it individually. Since you are saying that handling host
> notify is property of the slave/driver maybe we should be adding a flag
> to the *i2c_driver* structure to let i2c core that we want to have host
> notify mapped as interrupt if "native" interrupt is not supplied by the
> platform code?
I don't think this is a good idea. It's still a property of the I2C
device, not the driver. It's crappy under Windows, but that doesn't
prevent us to do the right thing.
I think the idea of having it at the i2c_board_info level is the good
one. It's a property of the device node and it wouldn't hurt me to have
a device tree property for that too (not entering the DT field now).
There is no ACPI prop for it too, but I wouldn't be surprised if it
comes in a later revision. The advantage of having it turned on
unconditionally is that we can instantiate it from userspace without
breaking the sysfs ABI.
Note that in the 2 uses I have seen so far of Host Notify, in both cases
I need to instantiate the I2C device from an other driver (psmouse) so I
can control the content of i2c_board_info.
Cheers,
Benjamin
^ 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