Re: Handling clocks on external busses
On Wed, Dec 02, 2015 at 12:58:55PM +, Charles Keepax wrote: > So after a bit more digging it seems this has been mitigated slightly > as a lot of SPI driver have been updated to execute transfers in > thread rather than from a worker thread and it seems the clock > framework lets you re-enter the locked sections if called from the > same thread. No, this isn't something the drivers support (it's a framework feature) and it isn't something that we're guaranteed to do, if the bus is busy we defer to the thread and it's possible the drivers might do something multi-threaded themselves. It might work a lot of the time but it's never going to be guaranteed to work. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Sun, Oct 25, 2015 at 02:54:39PM +0100, Rafael J. Wysocki wrote: > On Sun, Oct 25, 2015 at 12:06 AM, Mark Brown <broo...@kernel.org> wrote: > > There's also the understanding people had that the order things get > > bound changes the ordering for some of the other cases (perhaps it's a > > good idea to do that, it seems likely to be sensible?). > But it really doesn't do that. Also making it do so doesn't help much > in the cases where things can happen asynchronously (system > suspend/resume, runtime PM). Yeah, people seem to have that impression though. :( > If, instead, there was a way to specify a functional dependency at the > device registration time, it might be used to change the order of > everything relevant, including probe. That should help to reduce the > noise you're referring to. This links back to the idea of having generic support for pre-probe actions which is also generally useful (the ability to do things like power on regulators for devices on enumerable buses so they can be enumerated as standard). signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Sat, Oct 24, 2015 at 04:17:12PM +0200, Rafael J. Wysocki wrote: > Well, I'm not quite sure why exactly everyone is so focused on probing here. Probe deferral is really noisy even if it's working fine on a given system so it's constantly being highlighted to people in a way that other issues aren't if you're not directly having problems. There's also the understanding people had that the order things get bound changes the ordering for some of the other cases (perhaps it's a good idea to do that, it seems likely to be sensible?). signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Thu, Oct 22, 2015 at 04:02:13PM +0100, Russell King - ARM Linux wrote: > If it was such a problem, then in the _eight_ days that this has been > discussed so far, _someone_ would have sent some data showing the > problem. I think the fact is, there is no data. > Someone prove me wrong. Someone post the verifiable data showing that > there is a problem to be solved here. > Someone show what the specific failure cases are that are hampering > vendors moving forwards. Someone show the long boot times by way of > kernel message log. Someone show some evidence of the problems that > have been alluded to. > If no one can show some evidence, there isn't a problem here. :) Yeah, I'm not convinced the timing is *such* a big deal either - I do think that the log spam is a real problem but I think something much less invasive like the interface you proposed is good for addressing that. signature.asc Description: PGP signature
Re: Alternative approach to solve the deferred probe (was: [GIT PULL] On-demand device probing)
On Tue, Oct 20, 2015 at 04:46:56PM +0100, Russell King - ARM Linux wrote:
> Something like this. I haven't put a lot of effort into it to change all
> the places which return an -EPROBE_DEFER, and it also looks like we need
> some helpers to report when we have only an device_node (or should that
> be fwnode?) See the commented out of_warn_deferred() in
> drivers/gpio/gpiolib-of.c. Adding this stuff in the subsystems searching
> for resources should make debugging why things are getting deferred easier.
Yeah, plus I'd expect it to also result in better error reporting
overall if the subsystems are able to report when they fail to get
something rather than just returning an error to the driver.
> +/**
> + * dev_warn_deferred() - report why a probe has been deferred
> + */
> +void dev_warn_deferred(struct device *dev, const char *fmt, ...)
> +{
> + if (driver_deferred_probe_report) {
> + struct va_format vaf;
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vaf.fmt = fmt;
> + vaf.va =
> +
> + dev_warn(dev, "deferring probe: %pV", );
> + va_end(ap);
> + }
> +}
> +EXPORT_SYMBOL_GPL(dev_warn_deferred);
I'm not currently able to think of a nice way of writing this but I think
what I'd really like to see from a driver point of view is something
which decays into dev_err() if it's a non-deferral error. That way
drivers can have minimal log and return error handling code and we will
still get the output sensibly. The best I can think of is something
like
void dev_warn_deferred(struct device *dev, int err, const char *fmt, ...)
which requires the caller to pass in err twice to get it logged. That's
not a thing of beauty but it gets the job done... but perhaps your
original interface is better, it's a bit cleaner.
signature.asc
Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Wed, Oct 21, 2015 at 08:59:51AM -0700, Frank Rowand wrote: > On 10/19/2015 5:34 AM, Tomeu Vizoso wrote: > > To be clear, I was saying that this series should NOT affect total > > boot times much. > I'm confused. If I understood correctly, improving boot time was > the key justification for accepting this patch set. For example, > from "[PATCH v7 0/20] On-demand device probing": > >I have a problem with the panel on my Tegra Chromebook taking longer >than expected to be ready during boot (Stéphane Marchesin reported what >is basically the same issue in [0]), and have looked into ordered >probing as a better way of solving this than moving nodes around in the >DT or playing with initcall levels and linking order. > >... > >With this series I get the kernel to output to the panel in 0.5s, >instead of 2.8s. Overall boot time and time to get some individual built in component up and running aren't the same thing - what this'll do is get things up more in the link order of the leaf consumers rather than deferring those leaf consumers when their dependencies aren't ready yet. > While not as dramatic as your results, they are somewhat supportive. > What has changed your assessment that the on-demand device probing > patches will give a big boot performance increase? Do you have > new data or analysis? See above, my understanding was that the performance improvements were more around improved control/predictability/handwave of the boot ordering rather than total time. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Wed, Oct 21, 2015 at 11:18:08AM -0700, Frank Rowand wrote: > On 10/21/2015 9:27 AM, Mark Brown wrote: > > Overall boot time and time to get some individual built in component up > > and running aren't the same thing - what this'll do is get things up > > more in the link order of the leaf consumers rather than deferring those > > leaf consumers when their dependencies aren't ready yet. > Thanks! I read too much into what was being improved. > So this patch series, which on other merits may be a good idea, is as > a by product solving a specific ordering issue, moving successful panel > initialization to an earlier point in the boot sequence, if I now > understand more correctly. Yeah, that's my understanding. > In that context, this seems like yet another ad hoc way of causing the > probe order to change in a way to solves one specific issue? Could > it just as likely move the boot order of some other driver on some > other board later, to the detriment of somebody else? Indeed. My general feeling is that it does make the link order stuff more predictable and easier to work with and it does have other merits (in terms of the error reporting, though there's other ways to address that like the one Russell is proposing). signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Tue, Oct 20, 2015 at 01:14:46PM -0400, Alan Stern wrote: > On Tue, 20 Oct 2015, Tomeu Vizoso wrote: > > This iteration of the series would make this quite easy, as > > dependencies are calculated before probes are attempted: > > https://lkml.org/lkml/2015/6/17/311 > But what Rafael is proposing is quite general; it would apply to _all_ > dependencies as opposed to just those present in DT drivers or those > affecting platform_devices. We'll still need most of the DT bits that are there at the minute (the ones strewn around the subsystems) AFAICT since it's at the point where we parse the DT and work out what the dependencies are which we probably want to do prior to getting the drivers up and will be different for ACPI. I think the level of DT dependency here looks a lot larger than it actually is due to the fact that a lot of what's being modified is DT parsing code. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote: > Furthermore, that applies only to devices that use synchronous suspend. > Async suspend is becoming common, and there the only restrictions are > parent-child relations plus whatever explicit requirements that drivers > impose by calling device_pm_wait_for_dev(). Hrm, this is the first I'd noticed that feature though I see the initial commit dates from January. It looks like most of the users are PCs at the minute but we should be using it more widely for embedded things, there's definitely some cases I'm aware of where it will allow us to remove some open coding. It does seem like we want to be feeding dependency information we discover for probing way into the suspend dependencies... signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Mon, Oct 19, 2015 at 10:44:41AM +0100, David Woodhouse wrote: > On Sun, 2015-10-18 at 20:53 +0100, Mark Brown wrote: > > Do you mean firmware rather than bus here? I think that's the confusion > > I have... > Certainly, if it literally is adding of_* calls then that would seem to > be gratuitously firmware-specific. Nothing should be using those these > days; any new code should be using the generic device property APIs > (except in special cases). It's not entirely clear to me that we should be moving to fwnode_ wholesale yet - the last advice was to hold off for a little while which makes sense given that the ACPI community still doesn't seem to have worked out what it wants to do here and how. The x86 embedded people are all gung ho but it's less clear that anyone else wants to use _DSD in quite the same way (I know of some efforts to use _DSD separately to the DT compatibility stuff) and there are some vendors who definitely do have completely different binding schemes for ACPI and DT and therefore specifically care which is in use. It would really help if ACPI could get their binding review process in place, and if we do want to actually start converting everything to fwnode_ we need to start communicating that actively since otherwise people can't really be expected to know. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Mon, Oct 19, 2015 at 01:47:50PM +0100, David Woodhouse wrote: > On Mon, 2015-10-19 at 07:35 -0500, Rob Herring wrote: > > See version 2 of the series[1] which did that. It became obvious that > > was pointless because the call paths ended up looking like this: > > Generic subsystem code -> DT look-up code -> fwnode_probe_device -> > > of_probe_device > You link to a thread which says that "AT LEAST CURRENTLY, the calling > locations [the 'DT look-up code' you mention above] are DT specific > functions anyway. > But the point I'm making is that we are working towards *fixing* that, > and *not* using DT-specific code in places where we should be using the > generic APIs. What is the plan for fixing things here? It's not obvious (at least to me) that we don't want to have the subsystems having knowledge of how they are bound to a specific firmware which is what you seem to imply here. That seems like it's going to fall down since the different firmware interfaces do have quite different ideas about how things fit together at a system level and different compatibility needs which do suggest that just trying to do a direct mapping from DT into ACPI may well not make people happy but it sounds like that's the intention. When it gets to drivers the situation is much more clear since it's normally just simple properties, it's generally a bit more worrying if drivers are needing to directly interact with cross-device linkage. This is all subsystem level code though. > None of that really negates that fact that we are *working* on cleaning > these code paths up to be firmware-agnostic, and the fact that we > haven't got to this one *yet* isn't necessarily a good reason to make > it *worse* by adding new firmware-specificity to it. It seems like we're going to have to refactor these bits of code when they get generalised anyway so I'm not sure that the additional cost here is that big. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Mon, Oct 19, 2015 at 04:29:40PM +0100, David Woodhouse wrote: > On Mon, 2015-10-19 at 15:50 +0100, Mark Brown wrote: > > > But the point I'm making is that we are working towards *fixing* that, > > > and *not* using DT-specific code in places where we should be using the > > > generic APIs. > > What is the plan for fixing things here? It's not obvious (at least to > > me) that we don't want to have the subsystems having knowledge of how > > they are bound to a specific firmware which is what you seem to imply > > here. > I don't know that there *is* a coherent plan here to address it all. > Certainly, we *will* need subsystems to have firmware-specific > knowledge in some cases. Take GPIO as an example; ACPI *has* a way to > describe GPIO, and properties which reference GPIO pins are intended to > work through that — while in DT, properties which reference GPIO pins > will have different contents. They'll be compatible at the driver > level, in the sense that there's a call to get a given GPIO given the > property name, but the subsystems *will* be doing different things > behind the scenes. I'd expect that to be the norm rather than the exception. > > It seems like we're going to have to refactor these bits of code when > > they get generalised anyway so I'm not sure that the additional cost > > here is that big. > That's an acceptable answer — "we're adding legacy code here but we > know it's going to be refactored anyway". If that's true, all it takes > is a note in the commit comment to that effect. That's different from > having not thought about it :) Given the above I'm not even sure it's legacy code, it's just as likely we're going to get some parallel ACPI code added to the subsystems for parsing their bindings. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Sat, Oct 17, 2015 at 08:47:09AM -0700, Greg Kroah-Hartman wrote: > On Sat, Oct 17, 2015 at 10:04:55AM -0500, Rob Herring wrote: > > I think Linus W, Mark B, and I all said a similar thing initially in > > that dependencies should be handled in the driver core. We went down > > the path of making this not firmware (aka bus) specific and an earlier > > version had just that (with fwnode_* calls). That turned out to be > > pointless as the calling locations were almost always in DT specific > > code anyway. If you notice, the calls are next to other DT specific > > calls generally (usually a "get"). So yes, I'd prefer not to have to > > touch every subsystem, but we had to do that anyway to add DT support. > If they are "next" to a call like that, why not put it in that call? I > really object to having to "sprinkle" this all over the kernel, for no > obvious reason why that is happening at all (look at the USB patch for > one such example.) I did ask that question myself IIRC - we could probably get a long way by trying to instantiate anything that looks probable when we do a phandle lookup on it. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Sun, Oct 18, 2015 at 12:37:57PM -0700, Greg Kroah-Hartman wrote: > On Sun, Oct 18, 2015 at 08:29:31PM +0100, Mark Brown wrote: > > On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote: > > > I can't see adding calls like this all over the tree just to solve a > > > bus-specific problem, you are adding of_* calls where they aren't > > > needed, or wanted, at all. > > This isn't bus specific, I'm not sure what makes you say that? > You are making it bus-specific by putting these calls all over the tree > in different bus subsystems semi-randomly for all I can determine. Do you mean firmware rather than bus here? I think that's the confusion I have... > > One is that regardless of the actual performance of the system when > > deferred probe goes off it splats errors all over the console which > > makes it look like something is going wrong even if everything is fine > > in the end. If lots of deferred probing happens then the volume gets > > big too. People find this distracting, noisy and ugly - it obscures > > actual issues and trains people to ignore errors. I do think this is a > > reasonable concern and that it's worth trying to mitigate against > > deferral for this reason alone. We don't want to just ignore the errors > > and not print anything either since if the resource doesn't appear the > > user needs to know what is preventing the driver from instantiating so > > they can try to fix it. > This has come up many times, I have no objection to just turning that > message into a debug message that can be dynamically enabled for those > people wanting to debug their systems for boot time issues. It's not just the driver core logging, it's also all the individual drivers logging that they failed to get whatever resource since silently failing is not a great user experience. Many, hopefully most, of the drivers don't actually have special handling for probe deferral since half the beauty of probe deferral is that the subsystem supplying the resource can just return -EPROBE_DEFER when it notices something is missing but might appear and then the drivers will do the right thing so long as they have error handling code that they really should have anyway. We'd need to have a special dev_err() that handled probe deferral errors for drivers to use during probe or some other smarts in the logging infrastructure. Which isn't a totally horrible idea. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote: > I can't see adding calls like this all over the tree just to solve a > bus-specific problem, you are adding of_* calls where they aren't > needed, or wanted, at all. This isn't bus specific, I'm not sure what makes you say that? > What is the root-problem of your delay in device probing? I read your > last patch series and I can't seem to figure out what the issue is that > this is solving in any "better" way from the existing deferred probing. So, I don't actually have any platforms that are especially bothered by this (at least not for my use cases) so there's a bit of educated guessing going on here but there's two broad things I'm aware of. One is that regardless of the actual performance of the system when deferred probe goes off it splats errors all over the console which makes it look like something is going wrong even if everything is fine in the end. If lots of deferred probing happens then the volume gets big too. People find this distracting, noisy and ugly - it obscures actual issues and trains people to ignore errors. I do think this is a reasonable concern and that it's worth trying to mitigate against deferral for this reason alone. We don't want to just ignore the errors and not print anything either since if the resource doesn't appear the user needs to know what is preventing the driver from instantiating so they can try to fix it. The other is that if you're printing to a serial console then that's not an especially fast operation so if you're getting lots of messages being printed simply physically outputting them takes measurable time. I'm not aware of any performance concerns outside of that, but like I say I'm not affected by this myself in any great way. Obviously this can be configured but not having actual errors on the console isn't super awesome either for systems that make use of the logging there and we don't have a good way of telling what's from deferral and what's not. signature.asc Description: PGP signature
Re: [GIT PULL] On-demand device probing
On Wed, Oct 14, 2015 at 10:34:00AM +0200, Tomeu Vizoso wrote: > git+ssh://git.collabora.co.uk/git/user/tomeu/linux.git > on-demand-probes-for-next In don't think that's the URL you intended to use (also everything looks word wrapped here)? signature.asc Description: PGP signature
Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
On Tue, Aug 25, 2015 at 04:57:56PM +0200, Wolfram Sang wrote: On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote: On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote: Commit 70762abb9f89 (i2c: Use stable dev_name for ACPI enumerated I2C slaves) broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as x-00yz. However if those hwmon devices are ACPI 5 enumerated their name became i2c-INTABCD:ij and sysfs code in lm-sensors does not find them anymore: Acked-by: Mark Brown broo...@kernel.org Don't you think there will be regressions given that the new naming scheme was around for 18 months? That's a whatever from the ASoC point of view. I don't particularly care about the userspace ABI, on the one hand there aren't that many ACPI 5 devices that might be affected and there's other devices that won't work anyway. On the other hand I guess there's other devices that would have worked with the old kernel. signature.asc Description: Digital signature
Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote: Commit 70762abb9f89 (i2c: Use stable dev_name for ACPI enumerated I2C slaves) broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as x-00yz. However if those hwmon devices are ACPI 5 enumerated their name became i2c-INTABCD:ij and sysfs code in lm-sensors does not find them anymore: Acked-by: Mark Brown broo...@kernel.org signature.asc Description: Digital signature
[PATCH] i2c: jz4780: Explicitly include linux/io.h
This driver uses readw() and writew() which are declared in asm/io.h but does not explicitly include that header, causing build failures on architectures where there is not an implicit inclusion such as arm and arm64: ../drivers/i2c/busses/i2c-jz4780.c:181:2: error: implicit declaration of function 'readw' [-Werror=implicit-function-declaration] ../drivers/i2c/busses/i2c-jz4780.c:187:2: error: implicit declaration of function 'writew' [-Werror=implicit-function-declaration] Add an explicit inclusion to fix this. Signed-off-by: Mark Brown broo...@kernel.org --- drivers/i2c/busses/i2c-jz4780.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c index ce1d69324169..19b2d689a5ef 100644 --- a/drivers/i2c/busses/i2c-jz4780.c +++ b/drivers/i2c/busses/i2c-jz4780.c @@ -23,6 +23,7 @@ #include linux/i2c.h #include linux/init.h #include linux/interrupt.h +#include linux/io.h #include linux/kernel.h #include linux/module.h #include linux/platform_device.h -- 2.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: master build: 2 failures 15 warnings (v4.0-5833-g6c373ca89399)
On Wed, Apr 15, 2015 at 06:30:01PM +0100, Build bot for Mark Brown wrote: For the past couple of days -next has been failing to build on both arm and arm64 with: arm64-allmodconfig ../drivers/i2c/busses/i2c-jz4780.c:181:2: error: implicit declaration of function 'readw' [-Werror=implicit-function-declaration] ../drivers/i2c/busses/i2c-jz4780.c:187:2: error: implicit declaration of function 'writew' [-Werror=implicit-function-declaration] arm-allmodconfig ../drivers/i2c/busses/i2c-jz4780.c:181:2: error: implicit declaration of function 'readw' [-Werror=implicit-function-declaration] ../drivers/i2c/busses/i2c-jz4780.c:187:2: error: implicit declaration of function 'writew' [-Werror=implicit-function-declaration] due to relying on an implicit inclusion of asm/io.h. This now also affects Linus' tree. I've sent a patch. signature.asc Description: Digital signature
[PATCH] i2c: core: Export bus recovery functions
Current -next fails to link an ARM allmodconfig because drivers that use
the core recovery functions can be built as modules but those functions
are not exported:
ERROR: i2c_generic_gpio_recovery [drivers/i2c/busses/i2c-davinci.ko]
undefined!
ERROR: i2c_generic_scl_recovery [drivers/i2c/busses/i2c-davinci.ko] undefined!
ERROR: i2c_recover_bus [drivers/i2c/busses/i2c-davinci.ko] undefined!
Add exports to fix this.
Fixes: 5f9296ba21b3c (i2c: Add bus recovery infrastructure)
Signed-off-by: Mark Brown broo...@kernel.org
---
drivers/i2c/i2c-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 1672e6b12774..098f698fe8f4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -596,6 +596,7 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
adap-bus_recovery_info-set_scl(adap, 1);
return i2c_generic_recovery(adap);
}
+EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
{
@@ -610,6 +611,7 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
return ret;
}
+EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);
int i2c_recover_bus(struct i2c_adapter *adap)
{
@@ -619,6 +621,7 @@ int i2c_recover_bus(struct i2c_adapter *adap)
dev_dbg(adap-dev, Trying i2c bus recovery\n);
return adap-bus_recovery_info-recover_bus(adap);
}
+EXPORT_SYMBOL_GPL(i2c_recover_bus);
static int i2c_device_probe(struct device *dev)
{
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: master build: 2 failures 15 warnings (v4.0-5833-g6c373ca89399)
On Wed, Apr 15, 2015 at 08:59:53PM +0200, Wolfram Sang wrote: On Wed, Apr 15, 2015 at 07:08:25PM +0100, Mark Brown wrote: arm64-allmodconfig ../drivers/i2c/busses/i2c-jz4780.c:181:2: error: implicit declaration of function 'readw' [-Werror=implicit-function-declaration] ../drivers/i2c/busses/i2c-jz4780.c:187:2: error: implicit declaration of function 'writew' [-Werror=implicit-function-declaration] Where do these reports got to? I sadly missed them. kernel-build-repo...@lists.linaro.org is probably the best place to subscribe. signature.asc Description: Digital signature
Re: ARM: shmobile: koelsch: da9210/da9063 interrupt storm (was: Re: [PATCH 3/4] ARM: shmobile: koelsch: Add DA9063 PMIC device node for system restart)
On Tue, Feb 17, 2015 at 01:05:07PM +0100, Geert Uytterhoeven wrote: However, as soon as the da9210 driver will get interrupt support (I wrote something, based on the da9211 driver) and the da9210 will have an interrupts property in DTS, the interrupt storm will reappear, irrespectively of the order in which the two drivers are initialized. Well, there's another problem there - unless things changed since I last looked we don't support shared threaded IRQs. So far I see only two solutions: - Add platform code that matches against renesas,koelsch (and renesas,lager), and mask the interrupts in both the da9210 and da9063. This code has to run after the i2c master driver has been initialized, but before the i2c slave drivers are initialized. - Handle the masking in the bootloader (u-boot), which already knows how to reset the board by kicking the da9063. This requires everybody to upgrade his bootloader, though. Does anyone have a better solution? We could try to make the code that masks interrupts on error handle screaming shared interrupts a bit more gracefully during bootstrap by shutting things up a bit more aggressively and retrying when something else registers but that just feels like it's going to open up a particularly messy rabbit hole. Of the options you list above the platform code sounds the most palatable. Question: Is it really the default state of the regulator to not mask any interrupts? Or could this have been changed by the bootloader? I couldn't find code related to that in U-Boot, but as I didn't compile the bootloader myself, I can't be 100% certain about the source code. It wouldn't surprise me, having talked with designers of analogue chips they can at times have some innovative ideas about interaction with the processor and interrupts in particular. It's also possible that there's some transient condition that occurs during startup that causes the interrupts to be flagged. signature.asc Description: Digital signature
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On Tue, Feb 03, 2015 at 08:34:01PM +0200, Antti Palosaari wrote: On 02/03/2015 07:53 PM, Mauro Carvalho Chehab wrote: If latter a better way to lock the I2C mux appears, we can reverse this change. More I am worried about next patch in a serie, which converts all that to regmap API... Same recursive mux register access comes to problem there, which I work-arounded by defining own I2C IO... And in that case I used i2c_lock_adapter/i2c_unlock_adapter so adapter is locked properly. Opne coding the register I/O is a terrible solution, we should allow people to keep this code factored out. signature.asc Description: Digital signature
Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem
On Tue, Jan 20, 2015 at 12:14:31PM +0100, Paul Osmialowski wrote: On Mon, 19 Jan 2015, Mark Brown wrote: OK, so that's what should go in the changelog (along with an explanation of why this preparation is required at all) - but I still don't see the async bit of this I'm afraid. I don't think preparation stage should be exposed for asynchronous transfer. Due to its nature, it shouldn't cause circular lock dependency as we can observe for synchronous io. Since i2c does not support async transfer anyway (so (map-async map-bus-async_write) will be always false for i2c transfers), let's use spi as an example. Again I come back to explaining this out of the context of this particular issue in terms of something that's comprehensible at the regmap level. With spi we have curious situation where both sync and async are handled by the same __spi_async() function, though for sync transfer wait_for_completion() is called soon after __spi_async() in order to ensure that transfer is completed. That's not actually that strange really, in general synchronous operation can always be implemented as a simple wrapper around asynchronous operation. Actual transfer is handled by spi_transfer_one_message() called from spi_pump_messages(). Note that spi_pump_message() before it calls spi_transfer_one_message() also calls prepare_message() callback (if provided): We are *massively* into implementation details here, if we're relying on these things then they could change at any time (and in fact what you say above is partly specific and is not true even in the default case for current code). Think in terms of abstractions and locking guarantees rather than the details of the current code. signature.asc Description: Digital signature
Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem
On Sun, Jan 18, 2015 at 11:54:56AM +0100, Krzysztof Kozlowski wrote: W dniu 18.01.2015 o 07:30, Tomasz Figa pisze: So, the question is, do we actually have hardware that _really_ requires _actual_ preparation or all the clk_prepare_enable()s in I2C drivers (at least in i2c-s3c2410) are just to simplify the code? I completely forgot that you already thought about this deadlock in 2014. I think we can try the no-prepare way for i2c-s3c2410. However this would be only workaround for specific chip. Other buses (like SPI) would require similar changes. Right, and it's every single driver which would need an update too which is a bit icky and sad. On the other hand a more detailed fix is going to involve trying to make the clock API locking more fine grained which isn't fun. I wondered why this was not observed (at least not observed by me with lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14 PMIC provides regulators and 32kHz clocks. I think it is exactly the same pattern as for max77686... but somehow lockdep never reported that deadlock there. Mostly the clocks on PMICs never get changed at runtime which helps a lot here. signature.asc Description: Digital signature
Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem
On Fri, Jan 16, 2015 at 03:39:53PM +0100, Paul Osmialowski wrote:
This uses the enhancement of i2c API in order to address following problem
caused by circular lock dependency:
Please don't just dump enormous backtraces into commit messages as
explanations, explain in words what the problem you are trying to
address is. If the backtrace is longer than the commit message things
probably aren't working well, and similarly if the first thing the
reader sees is several screenfuls of backtrace that's not really aiding
understanding.
This is all particularly important with something like locking where a
clear understanding of the rules and assumptions being made is very
important to ensuring correctness, it's easy to just paper over a
specific problem and miss a bigger problem or introduce new ones.
Apparently regulator and clock try to acquire lock which is protecting the
same regmap. Communication over i2c requires clock to be started. Both things
require access to the same regmap in order to complete.
To solve this, i2c clock should be started before attempting operation on
regulator (which requires locked regmap).
It sounds awfully like something is not doing the right thing with
preparing clocks somewhere along the line but since there's no
analysis it's hard to tell (I don't propose to spend time trawling
backtraces for something I don't know).
Please also use blank lines between paragraphs like all the other commit
messages, it makes things easier to read.
Exposing preparation and unpreparation stage of i2c transfer serves this
purpose.
I don't know what this means, sorry. I'm also very worried about the
fact that this is being discussed purely in terms of I2C - why would
this not affect other buses?
Note that this change does not require modifications in other places.
Signed-off-by: Paul Osmialowski p.osmialo...@samsung.com
---
drivers/base/regmap/internal.h | 2 +
drivers/base/regmap/regmap-i2c.c | 18 +++
drivers/base/regmap/regmap.c | 107
++-
include/linux/regmap.h | 7 +++
4 files changed, 133 insertions(+), 1 deletion(-)
Modification may not be required in other places but this is an
*enormous* change in the code which I can't really review.
+ int (*reg_prepare_sync_io)(void *context);
+ void (*reg_unprepare_sync_io)(void *context);
The first question here is why this only affects synchronous I/O or
alternatively why these operations have _sync in the name if they aren't
for synchronous I/O.
+ if (bus) {
+ map-reg_prepare_sync_io = regmap_bus_prepare_sync_io;
+ map-reg_unprepare_sync_io = regmap_bus_unprepare_sync_io;
+ }
Why are we using these indirections instead of assigning the operation
directly? They...
+static int regmap_bus_prepare_sync_io(void *context)
+{
+ struct regmap *map = context;
+
+ if (map-bus-prepare_sync_io)
+ return map-bus-prepare_sync_io(map-bus_context);
+
+ return 0;
+}
...seem to simply check for the operation which appears redundant
especially given that the caller...
+static void regmap_unprepare_sync_io(struct regmap *map)
+{
+ void *context = _regmap_map_get_context(map);
+
+ if (map-reg_unprepare_sync_io)
+ map-reg_unprepare_sync_io(context);
+}
...does essentially the same check again on every call anyway.
@@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int
reg, unsigned int val)
if (reg % map-reg_stride)
return -EINVAL;
+ ret = regmap_prepare_sync_io(map);
+ if (ret)
+ return ret;
+
map-lock(map-lock_arg);
So what are the rules for calling this operation and how are they
different to those for locking the map? It looks like they might be the
same in which case it seems better to combine them rather than having
to update every single caller and remember why they're always being
worked with in tandem.
signature.asc
Description: Digital signature
Re: [RFC 3/3] i2c: s3c2410: Adopt i2c-s3c2410 driver for new enhancement of i2c API
On Fri, Jan 16, 2015 at 03:39:54PM +0100, Paul Osmialowski wrote: This adopts i2c-s3c2410 driver for new enhancement of i2c API that exposes preparation and unpreparation stages of i2c transfer. This doesn't seem to have any dependency on the previous patch at all... it probably does want a better commit log, probably the subject should say what new enhancement of the API is being adopted. (Use prepare/unprepare transfer for example.) signature.asc Description: Digital signature
Re: [PATCH] of: spi: Export single device registration method and accessors (v2)
On Wed, Oct 29, 2014 at 10:40:37AM +0200, Pantelis Antoniou wrote: Dynamically inserting spi device nodes requires the use of a single device registration method. Rework and export it. Methods to lookup a device/master using a device node are added as well, of_find_spi_master_by_node() of_find_spi_device_by_node(). Why do we need to do this - I would expect that adding nodes would trigger parsing in the same way we normally do it. Where is the user and how does it know that it's handling a SPI node? This feels like there is an abstraction problem somewhere, whatever code is supposed to use this is going to need to be taught about each individual bus which is going to be tedious, I would expect that we'd have something like the bus being able to provide a callback which will get invoked whenever a new node appears on the parent node for the bus. Changes since v1: * Brown paper bug with parameter on of_register_spi_device(). Don't include noise like this in the changelog, put it after --- like SubmittingPatches says. Please also try to keep your CC list sane, CCing random people just means that you're increasing the volume of mail they have to process. I'm surprised kernel.org accepts so many CCs. I have to say I don't recall ever seeing v1... signature.asc Description: Digital signature
Re: [PATCH] of: spi: Export single device registration method and accessors (v2)
On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: On Oct 29, 2014, at 12:14 , Mark Brown broo...@kernel.org wrote: This feels like there is an abstraction problem somewhere, whatever code is supposed to use this is going to need to be taught about each individual bus which is going to be tedious, I would expect that we'd have something like the bus being able to provide a callback which will get invoked whenever a new node appears on the parent node for the bus. There’s a whole patchset that does exactly this. Look at OF: spi: Add OF notifier handler” and you’ll where this is used. I deleted that unread I'm afraid; one of the reasons that you should use subject lines matching the styles for the subsystems is that it's one of the things people use to filter out things that actually need attention, if things are busy things that at first glance don't look terribly relevant (like changes to the OF core in this case) are likely to get looked at less urgently or just skipped. A quick glance suggests that this is adding code inside the SPI core so it's still not explaining why anything is being exported, can you clarify please? SubmittingPatches says. Please also try to keep your CC list sane, CCing random people just means that you're increasing the volume of mail they have to process. I'm surprised kernel.org accepts so many CCs. I have to say I don't recall ever seeing v1... All of them are in the CC list for a reason. This is a single, standalone SPI patch - you didn't send it as part of a series (which is the only reason I read it). signature.asc Description: Digital signature
Re: [PATCH 1/2] regmap: Add eplicit dependencies to catch select misuse
On Sun, Aug 17, 2014 at 12:08:57PM +0200, Geert Uytterhoeven wrote: Add explicit dependencies for the various regmap modules, so Kconfig will print a warning message when another module selects a regmap module without fulfilling its dependencies. Applied, thanks. The second patch is a bug fix independent of this one. signature.asc Description: Digital signature
Re: [PATCH/RFC V8 1/1] clk: Support for clock parents and rates assigned from device tree
On Fri, Jul 25, 2014 at 02:42:31PM -0700, Mike Turquette wrote: Quoting Sylwester Nawrocki (2014-07-03 10:25:53) I would appreciate a DT, SPI or the I2C maintainer opinions. Yes, Acks from SPI and I2C maintainers would be good. I might need to drop those parts of this patch if they don't come through :-( So, I just noticed this. The reason I didn't see this earlier is that it was buried at the end of a large clock API patch rather than a split out patch for the SPI subsystem so it fell towards the bottom of my review queue. There seems to be no dependency on adding the feature as part of one commit so it should really have been split out. Please don't assume that people are going to look in detail at patches that aren't obviously for them - I know I end up ignoring a lot of what I get sent since I get CCed on a lot of random things for no apparent reason (or get tediously large numbers of resends of things that are tangentially related), I imagine others do the same. If you're doing that please draw attention to it, splitting out per subsystem is the best way of doing that. This is particularly unfortunate since I am actually a bit confused as to why we need to open code this for every bus rather than doing it in the driver core, there doesn't seem to be anything at all bus specific going on here. Why can't we just call this from the driver core? Having the feature work for some buses and not others is going to get old fast. It's also not clear to me why we're passing false to of_clk_set_defaults() when we do call it - it's quite common for I2C and SPI devices to be clock providers and have clock trees. As far as I can tell the reasoning is that what's actually going on here is that this is actually a mechanism to defer the initialisation to adding of the clock providers in which case contrary to the documentation for the function it's actually about device registration. Is that right? signature.asc Description: Digital signature
Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board
On Fri, Jun 13, 2014 at 02:58:26PM -0700, Doug Anderson wrote: Anyway, suffice to say that the i2c core needs to be extended to handle the idea that a single device has more than one compatible string. I'll leave it to an eager reader of this thread to implement this since we can also fix our own problem by just listing max98091 in sound/soc/codecs/max98090.c like has always been done in the past. Why do you need to register multiple compatible strings (I guess for fallback purposes?). A quick fix that is about as good is to take the first compatible only. signature.asc Description: Digital signature
Re: [PATCH 7/7] OF/ACPI/I2C: Add generic match function for the aforementioned systems
On Thu, Jun 05, 2014 at 04:55:09PM +0100, Lee Jones wrote: On Thu, 05 Jun 2014, Grant Likely wrote: I still think the way to do it is to emulate the missing i2c_device_id when calling the drivers .probe() hook by having a temporary copy on the stack and filling it with data from the OF or ACPI table That's the opposite of what I'm trying to achieve. I'm trying to get rid of unused i2c_device_id tables, rather than reinforce their mandatory existence. I think an i2c_of_match_device() with knowledge of how to match via pure DT principles (of_node/compatible) and a fall-back, which is able to match on a provided of_device_id table alone i.e. without the requirement of an existing of_node. I've also been mulling over the idea of removing the second probe() parameter, as suggested by Wolfram. However, this has quite deep ramifications which would require a great deal of driver adaptions. If you're going to do that another option is to refactor the probe() function to take the driver_data as an argument and then have the core pass that from whatever table it matched from rather than the entire i2c_device_id structure. That way the driver just needs to supply all the ID tables mapping binding information to whatever it needs and the core can pass in the driver data from whatever table it matched against. signature.asc Description: Digital signature
Re: [PATCH] i2c: exynos5: Initialise Samsung High Speed I2C controller early
On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: On 24 April 2014 21:55, Mark Brown broo...@kernel.org wrote: Such solution has been proposed by Mark Brown to fix the problem of the regulators not beeing available on the peripheral device probe(): http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html What specifically is this needed for? We *should* be able to use deferred probe for most things, but I know that not all subsystems are able to yet. DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP during the boot. The real problem comes when, one of these drivers do a regulator_get(). If the physical supply is not enabled/hookedup the regulator_get() call assumes that physical supply is present and returns a dummy_regulator (But, not an error). Because of which, Display and several other devices fails to work. These drivers are buggy, if they geniunely expect and handle a missing supply then they should be using regulator_get_optional() to request the regulator and even if they don't the use of subsys_initcall() is not going to fix anything here - if a dummy regulator is going to be returned the time things are probed won't make a difference. I2C, I2C_TUNNEL, SPI and DMA drivers are required as subsys_initcall() for similar reason. What makes you say this? Typically those drivers don't use regulators at all. signature.asc Description: Digital signature
Re: [PATCH] i2c: exynos5: Initialise Samsung High Speed I2C controller early
On Fri, May 09, 2014 at 08:12:47PM +0530, Naveen Krishna Ch wrote: On 9 May 2014 19:21, Mark Brown broo...@kernel.org wrote: On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP during the boot. The real problem comes when, one of these drivers do a regulator_get(). If the physical supply is not enabled/hookedup the regulator_get() call assumes that physical supply is present and returns a dummy_regulator (But, not an error). Because of which, Display and several other devices fails to work. These drivers are buggy, if they geniunely expect and handle a missing supply then they should be using regulator_get_optional() to request the regulator and even if they don't the use of subsys_initcall() is not going to fix anything here - if a dummy regulator is going to be returned the time things are probed won't make a difference. ... If all the I2C, SPI, DMA, I2C_TUNNEL, DRM based LCD are all mod_probes() DRM drivers are probing a head of the PMIC probe. Which is causing the display drivers to get dummy_regulators instead of the real supplies. No, it really won't - I have no idea what you are doing but it's not mainline. If you are getting dummy regulators then you don't have a supply present at all and probe ordering isn't going to make a blind bit of difference. Please provide a specific technical description of the problem you are seeing in mainline. signature.asc Description: Digital signature
Re: [PATCH] i2c: exynos5: Initialise Samsung High Speed I2C controller early
On Thu, Apr 24, 2014 at 08:18:36PM +0530, Naveen Krishna Chatradhi wrote: This patch moves initialization code to subsys_initcall() to ensure that the i2c bus is available early so the regulators can be quickly probed and available for other devices on their probe() call. Such solution has been proposed by Mark Brown to fix the problem of the regulators not beeing available on the peripheral device probe(): http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html What specifically is this needed for? We *should* be able to use deferred probe for most things, but I know that not all subsystems are able to yet. signature.asc Description: Digital signature
Re: I2C dummy adapter driver ?
On Tue, Mar 04, 2014 at 12:38:28PM +0100, Sylwester Nawrocki wrote: On 28/02/14 07:07, Mark Brown wrote: On Fri, Feb 21, 2014 at 12:45:21PM +0100, Sylwester Nawrocki wrote: The I2C bus driver with empty i2c_algorithm.master_xfer() helps WRT to using standard DT binding and v4l2_subdev interface. Wouldn't a platform device do just as well here if there's no actual control? Then the I2C client devices would have to be instantiated manually, I think it's more trouble. I2C is not that much more enumerable than platform bus, I don't see the difference here? To the extent I2C is enumerable a dummy adaptor isn't going to support that. I could as well create custom I2C client drivers per ISP, but then the I2C devices would have to be represented somehow in DT, to pass stuff like voltage regulators and GPIOs. Anyway, it's not something could be done in mainline. Why not? Even if there is no actual I2C communication on the host CPU side, the power up/down sequence is handled there. The intention is to keep this common per an I2C client, regardless of whether I2C communication is done by firmware or the host the CPU. The way you're talking it sounds like your code is very hard coded to use I2C here. What happens if someone uses SPI or some other bus to control an ISP? signature.asc Description: Digital signature
Re: I2C dummy adapter driver ?
On Fri, Feb 21, 2014 at 12:45:21PM +0100, Sylwester Nawrocki wrote: The I2C bus driver with empty i2c_algorithm.master_xfer() helps WRT to using standard DT binding and v4l2_subdev interface. Wouldn't a platform device do just as well here if there's no actual control? signature.asc Description: Digital signature
Re: [PATCH 06/17] spi: pl022: Let runtime PM callbacks be available for CONFIG_PM
On Wed, Feb 19, 2014 at 03:20:07PM +0100, Ulf Hansson wrote: Also note that, Russell has already applied the corresponding part in the amba bus (patch 1) Why would this depend on an AMBA patch and if it does surely the two need to be merged together somehow? signature.asc Description: Digital signature
Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend
On Mon, Feb 10, 2014 at 01:33:50PM +0100, Ulf Hansson wrote: On 4 February 2014 20:16, Mark Brown broo...@kernel.org wrote: This seems like a fairly hideous thing to be having to open code in an individual driver, it all looks generic and like something that most if ... Putting it in a helper would at least mean that it's easier for the mechanics to be transferred to the core proper later on. I agree, a helper function would be nice. I have earlier sent a patch to the PM core, that is similar to the code above, though it was rejected in it's current form. Let me follow up on this again. At this point, would a way forward be to still go ahead with this patch and then convert to, when/if, the helper function from the PM core becomes available? It's definitely *a* way forward, but I'm not convinced it's a good way forward. Since it's something that I'd expect us to be doing in all drivers we'd want to replicate it all over the place which is obviously not good, or conversely if there are issues that prevented the code being added to the PM core then presumably we're just adding problematic code to the driver (you've not mentioned what the problems were with doing this in the PM core). signature.asc Description: Digital signature
Re: [PATCH 05/17] mmc: mmci: Put the device into low power state at system suspend
On Wed, Feb 05, 2014 at 01:49:49PM +0100, Linus Walleij wrote: On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman khil...@linaro.org wrote: I'm trying to thing of a good reason to not make PM_SLEEP depend on PM_RUNTIME for platforms like this. Isn't the typical Android platform using PM_SLEEP without using PM_RUNTIME? No, not at all. Android does make aggressive use of sleep but it's also highly desirable to use runtime PM - for example you don't want to have to power up the entire SoC simply because the system is getting a new e-mail pushed to it or location updates, most of the hardware is doing nothing. signature.asc Description: Digital signature
Re: [PATCH 3/6] mfd: add bcm59056 pmu driver
On Tue, Feb 04, 2014 at 09:31:19AM -0500, Matt Porter wrote:
On Tue, Feb 04, 2014 at 01:29:51PM +, Lee Jones wrote:
+static struct i2c_driver bcm59056_i2c_driver = {
+ .driver = {
+.name = bcm59056,
+.owner = THIS_MODULE,
+.of_match_table = of_match_ptr(bcm59056_of_match),
No need to use of_match_ptr() here.
Will remove.
What using of_match_ptr() should do is allow the compiler to figure out
that the table isn't used when DT is disabled and discard it without
ifdefs. Not sure if that actually works yet but that's the idea.
signature.asc
Description: Digital signature
Re: [PATCH 08/17] spi: pl022: Fully gate clocks at request inactivity
On Tue, Feb 04, 2014 at 04:58:49PM +0100, Ulf Hansson wrote: Use clk_disable_unprepare and clk_prepare_enable from the runtime PM callbacks, to fully gate|ungate clocks. Potentially this will save more power, depending on the clock tree for the current SOC. The same patch has already been applied (I noticed and fixed it while doing unrelated code review). pinctrl_pm_select_default_state(dev); - clk_enable(pl022-clk); + clk_prepare_enable(pl022-clk); Someone should really make this check errors though! signature.asc Description: Digital signature
Re: [PATCH 3/6] mfd: add bcm59056 pmu driver
On Tue, Feb 04, 2014 at 05:08:32PM +, Lee Jones wrote: What using of_match_ptr() should do is allow the compiler to figure out that the table isn't used when DT is disabled and discard it without ifdefs. Not sure if that actually works yet but that's the idea. Right, but I'm guessing that as there is no support for platform data then this device(s) is going to be DT only? If that's the case perhaps a 'depends OF' might be in order. If that's not the case then I'm more than happy to leave the of_match_ptr() in. Hey, we'll all be using ACPI soon! :P signature.asc Description: Digital signature
Re: [PATCH 2/6] regulator: add bcm59056 pmu DT binding
On Tue, Feb 04, 2014 at 07:19:08AM -0500, Matt Porter wrote: Add a DT binding for the BCM59056 PMU. The binding inherits from the generic regulator bindings. Is this really only a regulator - does the chip have no other functions? +- regulators: This is the list of child nodes that specify the regulator + initialization data for defined regulators. Generic regulator bindings + are described in regulator/regulator.txt. The regulators property should always be optional, the driver should be able to start up and read back the hardware state without any further configuration. signature.asc Description: Digital signature
Re: [PATCH 4/6] regulator: add bcm59056 regulator driver
On Tue, Feb 04, 2014 at 07:19:10AM -0500, Matt Porter wrote:
+static unsigned int bcm59056_get_mode(struct regulator_dev *dev)
+{
+ return REGULATOR_MODE_NORMAL;
+}
+
+static int bcm59056_set_mode(struct regulator_dev *dev, unsigned int mode)
+{
+ if (mode == REGULATOR_MODE_NORMAL)
+ return 0;
+ else
+ return -EINVAL;
+}
These do nothing, don't implement them.
+ if (bcm59056-dev-of_node)
+ pmu_data = bcm59056_parse_dt_reg_data(pdev,
+ bcm59056_reg_matches);
It'd seem a bit neater to put the OF check into the parse function but
meh.
+ if (!pmu_data) {
+ dev_err(pdev-dev, Platform data not found\n);
+ return -EINVAL;
+ }
Like I said when reviewing the binding this should not cause the driver
to fail to load.
+ /*
+ * Regulator API handles empty constraints but not NULL
+ * constraints
+ */
+ if (!reg_data)
+ continue;
It should do... if not then make it so since that'd mean most drivers
are buggy.
signature.asc
Description: Digital signature
Re: [PATCH 00/17] amba: PM fixups for amba bus and some amba drivers
On Tue, Feb 04, 2014 at 04:58:41PM +0100, Ulf Hansson wrote: The fixes for the amba bus needs to be merged prior to the other, thus I think it could make sense to merge this complete patchset through Russell's tree, if he and the other maintainers think this is okay. What are the actual dependencies for the SPI bit? AFAICT it's probably the first patch needs the bus updating? signature.asc Description: Digital signature
Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend
On Tue, Feb 04, 2014 at 04:58:48PM +0100, Ulf Hansson wrote:
@@ -2328,8 +2300,23 @@ static int pl022_suspend(struct device *dev)
return ret;
}
- pm_runtime_get_sync(dev);
- pl022_suspend_resources(pl022, false);
+ pm_runtime_disable(dev);
+
+ if (!pm_runtime_status_suspended(dev)) {
+ if (dev-pm_domain dev-pm_domain-ops.runtime_suspend)
+ ret = dev-pm_domain-ops.runtime_suspend(dev);
+ else
+ ret = dev-bus-pm-runtime_suspend(dev);
+
+ if (ret) {
+ pm_runtime_enable(dev);
+ return ret;
+ }
+
+ pm_runtime_set_suspended(dev);
+ }
This seems like a fairly hideous thing to be having to open code in an
individual driver, it all looks generic and like something that most if
not all devices ought to be doing and it looks very vulnerable to being
broken by changes in the core. At the very least I would expect this to
be done in a helper function, though it would be even nicer if the
driver core were figuring this stuff out for us based on the device
level callback so that drivers didn't need to worry about being in power
domains or manually calling bus level callbacks.
Putting it in a helper would at least mean that it's easier for the
mechanics to be transferred to the core proper later on.
signature.asc
Description: Digital signature
Re: [PATCH 09/17] spi: pl022: Simplify clock handling
On Tue, Feb 04, 2014 at 04:58:50PM +0100, Ulf Hansson wrote: Make use of clk_prepare_enable and clk_disable_unprepare to simplify code. No functional change. I went ahead and applied this since it looks good and seems like an unrelated cleanup to the runtime PM stuff which seems to be where the interdependencies are - thanks! It's on a separate branch with the already applied change so if it does need to be cross merged we can do that easily or even drop the branch. signature.asc Description: Digital signature
Re: [PATCH 2/6] regulator: add bcm59056 pmu DT binding
On Tue, Feb 04, 2014 at 04:16:38PM -0500, Matt Porter wrote: On Tue, Feb 04, 2014 at 05:23:09PM +, Mark Brown wrote: Is this really only a regulator - does the chip have no other functions? It's your average multi-function device with other functions as you are suspecting. Buried in the the MFD driver comments is me admitting that I need to split this into two bindings. The base device, bcm59056 and then bcm59056-reg. So point noted, I'll updated with the appropriate binding for each. It doesn't need to be two bindings - just move it to the MFD section and document it there. The existing binding is totally fine from a regulator standpoint and should continue to be so as other functions are added. signature.asc Description: Digital signature
Re: [PATCH 4/6] regulator: add bcm59056 regulator driver
On Tue, Feb 04, 2014 at 04:29:14PM -0500, Matt Porter wrote: On Tue, Feb 04, 2014 at 05:28:36PM +, Mark Brown wrote: + /* + * Regulator API handles empty constraints but not NULL + * constraints + */ + if (!reg_data) + continue; It should do... if not then make it so since that'd mean most drivers are buggy. Ahh, I see there is a check for NULL in the core. Will drop. It was missing in some older kernels (much older now IIRC) - it's possible that comment was written before this got fixed. signature.asc Description: Digital signature
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Fri, Jan 17, 2014 at 09:37:56AM +0200, Jarkko Nikula wrote: On 01/16/2014 09:46 PM, Mark Brown wrote: This is needed for ACPI because we rewrite the device names to give us stable names. With OF for I2C and SPI nothing bus specific is done that affects this stuff so the default behaviour works. Sidenote: actually this modalias/module loading issue is different and not related to stable ACPI i2c/spi slave device names. Oh, I'd been under the impression that it was the rewrite that was triggering this? signature.asc Description: Digital signature
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote: ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. Currently, in Platform, I2C and SPI bus, the ACPI style driver matching is supported by invoking acpi_driver_match_device() in bus .match() callback. But, the module autoloading is still broken. Acked-by: Mark Brown broo...@linaro.org modulo the PAGE_SIZE stuff Mika noted - unless this changes radically please just assume I'm OK with it. signature.asc Description: Digital signature
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Thu, Jan 16, 2014 at 09:05:09PM +0800, Zhang Rui wrote: On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote: I wonder why we don't have/need that with CONFIG_OF? Because probably nobody is using modules with i2c devices there? This seems a gap to me but I'm not 100% sure. I saw Grant Likely introduced the OF style MODALIAS to platform bus, and OF style registration/binding to i2c bus, maybe he has an answer for this. This is needed for ACPI because we rewrite the device names to give us stable names. With OF for I2C and SPI nothing bus specific is done that affects this stuff so the default behaviour works. signature.asc Description: Digital signature
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote: On Mon, 2014-01-13 at 17:35 +, Mark Brown wrote: On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote: ACPI enumerated devices has ACPI style _HID and _CID strings, all of these strings can be used for both driver loading and matching. If this piece of code is used in an *SPI* driver for an ACPI enumerated spi device, the spi driver module_alias is acpi:INTABCD, but the uevent of its spi device node is spi:INTABCD (PREFIX:spi_device-modalias). OK that makes sense, but what does this have to do with the _HID and _CID methods? Surely we're just replacing spi: with acpi: in the uevent? signature.asc Description: Digital signature
Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices
On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
ACPI enumerated devices has ACPI style _HID and _CID strings,
all of these strings can be used for both driver loading and matching.
But currently, in Platform, I2C and SPI bus, only the ACPI style
driver matching is supported by invoking acpi_driver_match_device()
in bus .match() callback.
I don't understand what this means, sorry. As far as I can tell ACPI
style _HID and _CID strings are something different to ACPI style
driver matching but what that actually means is not at all clear to me
so I don't know what problem this is intended to fix.
Please also always remember to CC maintainers on patches.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
modalias_show(struct device *dev, struct device_attribute *a, char *buf)
{
const struct spi_device *spi = to_spi_device(dev);
+ int len;
+
+ len = acpi_device_modalias(dev, buf, PAGE_SIZE);
+ if (len != -ENODEV)
+ return len;
return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias);
}
What does this do and why can't acpi_driver_match_device() handle this
like it does for other ACPI devices? We don't need to add such code for
other firmware interfaces...
signature.asc
Description: Digital signature
Re: [PATCH] I2C: BCM2835: Linking platform nodes to adapter nodes
On Tue, Nov 26, 2013 at 01:05:53PM +, Charles Keepax wrote: On Fri, Nov 08, 2013 at 09:59:28AM -0700, Stephen Warren wrote: That all said, I wonder if the I2C core shouldn't do something like the following inside i2c_add_adapter(): if (!adap-dev.of_node adap-dev.parent) adap-dev.of_node = adap-dev.parent-of_node; Should this not also have an of_node_get to increment the ref count on the node? Given that it's pointing to the device that registered the I2C adaptor it should be able to assume that there's a reference already. signature.asc Description: Digital signature
Re: [PATCHv3 3/3] spi: Use stable dev_name for ACPI enumerated SPI slaves
On Thu, Nov 14, 2013 at 11:01:01AM +0200, Jarkko Nikula wrote: Current spi bus_num.chip_select spix.y based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. Acked-by: Mark Brown broo...@linaro.org signature.asc Description: Digital signature
Re: [RFC 1/2] i2c: Use stable dev_name for ACPI enumerated I2C slaves
On Mon, Oct 28, 2013 at 03:15:25PM +0200, Jarkko Nikula wrote: One possible thing to do is to let structure definitions to be available for non-ACPI builds. Then compiler won't fail on structure access which will be anyway optimized away by later compiler stages. You could also have inline accessor functions which can be stubbed out when not needed. signature.asc Description: Digital signature
Re: [RFC 2/2] spi: Use stable dev_name for ACPI enumerated SPI slaves
On Fri, Oct 25, 2013 at 03:19:00PM +0300, Jarkko Nikula wrote: Current spi bus_num.chip_select spix.y based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem. I'm happy with this from the SPI side modulo Raphael's comments - are folks happy from the ACPI side? signature.asc Description: Digital signature
Re: [PATCH 3/3] spi: attach/detach SPI device to the ACPI power domain
On Thu, Oct 10, 2013 at 09:12:56AM +0300, Mika Westerberg wrote: On Wed, Oct 09, 2013 at 06:55:28PM +0100, Mark Brown wrote: On Wed, Oct 09, 2013 at 05:04:21PM +0300, Mika Westerberg wrote: + if (ACPI_HANDLE(spi-dev)) + acpi_dev_pm_attach(spi-dev, true); Though I do wonder if it wouldn't be sensible to push the if () here inside acpi_dev_pm_attach() and similarly for _detach(). Terribly trivial either way. Actually, the check is already there in acpi_dev_pm_attach()/detach(). The above code follows what Rafael did for platform bus previously. I think the idea is to have visual hint that this is only for ACPI enumerated devices. If preferred, I can drop the if() checks, though. It'd seem neater - the fact that the function is acpi_ ought to be enough of a hint. signature.asc Description: Digital signature
Re: [PATCH v4] i2c: enable runtime PM for I2C adapter devices enumerated from ACPI
On Sat, Oct 05, 2013 at 11:09:01AM +0300, Mika Westerberg wrote: It looks like Windows actually powers the I2C controller off independently of the I2C client power state. We should probably do the same in Linux even though it is not following what the ACPI spec says (but makes sense with serial buses like I2C and SPI). Sounds like it's probably worth going to the effort of getting the spec amended... signature.asc Description: Digital signature
Re: [PATCH] i2c: s3c2410 : Add polling mode support
On Tue, Oct 01, 2013 at 04:03:40PM +0530, Yuvaraj Kumar C D wrote:
+static bool is_ack(struct s3c24xx_i2c *i2c)
+{
+ u32 time_out = i2c-tx_setup;
+
+ while (--time_out) {
+ if (readl(i2c-regs + S3C2410_IICCON)
+ S3C2410_IICCON_IRQPEND) {
+ if (!(readl(i2c-regs + S3C2410_IICSTAT)
+ S3C2410_IICSTAT_LASTBIT))
+ return true;
+ }
+ udelay(time_out);
+ }
+
+ return false;
This is a bit weird - the amount of time the driver waits between polls
shrinks as the timeout approaches. It'd be more normal to see either an
even period between polls or (ideally if the delay is non-trivial) a
dead reckoning sleep based on the expected time to complete followed by
polling at even intervals.
Also consider usleep_range() for the delay, it's a bit nicer to the rest
of the system than udelay().
signature.asc
Description: Digital signature
Re: [PATCH 1/4] driver core: introduce helper macro initcall_driver()
On Mon, Sep 30, 2013 at 01:13:52PM +0800, Hanjun Guo wrote: For some devices especially on platform/I2C/SPI bus, they want to be initialized earlier than other devices, so the driver use initcall such as subsys_initcall to make this device initialize earlier. We're trying to move away from needing to do this and to using deferred probing to resolve init ordering issues. Should we not be able to convert the drivers to module_X_driver()? signature.asc Description: Digital signature
Re: [PATCH 7/8] ASoC: imx-wm8962: Don't use i2c_client-driver
On Sun, Sep 29, 2013 at 10:51:05AM +0200, Lars-Peter Clausen wrote: The 'driver' field of the i2c_client struct is redundant and is going to be removed. Check i2c_client-dev.driver instead to see if a driver is bound to the device. Acked-by: Mark Brown broo...@linaro.org signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 17, 2013 at 03:25:25AM +0200, Rafael J. Wysocki wrote: On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote: It shouldn't even need to do that, it should just be able to rely on the controller to power itself up when asked to do work. This is how the existing implementations are done - the controller power management is totally transparent to the slave. If both the I2C client and I2C controller have corresponding objects in the ACPI namespace and the client's object is a child of the controller's object, then in order to power up the client we need to power up the controller even if no transactions are going to be carried out. That's what the spec simply requires us to do in that case. Like I said I think this should be handled by the power domains (or otherwise in the ACPI specific code) - we shouldn't be needing to modify individual drivers to work around thoughtlessness in the ACPI spec. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Sun, Sep 15, 2013 at 04:28:24PM +0300, Mika Westerberg wrote: On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote: On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote: 1. In I2C core i2c_device_probe() we power on the I2C controller and attach the client device to the ACPI power domain. Just like in this patch but we don't touch the I2C client device runtime PM. - This should allow the existing drivers to keep using whatever runtime PM strategy they have chosen. There should be no explicit need to power on the I2C controller if it's implemented the same way the existing ones are - just have it power itself on when it is doing a transfer. The problem is that the ACPI child device can't be in higher power state than its parent (and this is also what the runtime PM expects). If we don't power the I2C controller device before we power on the I2C client device that rule is violated and we get an complaint to the console. That's definitely an ACPI specific (probably x86 specific ACPI?) requirement not a generic one, on some systems it would increase power consumption since the controller will need to sit on while the device is functioning autonomously. Even though the controller power consumption is going to be minimal the power domain it is in may be relatively large. Can't the power domains for ACPI deal with this requirement, for example by making the I2C slave power domains children of the controller power domain? signature.asc Description: Digital signature
Re: [PATCH 1/4 v2] mfd: add STw481x driver
On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
I've tried to fix this for DT-only I2C devices
and this very driver was the reason.
But a tiresome regression due to drivers relying on this
i2c_device_id not being NULL and inability to remove it from the I2C
core without refactoring the world ensued, see:
commit c80f52847c50109ca248c22efbf71ff10553dca4
Oh, that was the change...
Reverted in:
commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
For this reason I think:
http://marc.info/?l=linux-nextm=137148411231784w=2
I have tentatively given up getting pure DT I2C drivers
to probe, I don't think I have the whole picture, but
Wolfram has serious doubts about this and say we have
to be careful
Wolfram, do you have some ideas on how we should
proceed or ar you happy with merging this as-is?
I'd have expected that it should be possible to change things such that
the change in the core doesn't produce any change in behaviour for
existing drivers. Can we not change the patch so that i2c_match_id()
copes with getting a NULL id_table? Something like this:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..61087ea 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -67,6 +67,9 @@ static int i2c_detect(struct i2c_adapter *adapter, struct
i2c_driver *driver);
static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
+ if (!id)
+ return NULL;
+
while (id-name[0]) {
if (strcmp(client-name, id-name) == 0)
return id;
@@ -92,11 +95,8 @@ static int i2c_device_match(struct device *dev, struct
device_driver *drv)
return 1;
driver = to_i2c_driver(drv);
- /* match on an id table if there is one */
- if (driver-id_table)
- return i2c_match_id(driver-id_table, client) != NULL;
- return 0;
+ return i2c_match_id(driver-id_table, client) != NULL;
}
@@ -246,7 +246,7 @@ static int i2c_device_probe(struct device *dev)
return 0;
driver = to_i2c_driver(dev-driver);
- if (!driver-probe || !driver-id_table)
+ if (!driver-probe)
return -ENODEV;
client-driver = driver;
if (!device_can_wakeup(client-dev))
signature.asc
Description: Digital signature
Re: [PATCH 1/4 v2] mfd: add STw481x driver
On Mon, Sep 16, 2013 at 05:05:37PM +0100, Lee Jones wrote: So in the mean time are you happy with this dummy approach? No. dummy is reserved for a dummy device in case an i2c slave needs more than one address. The proper solution would be if i2c_sysfs_new_device() could recognize the of_device_ids. Okay, thanks for clarifying. Or for now to use something like stw481x (even if it's never expected to actually make a difference). signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote: That may be left to the client driver altogether. I mean, if the client wants the controller to be powered up, it should just call pm_runtime_get_sync(controller device) at a suitable place (and then do the corresponding _put when the controller is not necessary anu more) from its -probe() callback. It shouldn't even need to do that, it should just be able to rely on the controller to power itself up when asked to do work. This is how the existing implementations are done - the controller power management is totally transparent to the slave. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote: On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote: For hardware that is disabled/powered-off on startup, there will now be a mismatch between the hardware state an the RPM core state. The call to pm_runtime_get_noresume() should make sure that the device is in active state (at least in state where it can access the bus) if I'm understanding this right. Accessing the bus isn't an issue for I2C outside of ACPI, the power management of the device is totally disassociated from the bus and the controller is responsible for ensuring it is available during transfers. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 09:14:20AM +0800, Aaron Lu wrote: On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote: So there is currently no way to avoid this behaviour, i.e. to have the adapter not activated before any of its client devices is probed, but only later on, after explicit call to pm_runtime_get*(client-dev) in the client driver ? The above pm_runtime_get_sync is used to make sure when the client I2C device is going to be probed, its host adapter device is turned on(or we will fail the probe). It doesn't affect the adapter's status before the probe of I2C client device. The expecation is that if the adaptor needs to do anything to transfer it'll do that when asked to transfer - that way it can sit in a low power state when the bus is idle. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote: On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote: Accessing the bus isn't an issue for I2C outside of ACPI, the power management of the device is totally disassociated from the bus and the controller is responsible for ensuring it is available during transfers. Yes, but since we want to support ACPI as well, we must make sure that the adapter (and the associated controller) is available when client -probe() is called. Right, but this probably needs to be highlighted more since it's a very surprising thing for I2C and is causing confusion. signature.asc Description: Digital signature
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Fri, Sep 13, 2013 at 02:50:35PM +0300, Mika Westerberg wrote: On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote: Right, but this probably needs to be highlighted more since it's a very surprising thing for I2C and is causing confusion. By highlighted more, do you mean something like adding a comment in the code about this or? Perhaps, yes. Or possibly the commit log is going to be enough going forwards. signature.asc Description: Digital signature
Re: [PATCH v2 8/9] spi: prepare runtime PM support for SPI devices
On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote: On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote: I would be able to have this and the other patch in the SPI tree in case it overlaps with other work - I'm not sure what the plan will be for merging this stuff but if there were a branch which I could merge into the SPI tree that'd be good. I think these two can go via your SPI tree as they shouldn't have dependencies to the I2C tree. There's all the driver changes though - it seems best to push the whole series through one branch so there's fewer bisection problems. signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Thu, Sep 12, 2013 at 02:07:48PM -0700, Kevin Hilman wrote: IMO, this decision belongs to the PM domain, not to the core. We have an established legacy with the current core default (auto) and changing that means lots of breakage. Yup. The forbid by default can just as easily be handled in the PM domain for the group of devices that need it, so why not do it there? Or at the device level - I'd guess most I2C devices won't end up in a domain outside of ACPI. Mika's latest version of the patches address this issue, the default is left alone. signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Wed, Sep 11, 2013 at 09:01:16AM +0800, Aaron Lu wrote: Looks like, it all boils down to how many I2C devices should be allowed for runtime PM by default and how many I2C devices should be forbidden. , and then we allow/forbid runtime PM for the majority case in I2C core while individual driver can still forbid/allow it in their own code. So if the majority case is runtime PM should be allowed by default, I'm also OK to not forbid runtime PM for I2C client device in I2C core. My original intention to forbid runtime PM by default is to make sure no adverse effect would occur to some I2C devices that used to work well before runtime PM. The really big problem here is that there are I2C devices currently using runtime PM quite happily and forbidding it by default will break them. In general though requiring userspace to manually activate power saving features isn't going to make people happy. signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Wed, Sep 11, 2013 at 02:05:43PM +0300, Mika Westerberg wrote: I'll also look into converting the existing I2C client drivers to use this method. One question, though, is it better to have the conversion in the same patch that introduces the I2C core runtime PM change or as a separate patch? In theory it ought to be part of the same patch but in practice a brief bit of bisection breakage on a single branch is probably worth the ease of review from my point of view, others may disagree though. Like I say I think you'll need to convert SPI at the same time due to the devices with both buses sharing code. signature.asc Description: Digital signature
Re: [PATCH v2 7/9] ASoC: codecs: convert existing I2C client drivers to use I2C core runtime PM
On Wed, Sep 11, 2013 at 06:32:38PM +0300, Mika Westerberg wrote: The I2C core now prepares runtime PM on behalf of the I2C client device, so only thing the driver needs to do is to call pm_runtime_put() at the end of its -probe(). This patch converts ASoC codec drivers to use this model. Acked-by: Mark Brown broo...@linaro.org signature.asc Description: Digital signature
Re: [PATCH v2 8/9] spi: prepare runtime PM support for SPI devices
On Wed, Sep 11, 2013 at 06:32:39PM +0300, Mika Westerberg wrote: This patch adds runtime PM support for the SPI bus analogous to what has been done for the I2C bus. This means that the SPI core prepares runtime PM for a client device just before a driver is about to be bound to it. Devices that are not bound to any driver are not prepared for runtime PM. Acked-by: Mark Brown broo...@linaro.org I would be able to have this and the other patch in the SPI tree in case it overlaps with other work - I'm not sure what the plan will be for merging this stuff but if there were a branch which I could merge into the SPI tree that'd be good. signature.asc Description: Digital signature
Re: [PATCH v2 0/9] runtime PM support for I2C and SPI client devices
On Wed, Sep 11, 2013 at 06:32:31PM +0300, Mika Westerberg wrote: Hi, This is second version of the patches. The previous version can be found here: http://www.spinics.net/lists/linux-i2c/msg13152.html Looks good to me now: Reviwed-by: Mark Brown broo...@linaro.org for what it's worth. signature.asc Description: Digital signature
Re: [PATCH v2 9/9] spi: attach/detach SPI device to the ACPI power domain
On Wed, Sep 11, 2013 at 06:32:40PM +0300, Mika Westerberg wrote: If the SPI device is enumerated from ACPI namespace (it has an ACPI handle) it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx). Acked-by: Mark Brown broo...@linaro.org signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 10, 2013 at 10:51:00AM +0300, Mika Westerberg wrote: On Mon, Sep 09, 2013 at 04:40:28PM +0100, Mark Brown wrote: How is this going to interact with client devices which are already enabling runtime PM for themselves, and what are the advantages of doing this over having the client device enable runtime PM for itself (given that the client still needs an explicit put() adding)? My understanding is that you can call pm_runtime_enable() several times (provided that pm_runtime_disable() is called as many times). So that should have no effect on the current drivers that already take advantage of runtime PM. Not quite... There is one difference though -- runtime PM is now blocked by default and it needs to be unblocked from the userspace per each device. ...as you say. This seems crazy, why on earth would we want to have userspace be forced to manually go through every single device and manually enable power saving? I can't see anyone finding that helpful and it's going to be a pain to deploy. However I had thought it was just a case of the drivers doing a put() instead of their current code to enable runtime PM (you mention that later on)? That seems both sensible and doable, though it would need doing as part of the conversion to avoid regressions and I'd expect it does mean that SPI needs to be converted at the same time. For the advantages compared to each driver handling it completely themselves: - Few lines less as you only need to call _put(). - It follows what is already been done for other buses, like PCI and AMBA . - The I2C core makes sure that the device is available (from bus point of view) when the driver -probe() is called. I can't understand your last point here at all, sorry. Can you expand please? Given that it's relatively common for devices to have both I2C and SPI control it seems like it'd be sensible to keep the policy common between the two buses to simplify driver implementation. Yes and IMHO if I2C and SPI follows what has already been done for other buses it should make the driver writer's job easier as the usage is similar from one bus to another. So then the obvious followup question is why this is even something that needs to be implemented per bus? Shouldn't we be enhancing the driver core to do this, or is that the long term plan and this is a step on the road to doing that? signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 10, 2013 at 05:26:31PM +0300, Mika Westerberg wrote: On Tue, Sep 10, 2013 at 01:27:54PM +0100, Mark Brown wrote: There is one difference though -- runtime PM is now blocked by default and it needs to be unblocked from the userspace per each device. ...as you say. This seems crazy, why on earth would we want to have userspace be forced to manually go through every single device and manually enable power saving? I can't see anyone finding that helpful and it's going to be a pain to deploy. There are things like HID over I2C devices (e.g touch screen) where going to lower power states too aggressively makes the touch experience really sluggish. However, other HID over I2C devices like sensor hubs it doesn't matter that much. In order to get the best performance we have runtime PM blocked by default (and leave it up to the user to unblock to get power savings). That's basically what PCI drivers currently do. So those specific devices need to implement runtime PM in an appropriate fashion. That's no need to implement a poor default for every single device to work around poor implementations from some, especially when it requires a userspace update to get acceptable performance again from the unaffected devices. However I had thought it was just a case of the drivers doing a put() instead of their current code to enable runtime PM (you mention that later on)? User still needs to unblock runtime PM for the device. The driver can call the runtime PM functions but they don't have any effect until runtime PM is unblocked by the user. However, I don't have problems dropping the call to pm_runtime_forbid() in this patch and leave it up to the user to decide whether runtime PM should be blocked for the device. I think this is essential - we can't really go around forcing userspace updates to restore runtime power management, nobody is going to thank us for that and it sounds like the issue you're trying to solve is device specific anyway. - The I2C core makes sure that the device is available (from bus point of view) when the driver -probe() is called. I can't understand your last point here at all, sorry. Can you expand please? Sorry about that. At least with ACPI enumerated I2C client devices, they might be powered off by the BIOS (there are power resources attached to the devices). So when the driver -probe() is called we can't access the device's registers etc. So we bind the device to the ACPI power domain (the second patch in this series) and then call pm_runtime_get() for the device. That makes sure that the device is accessible when -probe() is called. OK, that is very much not the model which embedded systems follow, in embedded systems the driver for the device is fully in control of its own power. It gets resources like GPIOs and regulators which allow it to make fine grained decisions. So then the obvious followup question is why this is even something that needs to be implemented per bus? Shouldn't we be enhancing the driver core to do this, or is that the long term plan and this is a step on the road to doing that? If we end up all buses implementing the same mechanism, I think it makes sense to move it to the driver core. If we're starting to get a reasonable number of buses following the same pattern it seems like we're in a position to start signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote: On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote: OK, that is very much not the model which embedded systems follow, in embedded systems the driver for the device is fully in control of its own power. It gets resources like GPIOs and regulators which allow it to make fine grained decisions. There are platforms where those resources are simply not available for direct manipulation and we need to use ACPI methods for power management. Now, since those methods are used in pretty much the same way for all I2C devices, we add a PM domain for that to avoid duplicating the same code in all of the drivers in question (patch [2/2]). Does that make sense to you? It doesn't seem like a particular problem, but the existing usage does need to be preserved for the systems that use it so things like having auto as the default and updating the drivers seem like they're needed. If we're starting to get a reasonable number of buses following the same pattern it seems like we're in a position to start We need that for exactly 3 buses: platform (already done), I2C and SPI. No other bus types are going to use ACPI this way for PM, at least for the time being, simply because PCI, USB and SATA/IDE have their own ways of doing this (which are bus-specific) and the spec doesn't cover other bus types directly (it defines support for UART, but we don't have a UART bus type). Moreover, because PCI and USB use ACPI for PM in their own ways, moving that thing up to the driver core would be rather inconvenient. That only applies to the power domains though, what Mika was saying was that the process for enabling runtime PM (just drop a reference) also becomes easier with this method regardless of anything else - that makes sense to me as something we might want to end up with so do we want to just move towards making that a default? signature.asc Description: Digital signature
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
On Mon, Sep 09, 2013 at 04:34:38PM +0300, Mika Westerberg wrote: + /* + * Enable runtime PM for the client device. If the client wants to + * participate on runtime PM it should call pm_runtime_put() in its + * probe() callback. + * + * User still needs to allow the PM runtime before it can actually + * happen. + */ + pm_runtime_forbid(client-dev); + pm_runtime_get_noresume(client-dev); + pm_runtime_set_active(client-dev); + pm_runtime_enable(client-dev); How is this going to interact with client devices which are already enabling runtime PM for themselves, and what are the advantages of doing this over having the client device enable runtime PM for itself (given that the client still needs an explicit put() adding)? Given that it's relatively common for devices to have both I2C and SPI control it seems like it'd be sensible to keep the policy common between the two buses to simplify driver implementation. signature.asc Description: Digital signature
Re: passing two interrupts two an I2C driver
On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote: If the platform data used to carry the (custom) irq data, the DT-powered driver could interrogate the DT on is own, couldn't it? Of course there should be some helper available, maybe something of that sort? (warning, untested) Yes, that's probably the most straightforward thing - we'd need to either have the bindings specify which interrupt must be first for reading i2c-irq or just have the drivers always do a name based lookup if there's more than one interrupt. signature.asc Description: Digital signature
Re: passing two interrupts two an I2C driver
On Thu, Aug 22, 2013 at 12:44:04PM +0100, Pawel Moll wrote: On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote: Yes, that's probably the most straightforward thing - we'd need to either have the bindings specify which interrupt must be first for reading i2c-irq or just have the drivers always do a name based lookup if there's more than one interrupt. ... or make sure that of_i2c_register_devices() does *not* set i2c-irq (or rather: set it to 0) when there is more than one interrupt in the tree... Yes, that'd be a really good idea if the drivers are always getting the interrupts by name, provides a backup. signature.asc Description: Digital signature
Re: passing two interrupts two an I2C driver
On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote: So let me ask such question... If Device Tree didn't exist, how would you make drive such device? I guess it would require some custom code, It's always done using platform data, same for SPI - if we update one we should probably update both. signature.asc Description: Digital signature
Re: MTD EEPROM support and driver integration
On Mon, Jul 08, 2013 at 10:25:38PM +0200, Maxime Ripard wrote: On Mon, Jul 08, 2013 at 09:34:26AM +0100, Mark Brown wrote: I'd really like to see more discussion of this DT parsing code for regmap idea... I've missed almost all the context here. The context was that I found we lack a way to simply express the need for one driver to get a value from an EEPROM-like device, for example to get a MAC Address, or a serial number, in a generic way, without having to poke directly with some custom function that would be exported by the EEPROM driver. This sort of information is often stored in places like flash partitions too. Are we sure that regmap is a good place to be hooking in here? The use case is sane, and being able to use regmap to do some of it seems sensible (I've seen people use OTP in PMICs for similar purposes) but perhaps an additional layer of abstraction on top makes sense. What we've been discussing so far is that: - To have a common framework we could base our work on, we could move the EEPROM drivers from drivers/misc/eeprom to MTD - To declare the ranges that needed to be used by a driver that was needing a value from one of those MTD drivers, we would use regmap with a MTD backend - And since we actually need to declare which ranges and in which device one driver would have to retrieve this value from, we were actually in need of DT bindings. This is pretty much the only context involved, and we are at the early stage of the discussion, so any comment is very welcome :) If this stuff is being represented in MTD doesn't MTD already have adequate abstractions for saying this region in flash. But otherwise this seems fine, it's not a generic regmap DT binding but instead rather more specific than that. signature.asc Description: Digital signature
Re: MTD EEPROM support and driver integration
On Sat, Jul 06, 2013 at 09:06:49PM +0200, Arnd Bergmann wrote: On Saturday 06 July 2013 14:01:12 Maxime Ripard wrote: a) like interrupts, regs, dmas, clocks, pinctrl, reset, pwm: fixed property names regmap = at25 0xstart 0xlen; regmap-names = mac-address; b) like gpio, regulator: variable property names mac-storage = at25 0xstart 0xlen; It's unfortunate that we already have examples of both. They are largely equivalent, but the tendency is towards the first. I don't have a strong feeling for one against another, so whatever works best. Both solutions will be a huge improvement anyway Just out of curiosity, is there any advantages besides having a fixed property name to the first solution? I think it's mostly for consistency: trying to get most subsystems to do it the same way to make it easier for people to write dts files. A lesser point is that it simplifies the driver code if you don't have to pass a name. On the other hand something with human readable names is much more legible if humans ever have to read or write the DT bindings. This mostly applies when there are many instances of the property (for example, many devices have lots of power supplies) or when some instances of the property are optional (for example, many devices can use GPIOs for many different functions but usually not all of them are connected and there's no particular order in which they might get connected). So that leave us with mainly one path to achieve this goal: - Add a regmap-mtd backend - Add DT parsing code for regmap - Move the EEPROM drivers from misc to mtd Yes, I think that would be good. For the last step, we definitely need buy-in from Wolfgand and Jean, as they are maintaining the current eeprom drivers. I'd really like to see more discussion of this DT parsing code for regmap idea... I've missed almost all the context here. We also have a bunch of OTP drivers spread around the kernel, it probably makes sense to consolidate them at the same time, at least on the DT binding side if not the device drivers. I'm not sure how viable this is, the OTP interfaces aren't that consistent and are frequently embedded in random PMICs or whatever. signature.asc Description: Digital signature
Re: i2c: introduce i2c helper i2c_find_client_by_name()
On Thu, Jun 06, 2013 at 11:33:46AM -0700, Bin Gao wrote: A good example is that an ISP(Imaging Signal Processor) driver needs register i2c camera sensor devices via v4l2, so it has to unregister all i2c clients that were previously registered by calling i2c_register_board_info(), and then re-register. For this case we can use this helper to get i2c_client by passing the client name. Please look at the work that IIRC Laurent Pichart (CCed) was doing on developing generic DT bindings for v4l in embedded systems, it sounds like you're working on similar hardware here. The general ideas should apply to any enumeration mechanism, not just DT. signature.asc Description: Digital signature
Re: [PATCH] regulator: tps51632: Get regulator name from i2c_client
On Tue, Jun 18, 2013 at 01:30:13PM +0300, Mikko Perttunen wrote: Commit i2c: core: make it possible to match a pure device tree driver changed semantics of the i2c probing for device tree devices. Device tree probed devices now get a NULL i2c_device_id pointer. This causes the regulator name to be set to NULL and the regulator registration to fail. Applied, thanks. Though it does seem like we should just be hard coding this string anyway... signature.asc Description: Digital signature
Re: [PATCH] i2c: sirf: move driver init from module_init to subsys_initcall
On Tue, Jun 11, 2013 at 09:14:41AM +0800, Barry Song wrote: the mainline idea you mentioned is that you don't care about any early device, which means some devices want to start earlier than others in some real automative user scenerioes. but i think another important idea is that mainline codes come from branches of different vendors and serve final users of those drivers, but not only make source codes no difference to all environments. the auto users i2c-sirf serves, here, actually means some differences with PC/tablet/mobilephone. we don't make codes good-looking by losing functionality and not close to final users. It's not that people don't care about these users, it's that people don't care too much about out of tree users. Part of the goal here is to convince people working on such applications that they should make mainline better for everyone so that you don't need external code to make the kernel useful. signature.asc Description: Digital signature
Re: [PATCH] i2c: sirf: move driver init from module_init to subsys_initcall
On Tue, Jun 11, 2013 at 07:13:33PM +0800, Barry Song wrote: 2013/6/11 Mark Brown broo...@kernel.org: It's not that people don't care about these users, it's that people don't care too much about out of tree users. Part of the goal here is to convince people working on such applications that they should make mainline better for everyone so that you don't need external code to make the kernel useful. Mark, then this is really confusing me. for this reason, the target should be making this out-of-tree stuff be inside the tree. to make i2c-sirf mainline better and not need external code, the target should be making this external code not external but become internal. otherwise, my mainline users will always need this external code. It's not just this one bit of code that they're relying on, this also gets built on with other things that are also out of tree. The thing we need to do is figure out what problem the solution as a whole is fixing and then make sure that mainline can deal with that. signature.asc Description: Digital signature
Re: [PATCH] i2c: sirf: move driver init from module_init to subsys_initcall
On Mon, May 27, 2013 at 09:54:56AM +0800, Barry Song wrote: Mark, the case is not that deferred probing is slow or not. deferred probing is pretty good. the case is that we want to i2c and media connected with i2c probed earlier than other devices. in auto infotainment devices, we actually do some hacking in kernel that makes rear view work earlier than other device driver initialization with a kernel thread which take care of backing-car policy not only mechanism. that means, we make camera work to see backview image even earlier than other drivers' initialization. we don't want media deferred to wait for i2c. we want make some early jobs ready earlier. So this change makes no practical difference in mainline and exists to support out of tree hacks for performance? It doesn't seem like that big a patch to carry along with the out of tree stuff... signature.asc Description: Digital signature
Re: [PATCH] i2c: sirf: move driver init from module_init to subsys_initcall
On Thu, May 16, 2013 at 06:25:39PM +0800, Barry Song wrote: 2013/5/16 Wolfram Sang w...@the-dreams.de: What about deferred probing? deferred probing could work but could not work for automative. what we really want is that devices begin to work as early as possible. we want i2c clients like lcd, camera begin to show images as early as possible as an automative requirement. Probe deferral should have at most a mimimal impact on the boot time - it's pretty much just changing the order providing the probe functions aren't excessively slow to defer. If the probe functions are taking a long time to defer that's probably what needs looking at. If it really is probe deferral itself that's too slow then we ought to be looking at improving that rather than boding around it. signature.asc Description: Digital signature
Re: i2cget with 16-bit address, 16-bit data
On Thu, May 23, 2013 at 04:27:20AM +, Craig McQueen wrote: I'm trying to read data from a SGTL5000 audio CODEC, which has 16-bit addresses and 16-bit wide data registers. Unless I'm missing something, it looks impossible to read these registers using i2cget, because it only supports 8-bit addresses. Is there any other similar tool, or should I patch i2cget to support 16-bit addresses? If you're running Linux why are you trying to use i2cget to interact with the register map? There's a driver for the device using regmap... signature.asc Description: Digital signature
Re: [PATCH 7/9] I2C: mv64xxx: remove I2C_M_NOSTART code
On Thu, May 16, 2013 at 09:37:11PM +0100, Russell King wrote: As this driver does not advertise protocol mangling support (I2C_FUNC_PROTOCOL_MANGLING is not set), having code to act on I2C_M_NOSTART is illogical, and in any case isn't supportable on anything but the first message - which makes no sense. Remove the I2C_M_NOSTART code. There is I2C_FUNC_NOSTART, though if it's only possible to use it on the first message that is pretty much useless as you say. signature.asc Description: Digital signature
