* 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 v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Jonathan Cameron @ 2017-01-02 21:13 UTC (permalink / raw)
To: Peter Rosin, Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <cdb3b543-979d-69cc-fb7d-a1d968a75056@axentia.se>
On 2 January 2017 20:47:58 GMT+00:00, Peter Rosin <peda@axentia.se> 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@axentia.se>
>>>> 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?
>
>Cheers,
>peda
>
>>>
>>> Cheers,
>>> peda
>>>
>>>>> +
>>>>> +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@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Peter Rosin @ 2017-01-02 20:47 UTC (permalink / raw)
To: Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <86bfcc29-df24-d877-915f-d62fc3987c05@kernel.org>
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@axentia.se>
>>> 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?
Cheers,
peda
>>
>> Cheers,
>> peda
>>
>>>> +
>>>> +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";
>>>> + };
>>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v6 9/9] misc: mux-adg792a: add mux controller driver for ADG792A/G
From: Jonathan Cameron @ 2017-01-02 18:08 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <df2f2896-8fde-09a9-add5-ded77234a66e@axentia.se>
On 02/01/17 11:00, Peter Rosin wrote:
> On 2017-01-01 12:24, 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@axentia.se>
>> Looks pretty good. Some minor suggestions inline.
>>
>> This convinced me of two things:
>> 1. Need a separate subsystem directory for muxes - having them under misc
>> is going to lead to long term mess.
>> 2. Devm alloc and registration functions will make the drivers all simpler.
>
> Ok, I'm making the move to drivers/mux/* for v7 and adding more devm_*
> functions.
>
>> Also, browsing through ADIs list of muxes and switches it's clear that
>> one classic case will be where an i2c octal or similar switch is used with
>> outputs wired together in weird combinations to act as a mux. Going to
>> be 'fun' describing that.
>>
>> There are also potentially cross point switches to be described ;)
>> (I had to look up what one of those was ;)
>>
>> Crosspoints aren't implausible as front ends for ADCs as you might
>> want to be able rapidly sample any 2 of say 16 channels coming from
>> for example a max14661. We'd have to figure out how to add buffered
>> capture support with sensible restrictions to the iio-mux driver
>> to do that - realistically I think we would just not allow buffered
>> capture with the mux having to switch. Without hardware support
>> (i.e. an ADC with external mux control) it would be too slow.
>>
>> Always good to bury some idle thoughts deep in the review of a random
>> driver ;) I'll never be able to remember where they were let alone
>> anyone else.
>
> But that's switches, and this is muxes. Switches are way more flexible,
> so it's only natural that they are on a completely different level when
> it comes to trying a generic description of them... Intentionally not
> going there :-)
A switch is just a load of muxes (one per output) with the inputs
wired together. All a matter of definition!
>
>>> ---
>>> drivers/misc/Kconfig | 12 ++++
>>> drivers/misc/Makefile | 1 +
>>> drivers/misc/mux-adg792a.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 167 insertions(+)
>>> create mode 100644 drivers/misc/mux-adg792a.c
>>>
>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>> index 2ce675e410c5..45567a444bbf 100644
>>> --- a/drivers/misc/Kconfig
>>> +++ b/drivers/misc/Kconfig
>>> @@ -780,6 +780,18 @@ menuconfig MULTIPLEXER
>>>
>>> if MULTIPLEXER
>>>
>>> +config MUX_ADG792A
>>> + tristate "Analog Devices ADG792A/ADG792G Multiplexers"
>>> + depends on I2C
>>> + help
>>> + ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
>>> +
>>> + The driver supports both operating the three multiplexers in
>>> + parellel and operating them independently.
>> parallel
>>> +
>>> + To compile the driver as a module, choose M here: the module will
>>> + be called mux-adg792a.
>>> +
>>> config MUX_GPIO
>>> tristate "GPIO-controlled Multiplexer"
>>> depends on OF && GPIOLIB
>>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>>> index 0befa2bba762..10ab8d34c9e5 100644
>>> --- a/drivers/misc/Makefile
>>> +++ b/drivers/misc/Makefile
>>> @@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>>> obj-$(CONFIG_CXL_BASE) += cxl/
>>> obj-$(CONFIG_PANEL) += panel.o
>>> obj-$(CONFIG_MULTIPLEXER) += mux-core.o
>>> +obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
>>> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>>>
>>> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o
>>> diff --git a/drivers/misc/mux-adg792a.c b/drivers/misc/mux-adg792a.c
>>> new file mode 100644
>>> index 000000000000..7d309a78af65
>>> --- /dev/null
>>> +++ b/drivers/misc/mux-adg792a.c
>>> @@ -0,0 +1,154 @@
>>> +/*
>>> + * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
>>> + *
>>> + * Copyright (C) 2016 Axentia Technologies AB
>>> + *
>>> + * Author: Peter Rosin <peda@axentia.se>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mux.h>
>>> +
>>> +#define ADG792A_LDSW BIT(0)
>>> +#define ADG792A_RESET BIT(1)
>>> +#define ADG792A_DISABLE(mux) (0x50 | (mux))
>>> +#define ADG792A_DISABLE_ALL (0x5f)
>>> +#define ADG792A_MUX(mux, state) (0xc0 | (((mux) + 1) << 2) | (state))
>>> +#define ADG792A_MUX_ALL(state) (0xc0 | (state))
>>> +
>>> +#define ADG792A_DISABLE_STATE (4)
>>> +#define ADG792A_KEEP_STATE (5)
>>> +
>>> +static int adg792a_set(struct mux_control *mux, int state)
>>> +{
>>> + struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
>>> + u8 cmd;
>>> +
>>> + if (mux->chip->controllers == 1) {
>>> + /* parallel mux controller operation */
>>> + if (state == ADG792A_DISABLE_STATE)
>>> + cmd = ADG792A_DISABLE_ALL;
>>> + else
>>> + cmd = ADG792A_MUX_ALL(state);
>>> + } else {
>>> + unsigned int controller = mux_control_get_index(mux);
>>> +
>>> + if (state == ADG792A_DISABLE_STATE)
>>> + cmd = ADG792A_DISABLE(controller);
>>> + else
>>> + cmd = ADG792A_MUX(controller, state);
>>> + }
>>> +
>>> + return i2c_smbus_write_byte_data(i2c, cmd, ADG792A_LDSW);
>>> +}
>>> +
>>> +static const struct mux_control_ops adg792a_ops = {
>>> + .set = adg792a_set,
>>> +};
>>> +
>>> +static int adg792a_probe(struct i2c_client *i2c,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct device *dev = &i2c->dev;
>>> + struct mux_chip *mux_chip;
>>> + bool parallel;
>>> + int ret;
>>> + int i;
>>> +
>>> + parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
>>> +
>>> + mux_chip = mux_chip_alloc(dev, parallel ? 1 : 3, 0);
>> This makes me wonder if we can have a more generic binding.
>> mux-poles = 3 vs mux-poles = 1?
>
> The adg729 in theory allows to create one double pole mux and one single
> pole mux (three variations, depending on which mux is single pole).
> However, I did not put all that much effort into this driver. It is
> mainly a proof of concept, as mentioned in the cover letter, to "prove"
> that the proposed mux bindings are valid and that it is right to
> have separate mux nodes in devicetree. I'm not even sure it should
> be going upstream as it has seen zero testing. (But hey, it builds, what
> can be wrong?)
>
>>> + if (!mux_chip)
>>> + return -ENOMEM;
>>> +
>>> + mux_chip->ops = &adg792a_ops;
>>> + dev_set_drvdata(dev, mux_chip);
>>> +
>>> + ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
>>> + ADG792A_RESET | ADG792A_LDSW);
>>> + if (ret < 0)
>>> + goto free_mux_chip;
>>> +
>>> + for (i = 0; i < mux_chip->controllers; ++i) {
>>> + struct mux_control *mux = &mux_chip->mux[i];
>>> + u32 idle_state;
>>> +
>>> + mux->states = 4;
>>> +
>>> + ret = of_property_read_u32_index(i2c->dev.of_node,
>>> + "adi,idle-state", i,
>>> + &idle_state);
>>> + if (ret >= 0) {
>>> + if (idle_state > ADG792A_KEEP_STATE) {
>>> + dev_err(dev, "invalid idle-state %u\n",
>>> + idle_state);
>>> + ret = -EINVAL;
>>> + goto free_mux_chip;
>>> + }
>>> + if (idle_state != ADG792A_KEEP_STATE)
>>> + mux->idle_state = idle_state;
>>> + }
>>> + }
>>> +
>>> + ret = mux_chip_register(mux_chip);
>>> + if (ret < 0) {
>>> + dev_err(dev, "failed to register mux-chip\n");
>>> + goto free_mux_chip;
>>> + }
>>> +
>>> + if (parallel)
>>> + dev_info(dev, "1 triple 4-way mux-controller registered\n");
>> I'd use the relay / switch standard description for this so
>> 'triple pole, quadruple throw mux registered'.
>>> + else
>>> + dev_info(dev, "3 4-way mux-controllers registered\n");
>> '3x single pole, quadruple throw muxes registered'.
>
> Ok, fine by me.
>
>>> +
>>> + return 0;
>>> +
>>> +free_mux_chip:
>>> + mux_chip_free(mux_chip);
>>> + return ret;
>>> +}
>>> +
>>> +static int adg792a_remove(struct i2c_client *i2c)
>>> +{
>>> + struct mux_chip *mux_chip = dev_get_drvdata(&i2c->dev);
>>> +
>> Definitely looking like it's worth managed versions of mux_chip_register and
>> mux_chip_alloc given this is another case where they would let us get rid
>> of the remove function entirely.
>>> + mux_chip_unregister(mux_chip);
>>> + mux_chip_free(mux_chip);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id adg792a_id[] = {
>>> + { .name = "adg792a", },
>>> + { .name = "adg792g", },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, adg792a_id);
>>> +
>>> +static const struct of_device_id adg792a_of_match[] = {
>>> + { .compatible = "adi,adg792a", },
>>> + { .compatible = "adi,adg792g", },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, adg792a_of_match);
>>> +
>>> +static struct i2c_driver adg792a_driver = {
>>> + .driver = {
>>> + .name = "adg792a",
>>> + .of_match_table = of_match_ptr(adg792a_of_match),
>>> + },
>>> + .probe = adg792a_probe,
>>> + .remove = adg792a_remove,
>>> + .id_table = adg792a_id,
>>> +};
>>> +module_i2c_driver(adg792a_driver);
>>> +
>>> +MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
>>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Jonathan Cameron @ 2017-01-02 18:05 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <3fa5b6f0-76c1-0ca6-978e-9407a44a2e7e@axentia.se>
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@axentia.se>
>> 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.
>
> Cheers,
> peda
>
>>> +
>>> +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";
>>> + };
>>>
>>
>
^ permalink raw reply
* Urgent Please;;
From: Dr. David White @ 2017-01-02 17:34 UTC (permalink / raw)
Dear,
With due respect to your person and much sincerity of purpose. I have a business proposal which I will like to handle with you and $14.5 Million USD is involve. But be rest assured that everything is legal and risk free as I have concluded all the arrangements and the legal papers that will back the transaction up. Kindly indicate your interest as to enable me tell you more detail of the proposal. Waiting for your urgent response.
Yours Faithfully,
Dr. David White
^ permalink raw reply
* Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Peter Rosin @ 2017-01-02 16:01 UTC (permalink / raw)
To: Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <d4c41153-aebf-be05-a323-1298e8c4ee65@kernel.org>
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@axentia.se>
> 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?
Cheers,
peda
>> +
>> +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";
>> + };
>>
>
^ permalink raw reply
* Re: [PATCH v6 7/9] i2c: i2c-mux-simple: new driver
From: Peter Rosin @ 2017-01-02 15:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Wolfram Sang, Rob Herring, Mark Rutland, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Jonathan Corbet, Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, devicetree,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux Documentation List
In-Reply-To: <CAHp75Vc2uYhaYJu_eU-uBq_PZrUgaoNt8sxpuRF8eEEs2kPUwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 2017-01-01 16:38, Andy Shevchenko wrote:
> On Wed, Nov 30, 2016 at 10:17 AM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>> This is a generic simple i2c mux that uses the generic multiplexer
>> subsystem to do the muxing.
>
>> +static const struct of_device_id i2c_mux_of_match[] = {
>> + { .compatible = "i2c-mux-simple,parent-locked",
>> + .data = (void *)0, },
>> + { .compatible = "i2c-mux-simple,mux-locked",
>> + .data = (void *)1, },
>> + {},
>> +};
>
> Perhaps
>
> #define I2C_MUX_LOCKED_PARENT 0
> #define I2C_MUX_LOCKED 1
I2C_MUX_LOCKED is already "taken" in include/linux/i2c-mux.h but I'll
use SIMPLE_PARENT_LOCKED and SIMPLE_MUX_LOCKED instead.
Cheers,
peda
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Ville Syrjälä @ 2017-01-02 15:08 UTC (permalink / raw)
To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102150225.GA31595@intel.com>
On Mon, Jan 02, 2017 at 05:02:25PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote:
> > On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > > > >>
> > > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > > >>---
> > > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > > >>
> > > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>index 4b12637..cc4fbd7 100644
> > > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > > > >>
> > > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > > > >> {
> > > > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > > > >>+ /* Wake up the media well, as that takes a lot less
> > > > > > > >>+ * power than the Render well.
> > > > > > > >>+ */
> > > > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > > > >> valleyview_set_rps(dev_priv, val);
> > > > > > > >
> > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > > > >benefit, and very misleading.)
> > > > > > >
> > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > > > existing code in vlv_set_rps_idle().
> > > > > >
> > > > > > It's not correct there either...
> > > > >
> > > > > Why not? If the render well is in rc6 already we don't want to waste
> > > > > power by waking it. The only reason we have to wake up *something* is
> > > > > so that the gpll frequency actually gets changed.
> > > >
> > > > If the register write requires the powerwell, the mmio access will take
> > > > the powerwell.
> > >
> > > The register write doesn't require a power well. It's a sideband access.
> > > The punit will simply not change the GPLL frequency if the GPLL is
> > > currently not running (which will/can happen when all power wells are
> > > asleep). That in itself doesn't sound too back (why change the
> > > frequency if the thing isn't even running, right?). But the real problem
> > > is that the punit will not let the voltage on the rail to drop
> > > either until the new frequency gets programmed into the GPLL. Hence if
> > > the GPU goes idle before we've dropped the GPLL frequency to the
> > > minimum value, we will waste power by having a needlessly high voltage.
> > >
> > > Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> > > which should just force the GPLL to turn on without powering on
> > > any power wells. But that caused some spurious WARNs or something
> > > IIRC, so we had to come up with another workaround. And since powering
> > > either well is sufficient we chose to use the cheaper media well.
> >
> > That explains set_idle() (and would be a better comment that the one
> > there). But not set_rps(), since there we don't care if the write is
> > delayed until the GPU is next active.
>
> I don't see any forcewakes in set_rps()
Ah, it was this patch which wants to add that. And indeed that doesn't
make sense.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Ville Syrjälä @ 2017-01-02 15:02 UTC (permalink / raw)
To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102145359.GF16295@nuc-i3427.alporthouse.com>
On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote:
> On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > > >>
> > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > >>---
> > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > >>
> > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>index 4b12637..cc4fbd7 100644
> > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > > >>
> > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > > >> {
> > > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > > >>+ /* Wake up the media well, as that takes a lot less
> > > > > > >>+ * power than the Render well.
> > > > > > >>+ */
> > > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > > >> valleyview_set_rps(dev_priv, val);
> > > > > > >
> > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > > >benefit, and very misleading.)
> > > > > >
> > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > > existing code in vlv_set_rps_idle().
> > > > >
> > > > > It's not correct there either...
> > > >
> > > > Why not? If the render well is in rc6 already we don't want to waste
> > > > power by waking it. The only reason we have to wake up *something* is
> > > > so that the gpll frequency actually gets changed.
> > >
> > > If the register write requires the powerwell, the mmio access will take
> > > the powerwell.
> >
> > The register write doesn't require a power well. It's a sideband access.
> > The punit will simply not change the GPLL frequency if the GPLL is
> > currently not running (which will/can happen when all power wells are
> > asleep). That in itself doesn't sound too back (why change the
> > frequency if the thing isn't even running, right?). But the real problem
> > is that the punit will not let the voltage on the rail to drop
> > either until the new frequency gets programmed into the GPLL. Hence if
> > the GPU goes idle before we've dropped the GPLL frequency to the
> > minimum value, we will waste power by having a needlessly high voltage.
> >
> > Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> > which should just force the GPLL to turn on without powering on
> > any power wells. But that caused some spurious WARNs or something
> > IIRC, so we had to come up with another workaround. And since powering
> > either well is sufficient we chose to use the cheaper media well.
>
> That explains set_idle() (and would be a better comment that the one
> there). But not set_rps(), since there we don't care if the write is
> delayed until the GPU is next active.
I don't see any forcewakes in set_rps()
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Chris Wilson @ 2017-01-02 14:53 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102144005.GZ31595@intel.com>
On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > Hi,
> > > > >
> > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > >>
> > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > >>---
> > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > >>
> > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>index 4b12637..cc4fbd7 100644
> > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > >>
> > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > >> {
> > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > >>+ /* Wake up the media well, as that takes a lot less
> > > > > >>+ * power than the Render well.
> > > > > >>+ */
> > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > >> valleyview_set_rps(dev_priv, val);
> > > > > >
> > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > >benefit, and very misleading.)
> > > > >
> > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > existing code in vlv_set_rps_idle().
> > > >
> > > > It's not correct there either...
> > >
> > > Why not? If the render well is in rc6 already we don't want to waste
> > > power by waking it. The only reason we have to wake up *something* is
> > > so that the gpll frequency actually gets changed.
> >
> > If the register write requires the powerwell, the mmio access will take
> > the powerwell.
>
> The register write doesn't require a power well. It's a sideband access.
> The punit will simply not change the GPLL frequency if the GPLL is
> currently not running (which will/can happen when all power wells are
> asleep). That in itself doesn't sound too back (why change the
> frequency if the thing isn't even running, right?). But the real problem
> is that the punit will not let the voltage on the rail to drop
> either until the new frequency gets programmed into the GPLL. Hence if
> the GPU goes idle before we've dropped the GPLL frequency to the
> minimum value, we will waste power by having a needlessly high voltage.
>
> Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> which should just force the GPLL to turn on without powering on
> any power wells. But that caused some spurious WARNs or something
> IIRC, so we had to come up with another workaround. And since powering
> either well is sufficient we chose to use the cheaper media well.
That explains set_idle() (and would be a better comment that the one
there). But not set_rps(), since there we don't care if the write is
delayed until the GPU is next active.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply
* Re: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Ville Syrjälä @ 2017-01-02 14:40 UTC (permalink / raw)
To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102142158.GE16295@nuc-i3427.alporthouse.com>
On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > Hi,
> > > >
> > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > >>
> > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > >>---
> > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >>
> > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > >>index 4b12637..cc4fbd7 100644
> > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > >>
> > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > >> {
> > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > >>+ /* Wake up the media well, as that takes a lot less
> > > > >>+ * power than the Render well.
> > > > >>+ */
> > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > >> valleyview_set_rps(dev_priv, val);
> > > > >
> > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > >benefit, and very misleading.)
> > > >
> > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > existing code in vlv_set_rps_idle().
> > >
> > > It's not correct there either...
> >
> > Why not? If the render well is in rc6 already we don't want to waste
> > power by waking it. The only reason we have to wake up *something* is
> > so that the gpll frequency actually gets changed.
>
> If the register write requires the powerwell, the mmio access will take
> the powerwell.
The register write doesn't require a power well. It's a sideband access.
The punit will simply not change the GPLL frequency if the GPLL is
currently not running (which will/can happen when all power wells are
asleep). That in itself doesn't sound too back (why change the
frequency if the thing isn't even running, right?). But the real problem
is that the punit will not let the voltage on the rail to drop
either until the new frequency gets programmed into the GPLL. Hence if
the GPU goes idle before we've dropped the GPLL frequency to the
minimum value, we will waste power by having a needlessly high voltage.
Originally we tried to avoid this problem via vlv_force_gfx_clock(),
which should just force the GPLL to turn on without powering on
any power wells. But that caused some spurious WARNs or something
IIRC, so we had to come up with another workaround. And since powering
either well is sufficient we chose to use the cheaper media well.
> Manually taking the wakelock has to be for a reason, the
> one given is bogus. (That is if the register write only requires meda
> access that is taken, but iirc, rps requires both, so both are taken
> inside.) If the claimed benefit is indeed real, and there is indeed
> freedom of choice, we want that inside the mmio accessor to choose the
> cheaper well.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Chris Wilson @ 2017-01-02 14:21 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102141049.GX31595@intel.com>
On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 01-01-17 21:24, Chris Wilson wrote:
> > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > >>
> > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > >>---
> > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >>
> > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > >>index 4b12637..cc4fbd7 100644
> > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > >>
> > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > >> {
> > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > >>+ /* Wake up the media well, as that takes a lot less
> > > >>+ * power than the Render well.
> > > >>+ */
> > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > >> valleyview_set_rps(dev_priv, val);
> > > >
> > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > >benefit, and very misleading.)
> > >
> > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > existing code in vlv_set_rps_idle().
> >
> > It's not correct there either...
>
> Why not? If the render well is in rc6 already we don't want to waste
> power by waking it. The only reason we have to wake up *something* is
> so that the gpll frequency actually gets changed.
If the register write requires the powerwell, the mmio access will take
the powerwell. Manually taking the wakelock has to be for a reason, the
one given is bogus. (That is if the register write only requires meda
access that is taken, but iirc, rps requires both, so both are taken
inside.) If the claimed benefit is indeed real, and there is indeed
freedom of choice, we want that inside the mmio accessor to choose the
cheaper well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-02 14:21 UTC (permalink / raw)
To: Ville Syrjälä
Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c,
Len Brown
In-Reply-To: <20170102141242.GY31595@intel.com>
Hi,
On 02-01-17 15:12, Ville Syrjälä wrote:
> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
>> The punit on baytrail / cherrytrail systems is not only accessed through
>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
>> the punit against simultaneous accesses and 2 functions to lock / unlock
>> this mutex.
>
> I'm not sure which part of punit you're actually trying to protect
> here. Some specific registers?
The theory I'm going by is that for certain actions / certain requests
we send to the punit, the punit needs to access the (axp288) pmic, to
change (or enable / disable) certain voltages.
So it needs to access the pmic i2c bus, but in some cases the kernel
is accessing this itself (e.g. for battery monitoring) and is holding
the punit i2c bus semaphore. At least with CPU-core C-state transitions,
this seems to be happening, if I do read i2c transfers on the pmic
i2c bus repeatedly without blocking the CPU from entering C6 (*) while
accessing the i2c bus my cherrytrail tablet will freeze in 10 - 30
seconds.
The findings of one of the users commenting in:
https://bugzilla.kernel.org/show_bug.cgi?id=155241
Seem to indicate a similar problem with the i915 driver doing
power-management while the i2c-designware-baytrail code is holding
the punit i2c bus semaphore. One would hope that the punit would be
smart enough to simply wait for the semaphore to get released in that
case, but at least for the C6 CPU core transition it seems that allowing
that to happen while holding the semaphore causes a hard crash of the
SoC. So I guess that for explicit requests the punit code assumes that
the OS is not holding the semaphore.
Regards,
Hans
*) which powers off most of the core, so likely causes interaction with
the pmic
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++
>> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
>> index b41ee16..02963bd 100644
>> --- a/arch/x86/include/asm/iosf_mbi.h
>> +++ b/arch/x86/include/asm/iosf_mbi.h
>> @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
>> */
>> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
>>
>> +/**
>> + * iosf_mbi_punit_lock() - Lock the punit mutex
>> + *
>> + * This function must be called before accessing the punit or the pmic, be it
>> + * through iosf_mbi_* or through other means.
>> + *
>> + * This function locks a mutex, as such it may sleep.
>> + */
>> +void iosf_mbi_punit_lock(void);
>> +
>> +/**
>> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
>> + */
>> +void iosf_mbi_punit_unlock(void);
>> +
>> #else /* CONFIG_IOSF_MBI is not enabled */
>> static inline
>> bool iosf_mbi_available(void)
>> @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
>> WARN(1, "IOSF_MBI driver not available");
>> return -EPERM;
>> }
>> +
>> +static inline void iosf_mbi_punit_lock(void) {}
>> +static inline void iosf_mbi_punit_unlock(void) {}
>> +
>> #endif /* CONFIG_IOSF_MBI */
>>
>> #endif /* IOSF_MBI_SYMS_H */
>> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
>> index edf2c54..75d8135 100644
>> --- a/arch/x86/platform/intel/iosf_mbi.c
>> +++ b/arch/x86/platform/intel/iosf_mbi.c
>> @@ -34,6 +34,7 @@
>>
>> static struct pci_dev *mbi_pdev;
>> static DEFINE_SPINLOCK(iosf_mbi_lock);
>> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>>
>> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>> {
>> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
>> }
>> EXPORT_SYMBOL(iosf_mbi_available);
>>
>> +void iosf_mbi_punit_lock(void)
>> +{
>> + mutex_lock(&iosf_mbi_punit_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
>> +
>> +void iosf_mbi_punit_unlock(void)
>> +{
>> + mutex_unlock(&iosf_mbi_punit_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
>> +
>> #ifdef CONFIG_IOSF_MBI_DEBUG
>> static u32 dbg_mdr;
>> static u32 dbg_mcr;
>> --
>> 2.9.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Ville Syrjälä @ 2017-01-02 14:12 UTC (permalink / raw)
To: Hans de Goede
Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c,
Len Brown
In-Reply-To: <20170101201403.12132-2-hdegoede@redhat.com>
On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
> The punit on baytrail / cherrytrail systems is not only accessed through
> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
> the punit against simultaneous accesses and 2 functions to lock / unlock
> this mutex.
I'm not sure which part of punit you're actually trying to protect
here. Some specific registers?
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++
> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> index b41ee16..02963bd 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
> */
> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
>
> +/**
> + * iosf_mbi_punit_lock() - Lock the punit mutex
> + *
> + * This function must be called before accessing the punit or the pmic, be it
> + * through iosf_mbi_* or through other means.
> + *
> + * This function locks a mutex, as such it may sleep.
> + */
> +void iosf_mbi_punit_lock(void);
> +
> +/**
> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
> + */
> +void iosf_mbi_punit_unlock(void);
> +
> #else /* CONFIG_IOSF_MBI is not enabled */
> static inline
> bool iosf_mbi_available(void)
> @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
> WARN(1, "IOSF_MBI driver not available");
> return -EPERM;
> }
> +
> +static inline void iosf_mbi_punit_lock(void) {}
> +static inline void iosf_mbi_punit_unlock(void) {}
> +
> #endif /* CONFIG_IOSF_MBI */
>
> #endif /* IOSF_MBI_SYMS_H */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
> index edf2c54..75d8135 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -34,6 +34,7 @@
>
> static struct pci_dev *mbi_pdev;
> static DEFINE_SPINLOCK(iosf_mbi_lock);
> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>
> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> {
> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
> }
> EXPORT_SYMBOL(iosf_mbi_available);
>
> +void iosf_mbi_punit_lock(void)
> +{
> + mutex_lock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
> +
> +void iosf_mbi_punit_unlock(void)
> +{
> + mutex_unlock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
> +
> #ifdef CONFIG_IOSF_MBI_DEBUG
> static u32 dbg_mdr;
> static u32 dbg_mcr;
> --
> 2.9.3
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Ville Syrjälä @ 2017-01-02 14:10 UTC (permalink / raw)
To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102113759.GC16295@nuc-i3427.alporthouse.com>
On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 01-01-17 21:24, Chris Wilson wrote:
> > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > >>sysfs store functions, there is no guarantee this is already done, so add
> > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > >>
> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >>---
> > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > >>index 4b12637..cc4fbd7 100644
> > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > >>
> > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > >> {
> > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > >>+ /* Wake up the media well, as that takes a lot less
> > >>+ * power than the Render well.
> > >>+ */
> > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > >> valleyview_set_rps(dev_priv, val);
> > >
> > >Both powerwells are woken for rps. (Taking one but not the other has no
> > >benefit, and very misleading.)
> >
> > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > existing code in vlv_set_rps_idle().
>
> It's not correct there either...
Why not? If the render well is in rc6 already we don't want to waste
power by waking it. The only reason we have to wake up *something* is
so that the gpll frequency actually gets changed.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-02 13:31 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King, linux-i2c,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <CAOAejn1ocTSvdPWnZ-+DwMfJj45cXXEDc1+=B5d3zCy2Pw+0-A@mail.gmail.com>
Hello Uwe,
>> Is it possible to make it more obvious by doing:
>>
>> status = read_from_status_register() & read_from_interrupt_enable_register();
>>
>> at a single place?
Contrary to what I said previously I have to keep possible_status
variable as for one irq enabled we allow several events.
For example, ITBUFEN allows to generate an irq for RXNE and for TXE events.
So, using status = read_from_status_register() &
read_from_interrupt_enable_register(); is not possible.
Best regards,
Cedric
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Hans de Goede @ 2017-01-02 12:40 UTC (permalink / raw)
To: Chris Wilson, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170102113759.GC16295@nuc-i3427.alporthouse.com>
Hi,
On 02-01-17 12:37, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01-01-17 21:24, Chris Wilson wrote:
>>> On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
>>>> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
>>>> for intel_set_rps(). Since intel_set_rps can for example be called from
>>>> sysfs store functions, there is no guarantee this is already done, so add
>>>> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 4b12637..cc4fbd7 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>>>>
>>>> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>> {
>>>> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>>>> + /* Wake up the media well, as that takes a lot less
>>>> + * power than the Render well.
>>>> + */
>>>> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>>>> valleyview_set_rps(dev_priv, val);
>>>
>>> Both powerwells are woken for rps. (Taking one but not the other has no
>>> benefit, and very misleading.)
>>
>> The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
>> existing code in vlv_set_rps_idle().
>
> It's not correct there either...
>
>>> The forcewake is already held by the
>>> lower level routines, taking the wakelock in the caller is an optimisation
>>> that is only interesting if there is a danger from the forcewake being
>>> dropped mid-sequence (due to preemption whatever).
>>
>> We're also accessing the punit in valleyview_set_rps() and I've seen several
>> patches (in the android x86 kernel) suggesting that we need to take a wakelock
>> while doing this.
>
> It is quite likely that we do have to guarantee to keep the punit awake
> during long sequences. That would be an acceptable rationale (but not in
> the caller!).
So what would be a good approach, take FORCEWAKE_ALL in valleyview_set_rps()
itself (and drop the existing code from vlv_set_rps_idle()) ?
Regards,
Hans
^ permalink raw reply
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Chris Wilson @ 2017-01-02 11:37 UTC (permalink / raw)
To: Hans de Goede
Cc: Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx,
linux-i2c
In-Reply-To: <556a9c6d-33ba-0047-82df-8bf75e09e208@redhat.com>
On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01-01-17 21:24, Chris Wilson wrote:
> >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> >>for intel_set_rps(). Since intel_set_rps can for example be called from
> >>sysfs store functions, there is no guarantee this is already done, so add
> >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> >> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>index 4b12637..cc4fbd7 100644
> >>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> >>
> >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >> {
> >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> >>+ /* Wake up the media well, as that takes a lot less
> >>+ * power than the Render well.
> >>+ */
> >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> >> valleyview_set_rps(dev_priv, val);
> >
> >Both powerwells are woken for rps. (Taking one but not the other has no
> >benefit, and very misleading.)
>
> The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> existing code in vlv_set_rps_idle().
It's not correct there either...
> > The forcewake is already held by the
> >lower level routines, taking the wakelock in the caller is an optimisation
> >that is only interesting if there is a danger from the forcewake being
> >dropped mid-sequence (due to preemption whatever).
>
> We're also accessing the punit in valleyview_set_rps() and I've seen several
> patches (in the android x86 kernel) suggesting that we need to take a wakelock
> while doing this.
It is quite likely that we do have to guarantee to keep the punit awake
during long sequences. That would be an acceptable rationale (but not in
the caller!).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply
* Re: [PATCH v6 9/9] misc: mux-adg792a: add mux controller driver for ADG792A/G
From: Peter Rosin @ 2017-01-02 11:00 UTC (permalink / raw)
To: Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <fb8a7864-a6cf-351f-b9c5-a5036c30474e@kernel.org>
On 2017-01-01 12:24, 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@axentia.se>
> Looks pretty good. Some minor suggestions inline.
>
> This convinced me of two things:
> 1. Need a separate subsystem directory for muxes - having them under misc
> is going to lead to long term mess.
> 2. Devm alloc and registration functions will make the drivers all simpler.
Ok, I'm making the move to drivers/mux/* for v7 and adding more devm_*
functions.
> Also, browsing through ADIs list of muxes and switches it's clear that
> one classic case will be where an i2c octal or similar switch is used with
> outputs wired together in weird combinations to act as a mux. Going to
> be 'fun' describing that.
>
> There are also potentially cross point switches to be described ;)
> (I had to look up what one of those was ;)
>
> Crosspoints aren't implausible as front ends for ADCs as you might
> want to be able rapidly sample any 2 of say 16 channels coming from
> for example a max14661. We'd have to figure out how to add buffered
> capture support with sensible restrictions to the iio-mux driver
> to do that - realistically I think we would just not allow buffered
> capture with the mux having to switch. Without hardware support
> (i.e. an ADC with external mux control) it would be too slow.
>
> Always good to bury some idle thoughts deep in the review of a random
> driver ;) I'll never be able to remember where they were let alone
> anyone else.
But that's switches, and this is muxes. Switches are way more flexible,
so it's only natural that they are on a completely different level when
it comes to trying a generic description of them... Intentionally not
going there :-)
>> ---
>> drivers/misc/Kconfig | 12 ++++
>> drivers/misc/Makefile | 1 +
>> drivers/misc/mux-adg792a.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 167 insertions(+)
>> create mode 100644 drivers/misc/mux-adg792a.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 2ce675e410c5..45567a444bbf 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -780,6 +780,18 @@ menuconfig MULTIPLEXER
>>
>> if MULTIPLEXER
>>
>> +config MUX_ADG792A
>> + tristate "Analog Devices ADG792A/ADG792G Multiplexers"
>> + depends on I2C
>> + help
>> + ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
>> +
>> + The driver supports both operating the three multiplexers in
>> + parellel and operating them independently.
> parallel
>> +
>> + To compile the driver as a module, choose M here: the module will
>> + be called mux-adg792a.
>> +
>> config MUX_GPIO
>> tristate "GPIO-controlled Multiplexer"
>> depends on OF && GPIOLIB
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 0befa2bba762..10ab8d34c9e5 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>> obj-$(CONFIG_CXL_BASE) += cxl/
>> obj-$(CONFIG_PANEL) += panel.o
>> obj-$(CONFIG_MULTIPLEXER) += mux-core.o
>> +obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
>> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>>
>> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o
>> diff --git a/drivers/misc/mux-adg792a.c b/drivers/misc/mux-adg792a.c
>> new file mode 100644
>> index 000000000000..7d309a78af65
>> --- /dev/null
>> +++ b/drivers/misc/mux-adg792a.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
>> + *
>> + * Copyright (C) 2016 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +
>> +#define ADG792A_LDSW BIT(0)
>> +#define ADG792A_RESET BIT(1)
>> +#define ADG792A_DISABLE(mux) (0x50 | (mux))
>> +#define ADG792A_DISABLE_ALL (0x5f)
>> +#define ADG792A_MUX(mux, state) (0xc0 | (((mux) + 1) << 2) | (state))
>> +#define ADG792A_MUX_ALL(state) (0xc0 | (state))
>> +
>> +#define ADG792A_DISABLE_STATE (4)
>> +#define ADG792A_KEEP_STATE (5)
>> +
>> +static int adg792a_set(struct mux_control *mux, int state)
>> +{
>> + struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
>> + u8 cmd;
>> +
>> + if (mux->chip->controllers == 1) {
>> + /* parallel mux controller operation */
>> + if (state == ADG792A_DISABLE_STATE)
>> + cmd = ADG792A_DISABLE_ALL;
>> + else
>> + cmd = ADG792A_MUX_ALL(state);
>> + } else {
>> + unsigned int controller = mux_control_get_index(mux);
>> +
>> + if (state == ADG792A_DISABLE_STATE)
>> + cmd = ADG792A_DISABLE(controller);
>> + else
>> + cmd = ADG792A_MUX(controller, state);
>> + }
>> +
>> + return i2c_smbus_write_byte_data(i2c, cmd, ADG792A_LDSW);
>> +}
>> +
>> +static const struct mux_control_ops adg792a_ops = {
>> + .set = adg792a_set,
>> +};
>> +
>> +static int adg792a_probe(struct i2c_client *i2c,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &i2c->dev;
>> + struct mux_chip *mux_chip;
>> + bool parallel;
>> + int ret;
>> + int i;
>> +
>> + parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
>> +
>> + mux_chip = mux_chip_alloc(dev, parallel ? 1 : 3, 0);
> This makes me wonder if we can have a more generic binding.
> mux-poles = 3 vs mux-poles = 1?
The adg729 in theory allows to create one double pole mux and one single
pole mux (three variations, depending on which mux is single pole).
However, I did not put all that much effort into this driver. It is
mainly a proof of concept, as mentioned in the cover letter, to "prove"
that the proposed mux bindings are valid and that it is right to
have separate mux nodes in devicetree. I'm not even sure it should
be going upstream as it has seen zero testing. (But hey, it builds, what
can be wrong?)
>> + if (!mux_chip)
>> + return -ENOMEM;
>> +
>> + mux_chip->ops = &adg792a_ops;
>> + dev_set_drvdata(dev, mux_chip);
>> +
>> + ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
>> + ADG792A_RESET | ADG792A_LDSW);
>> + if (ret < 0)
>> + goto free_mux_chip;
>> +
>> + for (i = 0; i < mux_chip->controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> + u32 idle_state;
>> +
>> + mux->states = 4;
>> +
>> + ret = of_property_read_u32_index(i2c->dev.of_node,
>> + "adi,idle-state", i,
>> + &idle_state);
>> + if (ret >= 0) {
>> + if (idle_state > ADG792A_KEEP_STATE) {
>> + dev_err(dev, "invalid idle-state %u\n",
>> + idle_state);
>> + ret = -EINVAL;
>> + goto free_mux_chip;
>> + }
>> + if (idle_state != ADG792A_KEEP_STATE)
>> + mux->idle_state = idle_state;
>> + }
>> + }
>> +
>> + ret = mux_chip_register(mux_chip);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to register mux-chip\n");
>> + goto free_mux_chip;
>> + }
>> +
>> + if (parallel)
>> + dev_info(dev, "1 triple 4-way mux-controller registered\n");
> I'd use the relay / switch standard description for this so
> 'triple pole, quadruple throw mux registered'.
>> + else
>> + dev_info(dev, "3 4-way mux-controllers registered\n");
> '3x single pole, quadruple throw muxes registered'.
Ok, fine by me.
>> +
>> + return 0;
>> +
>> +free_mux_chip:
>> + mux_chip_free(mux_chip);
>> + return ret;
>> +}
>> +
>> +static int adg792a_remove(struct i2c_client *i2c)
>> +{
>> + struct mux_chip *mux_chip = dev_get_drvdata(&i2c->dev);
>> +
> Definitely looking like it's worth managed versions of mux_chip_register and
> mux_chip_alloc given this is another case where they would let us get rid
> of the remove function entirely.
>> + mux_chip_unregister(mux_chip);
>> + mux_chip_free(mux_chip);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id adg792a_id[] = {
>> + { .name = "adg792a", },
>> + { .name = "adg792g", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, adg792a_id);
>> +
>> +static const struct of_device_id adg792a_of_match[] = {
>> + { .compatible = "adi,adg792a", },
>> + { .compatible = "adi,adg792g", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, adg792a_of_match);
>> +
>> +static struct i2c_driver adg792a_driver = {
>> + .driver = {
>> + .name = "adg792a",
>> + .of_match_table = of_match_ptr(adg792a_of_match),
>> + },
>> + .probe = adg792a_probe,
>> + .remove = adg792a_remove,
>> + .id_table = adg792a_id,
>> +};
>> +module_i2c_driver(adg792a_driver);
>> +
>> +MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
>> +MODULE_LICENSE("GPL v2");
>>
>
^ permalink raw reply
* Re: [PATCH v5 5/6] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Andy Shevchenko @ 2017-01-02 10:06 UTC (permalink / raw)
To: Hans de Goede, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <d8525a7d-2363-3065-bc3d-22c7a0553d75@redhat.com>
On Mon, 2017-01-02 at 10:55 +0100, Hans de Goede wrote:
> Hi,
>
> On 02-01-17 10:36, Andy Shevchenko wrote:
> > On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
> > > Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so
> > > that
> > > we keep the iosf_mbi_lock locked during the read-modify-write done
> > > to
> > > reset the semaphore.
> > >
> >
> > While patch itself looks good to me, I think it reduces a
> > probability to
> > race and doesn't eliminate an issue completely.
>
> All accesses through this register are done through the
> iosf_mbi_ helpers, and the modify helper holds the
> iosf_mbi_lock during the entire read-modify-wrtie cycle,
> so I don't see how there can still be any issue left.
Ah, seems you are right.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> > Can you check i915 code
> > how it's done there?
>
> AFAIK the i915 code never takes the pmic i2c bus semaphore.
>
> Regards,
>
> Hans
>
> p.s.
>
> Since you're interested in this set, you may also be interested
> in this RFC patch-set:
>
> https://www.spinics.net/lists/intel-gfx/msg115833.html
>
>
> >
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v5:
> > > -New patch in v5 of this patch-set
> > > ---
> > > drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
> > > 1 file changed, 2 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> > > b/drivers/i2c/busses/i2c-designware-baytrail.c
> > > index 650a700..8df529c 100644
> > > --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> > > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> > > @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> > > *sem)
> > >
> > > static void reset_semaphore(struct dw_i2c_dev *dev)
> > > {
> > > - u32 data;
> > > -
> > > - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > > PUNIT_SEMAPHORE, &data)) {
> > > - dev_err(dev->dev, "iosf failed to reset punit
> > > semaphore during read\n");
> > > - return;
> > > - }
> > > -
> > > - data &= ~PUNIT_SEMAPHORE_BIT;
> > > - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> > > PUNIT_SEMAPHORE, data))
> > > + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > > PUNIT_SEMAPHORE,
> > > + 0, PUNIT_SEMAPHORE_BIT))
> > > dev_err(dev->dev, "iosf failed to reset punit
> > > semaphore during write\n");
> > >
> > > pm_qos_update_request(&dev->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5 5/6] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Hans de Goede @ 2017-01-02 9:55 UTC (permalink / raw)
To: Andy Shevchenko, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <1483349801.9552.187.camel@linux.intel.com>
Hi,
On 02-01-17 10:36, Andy Shevchenko wrote:
> On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
>> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
>> we keep the iosf_mbi_lock locked during the read-modify-write done to
>> reset the semaphore.
>>
>
> While patch itself looks good to me, I think it reduces a probability to
> race and doesn't eliminate an issue completely.
All accesses through this register are done through the
iosf_mbi_ helpers, and the modify helper holds the
iosf_mbi_lock during the entire read-modify-wrtie cycle,
so I don't see how there can still be any issue left.
> Can you check i915 code
> how it's done there?
AFAIK the i915 code never takes the pmic i2c bus semaphore.
Regards,
Hans
p.s.
Since you're interested in this set, you may also be interested
in this RFC patch-set:
https://www.spinics.net/lists/intel-gfx/msg115833.html
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> -New patch in v5 of this patch-set
>> ---
>> drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 650a700..8df529c 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>> *sem)
>>
>> static void reset_semaphore(struct dw_i2c_dev *dev)
>> {
>> - u32 data;
>> -
>> - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> PUNIT_SEMAPHORE, &data)) {
>> - dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during read\n");
>> - return;
>> - }
>> -
>> - data &= ~PUNIT_SEMAPHORE_BIT;
>> - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> PUNIT_SEMAPHORE, data))
>> + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> PUNIT_SEMAPHORE,
>> + 0, PUNIT_SEMAPHORE_BIT))
>> dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during write\n");
>>
>> pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>
^ permalink raw reply
* Re: [PATCH v5 5/6] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Andy Shevchenko @ 2017-01-02 9:36 UTC (permalink / raw)
To: Hans de Goede, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <20170101201521.12364-5-hdegoede@redhat.com>
On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
> we keep the iosf_mbi_lock locked during the read-modify-write done to
> reset the semaphore.
>
While patch itself looks good to me, I think it reduces a probability to
race and doesn't eliminate an issue completely. Can you check i915 code
how it's done there?
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -New patch in v5 of this patch-set
> ---
> drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 650a700..8df529c 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> *sem)
>
> static void reset_semaphore(struct dw_i2c_dev *dev)
> {
> - u32 data;
> -
> - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data)) {
> - dev_err(dev->dev, "iosf failed to reset punit
> semaphore during read\n");
> - return;
> - }
> -
> - data &= ~PUNIT_SEMAPHORE_BIT;
> - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
> + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE,
> + 0, PUNIT_SEMAPHORE_BIT))
> dev_err(dev->dev, "iosf failed to reset punit
> semaphore during write\n");
>
> pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v6 2/9] misc: minimal mux subsystem and gpio-based mux controller
From: Jonathan Cameron @ 2017-01-02 9:20 UTC (permalink / raw)
To: Peter Rosin, 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: <7080f03c-0ee3-13b2-b242-21f80052e479-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 2 January 2017 09:14:34 GMT+00:00, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
>On 2016-12-31 17:19, Jonathan Cameron wrote:
>> On 30/11/16 08:16, Peter Rosin wrote:
>>> Add a new minimalistic subsystem that handles multiplexer
>controllers.
>>> When multiplexers are used in various places in the kernel, and the
>>> same multiplexer controller can be used for several independent
>things,
>>> there should be one place to implement support for said multiplexer
>>> controller.
>>>
>>> A single multiplexer controller can also be used to control several
>>> parallel multiplexers, that are in turn used by different subsystems
>>> in the kernel, leading to a need to coordinate multiplexer accesses.
>>> The multiplexer subsystem handles this coordination.
>>>
>>> This new mux controller subsystem initially comes with a single
>backend
>>> driver that controls gpio based multiplexers. Even though not needed
>by
>>> this initial driver, the mux controller subsystem is prepared to
>handle
>>> chips with multiple (independent) mux controllers.
>>>
>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>> Few trivial bits inline + question of whether misc is the right
>location..
>> It's small, but not totally trivial as subsystems go, so perhaps it
>needs it's
>> own directory.
>
>In [9/9] you come to the conclusion that muxes should have their own
>directory, but do you think it should be
> drivers/mux/*
Probably this one...
>or
> drivers/misc/mux/*
>?
>
>>> ---
>>> Documentation/driver-model/devres.txt | 6 +-
>>> MAINTAINERS | 2 +
>>> drivers/misc/Kconfig | 30 ++++
>>> drivers/misc/Makefile | 2 +
>>> drivers/misc/mux-core.c | 311
>++++++++++++++++++++++++++++++++++
>>> drivers/misc/mux-gpio.c | 138 +++++++++++++++
>>> include/linux/mux.h | 197 +++++++++++++++++++++
>>> 7 files changed, 685 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/misc/mux-core.c
>>> create mode 100644 drivers/misc/mux-gpio.c
>>> create mode 100644 include/linux/mux.h
>>>
>>> diff --git a/Documentation/driver-model/devres.txt
>b/Documentation/driver-model/devres.txt
>>> index ca9d1eb46bc0..d64ede85b61b 100644
>>> --- a/Documentation/driver-model/devres.txt
>>> +++ b/Documentation/driver-model/devres.txt
>>> @@ -330,7 +330,11 @@ MEM
>>> devm_kzalloc()
>>>
>>> MFD
>>> - devm_mfd_add_devices()
>> Technically should be in a separate cleanup patch...
>>> + devm_mfd_add_devices()
>>> +
>>> +MUX
>>> + devm_mux_control_get()
>>> + devm_mux_control_put()
>>>
>>> PER-CPU MEM
>>> devm_alloc_percpu()
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 3d4d0efc2b64..dc7498682752 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -8407,6 +8407,8 @@ MULTIPLEXER SUBSYSTEM
>>> M: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>> S: Maintained
>>> F: Documentation/devicetree/bindings/misc/mux-*
>>> +F: include/linux/mux.h
>>> +F: drivers/misc/mux-*
>>>
>>> MULTISOUND SOUND DRIVER
>>> M: Andrew Veliath <andrewtv-Jdbf3xiKgS8@public.gmane.org>
>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>> index 64971baf11fa..2ce675e410c5 100644
>>> --- a/drivers/misc/Kconfig
>>> +++ b/drivers/misc/Kconfig
>>> @@ -766,6 +766,36 @@ config PANEL_BOOT_MESSAGE
>>> An empty message will only clear the display at driver init
>time. Any other
>>> printf()-formatted message is valid with newline and escape
>codes.
>>>
>>> +menuconfig MULTIPLEXER
>>> + bool "Multiplexer subsystem"
>>> + help
>>> + Multiplexer controller subsystem. Multiplexers are used in a
>>> + variety of settings, and this subsystem abstracts their use
>>> + so that the rest of the kernel sees a common interface. When
>>> + multiple parallel multiplexers are controlled by one single
>>> + multiplexer controller, this subsystem also coordinates the
>>> + multiplexer accesses.
>>> +
>>> + If unsure, say no.
>>> +
>> Fun question of the day. Is misc the place to put this or should it
>> have it's own directory. I'd go for own directory...
>
>I thought it too small for its own dir and that it could always be
>moved later. But I don't really care...
>
>> On the plus side, whilst it's in misc you get to pester Greg and Arnd
>> for review.
>
>I know :-]
>
>>> +if MULTIPLEXER
>>> +
>>> +config MUX_GPIO
>>> + tristate "GPIO-controlled Multiplexer"
>>> + depends on OF && GPIOLIB
>>> + help
>>> + GPIO-controlled Multiplexer controller.
>>> +
>>> + The driver builds a single multiplexer controller using a number
>>> + of gpio pins. For N pins, there will be 2^N possible multiplexer
>>> + states. The GPIO pins can be connected (by the hardware) to
>several
>
>*snip*
>
>>> +
>>> +void mux_chip_free(struct mux_chip *mux_chip)
>>> +{
>>> + if (!mux_chip)
>>> + return;
>> I'd drop a blank line in here. Slightly nicer to read.
>
>Right.
>
>>> + put_device(&mux_chip->dev);
>>> +}
>>> +EXPORT_SYMBOL_GPL(mux_chip_free);
>
>*snip*
>
>>> +
>>> +static int mux_gpio_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct mux_chip *mux_chip;
>>> + struct mux_gpio *mux_gpio;
>>> + int pins;
>>> + u32 idle_state;
>>> + int ret;
>>> +
>>> + if (!np)
>>> + return -ENODEV;
>>> +
>>> + pins = gpiod_count(dev, "mux");
>>> + if (pins < 0)
>>> + return pins;
>>> +
>>> + mux_chip = mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>>> + pins * sizeof(*mux_gpio->val));
>>> + if (!mux_chip)
>>> + return -ENOMEM;
>> Rather feels like mux_chip_alloc is a good candidate for a managed
>> allocator. Might be worth doing the register as well then the remove
>> below would go away. I guess perhaps not that worthwhile as not many
>> mux types are likely to ever exist...
>
>To me it seemed like too much extra support code for just two drivers.
>And it can always be added later if/when more mux drivers show up...
There will be others :) sure do it later.
>
>>> +
>>> + mux_gpio = mux_chip_priv(mux_chip);
>>> + mux_gpio->val = (int *)(mux_gpio + 1);
>
>*snip*
>
>Cheers,
>peda
>
>--
>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
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v6 2/9] misc: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-01-02 9:14 UTC (permalink / raw)
To: Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <8df0d60d-cc95-b0ce-f149-0e123af5046b@kernel.org>
On 2016-12-31 17:19, Jonathan Cameron wrote:
> On 30/11/16 08:16, Peter Rosin wrote:
>> Add a new minimalistic subsystem that handles multiplexer controllers.
>> When multiplexers are used in various places in the kernel, and the
>> same multiplexer controller can be used for several independent things,
>> there should be one place to implement support for said multiplexer
>> controller.
>>
>> A single multiplexer controller can also be used to control several
>> parallel multiplexers, that are in turn used by different subsystems
>> in the kernel, leading to a need to coordinate multiplexer accesses.
>> The multiplexer subsystem handles this coordination.
>>
>> This new mux controller subsystem initially comes with a single backend
>> driver that controls gpio based multiplexers. Even though not needed by
>> this initial driver, the mux controller subsystem is prepared to handle
>> chips with multiple (independent) mux controllers.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> Few trivial bits inline + question of whether misc is the right location..
> It's small, but not totally trivial as subsystems go, so perhaps it needs it's
> own directory.
In [9/9] you come to the conclusion that muxes should have their own
directory, but do you think it should be
drivers/mux/*
or
drivers/misc/mux/*
?
>> ---
>> Documentation/driver-model/devres.txt | 6 +-
>> MAINTAINERS | 2 +
>> drivers/misc/Kconfig | 30 ++++
>> drivers/misc/Makefile | 2 +
>> drivers/misc/mux-core.c | 311 ++++++++++++++++++++++++++++++++++
>> drivers/misc/mux-gpio.c | 138 +++++++++++++++
>> include/linux/mux.h | 197 +++++++++++++++++++++
>> 7 files changed, 685 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/misc/mux-core.c
>> create mode 100644 drivers/misc/mux-gpio.c
>> create mode 100644 include/linux/mux.h
>>
>> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
>> index ca9d1eb46bc0..d64ede85b61b 100644
>> --- a/Documentation/driver-model/devres.txt
>> +++ b/Documentation/driver-model/devres.txt
>> @@ -330,7 +330,11 @@ MEM
>> devm_kzalloc()
>>
>> MFD
>> - devm_mfd_add_devices()
> Technically should be in a separate cleanup patch...
>> + devm_mfd_add_devices()
>> +
>> +MUX
>> + devm_mux_control_get()
>> + devm_mux_control_put()
>>
>> PER-CPU MEM
>> devm_alloc_percpu()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3d4d0efc2b64..dc7498682752 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8407,6 +8407,8 @@ MULTIPLEXER SUBSYSTEM
>> M: Peter Rosin <peda@axentia.se>
>> S: Maintained
>> F: Documentation/devicetree/bindings/misc/mux-*
>> +F: include/linux/mux.h
>> +F: drivers/misc/mux-*
>>
>> MULTISOUND SOUND DRIVER
>> M: Andrew Veliath <andrewtv@usa.net>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 64971baf11fa..2ce675e410c5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -766,6 +766,36 @@ config PANEL_BOOT_MESSAGE
>> An empty message will only clear the display at driver init time. Any other
>> printf()-formatted message is valid with newline and escape codes.
>>
>> +menuconfig MULTIPLEXER
>> + bool "Multiplexer subsystem"
>> + help
>> + Multiplexer controller subsystem. Multiplexers are used in a
>> + variety of settings, and this subsystem abstracts their use
>> + so that the rest of the kernel sees a common interface. When
>> + multiple parallel multiplexers are controlled by one single
>> + multiplexer controller, this subsystem also coordinates the
>> + multiplexer accesses.
>> +
>> + If unsure, say no.
>> +
> Fun question of the day. Is misc the place to put this or should it
> have it's own directory. I'd go for own directory...
I thought it too small for its own dir and that it could always be
moved later. But I don't really care...
> On the plus side, whilst it's in misc you get to pester Greg and Arnd
> for review.
I know :-]
>> +if MULTIPLEXER
>> +
>> +config MUX_GPIO
>> + tristate "GPIO-controlled Multiplexer"
>> + depends on OF && GPIOLIB
>> + help
>> + GPIO-controlled Multiplexer controller.
>> +
>> + The driver builds a single multiplexer controller using a number
>> + of gpio pins. For N pins, there will be 2^N possible multiplexer
>> + states. The GPIO pins can be connected (by the hardware) to several
*snip*
>> +
>> +void mux_chip_free(struct mux_chip *mux_chip)
>> +{
>> + if (!mux_chip)
>> + return;
> I'd drop a blank line in here. Slightly nicer to read.
Right.
>> + put_device(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_free);
*snip*
>> +
>> +static int mux_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct mux_chip *mux_chip;
>> + struct mux_gpio *mux_gpio;
>> + int pins;
>> + u32 idle_state;
>> + int ret;
>> +
>> + if (!np)
>> + return -ENODEV;
>> +
>> + pins = gpiod_count(dev, "mux");
>> + if (pins < 0)
>> + return pins;
>> +
>> + mux_chip = mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> + pins * sizeof(*mux_gpio->val));
>> + if (!mux_chip)
>> + return -ENOMEM;
> Rather feels like mux_chip_alloc is a good candidate for a managed
> allocator. Might be worth doing the register as well then the remove
> below would go away. I guess perhaps not that worthwhile as not many
> mux types are likely to ever exist...
To me it seemed like too much extra support code for just two drivers.
And it can always be added later if/when more mux drivers show up...
>> +
>> + mux_gpio = mux_chip_priv(mux_chip);
>> + mux_gpio->val = (int *)(mux_gpio + 1);
*snip*
Cheers,
peda
^ 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