* [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
Take the punit lock to stop others from accessing the punit while the
pmic i2c bus is in use. This is necessary because accessing the punit
from the kernel may result in the punit trying to access the pmic i2c
bus, which results in a hang when it happens while we own the pmic i2c
bus semaphore.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 3effc9a..cf7a2a0 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+ iosf_mbi_punit_unlock();
}
static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
if (!dev->release_lock)
return 0;
+ iosf_mbi_punit_lock();
+
/*
* Disallow the CPU to enter C6 or C7 state, entering these states
* requires the punit to talk to the pmic and if this happens while
--
2.9.3
^ permalink raw reply related
* [PATCH 2/7] x86/platform/intel/iosf_mbi: Add a pmic bus access notifier
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
Some drivers may need to acquire punit managed resources from interrupt
context, where they cannot call iosf_mbi_punit_lock().
This commit adds a notifier chain which allows a driver to get notified
(in a process context) before other drivers start accessing the pmic bus,
so that the driver can acquire any resources, which it may need during
the window the other driver is accessing the pmic, before hand.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
arch/x86/include/asm/iosf_mbi.h | 54 ++++++++++++++++++++++++++++++++++++++
arch/x86/platform/intel/iosf_mbi.c | 36 +++++++++++++++++++++++++
2 files changed, 90 insertions(+)
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index 91f5d16..b8733bb 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -47,6 +47,10 @@
#define QRK_MBI_UNIT_MM 0x05
#define QRK_MBI_UNIT_SOC 0x31
+/* Action values for the pmic_bus_access_notifier functions */
+#define MBI_PMIC_BUS_ACCESS_BEGIN 1
+#define MBI_PMIC_BUS_ACCESS_END 2
+
#if IS_ENABLED(CONFIG_IOSF_MBI)
bool iosf_mbi_available(void);
@@ -115,6 +119,38 @@ void iosf_mbi_punit_lock(void);
*/
void iosf_mbi_punit_unlock(void);
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Register pmic bus notifier
+ *
+ * This function can be used by drivers which may need to acquire punit
+ * managed resources from interrupt context, where iosf_mbi_punit_lock()
+ * can not be used.
+ *
+ * This function allows a driver to register a notifier to get notified (in a
+ * process context) before other drivers start accessing the pmic bus.
+ *
+ * This allows the driver to acquire any resources, which it may need during
+ * the window the other driver is accessing the pmic, before hand.
+ *
+ * @nb: notifier_block to register
+ */
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Unregister pmic bus notifier
+ *
+ * @nb: notifier_block to unregister
+ */
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_call_pmic_bus_access_notifier_chain - Call pmic bus notifier chain
+ *
+ * @val: action to pass into listener's notifier_call function
+ * @v: data pointer to pass into listener's notifier_call function
+ */
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
+
#else /* CONFIG_IOSF_MBI is not enabled */
static inline
bool iosf_mbi_available(void)
@@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
static inline void iosf_mbi_punit_lock(void) {}
static inline void iosf_mbi_punit_unlock(void) {}
+static inline
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+ return 0;
+}
+
+static inline
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+ return 0;
+}
+
+static inline
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+ return 0;
+}
+
#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 75d8135..a995789 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -35,6 +35,7 @@
static struct pci_dev *mbi_pdev;
static DEFINE_SPINLOCK(iosf_mbi_lock);
static DEFINE_MUTEX(iosf_mbi_punit_mutex);
+static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
{
@@ -203,6 +204,41 @@ void iosf_mbi_punit_unlock(void)
}
EXPORT_SYMBOL(iosf_mbi_punit_unlock);
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+ int ret;
+
+ /* Wait for the bus to go inactive before registering */
+ mutex_lock(&iosf_mbi_punit_mutex);
+ ret = blocking_notifier_chain_register(
+ &iosf_mbi_pmic_bus_access_notifier, nb);
+ mutex_unlock(&iosf_mbi_punit_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
+
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+ int ret;
+
+ /* Wait for the bus to go inactive before unregistering */
+ mutex_lock(&iosf_mbi_punit_mutex);
+ ret = blocking_notifier_chain_unregister(
+ &iosf_mbi_pmic_bus_access_notifier, nb);
+ mutex_unlock(&iosf_mbi_punit_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
+
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+ return blocking_notifier_call_chain(
+ &iosf_mbi_pmic_bus_access_notifier, val, v);
+}
+EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
+
#ifdef CONFIG_IOSF_MBI_DEBUG
static u32 dbg_mdr;
static u32 dbg_mcr;
--
2.9.3
^ permalink raw reply related
* [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
One some systems the punit accesses the pmic to change various voltages
through the same bus as other kernel drivers use for e.g. battery
monitoring.
If a driver sends requests to the punit which require the punit to access
the pmic bus while another driver is also accessing the pmic bus various
bad things happen.
This commit adds a mutex to protect the punit against simultaneous accesses
and 2 functions to lock / unlock this mutex.
Note on these systems the i2c-bus driver will request a sempahore from the
punit for exclusive access to the pmic bus when i2c drivers are accessing
it, but this does not appear to be sufficient, we still need to avoid
making certain punit requests during the access window to avoid problems.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
arch/x86/include/asm/iosf_mbi.h | 31 +++++++++++++++++++++++++++++++
arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
2 files changed, 44 insertions(+)
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index b41ee16..91f5d16 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -88,6 +88,33 @@ 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
+ *
+ * One some systems the punit accesses the pmic to change various voltages
+ * through the same bus as other kernel drivers use for e.g. battery monitoring.
+ *
+ * If a driver sends requests to the punit which require the punit to access the
+ * pmic bus while another driver is also accessing the pmic bus various bad
+ * things happen.
+ *
+ * To avoid these problems this function must be called before accessing the
+ * punit or the pmic, be it through iosf_mbi* functions or through other means.
+ *
+ * Note on these systems the i2c-bus driver will request a sempahore from the
+ * punit for exclusive access to the pmic bus when i2c drivers are accessing it,
+ * but this does not appear to be sufficient, we still need to avoid making
+ * certain punit requests during the access window to avoid problems.
+ *
+ * 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 +142,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
^ permalink raw reply related
* [PATCH 0/7] coordinate cht i2c-pmic and i915-punit accesses
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
Hi All,
Here is a non RFC version of my previous RFC series for coordinating
cht i2c-pmic and i915-punit accesses. New in this non RFC version is
that forcewake accesses are coodinated now too.
The problem:
============
This series fixes an issue where the following messages are shown in dmesg:
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership
Usually followed by a gfx or system freeze, this is happening both on my
cherrytrail tablet as well as being seen by an user commenting on:
https://bugzilla.kernel.org/show_bug.cgi?id=155241
This problem is triggered by my patch series to fix the punit i2c bus
semaphore code in drivers/i2c/busses/designware-baytrail.c :
https://patchwork.ozlabs.org/patch/710067/
https://patchwork.ozlabs.org/patch/710068/
https://patchwork.ozlabs.org/patch/710069/
https://patchwork.ozlabs.org/patch/710070/
https://patchwork.ozlabs.org/patch/710071/
https://patchwork.ozlabs.org/patch/710072/
Which actually allows i2c access to pmic-s wich need bus-access arbritration
for the first time on cherrytrail, combined with patches from me to actually
make the axp288_fuel_gauge driver load, which leads to regular pmic i2c
accesses (when userspace checks the battery level).
The fix / workaround
====================
Testing has shown that just taking the punit i2c bus semaphore on
i2c accesses to a shared pmic i2c bus is not enough to avoid problem,
e.g. besides this series to coordinate i915 access, we also need:
https://patchwork.ozlabs.org/patch/710070/
This series introduces 2 mechanisms to coordinate direct pmic accesses
vs punit requests which lead to pmic accesses. The first mechanism is
a simple mutex, which works well for the direct punit accesses the i915
code does. However testing has shown this is not enough, we also need
to avoid doing a forcewake when a driver is directly accessing the pmic
i2c bus, otherwise we get the timeout errors quoted above, the fact
that after the forcewake errors the designware-baytrail.c code also
fails to acquire the lock, suggests that doing forcewakes during the
access window causes the i2c bus to get stuck (or the punit to get
confused).
The deal with the forcewake issue a notifier is introduced which allows
drivers to get notified before any pmic i2c transfer begins (and after
it ends). This is used in the i915 driver to do a
forcewake_get(FORCEWAKE_ALL) before the access begins avoiding needing to
it while the access is in progress.
Testing has shown that this avoids the problem for both me and the user,
where as before it could be reproduced at will.
The implementation of the fix
=============================
I've decided to put the mutex and the notifier in the iosf_mbi code,
as that is somewhat of a common middle ground between the i915 driver and
the designware-baytrail.c code, with the latter already depending on the
iosf_mbi code. I'm open for moving them if someone has a suggestion for a
better place to put them.
If iosf_mbi support is not enabled then all the functions used will become
static inline NOPs and nothing changes on the i915 side.
If the code is enabled and runs on a system which does not need pmic i2c
bus arbritration, then the mutex lock / unlock calls added to the i915 code
will still be made, but there won't be any contention, so other then the
overhead of the lock / unlock there will not be any delays. These calls are
not made in any hot paths.
The biggest downside IMHO is that taking FORCEWAKE_ALL before each bus
access will cause extra GPU wakeups, and thus powerdrain. I plan to make
the impact of this minimal by modifying any drivers (axp288 code, with
which I'm quite familiar by now) to limit their i2c accesses to a bare
minimum. Specifically I plan to e.g. cache the battery level and wait
at least 15 seconds (max 4 times a minute) before getting a fresh reading
even if userspace asks for it more often, combined with some mechanism
to only do 1 access-begin and 1 access-end notification for i2c accesses
in quick succession.
Alternatives
============
It would be tempting to say that this is not worth the trouble and we
should just disallow direct i2c accesses to the pmic bus. Unfortunately
that will cause some major issues on affected devices:
-No battery monitoring
-No "AC" plugged in monitoring
-If booted with a normal USB-A -> micro-USB cable, or no cable, plugged
in and then the user replaces the cable with an otg USB-host cable /
adapter, the id-pin shorting will enable a 5v boost convertor, but we
need to disable the pmic's USB-Vbus path otherwise it will start drawing
current from the boost convertor, leading to aprox 300mA of extra
battery drain, this is done by the axp288_charger driver, which needs
direct i2c access to the pmic bus
Way forward
===========
I realize the workaround this patch-set adds is not the prettiest code
ever seen, but I see no otherway to get things like battery monitoring
and proper OTG support working on affected devices without breaking
drm/i915 support. So I would like to see this merged in some form, I'm
happy to make any kind of requested changes; and/or to try alternative
fixes.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v7 00/12] mux controller abstraction and iio/i2c muxes
From: Wolfram Sang @ 2017-01-08 10:51 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Rob Herring, Mark Rutland, Jonathan Cameron,
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: <1483532187-28494-1-git-send-email-peda@axentia.se>
[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]
Hi peda,
> One thing that I would like to do, but don't see a solution
> for, is to move the mux control code that is present in
> various drivers in drivers/i2c/muxes to this new minimalistic
> muxing subsystem, thus converting all present i2c muxes (but
> perhaps not gates and arbitrators) to be i2c-mux-simple muxes.
In a few lines, what is preventing that?
> I'm using an rwsem to lock a mux, but that isn't really a
> perfect fit. Is there a better locking primitive that I don't
> know about that fits better? I had a mutex at one point, but
> that didn't allow any concurrent accesses at all. At least
> the rwsem allows concurrent access as long as all users
> agree on the mux state, but I suspect that the rwsem will
> degrade to the mutex situation pretty quickly if there is
> any contention.
Maybe ask this question in a seperate email thread on lkml cc-ing the
locking gurus (with a pointer to this thread)?
> Also, the "mux" name feels a bit ambitious, there are many muxes
> in the world, and this tiny bit of code is probably not good
> enough to be a nice fit for all...
"... and it probably never will support anything other than
AT-harddisks, as that's all I have..." ;))
Thanks for this work!
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 10/12] i2c: i2c-mux-simple: new driver
From: Wolfram Sang @ 2017-01-08 10:44 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Rob Herring, Mark Rutland, Jonathan Cameron,
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: <1483532187-28494-11-git-send-email-peda@axentia.se>
[-- Attachment #1: Type: text/plain, Size: 439 bytes --]
On Wed, Jan 04, 2017 at 01:16:25PM +0100, Peter Rosin wrote:
> This is a generic simple i2c mux that uses the generic multiplexer
> subsystem to do the muxing.
>
> The user can select if the mux is to be mux-locked and parent-locked
> as described in Documentation/i2c/i2c-topology.
>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Jonathan Cameron @ 2017-01-08 10:34 UTC (permalink / raw)
To: Hans de Goede, Jacob Pan
Cc: Chen-Yu Tsai, Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio, linux-i2c
In-Reply-To: <9c0346ee-753f-97ef-5f43-f7b561baf2e9@redhat.com>
On 08/01/17 10:15, Hans de Goede wrote:
> Hi,
>
> On 07-01-17 23:23, Jonathan Cameron wrote:
>> On 04/01/17 17:55, Jacob Pan wrote:
>>> On Tue, 3 Jan 2017 23:10:57 +0100
>>> Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>>> It could have been a quirk we had to do on our platforms, I just
>>>>> cannot recall the details. Are you testing this on x86 platforms?
>>>>
>>>> Yes, I've successfully tested this on 2 different models cherrytrail
>>>> tablets.
>>>
>>> [Jacob Pan] I am ok with your change. I last tested on baytrail
>>> tablets, I don't know if you have one available to verify. That would
>>> be ideal.
>>>
>> Stable material? As I read this it is a fairly major fix, but we aren't
>> entirely sure there wasn't a reason on some platforms for this 'interesting'
>> corner of code?
>>
>> So basically are we sure this won't cause regressions? If so I'll take
>> it as a fix and mark for stable.
>
> The main consumer of the axp288_adc code is the axp288_fuel_gauge driver,
> which until now was not really functional. It depended on platform data
> being attached to its mfd device, and the provider of that platform data
> never got merged.
>
> I've got patches queued up for 4.11 fixing this. So in practice the chance
> of this patch causing regressions is close to 0 as so far it had no
> consumers, likewise adding a Cc: stable is not really useful.
Cool. I'll queue it for 4.11 as well then.
Applied to the togreg branch of iio.git which will be pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
>
> Regards,
>
> Hans
> --
> 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
^ permalink raw reply
* Re: [PATCH v7 12/12] mux: adg792a: add mux controller driver for ADG792A/G
From: Jonathan Cameron @ 2017-01-08 10:32 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: <1483532187-28494-13-git-send-email-peda@axentia.se>
On 04/01/17 12:16, Peter Rosin wrote:
> Analog Devices ADG792A/G is a triple 4:1 mux.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
A nice little driver. Hopefully this example will do the job of
convincing people a subsystem makes sense!
Jonathan
> ---
> drivers/mux/Kconfig | 12 ++++
> drivers/mux/Makefile | 1 +
> drivers/mux/mux-adg792a.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 182 insertions(+)
> create mode 100644 drivers/mux/mux-adg792a.c
>
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index de5a35ffe7af..39b06e19aa96 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -16,6 +16,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
> + parallel and operating them independently.
> +
> + 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/mux/Makefile b/drivers/mux/Makefile
> index facc43da3648..fddbb073fc77 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -3,4 +3,5 @@
> #
>
> obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> diff --git a/drivers/mux/mux-adg792a.c b/drivers/mux/mux-adg792a.c
> new file mode 100644
> index 000000000000..c6d5c4571e7c
> --- /dev/null
> +++ b/drivers/mux/mux-adg792a.c
> @@ -0,0 +1,169 @@
> +/*
> + * 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)
> +
> +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 count;
> + int ret;
> + int i;
> +
> + parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
> +
> + mux_chip = devm_mux_chip_alloc(dev, parallel ? 1 : 3, 0);
> + if (!mux_chip)
> + return -ENOMEM;
> +
> + mux_chip->ops = &adg792a_ops;
> +
> + ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
> + ADG792A_RESET | ADG792A_LDSW);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + mux->states = 4;
> + }
> +
> + count = of_property_count_u32_elems(dev->of_node, "adi,idle-state");
> + for (i = 0; i < count; i += 2) {
> + u32 index;
> + u32 idle_state;
> +
> + ret = of_property_read_u32_index(dev->of_node,
> + "adi,idle-state", i,
> + &index);
> + if (ret < 0)
> + return ret;
> + if (index >= mux_chip->controllers) {
> + dev_err(dev, "invalid mux %u\n", index);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_index(dev->of_node,
> + "adi,idle-state", i + 1,
> + &idle_state);
> + if (ret < 0)
> + return ret;
> + if (idle_state >= ADG792A_DISABLE_STATE) {
> + dev_err(dev, "invalid idle-state %u for mux %u\n",
> + idle_state, index);
> + return -EINVAL;
> + }
> + mux_chip->mux[index].idle_state = idle_state;
> + }
> +
> + count = of_property_count_u32_elems(dev->of_node,
> + "adi,idle-high-impedance");
> + for (i = 0; i < count; ++i) {
> + u32 index;
> +
> + ret = of_property_read_u32_index(dev->of_node,
> + "adi,idle-high-impedance", i,
> + &index);
> + if (ret < 0)
> + return ret;
> + if (index >= mux_chip->controllers) {
> + dev_err(dev, "invalid mux %u\n", index);
> + return -EINVAL;
> + }
> +
> + mux_chip->mux[index].idle_state = ADG792A_DISABLE_STATE;
> + }
> +
> + ret = devm_mux_chip_register(dev, mux_chip);
> + if (ret < 0) {
> + dev_err(dev, "failed to register mux-chip\n");
> + return ret;
> + }
> +
> + if (parallel)
> + dev_info(dev, "triple pole quadruple throw mux registered\n");
> + else
> + dev_info(dev, "3x single pole quadruple throw muxes registered\n");
> +
> + 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,
> + .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 v7 11/12] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Jonathan Cameron @ 2017-01-08 10:31 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: <1483532187-28494-12-git-send-email-peda@axentia.se>
On 04/01/17 12:16, Peter Rosin wrote:
> Analog Devices ADG792A/G is a triple 4:1 mux.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
I think this is about as neat as we can make it.
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> .../devicetree/bindings/mux/mux-adg792a.txt | 79 ++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mux/mux-adg792a.txt
>
> diff --git a/Documentation/devicetree/bindings/mux/mux-adg792a.txt b/Documentation/devicetree/bindings/mux/mux-adg792a.txt
> new file mode 100644
> index 000000000000..0b26dd11f070
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mux-adg792a.txt
> @@ -0,0 +1,79 @@
> +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 for ADG792G:
> +- gpio-controller : if present, #gpio-cells below is required.
> +- #gpio-cells : should be <2>
> + - First cell is the GPO line number, i.e. 0 or 1
> + - Second cell is used to specify active high (0)
> + or active low (1)
> +
> +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 2-tuples with mux controller number
> + and state that mux controllers will have when idle. States 0 through 3
> + correspond to signals A through D in the datasheet.
> +- adi,idle-high-impedance : if present, array of mux controller numbers that
> + should be in the disconnected high-impedance state when idle.
> +
> +Mux controller states 0 through 3 correspond to signals A through D in the
> +datasheet. If a mux controller is mentioned in neither adi,idle-state nor
> +adi,idle-high-impedance it is left in its previously selected state when idle.
> +
> +Example:
> +
> + /*
> + * Three independent mux controllers (of which one is used).
> + * Mux 0 is disconnected when idle, mux 1 idles with signal C
> + * and mux 2 idles with signal A.
> + */
> + &i2c0 {
> + mux: adg792a@50 {
> + compatible = "adi,adg792a";
> + reg = <0x50>;
> + #mux-control-cells = <1>;
> +
> + adi,idle-high-impedance = <0>;
> + adi,idle-state = <1 2>, <2 0>;
> + };
> + };
> +
> + adc-mux {
> + compatible = "io-channel-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 = "io-channel-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&pmux>;
> +
> + channels = "sync-1", "", "out";
> + };
>
^ permalink raw reply
* Re: [PATCH v7 07/12] dt-bindings: iio: iio-mux: document iio-mux bindings
From: Jonathan Cameron @ 2017-01-08 10:29 UTC (permalink / raw)
To: Peter Rosin, 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: <1483532187-28494-8-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 04/01/17 12:16, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> .../bindings/iio/multiplexer/io-channel-mux.txt | 39 ++++++++++++++++++++++
> MAINTAINERS | 6 ++++
> 2 files changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
> new file mode 100644
> index 000000000000..c82794002595
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
> @@ -0,0 +1,39 @@
> +I/O channel multiplexer bindings
> +
> +If a multiplexer is used to select which hardware signal is fed to
> +e.g. an ADC channel, these bindings describe that situation.
> +
> +Required properties:
> +- compatible : "io-channel-mux"
> +- io-channels : Channel node of the parent channel that has multiplexed
> + input.
> +- io-channel-names : Should be "parent".
> +- #address-cells = <1>;
> +- #size-cells = <0>;
> +- mux-controls : Mux controller node to use for operating the mux
> +- channels : List of strings, labeling the mux controller states.
> +
> +For each non-empty string in the channels property, an io-channel will
> +be created. The number of this io-channel is the same as the index into
> +the list of strings in the channels property, and also matches the mux
> +controller state. The mux controller state is described in
> +../mux/mux-controller.txt
> +
> +Example:
> + mux: mux-controller {
> + compatible = "mux-gpio";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + adc-mux {
> + compatible = "io-channel-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&mux>;
> +
> + channels = "sync", "in", "system-regulator";
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ebe96f3e25a0..91775f2be209 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6274,6 +6274,12 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
> F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> F: drivers/iio/adc/envelope-detector.c
>
> +IIO MULTIPLEXER
> +M: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +L: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
> +
> IIO SUBSYSTEM AND DRIVERS
> M: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> R: Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
>
--
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: [PATCH v7 05/12] mux: support simplified bindings for single-user gpio mux
From: Jonathan Cameron @ 2017-01-08 10:28 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: <d89179f5-6ad6-3c40-cdb8-f49cf324e9ee@axentia.se>
On 05/01/17 16:21, Peter Rosin wrote:
> On 2017-01-04 13:16, Peter Rosin wrote:
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>> drivers/mux/mux-core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
>> drivers/mux/mux-gpio.c | 56 ++--------------------------------
>> include/linux/mux.h | 7 +++++
>> 3 files changed, 89 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>> index 21da15a264ad..d887ae1c0e55 100644
>> --- a/drivers/mux/mux-core.c
>> +++ b/drivers/mux/mux-core.c
>> @@ -15,6 +15,7 @@
>> #include <linux/device.h>
>> #include <linux/err.h>
>> #include <linux/idr.h>
>> +#include <linux/gpio/consumer.h>
>> #include <linux/module.h>
>> #include <linux/mux.h>
>> #include <linux/of.h>
>> @@ -288,6 +289,63 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>> return dev ? to_mux_chip(dev) : NULL;
>> }
>>
>> +#ifdef CONFIG_MUX_GPIO
>> +
>> +static int mux_gpio_set(struct mux_control *mux, int state)
>> +{
>> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>> + int i;
>> +
>> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>> + mux_gpio->val[i] = (state >> i) & 1;
>> +
>> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>> + mux_gpio->gpios->desc,
>> + mux_gpio->val);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mux_control_ops mux_gpio_ops = {
>> + .set = mux_gpio_set,
>> +};
>> +
>> +struct mux_chip *mux_gpio_alloc(struct device *dev)
>> +{
>> + struct mux_chip *mux_chip;
>> + struct mux_gpio *mux_gpio;
>> + int pins;
>> + int ret;
>> +
>> + pins = gpiod_count(dev, "mux");
>> + if (pins < 0)
>> + return ERR_PTR(pins);
>> +
>> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> + pins * sizeof(*mux_gpio->val));
>> + if (!mux_chip)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mux_gpio = mux_chip_priv(mux_chip);
>> + mux_gpio->val = (int *)(mux_gpio + 1);
>> + mux_chip->ops = &mux_gpio_ops;
>> +
>> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>> + if (IS_ERR(mux_gpio->gpios)) {
>> + ret = PTR_ERR(mux_gpio->gpios);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get gpios\n");
>> + return ERR_PTR(ret);
>> + }
>> + WARN_ON(pins != mux_gpio->gpios->ndescs);
>> + mux_chip->mux->states = 1 << pins;
>> +
>> + return mux_chip;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_gpio_alloc);
>> +
>> +#endif /* CONFIG_MUX_GPIO */
>> +
>> struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>> {
>> struct device_node *np = dev->of_node;
>> @@ -307,9 +365,28 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>> ret = of_parse_phandle_with_args(np,
>> "mux-controls", "#mux-control-cells",
>> index, &args);
>> +
>> +#ifdef CONFIG_MUX_GPIO
>> + if (ret == -ENOENT && !mux_name && gpiod_count(dev, "mux") > 0) {
>> + mux_chip = mux_gpio_alloc(dev);
>> + if (!IS_ERR(mux_chip)) {
>> + ret = devm_mux_chip_register(dev, mux_chip);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to register mux-chip\n");
>> + return ERR_PTR(ret);
>> + }
>> + get_device(&mux_chip->dev);
>> + return mux_chip->mux;
>> + }
>> +
>> + ret = PTR_ERR(mux_chip);
>> + }
>> +#endif
>> +
>> if (ret) {
>> - dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>> - np->full_name, mux_name ?: "", index);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>> + np->full_name, mux_name ?: "", index);
>> return ERR_PTR(ret);
>> }
>>
>> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
>> index 76b52bc63470..8a7bfbc0c4bb 100644
>> --- a/drivers/mux/mux-gpio.c
>> +++ b/drivers/mux/mux-gpio.c
>> @@ -11,37 +11,12 @@
>> */
>>
>> #include <linux/err.h>
>> -#include <linux/gpio/consumer.h>
>> #include <linux/module.h>
>> #include <linux/mux.h>
>> #include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> #include <linux/property.h>
>
> Instead of moving the mux-gpio guts from mux-gpio.c to mux-core.c, I
> will instead make CONFIG_MUX_GPIO a bool option (no module possible)
> and call it from the mux-core. That will be cleaner and less of a
> break of abstractions in my opinion.
Hmm. I wonder if the balance is right here or whether we should just not have the
simplified binding at all as it breaks the assumption that all muxes are of the
same level...
I like the binding, but it is causing significant complexity in here.
>
> Cheers,
> Peter
>
>> -struct mux_gpio {
>> - struct gpio_descs *gpios;
>> - int *val;
>> -};
>> -
>> -static int mux_gpio_set(struct mux_control *mux, int state)
>> -{
>> - struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>> - int i;
>> -
>> - for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>> - mux_gpio->val[i] = (state >> i) & 1;
>> -
>> - gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>> - mux_gpio->gpios->desc,
>> - mux_gpio->val);
>> -
>> - return 0;
>> -}
>> -
>> -static const struct mux_control_ops mux_gpio_ops = {
>> - .set = mux_gpio_set,
>> -};
>> -
>> static const struct of_device_id mux_gpio_dt_ids[] = {
>> { .compatible = "mux-gpio", },
>> { /* sentinel */ }
>> @@ -51,38 +26,13 @@ MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
>> static int mux_gpio_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> - struct device_node *np = 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 = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> - pins * sizeof(*mux_gpio->val));
>> - if (!mux_chip)
>> - return -ENOMEM;
>> -
>> - mux_gpio = mux_chip_priv(mux_chip);
>> - mux_gpio->val = (int *)(mux_gpio + 1);
>> - mux_chip->ops = &mux_gpio_ops;
>> -
>> - mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>> - if (IS_ERR(mux_gpio->gpios)) {
>> - ret = PTR_ERR(mux_gpio->gpios);
>> - if (ret != -EPROBE_DEFER)
>> - dev_err(dev, "failed to get gpios\n");
>> - return ret;
>> - }
>> - WARN_ON(pins != mux_gpio->gpios->ndescs);
>> - mux_chip->mux->states = 1 << pins;
>> + mux_chip = mux_gpio_alloc(dev);
>> + if (IS_ERR(mux_chip))
>> + return PTR_ERR(mux_chip);
>>
>> ret = device_property_read_u32(dev, "idle-state", &idle_state);
>> if (ret >= 0) {
>> diff --git a/include/linux/mux.h b/include/linux/mux.h
>> index 3b9439927f11..3bfee23cfb8c 100644
>> --- a/include/linux/mux.h
>> +++ b/include/linux/mux.h
>> @@ -241,4 +241,11 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>> */
>> void devm_mux_control_put(struct device *dev, struct mux_control *mux);
>>
>> +struct mux_gpio {
>> + struct gpio_descs *gpios;
>> + int *val;
>> +};
>> +
>> +struct mux_chip *mux_gpio_alloc(struct device *dev);
>> +
>> #endif /* _LINUX_MUX_H */
>>
>
^ permalink raw reply
* Re: [PATCH v7 04/12] dt-bindings: simplified bindings for single-user gpio mux
From: Jonathan Cameron @ 2017-01-08 10:25 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: <1483532187-28494-5-git-send-email-peda@axentia.se>
On 04/01/17 12:16, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda@axentia.se>
Looks neat and tidy to me.
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> .../devicetree/bindings/mux/mux-controller.txt | 26 ++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.txt b/Documentation/devicetree/bindings/mux/mux-controller.txt
> index 42b2177e5ae1..4e89df8b2392 100644
> --- a/Documentation/devicetree/bindings/mux/mux-controller.txt
> +++ b/Documentation/devicetree/bindings/mux/mux-controller.txt
> @@ -125,3 +125,29 @@ An example mux controller might look like this:
> reg = <0x50>;
> #mux-control-cells = <1>;
> };
> +
> +
> +Combinded controller and consumer of a GPIO mux
> +-----------------------------------------------
> +
> +For the common case of a single consumer of a GPIO controlled mux, there is
> +a simplified binding which will instantiate an implicit mux controller. Just
> +specify a mux-gpios property with the same interpretation as in mux-gpio.txt.
> +Note that other properties described in mux-gpio.txt are not available in
> +this simplified form and that the mux controller is unnamed. If you need
> +more than one mux controller, a shared mux controller or if you need a
> +specific idle-state, use the more flexible binding with the mux controller
> +in its own node.
> +
> +Example:
> +
> + adc-mux {
> + compatible = "io-channel-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> +
> + channels = "sync-1", "in", "out", "sync-2";
> + };
>
^ permalink raw reply
* Re: [PATCH v7 03/12] mux: minimal mux subsystem and gpio-based mux controller
From: Jonathan Cameron @ 2017-01-08 10:23 UTC (permalink / raw)
To: Peter Rosin, 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: <1483532187-28494-4-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 04/01/17 12: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>
Reviewed-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> Documentation/driver-model/devres.txt | 8 +
> MAINTAINERS | 2 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/mux/Kconfig | 33 +++
> drivers/mux/Makefile | 6 +
> drivers/mux/mux-core.c | 398 ++++++++++++++++++++++++++++++++++
> drivers/mux/mux-gpio.c | 120 ++++++++++
> include/linux/mux.h | 244 +++++++++++++++++++++
> 9 files changed, 814 insertions(+)
> create mode 100644 drivers/mux/Kconfig
> create mode 100644 drivers/mux/Makefile
> create mode 100644 drivers/mux/mux-core.c
> create mode 100644 drivers/mux/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 dc51fb024190..1e9ae701a587 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -332,6 +332,14 @@ MEM
> MFD
> devm_mfd_add_devices()
>
> +MUX
> + devm_mux_chip_alloc()
> + devm_mux_chip_free()
> + devm_mux_chip_register()
> + devm_mux_chip_unregister()
> + devm_mux_control_get()
> + devm_mux_control_put()
> +
> PER-CPU MEM
> devm_alloc_percpu()
> devm_free_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32abef2b6d05..ebe96f3e25a0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8442,6 +8442,8 @@ MULTIPLEXER SUBSYSTEM
> M: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> S: Maintained
> F: Documentation/devicetree/bindings/mux/
> +F: include/linux/mux.h
> +F: drivers/mux/
>
> MULTISOUND SOUND DRIVER
> M: Andrew Veliath <andrewtv-Jdbf3xiKgS8@public.gmane.org>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index e1e2066cecdb..993aeb65affa 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -76,6 +76,8 @@ source "drivers/hwmon/Kconfig"
>
> source "drivers/thermal/Kconfig"
>
> +source "drivers/mux/Kconfig"
> +
Why this location in the list? I think the convention for new subystems is
to just go last in the list.
> source "drivers/watchdog/Kconfig"
>
> source "drivers/ssb/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 060026a02f59..d089baa57965 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -112,6 +112,7 @@ obj-$(CONFIG_W1) += w1/
> obj-y += power/
> obj-$(CONFIG_HWMON) += hwmon/
> obj-$(CONFIG_THERMAL) += thermal/
> +obj-$(CONFIG_MULTIPLEXER) += mux/
> obj-$(CONFIG_WATCHDOG) += watchdog/
> obj-$(CONFIG_MD) += md/
> obj-$(CONFIG_BT) += bluetooth/
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> new file mode 100644
> index 000000000000..de5a35ffe7af
> --- /dev/null
> +++ b/drivers/mux/Kconfig
> @@ -0,0 +1,33 @@
> +#
> +# Multiplexer devices
> +#
> +
> +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.
> +
> +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
> + multiplexers, which in that case will be operated in parallel.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index 000000000000..facc43da3648
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index 000000000000..21da15a264ad
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,398 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};
> +
> +static int __init mux_init(void)
> +{
> + return class_register(&mux_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);
> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(&mux_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (!dev || !controllers)
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> + controllers * sizeof(*mux_chip->mux) +
> + sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;
> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = &mux_class;
> + mux_chip->dev.type = &mux_type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(&mux_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(&mux->lock);
> + mux->cached_state = -1;
> + mux->idle_state = -1;
> + }
> +
> + device_initialize(&mux_chip->dev);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? -1 : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
> +
> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return device_add(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
> +
> +void mux_chip_unregister(struct mux_chip *mux_chip)
> +{
> + device_del(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
> +
> +void mux_chip_free(struct mux_chip *mux_chip)
> +{
> + if (!mux_chip)
> + return;
> +
> + put_device(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_free);
> +
> +static void devm_mux_chip_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_free(mux_chip);
> +}
> +
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> + unsigned int controllers,
> + size_t sizeof_priv)
> +{
> + struct mux_chip **ptr, *mux_chip;
> +
> + ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
> + if (IS_ERR(mux_chip)) {
> + devres_free(ptr);
> + return mux_chip;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> +
> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_chip **r = res;
> +
> + if (!r || !*r) {
> + WARN_ON(!r || !*r);
> + return 0;
> + }
> +
> + return *r == data;
> +}
> +
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_release,
> + devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
> +
> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_unregister(mux_chip);
> +}
> +
> +int devm_mux_chip_register(struct device *dev,
> + struct mux_chip *mux_chip)
> +{
> + struct mux_chip **ptr;
> + int res;
> +
> + ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + res = mux_chip_register(mux_chip);
> + if (res) {
> + devres_free(ptr);
> + return res;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return res;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> +
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
> + devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
> +
> +int mux_control_select(struct mux_control *mux, int state)
> +{
> + int ret;
> +
> + if (down_read_trylock(&mux->lock)) {
> + if (mux->cached_state == state)
> + return 0;
> +
> + /* Sigh, the mux needs updating... */
> + up_read(&mux->lock);
> + }
> +
> + /* ...or it's just contended. */
> + down_write(&mux->lock);
> +
> + if (mux->cached_state == state) {
> + /*
> + * Hmmm, someone else changed the mux to my liking.
> + * That makes me wonder how long I waited for nothing?
> + */
> + downgrade_write(&mux->lock);
> + return 0;
> + }
> +
> + ret = mux_control_set(mux, state);
> + if (ret < 0) {
> + if (mux->idle_state != -1)
> + mux_control_set(mux, mux->idle_state);
> +
> + up_write(&mux->lock);
> + return ret;
> + }
> +
> + downgrade_write(&mux->lock);
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_select);
> +
> +int mux_control_deselect(struct mux_control *mux)
> +{
> + int ret = 0;
> +
> + if (mux->idle_state != -1 && mux->cached_state != mux->idle_state)
> + ret = mux_control_set(mux, mux->idle_state);
> +
> + up_read(&mux->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_deselect);
> +
> +static int of_dev_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
> +
> + return dev ? to_mux_chip(dev) : NULL;
> +}
> +
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> + struct device_node *np = dev->of_node;
> + struct of_phandle_args args;
> + struct mux_chip *mux_chip;
> + unsigned int controller;
> + int index = 0;
> + int ret;
> +
> + if (mux_name) {
> + index = of_property_match_string(np, "mux-control-names",
> + mux_name);
> + if (index < 0)
> + return ERR_PTR(index);
> + }
> +
> + ret = of_parse_phandle_with_args(np,
> + "mux-controls", "#mux-control-cells",
> + index, &args);
> + if (ret) {
> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
> + np->full_name, mux_name ?: "", index);
> + return ERR_PTR(ret);
> + }
> +
> + mux_chip = of_find_mux_chip_by_node(args.np);
> + of_node_put(args.np);
> + if (!mux_chip)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (args.args_count > 1 ||
> + (!args.args_count && (mux_chip->controllers > 1))) {
> + dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
> + np->full_name, args.np->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + controller = 0;
> + if (args.args_count)
> + controller = args.args[0];
> +
> + if (controller >= mux_chip->controllers)
> + return ERR_PTR(-EINVAL);
> +
> + get_device(&mux_chip->dev);
> + return &mux_chip->mux[controller];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get);
> +
> +void mux_control_put(struct mux_control *mux)
> +{
> + put_device(&mux->chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_put);
> +
> +static void devm_mux_control_release(struct device *dev, void *res)
> +{
> + struct mux_control *mux = *(struct mux_control **)res;
> +
> + mux_control_put(mux);
> +}
> +
> +struct mux_control *devm_mux_control_get(struct device *dev,
> + const char *mux_name)
> +{
> + struct mux_control **ptr, *mux;
> +
> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + mux = mux_control_get(dev, mux_name);
> + if (IS_ERR(mux)) {
> + devres_free(ptr);
> + return mux;
> + }
> +
> + *ptr = mux;
> + devres_add(dev, ptr);
> +
> + return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
> +
> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_control **r = res;
> +
> + if (!r || !*r) {
> + WARN_ON(!r || !*r);
> + return 0;
> + }
> +
> + return *r == data;
> +}
> +
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
> +{
> + WARN_ON(devres_release(dev, devm_mux_control_release,
> + devm_mux_control_match, mux));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
> +
> +subsys_initcall(mux_init);
> +
> +MODULE_DESCRIPTION("Multiplexer subsystem");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
> new file mode 100644
> index 000000000000..76b52bc63470
> --- /dev/null
> +++ b/drivers/mux/mux-gpio.c
> @@ -0,0 +1,120 @@
> +/*
> + * GPIO-controlled multiplexer driver
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * 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/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +struct mux_gpio {
> + struct gpio_descs *gpios;
> + int *val;
> +};
> +
> +static int mux_gpio_set(struct mux_control *mux, int state)
> +{
> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> + int i;
> +
> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> + mux_gpio->val[i] = (state >> i) & 1;
> +
> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> + mux_gpio->gpios->desc,
> + mux_gpio->val);
> +
> + return 0;
> +}
> +
> +static const struct mux_control_ops mux_gpio_ops = {
> + .set = mux_gpio_set,
> +};
> +
> +static const struct of_device_id mux_gpio_dt_ids[] = {
> + { .compatible = "mux-gpio", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
> +
> +static int mux_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = 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 = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
> + pins * sizeof(*mux_gpio->val));
> + if (!mux_chip)
> + return -ENOMEM;
> +
> + mux_gpio = mux_chip_priv(mux_chip);
> + mux_gpio->val = (int *)(mux_gpio + 1);
> + mux_chip->ops = &mux_gpio_ops;
> +
> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> + if (IS_ERR(mux_gpio->gpios)) {
> + ret = PTR_ERR(mux_gpio->gpios);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get gpios\n");
> + return ret;
> + }
> + WARN_ON(pins != mux_gpio->gpios->ndescs);
> + mux_chip->mux->states = 1 << pins;
> +
> + ret = device_property_read_u32(dev, "idle-state", &idle_state);
> + if (ret >= 0) {
> + if (idle_state >= mux_chip->mux->states) {
> + dev_err(dev, "invalid idle-state %u\n", idle_state);
> + return -EINVAL;
> + }
> +
> + mux_chip->mux->idle_state = idle_state;
> + }
> +
> + ret = devm_mux_chip_register(dev, mux_chip);
> + if (ret < 0) {
> + dev_err(dev, "failed to register mux-chip\n");
> + return ret;
> + }
> +
> + dev_info(dev, "%u-way mux-controller registered\n",
> + mux_chip->mux->states);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mux_gpio_driver = {
> + .driver = {
> + .name = "mux-gpio",
> + .of_match_table = of_match_ptr(mux_gpio_dt_ids),
> + },
> + .probe = mux_gpio_probe,
> +};
> +module_platform_driver(mux_gpio_driver);
> +
> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..3b9439927f11
> --- /dev/null
> +++ b/include/linux/mux.h
> @@ -0,0 +1,244 @@
> +/*
> + * mux.h - definitions for the multiplexer interface
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#ifndef _LINUX_MUX_H
> +#define _LINUX_MUX_H
> +
> +#include <linux/device.h>
> +#include <linux/rwsem.h>
> +
> +struct mux_chip;
> +struct mux_control;
> +struct platform_device;
> +
> +struct mux_control_ops {
> + int (*set)(struct mux_control *mux, int state);
> +};
> +
> +/**
> + * struct mux_control - Represents a mux controller.
> + * @lock: Protects the mux controller state.
> + * @chip: The mux chip that is handling this mux controller.
> + * @states: The number of mux controller states.
> + * @cached_state: The current mux controller state, or -1 if none.
> + * @idle_state: The mux controller state to use when inactive, or -1
> + * for none.
> + */
> +struct mux_control {
> + struct rw_semaphore lock; /* protects the state of the mux */
> +
> + struct mux_chip *chip;
> +
> + unsigned int states;
> + int cached_state;
> + int idle_state;
> +};
> +
> +/**
> + * struct mux_chip - Represents a chip holding mux controllers.
> + * @controllers: Number of mux controllers handled by the chip.
> + * @mux: Array of mux controllers that is handled.
> + * @dev: Device structure.
> + * @id: Used to identify the device internally.
> + * @ops: Mux controller operations.
> + */
> +struct mux_chip {
> + unsigned int controllers;
> + struct mux_control *mux;
> + struct device dev;
> + int id;
> +
> + const struct mux_control_ops *ops;
> +};
> +
> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
> +
> +/**
> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
> + * @mux_chip: The mux-chip to get the private memory from.
> + *
> + * Return: Pointer to the private memory reserved by the allocator.
> + */
> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
> +{
> + return &mux_chip->mux[mux_chip->controllers];
> +}
> +
> +/**
> + * mux_chip_alloc() - Allocate a mux-chip.
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv);
> +
> +/**
> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
> + * for use.
> + * @mux_chip: The mux-chip to register.
> + *
> + * Do not retry registration of the same mux-chip on failure. You should
> + * instead put it away with mux_chip_free() and allocate a new one, if you
> + * for some reason would like to retry registration.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_register(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_unregister() - Take the mux-chip off-line.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * mux_chip_unregister() reverses the effects of mux_chip_register().
> + * But not completely, you should not try to call mux_chip_register()
> + * on a mux-chip that has been registered before.
> + */
> +void mux_chip_unregister(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_free() - Free the mux-chip for good.
> + * @mux_chip: The mux-chip to free.
> + *
> + * mux_chip_free() reverses the effects of mux_chip_alloc().
> + */
> +void mux_chip_free(struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * See mux_chip_alloc() for more details.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> + unsigned int controllers,
> + size_t sizeof_priv);
> +
> +/**
> + * devm_mux_chip_register() - Resource-managed version mux_chip_register().
> + * @dev: The parent device implementing the mux interface.
> + * @mux_chip: The mux-chip to register.
> + *
> + * See mux_chip_register() for more details.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
> + * @dev: The device that originally registered the mux-chip.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * See mux_chip_unregister() for more details.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_free() - Resource-managed version mux_chip_free().
> + * @dev: The device that originally got the mux-chip.
> + * @mux_chip: The mux-chip to free.
> + *
> + * See mux_chip_free() for more details.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * mux_control_select() - Select the given multiplexer state.
> + * @mux: The mux-control to request a change of state from.
> + * @state: The new requested state.
> + *
> + * Make sure to call mux_control_deselect() when the operation is complete and
> + * the mux-control is free for others to use, but do not call
> + * mux_control_deselect() if mux_control_select() fails.
> + *
> + * Return: 0 if the requested state was already active, or 1 it the
> + * mux-control state was changed to the requested state. Or a negavive
> + * errno on error.
> + *
> + * Note that the difference in return value of zero or one is of
> + * questionable value; especially if the mux-control has several independent
> + * consumers, which is something the consumers should perhaps not be making
> + * assumptions about.
> + */
> +int mux_control_select(struct mux_control *mux, int state);
> +
> +/**
> + * mux_control_deselect() - Deselect the previously selected multiplexer state.
> + * @mux: The mux-control to deselect.
> + *
> + * Return: 0 on success and a negative errno on error. An error can only
> + * occur if the mux has an idle state. Note that even if an error occurs, the
> + * mux-control is unlocked for others to access.
> + */
> +int mux_control_deselect(struct mux_control *mux);
> +
> +/**
> + * mux_control_get_index() - Get the index of the given mux controller
> + * @mux: The mux-control to the the index for.
> + *
> + * Return: The index of the mux controller within the mux chip the mux
> + * controller is a part of.
> + */
> +static inline unsigned int mux_control_get_index(struct mux_control *mux)
> +{
> + return mux - mux->chip->mux;
> +}
> +
> +/**
> + * mux_control_get() - Get the mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
> +
> +/**
> + * mux_control_put() - Put away the mux-control for good.
> + * @mux: The mux-control to put away.
> + *
> + * mux_control_put() reverses the effects of mux_control_get().
> + */
> +void mux_control_put(struct mux_control *mux);
> +
> +/**
> + * devm_mux_control_get() - Get the mux-control for a device, with resource
> + * management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get(struct device *dev,
> + const char *mux_name);
> +
> +/**
> + * devm_mux_control_put() - Resource-managed version mux_control_put().
> + * @dev: The device that originally got the mux-control.
> + * @mux: The mux-control to put away.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux);
> +
> +#endif /* _LINUX_MUX_H */
>
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Hans de Goede @ 2017-01-08 10:15 UTC (permalink / raw)
To: Jonathan Cameron, Jacob Pan
Cc: Chen-Yu Tsai, Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio, linux-i2c
In-Reply-To: <c86f5ad2-f504-e2a9-32eb-d16d500a09fe@kernel.org>
Hi,
On 07-01-17 23:23, Jonathan Cameron wrote:
> On 04/01/17 17:55, Jacob Pan wrote:
>> On Tue, 3 Jan 2017 23:10:57 +0100
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>>> It could have been a quirk we had to do on our platforms, I just
>>>> cannot recall the details. Are you testing this on x86 platforms?
>>>
>>> Yes, I've successfully tested this on 2 different models cherrytrail
>>> tablets.
>>
>> [Jacob Pan] I am ok with your change. I last tested on baytrail
>> tablets, I don't know if you have one available to verify. That would
>> be ideal.
>>
> Stable material? As I read this it is a fairly major fix, but we aren't
> entirely sure there wasn't a reason on some platforms for this 'interesting'
> corner of code?
>
> So basically are we sure this won't cause regressions? If so I'll take
> it as a fix and mark for stable.
The main consumer of the axp288_adc code is the axp288_fuel_gauge driver,
which until now was not really functional. It depended on platform data
being attached to its mfd device, and the provider of that platform data
never got merged.
I've got patches queued up for 4.11 fixing this. So in practice the chance
of this patch causing regressions is close to 0 as so far it had no
consumers, likewise adding a Cc: stable is not really useful.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v7 01/12] devres: trivial whitespace fix
From: Jonathan Cameron @ 2017-01-07 22:29 UTC (permalink / raw)
To: Peter Rosin, 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: <1483532187-28494-2-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 04/01/17 12:16, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> Documentation/driver-model/devres.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index ca9d1eb46bc0..dc51fb024190 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -330,7 +330,7 @@ MEM
> devm_kzalloc()
>
> MFD
> - devm_mfd_add_devices()
> + devm_mfd_add_devices()
>
> PER-CPU MEM
> devm_alloc_percpu()
>
--
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: [PATCH v7 09/12] dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings
From: Jonathan Cameron @ 2017-01-07 22:28 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: <1483532187-28494-10-git-send-email-peda@axentia.se>
On 04/01/17 12:16, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda@axentia.se>
Looks good to me.
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> .../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
> new file mode 100644
> index 000000000000..253d5027843b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
> @@ -0,0 +1,81 @@
> +Simple I2C Bus Mux
> +
> +This binding describes an I2C bus multiplexer that uses a mux controller
> +from the mux subsystem to route the I2C signals.
> +
> + .-----. .-----.
> + | dev | | dev |
> + .------------. '-----' '-----'
> + | SoC | | |
> + | | .--------+--------'
> + | .------. | .------+ child bus A, on MUX value set to 0
> + | | I2C |-|--| Mux |
> + | '------' | '--+---+ child bus B, on MUX value set to 1
> + | .------. | | '----------+--------+--------.
> + | | MUX- | | | | | |
> + | | Ctrl |-|-----+ .-----. .-----. .-----.
> + | '------' | | dev | | dev | | dev |
> + '------------' '-----' '-----' '-----'
> +
> +Required properties:
> +- compatible: i2c-mux-simple,mux-locked or i2c-mux-simple,parent-locked
> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
> + port is connected to.
> +- mux-controls: The phandle of the mux controller to use for operating the
> + mux.
> +* Standard I2C mux properties. See i2c-mux.txt in this directory.
> +* I2C child bus nodes. See i2c-mux.txt in this directory. The sub-bus number
> + is also the mux-controller state described in ../mux/mux-controller.txt
> +
> +For each i2c child node, an I2C child bus will be created. They will
> +be numbered based on their order in the device tree.
> +
> +Whenever an access is made to a device on a child bus, the value set
> +in the relevant node's reg property will be set as the state in the
> +mux controller.
> +
> +Example:
> + mux: mux-controller {
> + compatible = "mux-gpio";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + i2c-mux {
> + compatible = "i2c-mux-simple,mux-locked";
> + i2c-parent = <&i2c1>;
> +
> + mux-controls = <&mux>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ssd1307: oled@3c {
> + compatible = "solomon,ssd1307fb-i2c";
> + reg = <0x3c>;
> + pwms = <&pwm 4 3000>;
> + reset-gpios = <&gpio2 7 1>;
> + reset-active-low;
> + };
> + };
> +
> + i2c@3 {
> + reg = <3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pca9555: pca9555@20 {
> + compatible = "nxp,pca9555";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x20>;
> + };
> + };
> + };
>
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Jonathan Cameron @ 2017-01-07 22:23 UTC (permalink / raw)
To: Jacob Pan, Hans de Goede
Cc: Chen-Yu Tsai, Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170104095517.6f7ee610@jacob-builder>
On 04/01/17 17:55, Jacob Pan wrote:
> On Tue, 3 Jan 2017 23:10:57 +0100
> Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>>> It could have been a quirk we had to do on our platforms, I just
>>> cannot recall the details. Are you testing this on x86 platforms?
>>
>> Yes, I've successfully tested this on 2 different models cherrytrail
>> tablets.
>
> [Jacob Pan] I am ok with your change. I last tested on baytrail
> tablets, I don't know if you have one available to verify. That would
> be ideal.
>
Stable material? As I read this it is a fairly major fix, but we aren't
entirely sure there wasn't a reason on some platforms for this 'interesting'
corner of code?
So basically are we sure this won't cause regressions? If so I'll take
it as a fix and mark for stable.
Thanks,
Jonathan
> Thanks,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] i2c: print correct device invalid address
From: Vladimir Zapolskiy @ 2017-01-07 21:43 UTC (permalink / raw)
To: John Garry, wsa; +Cc: linux-i2c, linux-kernel, linuxarm
In-Reply-To: <1483700577-60130-1-git-send-email-john.garry@huawei.com>
On 01/06/2017 01:02 PM, John Garry wrote:
> In of_i2c_register_device(), when the check for
> device address validity fails we print the info.addr,
> which has not been assigned properly.
>
> Fix this by printing the actual invalid address.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index cf9e396..03cfcdd 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1708,7 +1708,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>
> if (i2c_check_addr_validity(addr, info.flags)) {
> dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
> - info.addr, node->full_name);
> + addr, node->full_name);
> return ERR_PTR(-EINVAL);
> }
>
>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Fixes: b4e2f6ac1281 ("i2c: apply DT flags when probing")
Thank you for the fix.
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface
From: Guenter Roeck @ 2017-01-07 17:15 UTC (permalink / raw)
To: Edward James
Cc: andrew, corbet, devicetree, eajames.ibm, jdelvare, joel,
linux-doc, linux-hwmon, linux-i2c, linux-kernel, mark.rutland,
robh+dt, wsa
In-Reply-To: <OFA812992D.D19B1368-ON002580A0.007A2966-862580A0.007A72F0@notes.na.collabserv.com>
On 01/06/2017 02:17 PM, Edward James wrote:
[ ... ]
>> > +}
>> > +
>> > +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
>> > + store_occ_online);
>> > +
>> > +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
>> > + struct occ_sysfs_config *config)
>> > +{
>> > + struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
>> > + GFP_KERNEL);
>> > + int rc;
>> > +
>> > + if (!hwmon)
>> > + return ERR_PTR(-ENOMEM);
>> > +
>> > + hwmon->occ = occ;
>> > + hwmon->num_caps_fields = config->num_caps_fields;
>> > + hwmon->caps_names = config->caps_names;
>> > +
>> > + dev_set_drvdata(dev, hwmon);
>> > +
>> > + rc = device_create_file(dev, &dev_attr_online);
>> > + if (rc)
>> > + return ERR_PTR(rc);
>> > +
>> > + return hwmon;
>> > +}
>> > +EXPORT_SYMBOL(occ_sysfs_start);
>> > +
>> > +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
>> > +{
>> > + if (driver->dev) {
>> > + occ_remove_hwmon_attrs(driver);
>> > + hwmon_device_unregister(driver->dev);
>> > + }
>> > +
>> > + device_remove_file(driver->dev, &dev_attr_online);
>> > +
>> > + devm_kfree(dev, driver);
>>
>> Thw point of using devm_ functions is not to require remove/free functions.
>> Something is completely wrong here if you need that call.
>>
>> Overall, this is architectually completely wrong. One does not register
>> or instantiate drivers based on writing into sysfs attributes. Please
>> reconsider your approach.
>
> We had some trouble designing this driver because the BMC only has
> access to the OCC once the processor is powered on. This will happen
> sometime after the BMC boots (this driver runs only on the BMC). With
> no access to the OCC, we don't know what sensors are present on the
> system without a large static enumeration. Also any sysfs files created
> before we have OCC access won't be able to return any data.
>
> Instead of the "online" attribute, what do you think about using the
> "bind"/"unbind" API to probe the device from user space once the system
> is powered on? All the hwmon registration would take place in the probe
> function, it would just occur some time after boot.
>
A more common approach would be to have a platform driver. That platform
driver would need a means to detect if the OCC is up and running, and
instantiate everything else once it is.
A trigger from user space is problematic because there is no guarantee
that the OCC is really up (or that it even exists).
An alternative might be to have the hwmon driver poll for the OCC,
but that would be a bit more difficult and might require a kernel thread
or maybe asynchronous probing.
Guenter
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Wolfram Sang @ 2017-01-07 12:49 UTC (permalink / raw)
To: Pali Rohár
Cc: Michał Kępień, Jean Delvare, Steven Honeyman,
Valdis.Kletnieks, Jochen Eisinger, Gabriele Mazzotta,
Andy Lutomirski, Mario_Limonciello, Alex Hung, Takashi Iwai,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201612291517.37474@pali>
[-- Attachment #1: Type: text/plain, Size: 787 bytes --]
> > > > > + if (!known_i2c_address) {
> > > > > + dev_warn(&priv->pci_dev->dev,
> > > > > + "Accelerometer lis3lv02d i2c device is present "
> > > > > + "but its i2c address is unknown, skipping ...\n");
> > > >
> > > > You are probably well aware of this, but checkpatch prefers
> > > > keeping long log messages in one line. I am pointing it out
> > > > just in case.
> > >
> > > Yes, but I do not know how to fix it. Splitting message into two
> > > lines generates warning. Having long line generates warning too.
> >
> > Weird, checkpatch does not protest on my machine when the log message
> > is written on a single line...
>
> I hope that i2c maintainers decide how to format that line.
Make it one line saying "accelerometer address unknown"?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: core: helper function to detect slave mode
From: Vladimir Zapolskiy @ 2017-01-07 1:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, devicetree,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w, Joao Pinto,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <CAHp75Ve7zNN3m1ngLGi-81mFehKzZG07mh5H2T5+_k-=jJqqNA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>>> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
>>>>> + if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
>>>>
>>>> IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
>>>> should be sufficient.
>>>
>>> Sorry, but you missed the point.
>>> This will enable compile time optimization and basically be collapsed to no-op.
>>>
>>
>> Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
>> over the code to reduce the size of the built image?
>
> There is no black and white, don't be silly.
>
>>
>>>>> + }
>>>>> + } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
>>>>> + dev_dbg(dev, "ACPI slave is not supported yet\n");
>>>>> + }
>>>>
>>>> If so, then it might be better to drop else-if stub for now.
>>>
>>> Please, don't.
>>>
>>
>> Why do you ask for this stub to be added?
>
> 1. Exactly the reason you asked above. Here is the code which has
> built differently on different platforms. x86 usually is not using
> CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
> existing examples.
>From the context by the stub I mean dev_dbg() in i2c_slave_mode_detect()
function, I don't see a connection to GPIO library, please clarify.
> 2. We might add that support later, but here is again, just no-op.
>
> So, what is your strong argument here against that?
>
When the support is ready for ACPI case, you'll remove the added
dev_dbg(), and I don't see a good point by adding it temporarily.
What is wrong with the approach of adding the ACPI case handling
branch when it is ready and remove any kind of stubs right now?
On ACPI platforms the function returns 'false' always, will the
function work correctly (= corresponding to its description) as is?
PS, if it is possible, please give up on arrogance in discussion.
--
With best wishes,
Vladimir
--
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: [PATCH] i2c: core: helper function to detect slave mode
From: Andy Shevchenko @ 2017-01-07 0:19 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, devicetree,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w, Joao Pinto,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <b5788d1d-c951-0fc3-eed8-88457df1f27e-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
>>>> + if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
>>>
>>> IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
>>> should be sufficient.
>>
>> Sorry, but you missed the point.
>> This will enable compile time optimization and basically be collapsed to no-op.
>>
>
> Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
> over the code to reduce the size of the built image?
There is no black and white, don't be silly.
>
>>>> + }
>>>> + } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
>>>> + dev_dbg(dev, "ACPI slave is not supported yet\n");
>>>> + }
>>>
>>> If so, then it might be better to drop else-if stub for now.
>>
>> Please, don't.
>>
>
> Why do you ask for this stub to be added?
1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.
2. We might add that support later, but here is again, just no-op.
So, what is your strong argument here against that?
--
With Best Regards,
Andy Shevchenko
--
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: [PATCH] i2c: core: helper function to detect slave mode
From: Vladimir Zapolskiy @ 2017-01-06 23:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Andy Shevchenko, Mika Westerberg, linux-i2c,
devicetree, linux-kernel@vger.kernel.org, Ramiro.Oliveira,
Joao Pinto, CARLOS.PALMINHA
In-Reply-To: <CAHp75Ve87TE3y6hg1bMxSFAPVcY3wPifGuvtpXNhBw5-eOnPrg@mail.gmail.com>
On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>>> + if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
>>
>> IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
>> should be sufficient.
>
> Sorry, but you missed the point.
> This will enable compile time optimization and basically be collapsed to no-op.
>
Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
over the code to reduce the size of the built image?
>>> + }
>>> + } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
>>> + dev_dbg(dev, "ACPI slave is not supported yet\n");
>>> + }
>>
>> If so, then it might be better to drop else-if stub for now.
>
> Please, don't.
>
Why do you ask for this stub to be added?
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH] i2c: core: helper function to detect slave mode
From: Andy Shevchenko @ 2017-01-06 22:45 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, devicetree,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w, Joao Pinto,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <73246c4a-504c-52d7-dde4-970a45dca0bd-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
>> + if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
>
> IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
> should be sufficient.
Sorry, but you missed the point.
This will enable compile time optimization and basically be collapsed to no-op.
>> + }
>> + } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
>> + dev_dbg(dev, "ACPI slave is not supported yet\n");
>> + }
>
> If so, then it might be better to drop else-if stub for now.
Please, don't.
--
With Best Regards,
Andy Shevchenko
--
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: [PATCH v2] i2c: core: helper function to detect slave mode
From: Vladimir Zapolskiy @ 2017-01-06 21:49 UTC (permalink / raw)
To: Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <9ada747bd53bb5005a82f913412262b34b6e96b8.1483725370.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Hi Luis,
On 01/06/2017 08:14 PM, Luis Oliveira wrote:
> This function has the purpose of mode detection by checking the
> device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
> Currently only checks using OF functions (ACPI slave not supported yet).
>
I've accidentally reviewed v1, please take the same comments for v2.
--
With best wishes,
Vladimir
--
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
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