Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Thu, Oct 22, 2015 at 07:44:05AM -0700, Greg Kroah-Hartman wrote:
> 
> 
> On Thu, Oct 22, 2015 at 11:05:11AM +0200, Tomeu Vizoso wrote:
> > Given that downstreams are already carrying as many hacks as they
> > could think of to speed total boot up, I think this is effectively
> > telling them to go away.
> 
> No I'm not, I'm asking for real data, not hand-wavy-this-is-going-to
> solve-the-random-issue-i'm-having type patch by putting random calls in
> semi-random subsystems all over the kernel.

+1000, fully agree.

There's too much verbal diarrhoea going on in this thread and no facts.
I've been waiting for real data too, and there's not one shred of it, or
even a hint that it might appear.  So, the conclusion I'm coming to is
that there isn't any data to back up the claims made in this thread.

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. :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 07:55:29PM +0300, Grygorii Strashko wrote:
> On 10/21/2015 06:36 PM, Frank Rowand wrote:
> > The above is currently the last point for probe to succeed or defer
> > (until possibly, as you mentioned, module loading resolves the defer).
> > If a probe defers above, it will defer again below.  The set of defers
> > should be exactly the same above and below.
> > 
> 
> Unfortunately this is not "the last point for probe to succeed or defer".

Of course it isn't.  Being pedantic, there's actually no such thing,
because the point that the kernel as finished booting can never actually
be determined with things like modules being present.  That's something
I've acknowledged from the start of this.

> There are still a bunch of drivers in Kernel which will be probed at 
> late_initcall() level.
> (like ./drivers/net/ethernet/ti/cpsw.c => late_initcall(cpsw_init);
> Yes - they probably need to be updated to use module_init(), but that's what
> we have now). Those drivers will re-trigger deferred device probing if their
> probe succeeded.

Maybe this particular late_initcall() which triggers off the deferred
probing should be moved to its own really_late_initcall() which happens
as the very last thing - I think this is intended to run after everything
else has had a chance to probe once.

> As result, it is impossible to say when will it happen the 
> "final round of deferred device probing" :( and final list of drivers
> which was "deferred forever" will be know only when kernel exits to
> User space  ("deferred forever" - before loading modules).
> 
> May be, we also can consider adding debug_fs entry which can be used to
> display actual state of deferred_probe_pending_list? 

There are complaints in this thread about the existing deferred probing
implementation being hard to debug - where it's known that a device
has deferred, but it's not known why that happened.

That would be solved by my proposal, as this final round of probing
before entering userspace after _all_ normal device probes have been
attempted once and then we've tried to satisfy the deferred probe
(okay, that's what it's _supposed_ to be - and as it takes three lines
to write it, you'll excuse me if I just use the abbreviated "final
round of deferred probe" which is much shorter - but remember that
the long version is what I actually mean) would produce a list of
not only the devices that failed to probe, but also the cause of the
deferred probes.

My proposal would ensure that subsystems are happier to add these
prints, because in the normal scenario where we have deferred probing,
we're not littering the console log with lots of useless failure
messages which make people stop and think "now did device X probe?"
It also means scripts in our boot farms can more effectively analyse
the log and determine whether the boot was actually successful and
contained no errors.

Merely printing the list of devices which have been deferred is next
to useless.  The next question will always be "why did device X defer?"
and if that can't be answered, it means people having to spend a long
time adding lots of printks to the kernel at lots of -EPROBE_DEFER
returning sites or in the relevant drivers, tracing through the code
back towards the -EPROBE_DEFER sites to try and track it down.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 09:13:48PM +0300, Grygorii Strashko wrote:
> But I worry a bit (and that my main point) about these few additional
> rounds of deferred device probing which I have right now and which allows
> some of drivers to finish, finally, their probes successfully.
> With proposed change I'll get more messages in boot log, but some of
> them will belong to drivers which have been probed successfully and so,
> they will be not really useful.

Then you haven't properly understood my proposal.

I want to get rid of all the "X deferred its probing" messages up until
the point that we set the "please report deferred probes" flag.

That _should_ mean that all the deferred probing that goes on becomes
_totally_ silent and becomes hidden (unless you really want to see it,
in which case we can make a debug option which turns it on) up until
we're at the point where we want to enter userspace.

At that point, we then report into the kernel log which devices are
still deferring and, via appropriately placed dev_warn_deferred(),
the reasons why the devices are being deferred.

So, gone will be all the messages earlier in the log about device X
not having a GPIO/clock/whatever because the device providing the
GPIO/clock/whatever hasn't been probed.

If everything is satisfied by the time we run this last round (again,
I'm not using a three line sentence to describe exactly what I mean,
I'm sure you know by now... oops, I just did) then the kernel will
report nothing about any deferrals.  That's _got_ to be an improvement.

> 
> As result, I think, the most important thing is to identify (or create)
> some point during kernel boot when it will be possible to say that all
> built-in drivers (at least) finish their probes 100% (done or defer).
> 
> Might be do_initcalls() can be updated (smth like this):
> static void __init do_initcalls(void)
> {
>   int level;
> 
>   for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
>   do_initcall_level(level);
> 
> + wait_for_device_probe();
> + /* Now one final round, reporting any devices that remain deferred */
> + driver_deferred_probe_report = true;
> + driver_deferred_probe_trigger();
> + wait_for_device_probe();
> }
> 
> Also, in my opinion, it will be useful if this debugging feature will be
> optional.

I wonder why you want it optional... so I'm going to guess and cover
both cases I can think of below to head off another round of reply on
this point (sorry if this sucks eggs.)

I don't see it as being optional, because it's going to be cheap to run
in the case of a system which has very few or no errors - which is what
you should have for production systems, right?

Remember, only devices and drivers that are present and have been
probed once get added to the deferred probe list, not devices for
which their drivers are modules.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 08:36:23AM -0700, Frank Rowand wrote:
> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
> > On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
> >> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> 
> < snip >
> 
> >>> +
> >>>  static bool driver_deferred_probe_enable = false;
> >>> +
> >>>  /**
> >>>   * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> >>>   *
> >>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
> >>>   driver_deferred_probe_trigger();
> >>
> >> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
> >> not add another round of probes.
> > 
> > The idea is not to report anything for drivers that were deferred
> > during the normal bootup.  The above is part of the normal bootup,
> > and the deferred activity should not be warned about.
> 
> The above is currently the last point for probe to succeed or defer
> (until possibly, as you mentioned, module loading resolves the defer).
> If a probe defers above, it will defer again below.  The set of defers
> should be exactly the same above and below.

Why should it?  Isn't this late_initcall() the first opportunity that
deferred devices get to be re-probed from their first set of attempts
via the drivers having their initcalls called?

If what you're saying is true, what's the point of this late_initcall()?



-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> >> Hi Russell,
> >>
> >> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> >> <li...@arm.linux.org.uk> wrote:
> >>>>> What you can do is print those devices which have failed to probe at
> >>>>> late_initcall() time - possibly augmenting that with reports from
> >>>>> subsystems showing what resources are not available, but that's only
> >>>>> a guide, because of the "it might or might not be in a kernel module"
> >>>>> problem.
> >>>>
> >>>> Well, adding those reports would give you a changelog similar to the
> >>>> one in this series...
> >>>
> >>> I'm not sure about that, because what I was thinking of is adding
> >>> a flag which would be set at late_initcall() time prior to running
> >>> a final round of deferred device probing.
> >>
> >> Which round is the final round?
> >> That's the one which didn't manage to bind any new devices to drivers,
> >> which is something you only know _after_ the round has been run.
> >>
> >> So I think we need one extra round to handle this.
> >>
> >>> This flag would then be used in a deferred_warn() printk function
> >>> which would normally be silent, but when this flag is set, it would
> >>> print the reason for the deferral - and this would replace (or be
> >>> added) to the subsystems and drivers which return -EPROBE_DEFER.
> >>>
> >>> That has the effect of hiding all the deferrals up until just before
> >>> launching into userspace, which should then acomplish two things -
> >>> firstly, getting rid of the rather useless deferred messages up to
> >>> that point, and secondly printing the reason why the remaining
> >>> deferrals are happening.
> >>>
> >>> That should be a small number of new lines plus a one-line change
> >>> in subsystems and drivers.
> >>
> >> Apart from the extra round we probably can't get rid of, that sounds OK to 
> >> me.
> > 
> > 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.
> > 
> > We could make driver_deferred_probe_report something that can be
> > deactivated again after the last deferred probe run, and provide the
> > user with a knob that they can turn it back on again.
> > 
> > I've tried this out on two of my platforms, including forcing
> > driver_deferred_probe_report to be enabled, and I get exactly one
> > deferred probe, so not a particularly good test.
> > 
> > The patch won't apply as-is to mainline for all files; it's based on my
> > tree which has some 360 additional patches (which seems to be about
> > normal for my tree now.)
> 
> I like the concept (I have been thinking along similar lines lately).
> But I think this might make the console messages more confusing than
> they are now.

If messages end up being given from the subsystem rather than the driver,
surely they become more consistent?

> The problem is that debug, warn, and error messages
> come from a somewhat random set of locations at the moment.  Some
> come from the driver probe routines and some come from the subsystems
> that the probe routines call.  So the patch is suppressing some
> messages, but not others.

The patch is not complete (read the description above).

> > +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);
> 
> The places where dev_warn_deferred() replaces dev_dbg(), we lose the
> ability to turn on debugging and observe the driver reporting th

Re: Alternative approach to solve the deferred probe (was: [GIT PULL] On-demand device probing)

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> >> > What you can do is print those devices which have failed to probe at
> >> > late_initcall() time - possibly augmenting that with reports from
> >> > subsystems showing what resources are not available, but that's only
> >> > a guide, because of the "it might or might not be in a kernel module"
> >> > problem.
> >>
> >> Well, adding those reports would give you a changelog similar to the
> >> one in this series...
> >
> > I'm not sure about that, because what I was thinking of is adding
> > a flag which would be set at late_initcall() time prior to running
> > a final round of deferred device probing.
> 
> Which round is the final round?
> That's the one which didn't manage to bind any new devices to drivers,
> which is something you only know _after_ the round has been run.
> 
> So I think we need one extra round to handle this.
> 
> > This flag would then be used in a deferred_warn() printk function
> > which would normally be silent, but when this flag is set, it would
> > print the reason for the deferral - and this would replace (or be
> > added) to the subsystems and drivers which return -EPROBE_DEFER.
> >
> > That has the effect of hiding all the deferrals up until just before
> > launching into userspace, which should then acomplish two things -
> > firstly, getting rid of the rather useless deferred messages up to
> > that point, and secondly printing the reason why the remaining
> > deferrals are happening.
> >
> > That should be a small number of new lines plus a one-line change
> > in subsystems and drivers.
> 
> Apart from the extra round we probably can't get rid of, that sounds OK to me.

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.

We could make driver_deferred_probe_report something that can be
deactivated again after the last deferred probe run, and provide the
user with a knob that they can turn it back on again.

I've tried this out on two of my platforms, including forcing
driver_deferred_probe_report to be enabled, and I get exactly one
deferred probe, so not a particularly good test.

The patch won't apply as-is to mainline for all files; it's based on my
tree which has some 360 additional patches (which seems to be about
normal for my tree now.)

 drivers/base/dd.c   | 29 +
 drivers/base/power/domain.c |  7 +--
 drivers/clk/clkdev.c|  9 -
 drivers/gpio/gpiolib-of.c   |  5 +
 drivers/gpu/drm/bridge/dw_hdmi.c|  2 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c |  2 +-
 drivers/gpu/drm/imx/imx-ldb.c   |  5 +++--
 drivers/gpu/drm/msm/dsi/dsi.c   |  2 +-
 drivers/gpu/drm/msm/msm_drv.c   |  3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  3 ++-
 drivers/of/irq.c|  5 -
 drivers/pci/host/pci-mvebu.c|  1 +
 drivers/pinctrl/core.c  |  5 +++--
 drivers/pinctrl/devicetree.c|  4 ++--
 drivers/regulator/core.c|  5 +++--
 include/linux/device.h  |  1 +
 16 files changed, 71 insertions(+), 17 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..bb12224f2901 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -129,7 +129,29 @@ void driver_deferred_probe_del(struct device *dev)
mutex_unlock(_probe_mutex);
 }
 
+static bool driver_deferred_probe_report;
+
+/**
+ * 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);
+
 static bool driver_deferred_probe_enable = false;
+
 /**
  * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
  *
@@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible b

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 02:34:22PM +0200, Tomeu Vizoso wrote:
> ... If a device is available and has
> a compatible driver, but it cannot be probed because a dependency
> isn't going to be available, that's an error and is going to cause
> real-world problems unless the device is redundant. Currently we say
> nothing because with deferred probe the probe callbacks are also part
> of the mechanism that determines the dependency order.

So what if device X depends on device Y, and we have a driver for
device Y built-in to the kernel, but the driver for device X is a
module?

I don't see this being solvable in the way you describe above - it's
going to identify X as being unable to be satisfied, and report it as
an error - but it's not an error at all.

> Having a specific switch for enabling deferred probe logging sounds
> good, but there's going to be hundreds of spurious messages about
> deferred probes that were just deferrals and only one of them is going
> to be the actual error in which a device failed to find a dependency.

Why would there be?  Sounds like something's very wrong there.

You should only get deferred probes for devices which are declared to
be present, but their resources have not yet been satisfied.  It
doesn't change anything if you have a kernel with lots of device drivers
or just the device drivers you need - the device drivers you don't need
do not contribute to the deferred probing in any way.

So, really, after boot and all appropriate modules have been loaded,
you should end up with no deferred probes.  Are you saying that you
still have "hundreds" at that point?  If you do, that sounds like
there's something very wrong.

> 3) Regarding total boot time, I don't expect this series to make much
> of a difference because though we would save a lot of matching and
> querying for resources, that's little time compared with how long we
> wait for hardware to react during probing. Async probing is more
> likely to help with drivers that take a long time to probe.

For me, on my fastest ARM board, though running serial console:

[2.293468] VFS: Mounted root (ext4 filesystem) on device 179:1.

There's a couple of delays in there, but they're not down to deferred
probing.  The biggest one is serial console startup (due to the time
it takes to write out the initial kernel messages):

[0.289962] f1012000.serial: ttyS0 at MMIO 0xf1012000 (irq = 23, base_baud = 
15625000) is a 16550A
[0.944124] console [ttyS0] enabled

and DSA switch initialisation:

[1.530655] libphy: dsa slave smi: probed
[2.034426] dsa dsa@0 lan6 (uninitialized): attached PHY at address 0 
[Generic PHY]

I'm not sure what causes that, but at a guess it's having to talk to the
DSA switch over the MDIO bus via several layers of indirect accesses.
Of course, serial console adds to the boot time significantly anyway,
especially at the "standard" kernel logging level.

> One more thing about the breakage we have seen so far is that it's
> generally caused by implicit dependencies and hunting those is
> probably the second biggest timesink of the linux embedded developer
> after failed probes.

... which is generally caused by the crappy code which the average
embedded Linux developer creates, particularly with the crappy error
messages they like creating.  For the most part, they _might_ as well
just print "Error!\n" and be done with it, for all the use they are.
When creating an error print, your average embedded Linux developer
fails to print the _reason_ why something failed, which makes debugging
it much harder.

The first thing I do when I touch code that needs this kind of debugging
is to go through and add printing of the error code.  That normally lets
me quickly narrow down what's failed.

If embedded Linux developers are struggling with this, they only have
themselves to blame.

In the case of deferred probing, what _may_ help is if we got rid of the
core code printing that driver X requested deferred probing, instead
moving the responsibility to report this onto the driver or subsystem.
Resource claiming generally has the struct device, and can use dev_warn()
to report which device is being probed, along with which resource is
not yet available.

This debug problem is solvable without needing to resort to complex
probing solutions.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:10:56PM +0200, Tomeu Vizoso wrote:
> On 19 October 2015 at 15:18, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> > On Mon, Oct 19, 2015 at 02:34:22PM +0200, Tomeu Vizoso wrote:
> >> ... If a device is available and has
> >> a compatible driver, but it cannot be probed because a dependency
> >> isn't going to be available, that's an error and is going to cause
> >> real-world problems unless the device is redundant. Currently we say
> >> nothing because with deferred probe the probe callbacks are also part
> >> of the mechanism that determines the dependency order.
> >
> > So what if device X depends on device Y, and we have a driver for
> > device Y built-in to the kernel, but the driver for device X is a
> > module?
> >
> > I don't see this being solvable in the way you describe above - it's
> > going to identify X as being unable to be satisfied, and report it as
> > an error - but it's not an error at all.
> 
> It's going to probe Y at late_initcall, then probe X when its driver
> is registered. No deferred probes nor messages about it.
> 
> But if you meant to write the opposite case (X built-in and Y in a
> module), then I have to ask you in what situation that would make
> sense.

I did mean the opposite way around.  It may not make sense if you're
targetting a single platform, but it may make sense in a single zImage
kernel.

Consider something like a single zImage kernel that is built with
everything built-in to be able to boot and mount rootfs without
initramfs support on both platform A and platform B.  Both platforms
share some hardware (eg, an I2C GPIO expander) which is built as a
module.  It is a resource provider.  Platform B contains a driver
which is required to boot on platform A, but not platform B (so the
kernel has to have that driver built-in.)  On platform B, there is
a dependency to the I2C GPIO expander device.

> >> Having a specific switch for enabling deferred probe logging sounds
> >> good, but there's going to be hundreds of spurious messages about
> >> deferred probes that were just deferrals and only one of them is going
> >> to be the actual error in which a device failed to find a dependency.
> >
> > Why would there be?  Sounds like something's very wrong there.
> 
> Sorry about that, I have checked that only now and I "only" get 39
> deferred probe messages on exynos5250-snow.

I typically see one or two, maybe five maximum on the platforms I have
here, but normally zero.

> > So, really, after boot and all appropriate modules have been loaded,
> > you should end up with no deferred probes.  Are you saying that you
> > still have "hundreds" at that point?  If you do, that sounds like
> > there's something very wrong.
> 
> I was talking about messages if we log each -EPROBE_DEFER, not devices
> that remain to be probed. The point being that right now we don't have
> a way to know if we are deferring because the dependency will be
> around later, or if we have a problem and the dependency isn't going
> to be there at all.

What's the difference between a dependency which isn't around because
the driver is not built into the kernel but is available as a module,
and a dependency that isn't around because the module hasn't been
loaded yet?

How do you distinguish between those two scenarios?  In the former
scenario, the device will eventually come up when udev loads the
module.  In the latter case, it's a persistent failing case.

> Agreed, with the note from above on why it would be better to only
> print such a message only when the -EPROBE_DEFER is likely to be a
> problem.

... and my argument is that there's _no way_ to know for certain which
deferred probes will be a problem, and which won't.  The only way to
definitely know that is if you disable kernel modules, and require
all drivers to be built into the kernel.

What you can do is print those devices which have failed to probe at
late_initcall() time - possibly augmenting that with reports from
subsystems showing what resources are not available, but that's only
a guide, because of the "it might or might not be in a kernel module"
problem.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 05:00:54PM +0200, Tomeu Vizoso wrote:
> On 19 October 2015 at 16:30, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> > I typically see one or two, maybe five maximum on the platforms I have
> > here, but normally zero.
> 
> Hmm, I have given a look at our lava farm and have seen 2 dozens as
> common (with multi_v7).

No, because the lava farms tend not to be public.

> > What you can do is print those devices which have failed to probe at
> > late_initcall() time - possibly augmenting that with reports from
> > subsystems showing what resources are not available, but that's only
> > a guide, because of the "it might or might not be in a kernel module"
> > problem.
> 
> Well, adding those reports would give you a changelog similar to the
> one in this series...

I'm not sure about that, because what I was thinking of is adding
a flag which would be set at late_initcall() time prior to running
a final round of deferred device probing.

This flag would then be used in a deferred_warn() printk function
which would normally be silent, but when this flag is set, it would
print the reason for the deferral - and this would replace (or be
added) to the subsystems and drivers which return -EPROBE_DEFER.

That has the effect of hiding all the deferrals up until just before
launching into userspace, which should then acomplish two things -
firstly, getting rid of the rather useless deferred messages up to
that point, and secondly printing the reason why the remaining
deferrals are happening.

That should be a small number of new lines plus a one-line change
in subsystems and drivers.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:29:40PM +0100, David Woodhouse wrote:
> 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.

It's a bit ironic that you've chosen GPIO as an example there.  The
"new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
gpio descriptor.  There's no of_* method.

I'd like to use the gpiod_* stuff, but I feel that my options are
rather limited: either use fwnode_get_named_gpiod() with
>of_node->fwnode, which seems like a hack by going underneath
the covers of how fwnode is (partially) implemented with DT, or by
using of_get_named_gpio() and the converting the gpio number to a
descriptor via gpio_to_desc().  Both feel very hacky.

If ACPI already handles GPIOs internally, then I'm left wondering
why GPIO descriptor stuff went down the fwnode route at all - it
seems rather pointless in this case, and it seems to make the use
of the gpiod* interfaces where we _do_ need to use it (DT) harder
and more hacky.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 08:27:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Oct 19, 2015 at 04:43:24PM +0100, Russell King - ARM Linux wrote:
> > It's a bit ironic that you've chosen GPIO as an example there.  The
> > "new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
> > gpio descriptor.  There's no of_* method.
> 
> Without following all that fwnode discussion:
> gpiod_get et al. should work for you here, doesn't it? It just takes a
> struct device * and I'm happy with it.

What if you don't have a struct device?  I had that problem recently
when modifying the mvebu PCIe code.  The 'struct device' node doesn't
contain the GPIOs, it's the PCIe controller.  Individual ports on the
controller are described in DT as sub-nodes, and the sub-nodes can
have a GPIO for card reset purposes.  These sub-nodes don't have a
struct device.

Right now, I'm having to do this to work around this issue:

reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, );
if (reset_gpio == -EPROBE_DEFER) {
ret = reset_gpio;
goto err;
}

if (gpio_is_valid(reset_gpio)) {
unsigned long gpio_flags;

port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
  port->name);
if (!port->reset_name) {
ret = -ENOMEM;
goto err;
}

if (flags & OF_GPIO_ACTIVE_LOW) {
dev_info(dev, "%s: reset gpio is active low\n",
 of_node_full_name(child));
gpio_flags = GPIOF_ACTIVE_LOW |
 GPIOF_OUT_INIT_LOW;
} else {
gpio_flags = GPIOF_OUT_INIT_HIGH;
}

ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
port->reset_name);
if (ret) {
if (ret == -EPROBE_DEFER)
goto err;
goto skip;
}

port->reset_gpio = gpio_to_desc(reset_gpio);
}

Not nice, is it?  Not nice to have that in lots of drivers either.

However, switching to use any of_* or fwnode_* thing also carries with
it another problem: you can't control the name appearing in the
allocation, so you end up with a bunch of GPIOs requested with a "reset"
name - meaning you lose any identification of which port the GPIO was
bound to.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> >> > What you can do is print those devices which have failed to probe at
> >> > late_initcall() time - possibly augmenting that with reports from
> >> > subsystems showing what resources are not available, but that's only
> >> > a guide, because of the "it might or might not be in a kernel module"
> >> > problem.
> >>
> >> Well, adding those reports would give you a changelog similar to the
> >> one in this series...
> >
> > I'm not sure about that, because what I was thinking of is adding
> > a flag which would be set at late_initcall() time prior to running
> > a final round of deferred device probing.
> 
> Which round is the final round?

I said "a" not "the".  Maybe I should've said "one last round of deferred
probing before entering userspace".

> That's the one which didn't manage to bind any new devices to drivers,
> which is something you only know _after_ the round has been run.
> 
> So I think we need one extra round to handle this.

Yes - because the idea is that we do everything we do today without
printing anything about deferred probes.  We then set a flag which
indicates we should report defers, and then we trigger another round
of probes.

If everything probed successfully, the deferred probe list will be
empty and nothing will be seen.  Otherwise, we should end up with a
report of all the devices that weren't able to be bound to their
drivers due to -EPROBE_DEFER.

> 
> > This flag would then be used in a deferred_warn() printk function
> > which would normally be silent, but when this flag is set, it would
> > print the reason for the deferral - and this would replace (or be
> > added) to the subsystems and drivers which return -EPROBE_DEFER.
> >
> > That has the effect of hiding all the deferrals up until just before
> > launching into userspace, which should then acomplish two things -
> > firstly, getting rid of the rather useless deferred messages up to
> > that point, and secondly printing the reason why the remaining
> > deferrals are happening.
> >
> > That should be a small number of new lines plus a one-line change
> > in subsystems and drivers.
> 
> Apart from the extra round we probably can't get rid of, that sounds
> OK to me.

Yes, it means a little longer to boot, but if there's nothing pending
this _should_ only be microseconds - it should be nothing more than
setting the flag, possibly taking and releasing a lock, and checking
that the deferred probe list is empty.

Of course, if the deferred probe list isn't empty, then there'll be
more expense - but I'm willing to bet that for those developers with
serial console enabled, the kernel boot will be faster overall due
to the reduced number of characters printed during the boot.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH 12/12] i2c: pxa: enable/disable i2c module across msg xfer

[Russell King - ARM Linux]

On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
 From: Yi Zhang yizh...@marvell.com
 
 Enable i2c module/unit before transmission and disable when it finishes.
 
 why?
 It's because the i2c bus may be distrubed if the slave device,
 typically a touch, powers on.

disturbed

I'd recommend that this is a DT property - not every platform is going to
want this, and as there is rudimentary I2C slave support in this driver,
this change breaks that.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH 08/12] i2c: pxa: enable/disable irq across message xfer

[Russell King - ARM Linux]

On Thu, May 28, 2015 at 06:33:40PM +0530, Vaibhav Hiremath wrote:
 In order to avoid spurious irq caused by CP polling mode,
 enable irq at the entry of i2c_pxa_xfer() fn and disable it
 again before exit.

NAK.

It's really not nice for drivers to disable a potentially shared interrupt.
If the interrupt is shared, you disable the interrupt for other users of
that interrupt as well.  See:

commit c66dc529194be374556d166ee7ddb84a7d1d302b
Author: Sebastian Andrzej Siewior bige...@linutronix.de
Date:   Wed Feb 23 12:38:18 2011 +0100

i2c-pxa2xx: add support for shared IRQ handler

Sodaville has three of them on a single IRQ. IRQF_DISABLED is removed
because it is a NOP allready and scheduled for removal.

Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
Signed-off-by: Dirk Brandewie dirk.brande...@gmail.com
Signed-off-by: Ben Dooks ben-li...@fluff.org

So you're breaking Sodaville.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH v6 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Russell King - ARM Linux]

To see why atomic_t is pure obfuscation:

typedef struct {
int counter;
} atomic_t;

So, counter is a plain int.

On Mon, Jan 19, 2015 at 11:23:47AM -0800, Ray Jui wrote:
 +static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 +{
 + struct bcm_iproc_i2c_dev *iproc_i2c = data;
 + u32 status = readl(iproc_i2c-base + IS_OFFSET);
 +
 + status = ISR_MASK;
 +
 + if (!status)
 + return IRQ_NONE;
 +
 + writel(status, iproc_i2c-base + IS_OFFSET);
 + atomic_set(iproc_i2c-xfer_is_done, 1);

#define atomic_set(v,i) (((v)-counter) = (i))

So, this is the same as doing:

iproc_i2c-xfer_is_done.counter = 1;

which is merely setting the 'int' to 1.

 + time_left = wait_for_completion_timeout(iproc_i2c-done, time_left);
 +
 + /* disable all interrupts */
 + writel(0, iproc_i2c-base + IE_OFFSET);
 +
 + if (!time_left  !atomic_read(iproc_i2c-xfer_is_done)) {

#define atomic_read(v)  ACCESS_ONCE((v)-counter)

This is practically the same as:

if (!time_left  !iproc_i2c-xfer_is_done.counter) {

except that this access will be guaranteed to happen just once at this
location (see ACCESS_ONCE() in include/linux/compiler.h).

However, complete()..wait_for_completion() ensures that there are
barriers in the way: complete takes a spinlock on the waiter, so the
write to iproc_i2c-xfer_is_done.counter will be visible by the time
wait_for_completion() returns, and wait_for_completion() also does.
The same spinlock is also manipulated by wait_for_completion(), which
means there's barriers there as well, so it can't cache the value of
counter across that call.

So, the volatile access guaranteed by ACCESS_ONCE() isn't even
needed here.

(It would be needed if you were spinning in a loop, calling no other
functions - but then you're supposed to use cpu_relax() in that
circumstance, which has a compiler barrier in it, which ensures that
it will re-read such a variable each time.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Russell King - ARM Linux]

On Sat, Jan 17, 2015 at 04:30:33PM -0800, Ray Jui wrote:
 
 
 On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote:
  On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote:
 time_left = wait_for_completion_timeout(iproc_i2c-done, time_left);
 
 /* disable all interrupts */
 writel(0, iproc_i2c-base + IE_OFFSET);
 
 if (!time_left  !atomic_read(iproc_i2c-transfer_is_successful)) {
  
  Why are you using atomic_read() here?
  
 transfer_is_successful 1) will be reset to 0 in this function (before
 kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal
 completion of the I2C transfer), and 3) will be checked in this function
 here. I thought that means I should declare it volatile, because it can
 be modified in both the process context and interrupt context (and I use
 atomic because I remember Linux checkpatch warns against using volatile)?

You don't need volatile or atomic_t for that.

Rather than switching to atomic_t when seeing the checkpatch warning,
you'd do better to read Documentation/volatile-considered-harmful.txt
to understand why checkpatch issues the warning, and realise that you
don't need it for the above.

Note that in the above code, the compiler can't make an assumption
about iproc_i2c-transfer_is_successful because it can't tell whether
a called function (eg, wait_for_completion_timeout()) could modify it.

Another possible issue with the above code are these lines:

/* disable all interrupts */
writel(0, iproc_i2c-base + IE_OFFSET);

It would be nice to think that would hit the hardware immediately, but
that's making assumptions about hardware which are not necessary true.
Your interrupt handler could even be running on another CPU after you've
asked for that register to be written.

Depending on what you're trying to achieve here, you may need:

/* disable all interrupts */
writel(0, iproc_i2c-base + IE_OFFSET);
/* read it back to ensure the write has hit */
readl(iproc_i2c-base + IE_OFFSET);

/* make sure the interrupt handler isn't running */
synchronize_irq(...-irq);

if what you're trying to do is to ensure that the interrupt handler has
finished running.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Russell King - ARM Linux]

On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote:
   time_left = wait_for_completion_timeout(iproc_i2c-done, time_left);
 
   /* disable all interrupts */
   writel(0, iproc_i2c-base + IE_OFFSET);
 
   if (!time_left  !atomic_read(iproc_i2c-transfer_is_successful)) {

Why are you using atomic_read() here?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [RFC 02/11] i2c: add quirk checks to core

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 06:21:32PM +0100, Wolfram Sang wrote:
 +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, 
 char *err_msg)
 +{
 + dev_err(adap-dev, quirk: %s (addr 0x%04x, size %u)\n, err_msg, 
 msg-addr, msg-len);
 + return -EOPNOTSUPP;
 +}

So, what happens if I open an I2C adapter, find a message which causes
i2c_quirk_error() to be called, and then spin repeatedly calling that...
Shouldn't there be some rate limiting to this?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH V2 2/5] i2c: pxa: prepare/unprepare clocks

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 06:07:40PM +0300, Dmitry Eremin-Solenikov wrote:
 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unprepare().
 
 Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 ---
  drivers/i2c/busses/i2c-pxa.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
 index be671f7..2e75375 100644
 --- a/drivers/i2c/busses/i2c-pxa.c
 +++ b/drivers/i2c/busses/i2c-pxa.c
 @@ -1297,7 +1297,7 @@ static int i2c_pxa_suspend_noirq(struct device *dev)
   struct platform_device *pdev = to_platform_device(dev);
   struct pxa_i2c *i2c = platform_get_drvdata(pdev);
  
 - clk_disable(i2c-clk);
 + clk_disable_unprepare(i2c-clk);

Since clk_unprepare() and clk_prepare() can sleep, it is unwise to call
these with IRQs disabled - the _noirq variants of these are run with
IRQs disabled.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH V2 1/5] serial: pxa: prepare/unprepare clocks

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 06:07:39PM +0300, Dmitry Eremin-Solenikov wrote:
 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unprepare().

NAK.  Console writes can be called from IRQs-off regions.  clk_prepare()
can sleep.  Sleeping is not permitted with IRQs off.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH] i2c: axxia: Add I2C driver for AXM55xx

[Russell King - ARM Linux]

On Mon, Sep 22, 2014 at 11:59:39AM +0200, Wolfram Sang wrote:
 IRQ_NONE is this interrupt wasn't by me so for shared IRQs, the next
 handler can check.

Err, no it isn't.  IRQ_NONE has no such effect.  All handlers on a
shared interrupt are always run irrespective of the return value from
any particular interrupt handler.  See kernel/irq/handle.c.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH v2 1/5] i2c: Don't start transfers when suspended

[Russell King - ARM Linux]

On Tue, Jul 22, 2014 at 09:46:46PM +0300, Grygorii Strashko wrote:
  I'm not sure, this optimization is safe  (
 Because, in many cases the access to PMIC IC needs to be allowed till late
 stages of suspending (at least till suspend_noirq stage or even later).
 For example, on some OMAP SoC Voltage management code need to use services
 provided by PMIC IC, which is connected to I2C.

This is definitely not safe.  I worked on a PXA platform a while back
where the only way to tell the thing to sleep was to send an I2C
message to a microcontroller asking it to remove power.

Plus, as you say, you may also have PMICs that you need to talk to
in order to shut power off to various peripherals (and maybe even
the CPU itself) during the suspend process.

I2C is one of those cases where devices attached to the I2C bus are
themselves responsible for doing their own suspend shutdown at the
appropriate time; it's not the responsibility of the I2C core to
know this kind of system/driver dependent detail.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
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: [PATCH] i2c: sun6-p2wi: fix call to snprintf

[Russell King - ARM Linux]

On Fri, Jun 13, 2014 at 09:20:02AM +0200, Boris BREZILLON wrote:
 - snprintf(p2wi-adapter.name, sizeof(p2wi-adapter.name), pdev-name);
 + snprintf(p2wi-adapter.name, sizeof(p2wi-adapter.name), %s,
 +  pdev-name);

Isn't this just a complicated way to express:

strlcpy(p2wi-adapter.name, pdev-name, sizeof(p2wi-adapter.name));

?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
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: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Russell King - ARM Linux]

On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote:
 On Fri, Mar 07, 2014 at 04:08:36PM +, Russell King - ARM Linux wrote:
  On Fri, Mar 07, 2014 at 03:59:30PM +0100, Maxime Ripard wrote:
   @@ -900,7 +902,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
exit_free_irq:
 free_irq(drv_data-irq, drv_data);
exit_reset:
   - if (pd-dev.of_node  !IS_ERR(drv_data-rstc))
   + if (pd-dev.of_node  IS_ENABLED(CONFIG_RESET_CONTROLLER) 
   + !IS_ERR(drv_data-rstc))
 reset_control_assert(drv_data-rstc);
  
  Another question is... why do we need to check pd-dev.of_node here?
  If CONFIG_RESET_CONTROLLER is set, we always try to get the reset
  controller node, so drv_data-rstc is either going to be a valid
  pointer, or it's going to be an error pointer - neither
  reset_control_get() nor devm_reset_control_get return NULL.
 
 Following back on this as I was doing the patch, actually,
 drv_data-rstc will be NULL if we're not probed by DT, and hence never
 call reset_control_get, that would set an error pointer.
 
 But then, we can use IS_ERR_OR_NULL on drv_data-rstc.

I think you can also move the devm_reset_control_get() into the main
probe function: you're only checking for -EPROBE_DEFER from it to fail,
allowing other errors to continue with the driver init.  This means
that on non-OF, devm_reset_control_get() will fail with -ENOENT.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
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: [PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Russell King - ARM Linux]

On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
 The Allwinner A31 SoC using that IP has a reset controller maintaining
 it reset unless told otherwise.
 
 Add some optional reset support to the driver.
 
 Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
 Reviewed-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 Tested-by: Gregory CLEMENT gregory.clem...@free-electrons.com

This appears to be causing some build errors in Olof's next builder
for many of the ARM platforms which make use of this:

drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to 
`reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to 
`reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to 
`devm_reset_control_get'
drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to 
`reset_control_deassert'

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
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: [PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Russell King - ARM Linux]

On Fri, Mar 07, 2014 at 11:07:51AM +0100, Maxime Ripard wrote:
 Hi Russell,
 
 On Fri, Mar 07, 2014 at 09:52:23AM +, Russell King - ARM Linux wrote:
  On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
   The Allwinner A31 SoC using that IP has a reset controller maintaining
   it reset unless told otherwise.
   
   Add some optional reset support to the driver.
   
   Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
   Reviewed-by: Gregory CLEMENT gregory.clem...@free-electrons.com
   Tested-by: Gregory CLEMENT gregory.clem...@free-electrons.com
  
  This appears to be causing some build errors in Olof's next builder
  for many of the ARM platforms which make use of this:
  
  drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to 
  `reset_control_assert'
  drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to 
  `reset_control_assert'
  drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to 
  `devm_reset_control_get'
  drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to 
  `reset_control_deassert'
 
 The reset framework doesn't define its functions when its not
 selected, and somehow I think it was not here. What's odd is that
 there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
 it's the circular dependency issue that has been reported that cause
 this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/

If that patch has been taken, then yes, it will have caused the above -
because now we have Dove and Kirkwood platforms trying to build this
driver without RESET_CONTROLLER being set.

The problem with depending on RESET_CONTROLLER is that then these
platforms end up without their I2C controller - because there's nothing
which enables RESET_CONTROLLER in their configuration.

Since RESET_CONTROLLER is not required for those platforms, it really
should be optional - and I think the real fix is for the reset controller
support to provide stub functions.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
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: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Russell King - ARM Linux]

On Fri, Mar 07, 2014 at 03:59:30PM +0100, Maxime Ripard wrote:
 @@ -900,7 +902,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
  exit_free_irq:
   free_irq(drv_data-irq, drv_data);
  exit_reset:
 - if (pd-dev.of_node  !IS_ERR(drv_data-rstc))
 + if (pd-dev.of_node  IS_ENABLED(CONFIG_RESET_CONTROLLER) 
 + !IS_ERR(drv_data-rstc))
   reset_control_assert(drv_data-rstc);

Another question is... why do we need to check pd-dev.of_node here?
If CONFIG_RESET_CONTROLLER is set, we always try to get the reset
controller node, so drv_data-rstc is either going to be a valid
pointer, or it's going to be an error pointer - neither
reset_control_get() nor devm_reset_control_get return NULL.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
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: [PATCH 01/17] amba: Let runtime PM callbacks be available for CONFIG_PM

[Russell King - ARM Linux]

On Tue, Feb 04, 2014 at 04:58:42PM +0100, Ulf Hansson wrote:
 Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
 callbacks. This means the callbacks becomes available for both
 CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed by drivers and
 power domains.

This patch is wrong, because it causes build errors:

drivers/amba/bus.c:126:18: error: 'pm_generic_suspend_late' undeclared here 
(not in a function)
drivers/amba/bus.c:127:18: error: 'pm_generic_resume_early' undeclared here 
(not in a function)
drivers/amba/bus.c:128:17: error: 'pm_generic_freeze_late' undeclared here (not 
in a function)
drivers/amba/bus.c:129:16: error: 'pm_generic_thaw_early' undeclared here (not 
in a function)
drivers/amba/bus.c:130:19: error: 'pm_generic_poweroff_late' undeclared here 
(not in a function)
drivers/amba/bus.c:131:19: error: 'pm_generic_restore_early' undeclared here 
(not in a function)

The failing configuration is:

# CONFIG_SUSPEND is not set
CONFIG_PM_RUNTIME=y
CONFIG_PM=y

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was up to 13.2Mbit.
--
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: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend

[Russell King - ARM Linux]

On Tue, Feb 18, 2014 at 05:36:52PM +0100, Ulf Hansson wrote:
 If we add SDIO irq support to mmci in future; parts of that
 implementation includes a re-route of DAT1 to a GPIO irq when entering
 runtime suspend state. The mmci HW will in runtime suspend state, not
 be responsible for handling irqs, which is the same as of today.

Note that the irq thread in sdio_irq is scheduled for destruction -
it's buggy when the system is under high load and a driver claims a
SDIO irq.  Sched people hate it too...

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was up to 13.2Mbit.
--
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: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
 The Allwinner i2c controller uses the same logic as the Marvell one, but
 with slightly different register offsets.
 
 Introduce a structure that will be passed by either the pdata or
 associated to the compatible strings, and that holds the various
 registers that might be needed.

I don't like this change.  It introduces further indirection where it's
not really necessary, and it's also using platform data to specify this
which is in the opposite direction to what's required for moving towards
DT.

 @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data 
 *drv_data,
  static void
  mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
  {
 - writel(0, drv_data-reg_base + MV64XXX_I2C_REG_SOFT_RESET);
 + writel(0, drv_data-reg_base + drv_data-regs-soft_reset);

It'd be much better to copy the offsets themselves in drv_data.  You're
only talking about 7 bytes here, so there's no worry about bloating the
drv_data structure.

 @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
   drv_data-freq_n = pdata-freq_n;
   drv_data-irq = platform_get_irq(pd, 0);
   drv_data-adapter.timeout = msecs_to_jiffies(pdata-timeout);
 + drv_data-regs = pdata-regs;
   } else if (pd-dev.of_node) {
 - rc = mv64xxx_of_config(drv_data, pd-dev.of_node);
 + rc = mv64xxx_of_config(drv_data, pd-dev);

I'd suggest making the default register offsets be the drivers existing
offsets, and allowing it to be overriden.  That nicely sorts out the
next comment below, and also gets rid of it in platform data.  Moreover,
if you're going to re-use this driver, you should do it via a different
compatible name in DT, which the driver can then use to identify the
different register set layout.

 +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
 + .addr   = 0x00,
 + .ext_addr   = 0x10,
 + .data   = 0x04,
 + .control= 0x08,
 + .status = 0x0c,
 + .clock  = 0x0c,
 + .soft_reset = 0x1c,
 +};

No, this means every file which includes this header ends up defining
this structure, which is globally visiable, and therefore its a recipe
for link failures.
--
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: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 04:44:47PM +0200, Maxime Ripard wrote:
 Hi Russel,
 
 On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
  It'd be much better to copy the offsets themselves in drv_data.  You're
  only talking about 7 bytes here, so there's no worry about bloating the
  drv_data structure.
 
 It was more about keeping things separated. Moreover, the probe
 function gets smaller, since you have only a pointer to pass on, instead
 of assigning those 7 bytes.

struct driver_data {
struct mv64xxx_i2c_regs reg_offsets;
};

struct driver_data *drv_data;

memcpy(drv_data-reg_offsets, reg_offsets, 
sizeof(drv_data-reg_offsets));

No need to write it each member as a separate assignment.
--
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: [PATCH 1/2] i2c-mv64xxx: Add I2C Transaction Generator support

[Russell King - ARM Linux]

On Fri, Jun 07, 2013 at 05:42:22PM +0200, Gregory CLEMENT wrote:
 From: Zbigniew Bodek z...@semihalf.com
 
 The I2C Transaction Generator offloads CPU from managing I2C
 transfer step by step.
 
 This feature is currently only available on Armada XP, so usage of
 this mechanism is activated through device tree.
 
 [gregory.clem...@free-electrons.com: Rewrite some part to be applied
 on the the code modified by Russell King's fixes]
 [gregory.clem...@free-electrons.com: Merge and split the commits to
 push them to the accurate maintainer i2c and of]]
 [gregory.clem...@free-electrons.com: Reword the commit log]
 
 Signed-off-by: Piotr Ziecik ko...@semihalf.com
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  drivers/i2c/busses/i2c-mv64xxx.c | 143 
 +--
  1 file changed, 139 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-mv64xxx.c 
 b/drivers/i2c/busses/i2c-mv64xxx.c
 index 6356439..5376dc3 100644
 --- a/drivers/i2c/busses/i2c-mv64xxx.c
 +++ b/drivers/i2c/busses/i2c-mv64xxx.c
 @@ -24,7 +24,7 @@
  #include linux/clk.h
  #include linux/err.h
  
 -/* Register defines */
 +/* Register defines (I2C port) */
  #define  MV64XXX_I2C_REG_SLAVE_ADDR  0x00
  #define  MV64XXX_I2C_REG_DATA0x04
  #define  MV64XXX_I2C_REG_CONTROL 0x08
 @@ -59,6 +59,29 @@
  #define  MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
  #define  MV64XXX_I2C_STATUS_NO_STATUS0xf8
  
 +/* Register defines (I2C bridge) */
 +#define  MV64XXX_I2C_REG_TX_DATA_LO  0xC0
 +#define  MV64XXX_I2C_REG_TX_DATA_HI  0xC4
 +#define  MV64XXX_I2C_REG_RX_DATA_LO  0xC8
 +#define  MV64XXX_I2C_REG_RX_DATA_HI  0xCC
 +#define  MV64XXX_I2C_REG_BRIDGE_CONTROL  0xD0
 +#define  MV64XXX_I2C_REG_BRIDGE_STATUS   0xD4
 +#define  MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE   0xD8
 +#define  MV64XXX_I2C_REG_BRIDGE_INTR_MASK0xDC
 +#define  MV64XXX_I2C_REG_BRIDGE_TIMING   0xE0
 +
 +/* Bridge Control values */
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_WR   0x0001
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_RD   0x0002
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT   2
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT 0x1000
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT13
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT16
 +#define  MV64XXX_I2C_BRIDGE_CONTROL_ENABLE   0x0008
 +
 +/* Bridge Status values */
 +#define  MV64XXX_I2C_BRIDGE_STATUS_ERROR 0x0001
 +
  /* Driver states */
  enum {
   MV64XXX_I2C_STATE_INVALID,
 @@ -110,6 +133,7 @@ struct mv64xxx_i2c_data {
   spinlock_t  lock;
   struct i2c_msg  *msg;
   struct i2c_adapter  adapter;
 + int bridge_enabled;
  };
  
  static void
 @@ -157,6 +181,16 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
   writel(0, drv_data-reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
   writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
   drv_data-reg_base + MV64XXX_I2C_REG_CONTROL);
 +
 + if (drv_data-bridge_enabled) {
 + writel(0, drv_data-reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
 + writel(0, drv_data-reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
 + writel(0, drv_data-reg_base +
 + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
 + writel(0, drv_data-reg_base +
 + MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
 + }
 +
   drv_data-state = MV64XXX_I2C_STATE_IDLE;
  }
  
 @@ -368,6 +402,19 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
   irqreturn_t rc = IRQ_NONE;
  
   spin_lock_irqsave(drv_data-lock, flags);
 +
 + if (drv_data-bridge_enabled) {
 + if (readl(drv_data-reg_base +
 + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
 + writel(0, drv_data-reg_base +
 + MV64XXX_I2C_REG_BRIDGE_CONTROL);
 + writel(0, drv_data-reg_base +
 + MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
 + drv_data-block = 0;
 + wake_up(drv_data-waitq);
 + rc = IRQ_HANDLED;
 + }
 + }
   while (readl(drv_data-reg_base + MV64XXX_I2C_REG_CONTROL) 
   MV64XXX_I2C_REG_CONTROL_IFLG) {
   status = readl(drv_data-reg_base + MV64XXX_I2C_REG_STATUS);
 @@ -446,6 +493,81 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data 
 *drv_data, struct i2c_msg *msg,
   return drv_data-rc;
  }
  
 +static int
 

Re: [PATCH 1/9] I2C: mv64xxx: work around signals causing I2C transactions to be aborted

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 08:37:43AM +0200, Jean-Francois Moine wrote:
 On Thu, 16 May 2013 21:30:59 +0100
 Russell King rmk+ker...@arm.linux.org.uk wrote:
 
  Do not use interruptible waits in an I2C driver; if a process uses
  signals (eg, Xorg uses SIGALRM and SIGPIPE) then these signals can
  cause the I2C driver to abort a transaction in progress by another
  driver, which can cause that driver to fail.  I2C drivers are not
  expected to abort transactions on signals.
 
 Hi Russell,
 
 I had the same problem with my dove drm driver, but I don't fully agree
 with your solution.
 
 Using wait_event_timeout() stops the system, and reading the EDID from
 an external screen may take some time.

It doesn't stop the system.  It stops the process until that has completed.
Using the DRM TDA19988 driver, things are nice and quick while reading
the EDID, because it does reads an entire EDID block using one message.

Blocking the signals like you do has exactly the same effect - you
prevent the I2C driver being interrupted by signals.
--
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: [PATCH 3/9] I2C: mv64xxx: use devm_ioremap_resource()

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 11:23:16AM +0200, Jean-Francois Moine wrote:
 On Thu, 16 May 2013 21:33:09 +0100
 Russell King rmk+ker...@arm.linux.org.uk wrote:
 
  Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
  unchecked ioremap() return from this driver by using the devm_*
  API for requesting and ioremapping resources.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  ---
   drivers/i2c/busses/i2c-mv64xxx.c |   46 
  +-
   1 files changed, 6 insertions(+), 40 deletions(-)
  
  diff --git a/drivers/i2c/busses/i2c-mv64xxx.c 
  b/drivers/i2c/busses/i2c-mv64xxx.c
  index 0339cd8..19cc9bf 100644
  --- a/drivers/i2c/busses/i2c-mv64xxx.c
  +++ b/drivers/i2c/busses/i2c-mv64xxx.c
   [snip]
  @@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
   
  rc = i2c_del_adapter(drv_data-adapter);
  free_irq(drv_data-irq, drv_data);
  -   mv64xxx_i2c_unmap_regs(drv_data);
   #if defined(CONFIG_HAVE_CLK)
  /* Not all platforms have a clk */
  if (!IS_ERR(drv_data-clk)) {
 
 The patch does not apply: it seems it lacks:

I should've mentioned that the patches are against v3.9, not v3.10-rc1.

 + int rc;
 
 - i2c_del_adapter(drv_data-adapter);
 + rc = i2c_del_adapter(drv_data-adapter);
 
 - return 0;
 + return rc;
 
 BTW, is this return code useful?

No it isn't.  Removals *can't* fail.  Once the remove function is called,
the device _will_ be unbound no matter what the return value of that
function is.  The module will also be unloaded (if that's why it's being
unbound) whether or not you return an error.

So, removals must always succeed.  Why their return type hasn't been void
from the start I've no idea - but that's a question for Patrick Mochel
who implemented the original driver model design.
--
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: [PATCH 9/9] I2C: mv64xxx: fix race between FSM/interrupt and process context

[Russell King - ARM Linux]

On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote:
   {
  switch(drv_data-action) {
  case MV64XXX_I2C_ACTION_SEND_RESTART:
  +   /* We should only get here if we have further messages */
  +   BUG_ON(drv_data-num_msgs == 0);
  +
 
 ...
 
  @@ -453,16 +463,20 @@ static int
   mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
   {
  struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
  -   int i, rc;
  +   int rc, ret = num;
   
  -   for (i = 0; i  num; i++) {
  -   rc = mv64xxx_i2c_execute_msg(drv_data, msgs[i],
  -   i == 0, i + 1 == num);
  -   if (rc  0)
  -   return rc;
  -   }
  +   BUG_ON(drv_data-msgs != NULL);
 
 Can't we handle this more gracefully than to halt the kernel? Like
 WARN_ON and resetting the controller or disabling the bus or...

Well, the latter really is something which should never ever happen,
and if it does happen it can only really be because something's
screwed up in the locking in the I2C layer.

The former is more probable, and I thought about that, but I don't
have any good alternative solution.  Given the problems I've seen,
I don't think resetting the controller is really an option, because
that'll likely cause the bus to lock (that's the original problem
which I'm trying to solve in this patch.)  The thing really does
have to work according to the I2C protocol otherwise stuff goes
irrecoverably wrong to the point of needing an entire system reset.
--
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

[PATCH 0/9] Fix Marvell mv63xxx I2C driver

[Russell King - ARM Linux]

This patch series fixes a whole chunk of problems with this driver,
discovered with the Marvell Armada 510 chip on the Solid-run cubox.

Most notable of these is the I2C driver aborting a transaction
because a signal is pending - which might be SIGPIPE or SIGALRM
for the process.  Meanwhile, the calling driver may be in the
middle of a critical read-modify-write cycle on a device register,
causing it to fail.

Other problems are race conditions in the handling of multi-part
messages, where we end up sending multiple start conditions -
sometimes more times than we have i2c_msg's to send.

Lastly is the rather poor probe error handling, which ranges from
non-existent to lacking propagating the provided error code.
--
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: [i2c-mxs] Broken DMA operations from within a module

[Russell King - ARM Linux]

On Wed, Feb 13, 2013 at 01:00:50PM +0100, Enrico Scholz wrote:
 E.g. when doing something like
 
   struct i2c_msg xfer = {
.buf = something,
...
 };
 
 ret = i2c_transfer(client-adapter, xfer, 1);
 
 random data will be sent because buf is not DMA mapable (something is
 located in the .strtab section within module memory). MXS has a memory
 layout like

DMA buffers must be located in lowmem *always*.  This means placing them
in the .data segment does not work.  USB has been through this already
and it's well known there...
--
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: [PATCH] I2C: EXYNOS: Add slave support to i2c

[Russell King - ARM Linux]

On Mon, Dec 10, 2012 at 03:02:28PM +0530, Giridhar Maruthy wrote:
 Hi Russel,
 
 Thanks for review and please find my replies below.
 
 On 7 December 2012 18:03, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Fri, Dec 07, 2012 at 05:33:17PM +0530, Tushar Behera wrote:
  On 12/03/2012 05:46 PM, Giridhar Maruthy wrote:
   This patch adds slave support to i2c. The dt entry i2c-mode
   decides at probe time if the controller needs to work in
   slave mode and the controller is accordingly programmed.
 
  (I don't have the original patches.)
 
  Hmm.  How has slave-mode support been tested?
 
 I have taken two I2C controllers in exynos5250.
 I configured one as slave and the other as master port.
 Then physically connected the pins of the two ports.
 
  Remembering that I2C slave devices do not initiate bus accesses, all
  accesses will be started by some other master.  How are you dealing
  with the bytes received from the master,
 
 I run the slave read application in background.
 The resulting slave read will sleep till all bytes are received
 (wait_event_interruptible)

Oh god no, not more broken interruptible waits in I2C code.  I've seen
already the results of crap like this with a kernel I2C peripheral driver
falling over because the I2C layer returns -ERESTARTcrap with the Marvell
I2C bus adapter.

 once the master sends the data, an ack is given out by the slave controller
 in the ISR and the data is cached in the buffer(
 buffer sent by slave receive application).
 After all data is received, the read wakes up and the
 slave receive program gets the data.
 
  and how are you returning a response to the master in reply to a read 
  request?
 
 Similarl logic works in slave transmit mode (read request). Slave
 sleeps till the master initiates the transfer.
 
  We had support for this on PXA I2C through a callback from the driver
  into PXA code (used for the Psion Teklogix Netbook device) and it worked
  really well, but what you can't do is use the standard I2C interfaces
  for slave mode.
 
 I have a question here. Since the same framework can work for both
 master and slave, is there any technical limitations I have overseen
 which prevents the slave mode to work?

I don't really call your description above as working - it sounds like
a bodge - it sounds like trying to bend a layer designed for master mode
to sort-of-maybe-if-the-wind-is-in-the-right-direction-and-the-cows-are-
all-standing-up-work.  What if the application is trying to read in slave
mode while the master is also trying to read?  How do you deal with that?
What about the converse?  What if the application is trying to write data
and the master also issues a write request?

Finally, what about a multi-master situation?  I can see no way for your
implementation to have any hope of working there because there is no
distinction between operating the interface in master mode and slave mode.
--
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: [PATCH] I2C: EXYNOS: Add slave support to i2c

[Russell King - ARM Linux]

On Fri, Dec 07, 2012 at 05:33:17PM +0530, Tushar Behera wrote:
 On 12/03/2012 05:46 PM, Giridhar Maruthy wrote:
  This patch adds slave support to i2c. The dt entry i2c-mode
  decides at probe time if the controller needs to work in
  slave mode and the controller is accordingly programmed.

(I don't have the original patches.)

Hmm.  How has slave-mode support been tested?

Remembering that I2C slave devices do not initiate bus accesses, all
accesses will be started by some other master.  How are you dealing
with the bytes received from the master, and how are you returning a
response to the master in reply to a read request?

We had support for this on PXA I2C through a callback from the driver
into PXA code (used for the Psion Teklogix Netbook device) and it worked
really well, but what you can't do is use the standard I2C interfaces
for slave mode.
--
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: [PATCH 1/3] i2c: at91: fix compilation warning

[Russell King - ARM Linux]

On Fri, Nov 23, 2012 at 10:09:02AM +0100, ludovic.desroc...@atmel.com wrote:
 From: Ludovic Desroches ludovic.desroc...@atmel.com

Always include the warning itself in the commit log.
--
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: [PATCH 207/493] i2c: remove use of __devinit

[Russell King - ARM Linux]

On Tue, Nov 20, 2012 at 09:20:46AM +0100, Jean Delvare wrote:
 Hi Bill,
 
 On Mon, 19 Nov 2012 13:22:36 -0500, Bill Pemberton wrote:
  CONFIG_HOTPLUG is going away as an option so __devinit is no longer
  needed.
 
 Can you please point me/us to the discussion explaining the rationale
 behind this move, and the explanation of what will be done exactly?
 While I can easily understand that we want to drop CONFIG_HOTPLUG and
 always enable hot-plug support, I don't see where we are going with
 removing __devinit annotations and the like.

It's actually very simple to understand.

1. CONFIG_HOTPLUG is going away; it's already defined to always be 'Y'.
2. This means that the the devinit sections will not be discarded anymore.
3. As a result, there's no point the devinit sections existing anymore.
4. As there's no devinit sections, the __devinit marker is entirely
   redundant and useless.

The reason this is being done is because the benefit to cost ratio of this
is far too high; it's well proven that people constantly get these markings
wrong, and with most kernels having had hotplug enabled anyway, it's not
providing much in the way of space saving benefit over the number of section
conflicts it causes.  So, it's been decided a few years ago that this is
going to happen, with that justification, and it's been accepted by 300
odd kernel developers in at least one kernel summit when it was talked
about... and it's been mentioned on mailing lists several times.
--
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: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Sat, Oct 27, 2012 at 04:32:24PM +0200, Jean Delvare wrote:
 On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote:
  On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
   The original idea of using the hole in the i2c_msg structure is from
   David Brownell, who was apparently familiar with such practice, so I
   assumed it was OK. Actually I still assume it is, until someone comes
   with an supported architecture where it is not.
  
  According to Al Viro, that would be m68k.
 
 OK... So to make things clear, let me ask Al directly about it. Al, can
 you please tell us if:
 
 --- a/include/uapi/linux/i2c.h
 +++ b/include/uapi/linux/i2c.h
  struct i2c_msg {
   __u16 addr; /* slave address*/
   __u16 flags;
   __u16 len;  /* msg length   */
 + __u16 transferred;  /* actual bytes transferred */
   __u8 *buf;  /* pointer to msg data  */
  };
  
 would break binary compatibility on m68k or any other architecture you
 are familiar with? Note that struct i2c_msg isn't declared with
 attribute packed, so my assumption was that pointer buf was align on
 at least 4 bytes, leaving a hole between len and buf. Am I wrong?

So, you're attitude here is I don't believe you, you are lying.  Well,
if you have that level of distrust of your fellow developers, then you
don't deserve to be a Linux developer at all - and given that why should
I take any notice of you in the future over I2C stuff.

Especially as you've just proven that you don't know how to deal properly
with APIs...

Quite frankly I find your attitude towards me here totally disgusting and
insulting.

Not surprisingly, I didn't lie (I don't lie) and so I didn't make up
that M68k is one such architecture, and you've now had the confirmation
from Al.

So, I look forward to your apology for _implying_ that I'm a liar over
this issue, or if not, your resignation as I2C maintainer.

Thanks.
--
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: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
 Hi Felipe, Shubhrajyoti,
 
 On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
  From: Shubhrajyoti D shubhrajy...@ti.com
  
  In case of a NACK, it's wise to tell our clients
  drivers about how many bytes were actually transferred.
  
  Support this by adding an extra field to the struct
  i2c_msg which gets incremented the amount of bytes
  actually transferred.
  
  Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
  Signed-off-by: Felipe Balbi ba...@ti.com
  ---
   include/uapi/linux/i2c.h | 1 +
   1 file changed, 1 insertion(+)
  
  diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
  index 0e949cb..4b35c9b 100644
  --- a/include/uapi/linux/i2c.h
  +++ b/include/uapi/linux/i2c.h
  @@ -77,6 +77,7 @@ struct i2c_msg {
   #define I2C_M_NO_RD_ACK0x0800  /* if 
  I2C_FUNC_PROTOCOL_MANGLING */
   #define I2C_M_RECV_LEN 0x0400  /* length will be first 
  received byte */
  __u16 len;  /* msg length   */
  +   __u16 transferred;  /* actual bytes transferred */
  __u8 *buf;  /* pointer to msg data  */
   };
 
 On the principle I am fine with this, however you also need to define
 who should initialize this field, and to what value.

You also miss one very very very big point.  This will break every I2C
using userspace program out there unless it is rebuilt - this change will
require the exact right version of those userspace programs for the
kernel that they're being used on.

Now that we have the userspace API headers separated, this is now much
easier to detect: a patch which touches a uapi header needs much more
careful consideration than one which doesn't.

So no, strong NAK.  This is not how we treat userspace.

If we want to change userspace API then we do it in a sane manner, where
we provide the new functionality in a way that it doesn't break existing
users.  There's two ways to do this:

1. Leave the existing struct alone, introduce a new one with new ioctls.
   Schedule the removal of the old interfaces for maybe 10 years time.

2. Rename the existing struct (eg struct old_i2c_msg), and create a new
   struct called i2c_msg.  Rename the existing ioctls to have OLD_ in
   their names.  Provide the existing ioctls under those names, and
   make them print a warning once that userspace programs need updating.
   Schedule the removal of the old interfaces for a shorter number of
   years than (1);

Remember all those old syscalls we have... this is no different from
those.
--
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: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
 On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
  On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
   Hi Felipe, Shubhrajyoti,
   
   On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
From: Shubhrajyoti D shubhrajy...@ti.com

In case of a NACK, it's wise to tell our clients
drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct
i2c_msg which gets incremented the amount of bytes
actually transferred.

Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
Signed-off-by: Felipe Balbi ba...@ti.com
---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cb..4b35c9b 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -77,6 +77,7 @@ struct i2c_msg {
 #define I2C_M_NO_RD_ACK0x0800  /* if 
I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN 0x0400  /* length will be first 
received byte */
__u16 len;  /* msg length   
*/
+   __u16 transferred;  /* actual bytes transferred 
*/
__u8 *buf;  /* pointer to msg data  
*/
 };
   
   On the principle I am fine with this, however you also need to define
   who should initialize this field, and to what value.
  
  You also miss one very very very big point.  This will break every I2C
  using userspace program out there unless it is rebuilt - this change will
  require the exact right version of those userspace programs for the
  kernel that they're being used on.
 
 How that? The extra field is added in a hole, so we don't change the
 struct size nor the relative positions of existing fields. Why would
 user-space care?

You know the layout of that struct for certain across all Linux supported
architectures, including some of the more obscure ones which may not
require pointers to be aligned?
--
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: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
 On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote:
  On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
   On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
You also miss one very very very big point.  This will break every I2C
using userspace program out there unless it is rebuilt - this change 
will
require the exact right version of those userspace programs for the
kernel that they're being used on.
   
   How that? The extra field is added in a hole, so we don't change the
   struct size nor the relative positions of existing fields. Why would
   user-space care?
  
  You know the layout of that struct for certain across all Linux supported
  architectures, including some of the more obscure ones which may not
  require pointers to be aligned?
 
 No I don't, I naively supposed it would be fine. I would expect gcc to
 always align pointers even when the architecture doesn't need them to
 be, for performance reasons, even when it doesn't have to, as long as
 attribute packed isn't used. After all, integers don't _have_ to be
 aligned on x86, but gcc aligns them.
 
 The original idea of using the hole in the i2c_msg structure is from
 David Brownell, who was apparently familiar with such practice, so I
 assumed it was OK. Actually I still assume it is, until someone comes
 with an supported architecture where it is not.

According to Al Viro, that would be m68k.
--
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: RT throttling and suspend/resume (was Re: [PATCH] i2c: omap: revert i2c: omap: switch to threaded IRQ support)

[Russell King - ARM Linux]

On Mon, Oct 22, 2012 at 09:47:06AM -0700, Kevin Hilman wrote:
 Peter Zijlstra pet...@infradead.org writes:
 
  On Fri, 2012-10-19 at 16:54 -0700, Kevin Hilman wrote:
 
  So I did the same thing for my ARM SoC, and it definitley stops the RT
  throttling.  
  
  However, it has the undesriable (IMO) side effect of making timed printk
  output rather unhelpful for debugging suspend/resume since printk time
  stays constant throughout suspend/resume no matter how long you
  sleep. :(
  
  So does that mean we have to choose between useful printk times during
  suspend/resume or functioning IRQ threads during suspend/resume ?
 
  Urgh.. this was not something I considered. This being primarily the
  sched_clock infrastructure and such.
 
  So what exactly is the problem with the suspend resume thing (its not
  something I've ever debugged), is all you need a clean break between pre
  and post suspend, or do you need the actual time the machine was gone?
 
 I think it's more a question of what people are used to.  I think folks
 used to debugging suspend/resume (at least on ARM) are used to having
 the printk timestamps reflect the amount of time the machine was gone.
 
 With a sched_clock() that counts during suspend, that feature doesn't
 work anymore.  I'm not sure that this feature is a deal breaker, but it
 has been convenient.

IMHO, this isn't a deal breaker, it's nothing more than cosmetic issue.
The big hint about the sched_clock() behaviour is partly in the name,
and the behaviour you get from the scheduler if you don't give it what
it wants.  The scheduler sets the requirements for sched_clock(), not
printk, so if we have to fix sched_clock() to get correct behaviour
from the scheduler, that's what we have to do irrespective of cosmetic
printk issues.

And there are many other ways to measure time off in suspend... we have
at least three other functions which return time, and which will return
updated time after a resume event.
--
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: [PATCH] dma: add new DMA control commands

[Russell King - ARM Linux]

On Thu, Oct 18, 2012 at 02:45:41PM +0800, Huang Shijie wrote:
 于 2012年10月18日 14:18, Vinod Koul 写道:
 Why cant you do start (prepare clock etc) when you submit the descriptor
 to dmaengine. Can be done in tx_submit callback.
 Similarly remove the clock when dma transaction gets completed.
 I ever thought this method too.

 But it will become low efficient in the following case:

   Assuming the gpmi-nand driver has to read out 1024 pages in one  
 _SINGLE_ read operation.
 The gpmi-nand will submit the descriptor to dmaengine per page. So with  
 your method,
 the system will repeat the enable/disable dma clock 1024 time. At every  
 enable/disable dma clock,
 the system has to enable the clock chain and it's parents ...

And what if you stop using clk_prepare_enable(), and prepare the clock
when the channel is requested and only use clk_enable() in the tx_submit
method?
--
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: [PATCH] i2c: at91: add dma support

[Russell King - ARM Linux]

On Wed, Oct 10, 2012 at 03:43:07PM +0200, ludovic.desroc...@atmel.com wrote:
 + txdesc = chan_tx-device-device_prep_slave_sg(chan_tx, dma-sg,
 + 1, DMA_TO_DEVICE, DMA_PREP_INTERRUPT | DMA_CTRL_ACK, NULL);

No, a while back the DMA engine API changed.  It no longer takes
DMA_TO_DEVICE/DMA_FROM_DEVICE but DMA_MEM_TO_DEV and DMA_DEV_TO_MEM.

 + /* Keep in mind that we won't use dma to read the last two bytes */
 + at91_twi_irq_save(dev);
 + dma_addr = dma_map_single(dev-dev, dev-buf, dev-buf_len - 2,
 +   DMA_FROM_DEVICE);

Ditto.

 + dma-xfer_in_progress = true;
 + cookie = rxdesc-tx_submit(rxdesc);
 + if (dma_submit_error(cookie)) {

tx_submit never errors (anymore.)

 + slave_config.direction = DMA_TO_DEVICE;

Same comment as for the other directions.  Note that DMA engine drivers
really should ignore this parameter now, and DMA engine users should phase
it out.

 + if (dmaengine_slave_config(dma-chan_tx, slave_config)) {
 + dev_err(dev-dev, failed to configure tx channel\n);
 + ret = -EINVAL;
 + goto error;
 + }
 +
 + slave_config.direction = DMA_FROM_DEVICE;

Ditto.
--
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: [PATCH 1/3] i2c: omap: Do not enable the irq always

[Russell King - ARM Linux]

On Mon, Oct 08, 2012 at 02:35:14PM +0530, Shubhrajyoti D wrote:
 Enable the irq in the transfer and disable it after the
 transfer is done.

This description is missing the most vital bits of information.  In years
to come, someone will wonder why has this changed and there's no reason
given in the commit log.

Commit logs which are just mere translations from patch to English are
evil.  Instead, good commit logs briefly state what is being done and
then go on to describe _why_ the change is necessary.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c |3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index b6c6b95..ce41596 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
   if (IS_ERR_VALUE(r))
   goto out;
  
 + enable_irq(dev-irq);
   r = omap_i2c_wait_for_bb(dev);
   if (r  0)
   goto out;
 @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
  
   omap_i2c_wait_for_bb(dev);
  out:
 + disable_irq(dev-irq);
   pm_runtime_mark_last_busy(dev-dev);
   pm_runtime_put_autosuspend(dev-dev);
   return r;
 @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev)
   goto err_unuse_clocks;
   }
  
 + disable_irq(dev-irq);
   adap = dev-adapter;
   i2c_set_adapdata(adap, dev);
   adap-owner = THIS_MODULE;
 -- 
 1.7.5.4
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
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: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
 On 14/06/12 19:36, Mark Brown wrote:
 On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:

 Device Tree. However, we have just as much control by keeping them
 in separate structs in the C file and selecting the right one using
 the compatible sting.

 You're not understanding Linus' point.  The compatible string isn't
 useful here because properties like the maximum clock rate of the bus
 depend on the board design, not the silicon.  The controller may be
 perfectly happy to run at a given rate but other devices on the bus or
 the electrical engineering of the PCB itself may restrict this further.

 And you're not understanding mine. ;)

 You can have multiple compatible strings for a single driver. Which one  
 you reference from the Device Tree will dictate which group of settings  
 are used, including variation of clock rates.

However, if you list these settings separately, rather than selecting them
based on a compatible string, it means when other board designs come along,
you don't have to modify the kernel code to provide yet another compatible
to settings translation in the code - you can keep all that information
entirely within DT with no kernel code mods.

I thought that was partly the point of DT...
--
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: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:37PM +0300, Felipe Balbi wrote:
 stat  BIT(1) is the same as BIT(1), so let's
 simplify things a bit by removing stat  from
 all omap_i2c_ack_stat() calls.

This doesn't feel right, and the explanation is definitely wrong.

stat  BIT(1) is not the same as BIT(1) _unless_ you're saying that
stat always has BIT(1) already set.  Can you guarantee that in this code?
If so, how?

What happens if you read the status register, and it has bit 1 clear.
immediately after the read, the status register bit 1 becomes set, and
then you write bit 1 set (because you've dropped the stat  BIT(1) from
the code.)

Is it not going to acknowledge that bit-1-set but because you haven't
read it, you're going to miss that event?

This feels like a buggy change to me.
--
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: [PATCH 06/17] i2c: omap: improve 1p153 errata handling

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:39PM +0300, Felipe Balbi wrote:
 -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
  {
 - unsigned long timeout = 1;
 + unsigned long   timeout = 1;
 + u16 stat;

Eww, no, not more of this lets add tabs to align auto variable
declarations.  This is detrimental when you add another variable whose
type is longer than the current indentation - you have to reformat the
entire block.

It's really not worth it.  Stick to just using one space, like the rest
of the kernel code.  Like the code which Linus writes.  And thereby help
to avoid future pointless churn whinges.
--
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: [PATCH 09/17] i2c: omap: ack IRQ in parts

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:42PM +0300, Felipe Balbi wrote:
 According to flow diagrams on OMAP TRMs,
 we should ACK the IRQ as they happen.

You don't explain that you're adding a new dev_err() statement into the
driver for a missing acknowledge.

What if you're probing for a device - can this cause spam to the kernel
console?  What if you have protocol mangling enabled with the ignore
lack of ack bit set ?

 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c |   29 +
  1 file changed, 17 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index f978b14..c00ba7d 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
   }
  
  complete:
 - /*
 -  * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
 -  * acked after the data operation is complete.
 -  * Ref: TRM SWPU114Q Figure 18-31
 -  */
 - omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat 
 - ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
 - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
 -
 - if (stat  OMAP_I2C_STAT_NACK)
 + if (stat  OMAP_I2C_STAT_NACK) {
 + dev_err(dev-dev, No Acknowledge\n);
   err |= OMAP_I2C_STAT_NACK;
 + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
 + omap_i2c_complete_cmd(dev, err);
 + return IRQ_HANDLED;
 + }
  
   if (stat  OMAP_I2C_STAT_AL) {
   dev_err(dev-dev, Arbitration lost\n);
   err |= OMAP_I2C_STAT_AL;
 + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
 + omap_i2c_complete_cmd(dev, err);
 + return IRQ_HANDLED;
   }
  
   /*
 @@ -989,12 +988,18 @@ complete:
  
   if (stat  OMAP_I2C_STAT_ROVR) {
   dev_err(dev-dev, Receive overrun\n);
 - dev-cmd_err |= OMAP_I2C_STAT_ROVR;
 + err |= OMAP_I2C_STAT_ROVR;
 + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
 + omap_i2c_complete_cmd(dev, err);
 + return IRQ_HANDLED;
   }
  
   if (stat  OMAP_I2C_STAT_XUDF) {
   dev_err(dev-dev, Transmit underflow\n);
 - dev-cmd_err |= OMAP_I2C_STAT_XUDF;
 + err |= OMAP_I2C_STAT_XUDF;
 + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
 + omap_i2c_complete_cmd(dev, err);
 + return IRQ_HANDLED;
   }
   } while (stat);
  
 -- 
 1.7.10.4
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
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: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 04:48:56PM +0530, Shilimkar, Santosh wrote:
 On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi ba...@ti.com wrote:
  otherwise we could get our IRQ line disabled due
  to many spurious IRQs.
 
  Signed-off-by: Felipe Balbi ba...@ti.com
  ---
   drivers/i2c/busses/i2c-omap.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
  index fc5b8bc..5b78a73 100644
  --- a/drivers/i2c/busses/i2c-omap.c
  +++ b/drivers/i2c/busses/i2c-omap.c
  @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
                 }
         } while (stat);
 
  -       return count ? IRQ_HANDLED : IRQ_NONE;
  +       return IRQ_HANDLED;
 
 no sure if this is correct. if you have IRQ flood and instead of _actually_
 handling it, if you return handled, you still have interrupt pending, right?

The point of returning IRQ_NONE is to indicate to the interrupt layer that
the interrupt you received was not processed by any interrupt handler, and
therefore to provide a way of preventing the system being brought to a halt
though a stuck interrupt line.

So, if you do process an interrupt, you should always return IRQ_HANDLED
even if you couldn't complete its processing (eg, because you've serviced
it 100 times.)
--
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: [PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 11:02:27AM +0200, Linus Walleij wrote:
 On Fri, May 4, 2012 at 8:23 PM, Lee Jones lee.jo...@linaro.org wrote:
 
  This patch will allow the ab8500-core driver to be probed and set up
  when booting when Device Tree is enabled. This includes platform ID
  look-up which identifies the machine it is currently running on. If
  we are undergoing a DT enabled boot, we will refuse to setup each of
  the other ab8500-* devices, as they will be probed individually by DT.
 
  Signed-off-by: Lee Jones lee.jo...@linaro.org
 (...)
  +       else if (np)
  +               ret = of_property_read_u32(np, stericsson,irq-base, 
  ab8500-irq_base);
  +
  +       if (ab8500-irq_base == 0) {
 
 Shouldn't this be (av8500-irq_base == NO_IRQ) now that we're tranisitioning 
 to
 use 0 as NO_IRQ?

No it shouldn't.  We want new drivers to be rejecting on explicitly zero
IRQ numbers and not using NO_IRQ at all.  We want to get rid of NO_IRQ
entirely.

At the moment, NO_IRQ is a marker for this is a mistake which needs
fixing and allows them to be found by grep.

What we need is more of an effort so that platforms do not start numbering
IRQs at zero, but start at one.

(I could fix up SA11x0, which would then allow the SA code to be fixed,
but I've not had time to look at that during this last cycle.  Others need
their maintainers who know the code to fix them.)

I brought up the issue of the new introduction of NO_IRQ in the previously
clean versatile express code, and failed to get any reply.

So, I think what I'll do when I've eliminated all those that I can do is
to remove the definition from our asm/irq.h and cause a load of build
errors in linux-next.  This seems to be the _only_ way to motivate people
into fixing these kinds of issues.  Which is awfully sad.  I'd much
rather people just had the carrot instead but as ever its proven many
times that people just ignore such things.
--
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: [PATCH 1/4 v2] i2c/gpio: add DT support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 18:17 Mon 13 Feb , Karol Lewandowski wrote:
   + - udelay: delay between GPIO operations (may depend on each platform)
   + - timeout: timeout to get data (ms)
  
  
  If these are really needed then I would prefer to have these fully
  qualified (with unit type -ms/-millisecs appended).
  
  Regulator framework, with its -microvolt/-microamp, serve here as
  prime example of being quite descriptive (one doesn't neet to look up
  the docs). Please see:
  
http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
 timeout are usualy in ms I don't really see the need of -ms or so

Which is obviously total crap for udelay, which would be in _micro_seconds.

--
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: [PATCH 1/4 v2] i2c/gpio: add DT support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 10:08 Mon 20 Feb , Russell King - ARM Linux wrote:
  On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD 
  wrote:
   On 18:17 Mon 13 Feb , Karol Lewandowski wrote:
 + - udelay: delay between GPIO operations (may depend on each 
 platform)
 + - timeout: timeout to get data (ms)


If these are really needed then I would prefer to have these fully
qualified (with unit type -ms/-millisecs appended).

Regulator framework, with its -microvolt/-microamp, serve here as
prime example of being quite descriptive (one doesn't neet to look up
the docs). Please see:

  http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
   timeout are usualy in ms I don't really see the need of -ms or so
  
  Which is obviously total crap for udelay, which would be in _micro_seconds.
 agreed but here on i2c gpio I never see timetout as udelay so I don't see
 the mandatory to force the name in the binding
 
 futhermore it's maybe linux specific

Stop grabbing at straws.  There's nothing linux specific about the units
of specification.

What is linux specific is specifying the _delay_ rather than specifying
the bus frequency.  So as soon as you're trying to justify not adding
the units because they may be linux specific, you've already lost that
argument by using a delay rather than a bus frequency.  You can't have
it both ways.

Moreover, mixing microseconds and milliseconds in the properties for a
device is absolutely insane.

So, which ever way, your patch as it currently stands is wrong and broken.
--
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: [PATCH 1/4 v2] i2c/gpio: add DT support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 13:58 Mon 20 Feb , Russell King - ARM Linux wrote:
  On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
  wrote:
   On 14:37 Mon 20 Feb , Jean-Christophe PLAGNIOL-VILLARD wrote:
On 12:50 Mon 20 Feb , Russell King - ARM Linux wrote:
 On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe 
 PLAGNIOL-VILLARD wrote:
  On 10:08 Mon 20 Feb , Russell King - ARM Linux wrote:
   On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe 
   PLAGNIOL-VILLARD wrote:
On 18:17 Mon 13 Feb , Karol Lewandowski wrote:
  +   - udelay: delay between GPIO operations (may depend on 
  each platform)
  +   - timeout: timeout to get data (ms)
 
 
 If these are really needed then I would prefer to have these 
 fully
 qualified (with unit type -ms/-millisecs appended).
 
 Regulator framework, with its -microvolt/-microamp, serve 
 here as
 prime example of being quite descriptive (one doesn't neet to 
 look up
 the docs). Please see:
 
   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
timeout are usualy in ms I don't really see the need of -ms or 
so
   
   Which is obviously total crap for udelay, which would be in 
   _micro_seconds.
  agreed but here on i2c gpio I never see timetout as udelay so I 
  don't see
  the mandatory to force the name in the binding
  
  futhermore it's maybe linux specific
 
 Stop grabbing at straws.  There's nothing linux specific about the 
 units
 of specification.
 
 What is linux specific is specifying the _delay_ rather than 
 specifying
 the bus frequency.  So as soon as you're trying to justify not adding
 the units because they may be linux specific, you've already lost that
 argument by using a delay rather than a bus frequency.  You can't have
 it both ways.
 
 Moreover, mixing microseconds and milliseconds in the properties for a
 device is absolutely insane.
 
 So, which ever way, your patch as it currently stands is wrong and 
 broken.
 no what I said is the binding timeout is maybe linux specific for i2c 
gpio
   
   I do not argue about that here we do not even disucss about the bus 
   frequency
   but the specific bining to the i2c algo bit for it's internal timeout
   
   the timeout is used to do not end in an infinite loop while ready the scl 
   on
   slow device
  
  The patch is still wrong and broken.
  
  As you're not listening to me at all, I've lost patience, so I'm just going
  to say this:
  
  Explicit NAK on this patch.
  
  When you feel like you can _constructively_ _consider_ the point that both
  Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
  to continue this discussion.  If not, don't waste your time writing another
  email.  I hope that's plain.

 I do not discuss about the U_N_I_T_S at all in this reply
 so the NACK is no revelent

LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
TIMEOUT.

I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.

I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.

GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
OR DELAYS ARE APPROPRIATE IN THIS CASE.

ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
*U*N*I*T*S*.

HAVE YOU GOT THE FUCKING MESSAGE YET?

SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
--
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: [PATCH 1/4 v2] i2c/gpio: add DT support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 03:51:37PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 13:51 Mon 20 Feb , Russell King - ARM Linux wrote:
  On Mon, Feb 20, 2012 at 02:35:57PM +0100, Jean Delvare wrote:
   On Mon, 20 Feb 2012 12:50:54 +, Russell King - ARM Linux wrote:
What is linux specific is specifying the _delay_ rather than specifying
the bus frequency.  So as soon as you're trying to justify not adding
the units because they may be linux specific, you've already lost that
argument by using a delay rather than a bus frequency.  You can't have
it both ways.
   
   While I am not much into DT and did not follow this thread too
   carefully... I seem to understand that the dispute is mainly on
   frequency vs. udelay specification for the bus speed, Jean-Christophe
   arguing that hardware-specific delays are added when changing e.g. a
   GPIO pin output value and thus the frequency can't be guaranteed. Do I
   get this right?
  
  This sub-thread is more about the units of the properties rather
  than the properties themselves.
  
  What's being proposed is to have two properties, one named 'udelay'
  which takes microseconds, and one named 'timeout' which takes
  milliseconds.
  
  I'm saying that's a completely absurd proposal, as the proposal is
  for two opaque numeric properties with different units.  At least
  make the units the same, or as Karol said, incorporate the units
  into the property names.
  
  At least we can then create new properties in the future of we need
  to change the units, rather than thinking up a different name for
  'timeout'.
 
 please read the binding
 
 we have 2 properties
 
  - udelay: delay between GPIO operations (may depend on each platform)
  - timeout: timeout to get data (ms)
 
  please do not mixed them together
 
 udelay is related to bus frequency
 
 timeout is implelentation detail, that allow to parameter the timeout og i2c
 bit algo when reading the scl on slow device

FOR FUCKING SAKE.  MILLISECONDS FOR SOME STUFF VS MICROSECONDS FOR OTHER
STUFF IS BAD NEWS.  FIX THIS AND I WILL WITHDRAW MY NACK.  CONTINUE BEING
OBSTRUCTIVE OVER THIS AND MY NACK STANDS.

I HOPE USING BIG LETTERS HELPS TO GET MY POINT THROUGH.
--
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: [PATCH 1/4 v2] i2c/gpio: add DT support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 15:00 Mon 20 Feb , Russell King - ARM Linux wrote:
  On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
  wrote:
   On 13:58 Mon 20 Feb , Russell King - ARM Linux wrote:
On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe 
PLAGNIOL-VILLARD wrote:
 On 14:37 Mon 20 Feb , Jean-Christophe PLAGNIOL-VILLARD wrote:
  On 12:50 Mon 20 Feb , Russell King - ARM Linux wrote:
   On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe 
   PLAGNIOL-VILLARD wrote:
On 10:08 Mon 20 Feb , Russell King - ARM Linux wrote:
 On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe 
 PLAGNIOL-VILLARD wrote:
  On 18:17 Mon 13 Feb , Karol Lewandowski wrote:
+   - udelay: delay between GPIO operations (may 
depend on each platform)
+   - timeout: timeout to get data (ms)
   
   
   If these are really needed then I would prefer to have 
   these fully
   qualified (with unit type -ms/-millisecs appended).
   
   Regulator framework, with its -microvolt/-microamp, 
   serve here as
   prime example of being quite descriptive (one doesn't 
   neet to look up
   the docs). Please see:
   
 
   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
  timeout are usualy in ms I don't really see the need of -ms 
  or so
 
 Which is obviously total crap for udelay, which would be in 
 _micro_seconds.
agreed but here on i2c gpio I never see timetout as udelay so I 
don't see
the mandatory to force the name in the binding

futhermore it's maybe linux specific
   
   Stop grabbing at straws.  There's nothing linux specific about 
   the units
   of specification.
   
   What is linux specific is specifying the _delay_ rather than 
   specifying
   the bus frequency.  So as soon as you're trying to justify not 
   adding
   the units because they may be linux specific, you've already lost 
   that
   argument by using a delay rather than a bus frequency.  You can't 
   have
   it both ways.
   
   Moreover, mixing microseconds and milliseconds in the properties 
   for a
   device is absolutely insane.
   
   So, which ever way, your patch as it currently stands is wrong 
   and broken.
   no what I said is the binding timeout is maybe linux specific for 
  i2c gpio
 
 I do not argue about that here we do not even disucss about the bus 
 frequency
 but the specific bining to the i2c algo bit for it's internal timeout
 
 the timeout is used to do not end in an infinite loop while ready the 
 scl on
 slow device

The patch is still wrong and broken.

As you're not listening to me at all, I've lost patience, so I'm just 
going
to say this:

Explicit NAK on this patch.

When you feel like you can _constructively_ _consider_ the point that 
both
Karol and myself have raised with respect to the _U_N_I_T_S_ then feel 
free
to continue this discussion.  If not, don't waste your time writing 
another
email.  I hope that's plain.
  
   I do not discuss about the U_N_I_T_S at all in this reply
   so the NACK is no revelent
  
  LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
  MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
  TIMEOUT.
  
  I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
  
  I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
  
  GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
  NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
  OR DELAYS ARE APPROPRIATE IN THIS CASE.
  
  ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
  *U*N*I*T*S*.
  
  HAVE YOU GOT THE FUCKING MESSAGE YET?
  
  SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
 
 I just said we have 2 properties
 
 - timeout which is expressed in jiffies (today in C) which is at my sense a 
 linux specific
   propertie as it's representing a timeout of the i2c bit algo
   and here I don't see the mandatory to name it timeout-ms or 
 timeout-milisecond

THIS IS IN MILLISECONDS.

 - udelay which is the delay between GPIO operations

THIS IS IN MICROSECONDS.

TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
CONFUSING.  NACK STANDS.
--
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: [PATCH v4] I2C: add CSR SiRFprimaII on-chip I2C controllers driver

[Russell King - ARM Linux]

On Wed, Dec 14, 2011 at 04:55:27PM +0800, Barry Song wrote:
 +static int __devinit i2c_sirfsoc_probe(struct platform_device *pdev)
 +{
 + struct clk *clk;
 +
 + clk = clk_get(pdev-dev, NULL);
 + err = clk_prepare(clk);
 + err = clk_enable(clk);
...
 + clk_disable(clk);
 +
 + dev_info(pdev-dev,  I2C adapter ready to operate\n);
 +
 + return 0;
 +}
 +
 +static int __devexit i2c_sirfsoc_remove(struct platform_device *pdev)
 +{
 + clk_disable(siic-clk);
 + clk_unprepare(siic-clk);
 + clk_put(siic-clk);

This doesn't look right - look at the state which you leave the clk in
upon successful probe, and now look at what you do when you do a remove.

It seems that you disable an already disabled clock to me.
--
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: [PATCH V2 01/14] clk: add helper functions clk_prepare_enable and clk_disable_unprepare

[Russell King - ARM Linux]

On Fri, Nov 11, 2011 at 10:15:56AM +0100, Sascha Hauer wrote:
 On Fri, Nov 11, 2011 at 05:05:47PM +0800, Richard Zhao wrote:
  {
  int ret;
  
  ret = clk_prepare(clk);
  if (ret)
  return ret;
  ret = clk_enable(clk);
  if (ret)
  clk_unprepare(clk);
  return ret;
 
 Yes, looks good.

While this looks like a nice easy solution for converting existing
drivers, I'd suggest thinking about this a little more...

I would suggest some thought is given to the placement of clk_enable()
and clk_disable() when adding clk_prepare(), especially if your existing
clk_enable() function can only be called from non-atomic contexts.

Obviously, the transition path needs to be along these lines:

1. add clk_prepare() to drivers
2. implement clk_prepare() and make clk_enable() callable from non-atomic
   contexts
3. move clk_enable() in drivers to places it can be called from non-atomic
   contexts to achieve greater power savings (maybe via the runtime pm)

and where a driver is shared between different sub-architectures which
have non-atomic clk_enable()s, (3) can only happen when all those sub-
architectures have been updated to step (2).
--
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: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Wed, Nov 09, 2011 at 05:01:07PM +0100, Voss, Nikolaus wrote:
 Hi Ryan,
  
   + dev-irq = irq-start;
   + platform_set_drvdata(pdev, dev);
   +
   + dev-clk = clk_get(pdev-dev, twi_clk);
  
  
  This didn't get answered. Can't you just do:
  
  dev-clk = clk_get(pdev-dev, NULL);
  
  and clkdev should figure out the correct clock based on the device pointer?
 
 No, this doesn't work on at91 since the clocks have no dev_id property
 but only con_id. I cannot see a reason for this, but that's the way it's
 done. Multiple hardware interfaces are handled via a lookup table.

If you can, you could move over to doing this via the standard method
and start switching some of this stuff over to using dev_id.

We'll then have another platform starting to use this stuff correctly.
--
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: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 08:44:48PM +0200, Felipe Balbi wrote:
 hi,
 
 On Tue, Nov 08, 2011 at 06:29:55PM +, Russell King - ARM Linux wrote:
  On Tue, Nov 08, 2011 at 05:23:46PM +0200, Felipe Balbi wrote:
   On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/08/2011 03:41 PM, Felipe Balbi :

 +if (cpu_is_at91rm9200()) {  /* AT91RM9200 
 Errata #22 */
 
 I don't think you should be using cpu_is_* on drivers.

It is a common pattern in at91 drivers and has worked for ages.
Do you think it is related to the need to be able to compile the
driver for any SoC in the case of multi-SoC zImage support?
   
   we have drivers compiling on multiple OMAP versions without those hacks.
   Generally, we check the IP revision for that. Don't you have a Revision
   register of some sort ?
  
  I'm not sure what your objection is - this is no different from what
  happens with OMAP when detecting OMAP1,2,3,4 etc.  E.g.:
 
 so ? Instead of saying this to me, you should contact the
 authors/maintainers of those drivers and ask them to clean that up.

Oh for god sake, I was just asking you to clarify your statement in
light of what is currently being done.

Now, let me set something straight.  I've been saying that machine_is_xxx()
should not be used in drivers.  That's a platform thing and platform
specifics should not be in drivers - it should be passed in via DT or
platform data.  That's enforced by the way DT works (Grant's decision
not mine) - with DT you don't have any kind of testable machine ID for
machine_is_xxx() to use.

I've never said that cpu_is_xxx() should not - that's something *other*
people are saying (and quite rightly so) because if we're going to start
sharing drivers between different SoCs (or even architectures - eg, PXA
IP appearing on x86) then it doesn't make sense for the type of SoC to
be tested.  It makes more sense for the revision of the IP implementation
to be checked IFF such information is available.  If not, some other way
of controlling the 'features' needs to be sought.

As far as the use of asm/*.h includes, I've NEVER made any statement
about the use of those in drivers.  In fact, I don't see any reason to
avoid them _provided_ they're standard cross-arch includes.

As for mach/*.h includes, I don't think that I've made any statement
about those either, but at this point - given that we're working towards
a single zImage on ARM - it is _sensible_ to avoid such includes in
drivers.

So, I think your reaction to my statement is way off mark, and you're
attributing statements (that it seems you personally don't agree with)
to me.
--
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: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 09:02:02PM +0200, Felipe Balbi wrote:
 Hi,
 
 On Tue, Nov 08, 2011 at 06:55:25PM +, Russell King - ARM Linux wrote:
   so ? Instead of saying this to me, you should contact the
   authors/maintainers of those drivers and ask them to clean that up.
  
  Oh for god sake, I was just asking you to clarify your statement in
  light of what is currently being done.
  
  Now, let me set something straight.  I've been saying that machine_is_xxx()
  should not be used in drivers.  That's a platform thing and platform
  specifics should not be in drivers - it should be passed in via DT or
  platform data.  That's enforced by the way DT works (Grant's decision
  not mine) - with DT you don't have any kind of testable machine ID for
  machine_is_xxx() to use.
  
  I've never said that cpu_is_xxx() should not - that's something *other*
  people are saying (and quite rightly so) because if we're going to start
  sharing drivers between different SoCs (or even architectures - eg, PXA
  IP appearing on x86) then it doesn't make sense for the type of SoC to
  be tested.  It makes more sense for the revision of the IP implementation
  to be checked IFF such information is available.  If not, some other way
  of controlling the 'features' needs to be sought.
  
  As far as the use of asm/*.h includes, I've NEVER made any statement
  about the use of those in drivers.  In fact, I don't see any reason to
  avoid them _provided_ they're standard cross-arch includes.
  
  As for mach/*.h includes, I don't think that I've made any statement
  about those either, but at this point - given that we're working towards
  a single zImage on ARM - it is _sensible_ to avoid such includes in
  drivers.
  
  So, I think your reaction to my statement is way off mark, and you're
  attributing statements (that it seems you personally don't agree with)
  to me.
 
 If I did, then it's really my fault. But I _do_ remember you complaining
 about uses of asm/gpio.h instead of linux/gpio.h, for example.

Yes, I do complain about those with _good_ reason:
1. it's a strict checkpatch thing.
2. it's a correctness thing which matters as linux/gpio.h is the top-level
   include for gpio stuff, and at some point may have stuff pulled up
   from asm/gpio.h into it (or new stuff added to it.)

Same reasoning for linux/io.h - things get moved out of asm/io.h into
linux/io.h.  One such example is check_signature() which used to be
declared at the asm/io.h level but is now at the linux/io.h level.

And the overriding reason: if I can get them fixed _now_ they won't have
to be fixed in some massive patch in the future when we've got a few
thousand of them to deal with and tonnes of breakage because something
cross-arch changed there.

That point has been *well* proven by my recent clean ups to the mach/gpio.h
includes - which necessitated changing tonnes of mach/gpio.h includes
across the kernel tree.  Had people paid more attention to getting these
includes right, that cleanup would have been much easier.

But, that's something very different from your statement in your previous
message about my alleged stance on *all* asm/*.h includes in drivers,
which is FALSE.

 Now, all the other topics I agree and, in fact, have been pushing for
 that as I can. Specially with regards to IP cores being shared among
 several architectures (see drivers/usb/dwc3 where I have a core driver
 shared between ARM and PCI/x86).

Good, so you've just taken back most of what you said in your previous
message.
--
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: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 09:58:55PM +0200, Felipe Balbi wrote:
 Hi,
 
 On Tue, Nov 08, 2011 at 07:39:30PM +, Russell King - ARM Linux wrote:
  But, that's something very different from your statement in your previous
  message about my alleged stance on *all* asm/*.h includes in drivers,
  which is FALSE.
 
 http://marc.info/?l=linux-arm-kernelm=132077795410718w=2
 
 I don't see me complaining about *all* headers anywhere in that message.
 I did generalize, but I didn't stated you were complaining about *all*
 headers. Go back and read it for yourself.

I really can't believe what you're saying.

You were (actually still is) one of the biggest source of complaints for
drivers using mach/* and asm/* includes and now you're changing your
mind ?

That's an exact quote.

That statement is very clear in its meaning - and it doesn't match your
assertion that it doesn't mean all because it makes no distinction what
so ever between any of those header files.

 You see ? I was asking $author to try and use some revision register in
 order to apply erratas instead of using cpu_is_at91rm9200().

For fuck sake, - AND AGAIN - I was asking you to *qualify* your fucking
statement.  It was you who then launched an attack about irrelevant junk
like what includes and so forth.

You must be drunk this evening.  Please come back once you've sobered up.
--
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: [PATCH] I2C: add CSR SiRFprimaII on-chip I2C controllers driver

[Russell King - ARM Linux]

On Wed, Nov 02, 2011 at 10:39:04AM +, Jamie Iles wrote:
  +   clk = clk_get(pdev-dev, NULL);
  +   if (IS_ERR(clk)) {
  +   err = PTR_ERR(clk);
  +   dev_err(pdev-dev, Clock get failed\n);
  +   goto out;
  +   }
  +
  +   clk_enable(clk);
 
 The return value of clk_enable() should really be checked.

Now that the clk_prepare() patch has been enabled, new drivers should be
written assuming that clk_prepare() will be necessary before clk_enable().

And one may query why it's not possible to use clk_enable()...clk_disable()
around the transfer itself, so the clock can be turned off while the device
is idle.  Obviously if its expecting to be operated in slave mode as well
then you may need to keep the clock enabled.
--
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: [PATCH 1/2] pwm: Add __weak attributed functions for pwm operations

[Russell King - ARM Linux]

On Sun, May 15, 2011 at 01:00:31AM -0700, Dmitry Torokhov wrote:
 On Fri, May 13, 2011 at 06:13:21PM +0530, Mohan Pallaka wrote:
  For chip drivers that support both pwm and non-pwm modes
  would encounter compilation errors if the platform doesn't
  have support for pwm though the chip is programmed to work
  in non-pwm mode. Add __weak attributed pwm functions to avoid
  compilation issues in these scenarios.
  
 
 Russell,
 
 You seem to have authored pwm.h, do you have any objections to this
 change?

This seems to be a recipe for an oops.  Have a look at how
linux/regulator/consumer.h deals with this kind of problem, and notice
that we have CONFIG_HAVE_PWM to indicate whether this interface is
supported or not.
--
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: platform/i2c busses: pm runtime and system sleep

[Russell King - ARM Linux]

On Sat, Feb 19, 2011 at 10:54:57AM +0100, Linus Walleij wrote:
 2011/2/18 Russell King - ARM Linux li...@arm.linux.org.uk:
 
  Do we have any pressing need to convert AMBA stuff?  I haven't heard any
  reason yet to convert them to runtime PM - they don't even make any
  runtime PM calls.
 
  Maybe Linus can comment on the PM stuff as he has SoCs with these in.
  As my boards don't have any sensible PM support, I don't have any
  visibility of what PM facilities would be required.
 
 Sure, basically I ACK Rabins patch and his reasoning for it. (BTW
 Rabin spends most of his days working on the Ux500 SoCs too.)
 
 The runtime PM we need for Ux500 is to switch off silicon core
 voltage first and foremost. The call I've added to switch of a core
 voltage regulator will need to be called when the silicon is idle.
 
 In spi/amba-pl022.c I take the most brutal approach with a recent
 patch: hammer off this core switch (and clock) whenever the hardware
 is not used. This is simple in this driver since it has no state to preserve
 across transfers, it is written such that the core is loaded with the
 appropriate state for each message.
 
 Continuing this approach we run into two problems with this
 and other drivers:
 
 -  Hammering off/on the clock+voltage is causing delays in HW
so what you want is some hysteresis (usually, wait a few us/ms
then switch off) - sort of a takeoff/landing effect.
 
 -  Modelling voltage domains as regulators is nice, but require
us to switch on/off from process context, so we cannot do this
from interrupt handlers.
 
 Both of these problems are solved by elegance if we use runtime
 PM, since it will provide a hysteresis timeout that can be triggered
 from interrupt context and call the idling hooks in process context.

So what's the interdependence with the platform bus that was being talked
about earlier in this thread?
--
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: [PATCH 3/6] i2c/pxa2xx: Add PCI support for PXA I2C controller

[Russell King - ARM Linux]

On Wed, Jan 05, 2011 at 03:08:34PM -0800, Greg KH wrote:
 On Wed, Jan 05, 2011 at 11:03:42PM +, Russell King - ARM Linux wrote:
  On Wed, Jan 05, 2011 at 05:51:00PM +0100, Sebastian Andrzej Siewior wrote:
   +static void plat_dev_release(struct device *dev)
   +{
   + struct ce4100_i2c_device *sd = container_of(dev,
   + struct ce4100_i2c_device, pdev.dev);
   +
   + of_device_node_put(sd-pdev.dev);
   +}
   +static int add_i2c_device(struct pci_dev *dev, int bar,
   + struct ce4100_i2c_device *sd)
   +{
   + struct platform_device *pdev = sd-pdev;
   + struct i2c_pxa_platform_data *pdata = sd-pdata;
  ...
   + pdev-name = ce4100-i2c;
   + pdev-dev.release = plat_dev_release;
   + pdev-dev.parent = dev-dev;
   +
   + pdev-dev.platform_data = pdata;
   + pdev-resource = sd-res;
  ...
   +static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
   + const struct pci_device_id *ent)
   +{
   + sds = kzalloc(sizeof(*sds), GFP_KERNEL);
   + if (!sds)
   + goto err_mem;
   +
   + pci_set_drvdata(dev, sds);
   +
   + for (i = 0; i  ARRAY_SIZE(sds-sd); i++) {
   + ret = add_i2c_device(dev, i, sds-sd[i]);
   + if (ret) {
   + while (--i = 0)
   + platform_device_unregister(sds-sd[i].pdev);
  ...
   +static void __devexit ce4100_i2c_remove(struct pci_dev *dev)
  ...
   + for (i = 0; i  ARRAY_SIZE(sds-sd); i++)
   + platform_device_unregister(sds-sd[i].pdev);
   +
   + pci_disable_device(dev);
   + kfree(sds);
   +}
  
  I see we're still repeating the same mistakes with lifetime rules of
  sysfs objects.
  
  Any struct device which has been registered into the device model can
  remain indefinitely live after it's been unregistered (by, eg, if
  userspace holds a reference to it via sysfs.)
 
 Actually this race is almost not possible these days with the rework
 that Tejun did on sysfs, so it's not easy to test for this anymore.

Is it wise to make such a problematical bug harder to trigger without
completely preventing it triggering?

A different approach was taken with IRQ handling where people were
registering handlers before the driver was ready for it to be called -
request_irq() would explicitly call the handler as soon as it was
registered to provoke bugs.

Surely for these lifetime violations a similar approach should be taken.
Make the kernel more likely to oops should a violation occur before the
developer can get the code out the door.  One way I can think of doing
that is when devices are unregistered but not yet released, place them
on a list which is periodically scanned, and not only is the device
dereferenced by also the release function.

When the use count drops to zero, don't immediately release, but wait
a number of polls.

If either goes away before the device has been released, then we
predictably oops, and the developer gets to know about his violation
of the rules immediately.
--
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: [PATCH 3/6] i2c/pxa2xx: Add PCI support for PXA I2C controller

[Russell King - ARM Linux]

On Wed, Jan 05, 2011 at 05:51:00PM +0100, Sebastian Andrzej Siewior wrote:
 +static void plat_dev_release(struct device *dev)
 +{
 + struct ce4100_i2c_device *sd = container_of(dev,
 + struct ce4100_i2c_device, pdev.dev);
 +
 + of_device_node_put(sd-pdev.dev);
 +}
 +static int add_i2c_device(struct pci_dev *dev, int bar,
 + struct ce4100_i2c_device *sd)
 +{
 + struct platform_device *pdev = sd-pdev;
 + struct i2c_pxa_platform_data *pdata = sd-pdata;
...
 + pdev-name = ce4100-i2c;
 + pdev-dev.release = plat_dev_release;
 + pdev-dev.parent = dev-dev;
 +
 + pdev-dev.platform_data = pdata;
 + pdev-resource = sd-res;
...
 +static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
 + const struct pci_device_id *ent)
 +{
 + sds = kzalloc(sizeof(*sds), GFP_KERNEL);
 + if (!sds)
 + goto err_mem;
 +
 + pci_set_drvdata(dev, sds);
 +
 + for (i = 0; i  ARRAY_SIZE(sds-sd); i++) {
 + ret = add_i2c_device(dev, i, sds-sd[i]);
 + if (ret) {
 + while (--i = 0)
 + platform_device_unregister(sds-sd[i].pdev);
...
 +static void __devexit ce4100_i2c_remove(struct pci_dev *dev)
...
 + for (i = 0; i  ARRAY_SIZE(sds-sd); i++)
 + platform_device_unregister(sds-sd[i].pdev);
 +
 + pci_disable_device(dev);
 + kfree(sds);
 +}

I see we're still repeating the same mistakes with lifetime rules of
sysfs objects.

Any struct device which has been registered into the device model can
remain indefinitely live after it's been unregistered (by, eg, if
userspace holds a reference to it via sysfs.)

Only once the release method has been called is it safe to give up the
memory backing it - and that also goes for the code comprising the
function implementing the release.

This effectively prevents modules having release functions in them -
while you can put module use count manipulation in to prevent unloading,
it effectively prevents the module from being unloaded until you've
unbound the device.

I think you should be trying to use the platform_device_alloc()
interfaces here, rather than trying to reinvent them.  The only issue I
see with that is the of_device_node_put() call.  Maybe OF/DT/device model
people can provide some pointers?  Adding Greg for the device model
maintainer view.
--
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: [PATCH 2/7] arm/pxa2xx: reorganize I2C files

[Russell King - ARM Linux]

On Thu, Nov 25, 2010 at 11:55:20PM +, Ben Dooks wrote:
  diff --git a/arch/arm/include/asm/pxa_i2c.h b/arch/arm/include/asm/pxa_i2c.h
  new file mode 100644
  index 000..f6da8a1
  --- /dev/null
  +++ b/arch/arm/include/asm/pxa_i2c.h

 Anyone an opinion on whther to alter all arch-arm machine includes
 or add a re-direct of plat/i2c.h to linux/i2c/pxa-i2c.h

We're not going to litter arch/arm/include/asm with SoC specific includes.
If we start doing this, we'll end up with thousands of files in
arch/arm/include/asm which have no real business being there.

So there's not much option but to NAK this patch before it gets out of
hand.

The reason for this change seems to be because x86 has a different register
layout, and x86 doesn't have the clk API.  For the former, that can be
dealt with an ifdef along side the register definitions.

For the latter, why not just implement a simple clk API implementation
which always returns success, rather than requiring special headers
for various ARM drivers?
--
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: [PATCH 05/25] drivers/i2c: Use static const char arrays

[Russell King - ARM Linux]

On Tue, Sep 14, 2010 at 01:57:59PM +0100, Jonathan Cameron wrote:
 On 09/14/10 08:36, Lothar Waßmann wrote:
  Hi,
  
  Jonathan Cameron writes:
   Commit message is  somewhat inaccurate...
 
  On 09/13/10 20:47, Joe Perches wrote:
  Signed-off-by: Joe Perches j...@perches.com
  ---
   drivers/i2c/busses/i2c-stu300.c |4 ++--
   1 files changed, 2 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/i2c/busses/i2c-stu300.c 
  b/drivers/i2c/busses/i2c-stu300.c
  index 495be45..2f7c09c 100644
  --- a/drivers/i2c/busses/i2c-stu300.c
  +++ b/drivers/i2c/busses/i2c-stu300.c
  @@ -871,7 +871,7 @@ stu300_probe(struct platform_device *pdev)
struct resource *res;
int bus_nr;
int ret = 0;
  - char clk_name[] = I2C0;
  + char clk_name[sizeof(I2Cx)];
   
dev = kzalloc(sizeof(struct stu300_dev), GFP_KERNEL);
if (!dev) {
  @@ -881,7 +881,7 @@ stu300_probe(struct platform_device *pdev)
}
   
bus_nr = pdev-id;
  - clk_name[3] += (char)bus_nr;
  + sprintf(clk_name, I2C%c, '0' + bus_nr);
  I'm guessing that there are never more than a couple of these.
  Why is this method a better bet than just putting %d?
 
  '%c' will only ever produce one byte of output while '%d' may
  produce up to 11 bytes depending on the value of bus_nr thus
  overflowing the buffer.
 Then use an snprintf, or apply a check to ensure it isn't bigger than
 9.
 
 If you don't mind having clocks named i2c$ or something equally
 silly then I guess this is fine.  To my mind, if that is possible
 this is a bug that should be fixed rather than avoided.

Even better, fix the implementation so that it conforms to the API
spec rather than doing its own thing with the consumer connection ID
argument and ignoring the struct device argument.
--
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: [PATCH 05/25] drivers/i2c: Use static const char arrays

[Russell King - ARM Linux]

On Mon, Sep 13, 2010 at 12:47:43PM -0700, Joe Perches wrote:
 Signed-off-by: Joe Perches j...@perches.com
 ---
  drivers/i2c/busses/i2c-stu300.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-stu300.c b/drivers/i2c/busses/i2c-stu300.c
 index 495be45..2f7c09c 100644
 --- a/drivers/i2c/busses/i2c-stu300.c
 +++ b/drivers/i2c/busses/i2c-stu300.c
 @@ -871,7 +871,7 @@ stu300_probe(struct platform_device *pdev)
   struct resource *res;
   int bus_nr;
   int ret = 0;
 - char clk_name[] = I2C0;
 + char clk_name[sizeof(I2Cx)];
  
   dev = kzalloc(sizeof(struct stu300_dev), GFP_KERNEL);
   if (!dev) {
 @@ -881,7 +881,7 @@ stu300_probe(struct platform_device *pdev)
   }
  
   bus_nr = pdev-id;
 - clk_name[3] += (char)bus_nr;
 + sprintf(clk_name, I2C%c, '0' + bus_nr);

If people would implement the clk API correctly (matching by struct device
rather than giving every clock a name and trying to use the const char *
as the sole matching token), stuff like this would not be necessary.
The second argument to clk_get() is supposed to be a _consumer_ name
(iow, something to determine which clock for the struct device you want)
rather than a producer name.
--
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: [PATCH 2/3] i2c/pxa: only define 'blue_murder'-function if DEBUG is #defined

[Russell King - ARM Linux]

On Sun, Apr 18, 2010 at 03:22:51PM +0200, Wolfram Sang wrote:
 
 Sorry to bump this old thread, it dropped off my todo-list :(
 
 On Sun, Feb 28, 2010 at 03:55:02PM +, Russell King - ARM Linux wrote:
  On Wed, Feb 24, 2010 at 12:01:45PM +0100, Uwe Kleine-König wrote:
   From: Wolfram Sang w.s...@pengutronix.de
   
   This talkative function is also called on timeouts. As timeouts can
   happen on regular writes to EEPROMs (no error case), this creates false
   positives.  Giving lots of details is interesting only for developers
   anyhow, so just use the function if DEBUG is #defined.
  
  Are you sure this is safe?  If you time out the write before it completes,
  how do you know if the write was successful?
  
  I don't think this is no error code nor false positive.  If the timeout
  is too short for your EEPROMs, then the timeout needs to be increased.
 
 I am sure this is safe because we have retries. The eeprom driver first tries
 to write data without a delay, because EEPROMs often have buffers. Once the
 buffers are full, the chip will not answer to the next write request which 
 will
 result in a timeout for this write request. This is expected, so it will be
 retried after some delay. Something like -EBUSY. Only if another outer
 timeout passed after some retries, then we have a problem and this should be
 user visible. But the timeout for the write request is nothing exceptional and
 the user doesn't need to be informed about it, especially not in this detail.
 This is what the patch is addressing.

And what if it's not an EEPROM that you're talking to?
--
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: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
 On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
  On Sun, Apr 4, 2010 at 10:08 PM, Wolfram Sang w.s...@pengutronix.de wrote:
   The timeout value is in jiffies, so it should be using HZ, not a plain
   number. Assume '100' means 100ms here and adapt accordingly.
  
   Signed-off-by: Wolfram Sang w.s...@pengutronix.de
   Cc: Eric Miao eric.y.m...@gmail.com
   Cc: Russell King li...@arm.linux.org.uk
   Cc: Marc Zyngier m...@misterjones.org
   Cc: Paul Shen paul.s...@marvell.com
   Cc: Mike Rapoport m...@compulab.co.il
   ---
  
   Janitorial fix, not tested due to no hardware.
  
    arch/arm/mach-pxa/viper.c |    5 +++--
    1 files changed, 3 insertions(+), 2 deletions(-)
  
   diff --git a/arch/arm/mach-pxa/viper.c b/arch/arm/mach-pxa/viper.c
   index 1dd1334..c25921f 100644
   --- a/arch/arm/mach-pxa/viper.c
   +++ b/arch/arm/mach-pxa/viper.c
   @@ -33,6 +33,7 @@
    #include linux/pm.h
    #include linux/sched.h
    #include linux/gpio.h
   +#include linux/jiffies.h
    #include linux/i2c-gpio.h
    #include linux/serial_8250.h
    #include linux/smc91x.h
   @@ -453,7 +454,7 @@ static struct i2c_gpio_platform_data i2c_bus_data = {
          .sda_pin = VIPER_RTC_I2C_SDA_GPIO,
          .scl_pin = VIPER_RTC_I2C_SCL_GPIO,
          .udelay  = 10,
   -       .timeout = 100,
   +       .timeout = HZ / 10,
    };
  
    static struct platform_device i2c_bus_device = {
   @@ -778,7 +779,7 @@ static void __init viper_tpm_init(void)
                  .sda_pin = VIPER_TPM_I2C_SDA_GPIO,
                  .scl_pin = VIPER_TPM_I2C_SCL_GPIO,
                  .udelay  = 10,
   -               .timeout = 100,
   +               .timeout = HZ / 10,
          };
          char *errstr;
  
  
  One other better and cleaner approach to such inconsistency issue is
  to have a timeout_ms field, and having i2c-gpio.c driver to convert this
  to jiffies using msec_to_jiffies() at run-time.
 
 With what benefit? Expressing time values in units of HZ is very
 frequent in the kernel code and shouldn't actually surprise anyone.

Actually, this patch shows there is confusion.

Assume '100' means 100ms here and adapt accordingly.

Since this patch is for ARM, where HZ=100, the above patch is not a
simple convert how we derive this constant patch - it's a functional
change, reducing the timeouts by a factor of 10.

Could that be because the patch author misinterpreted the HZ-based
values?

I suspect I'm not the only one who thinks that the latter of HZ / 10
100ms is easier to read and comprehend without mistake.
--
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: [PATCH] mach-pxa/viper: Fix timeout usage for I2C

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 09:32:35PM +0200, Jean Delvare wrote:
 On Mon, 12 Apr 2010 20:20:10 +0100, Russell King - ARM Linux wrote:
  On Mon, Apr 12, 2010 at 09:13:19PM +0200, Jean Delvare wrote:
   On Tue, 13 Apr 2010 01:57:51 +0800, Eric Miao wrote:
One other better and cleaner approach to such inconsistency issue is
to have a timeout_ms field, and having i2c-gpio.c driver to convert this
to jiffies using msec_to_jiffies() at run-time.
   
   With what benefit? Expressing time values in units of HZ is very
   frequent in the kernel code and shouldn't actually surprise anyone.
  
  Actually, this patch shows there is confusion.
  
  Assume '100' means 100ms here and adapt accordingly.
  
  Since this patch is for ARM, where HZ=100, the above patch is not a
  simple convert how we derive this constant patch - it's a functional
  change, reducing the timeouts by a factor of 10.
  
  Could that be because the patch author misinterpreted the HZ-based
  values?
 
 I admit I would have assumed 100 - HZ, as hard-coded HZ-dependent
 value typically assume HZ=100.
 
  I suspect I'm not the only one who thinks that the latter of HZ / 10
  100ms is easier to read and comprehend without mistake.
 
 OTOH, converting from ms to jiffies each time you need the value has a
 cost.

True; what I did for MMC stuff is converted it from ms to jiffies at
initialization time when copying it in from platform data in the
driver's probe function.

I'm not saying that I care either way, I'm merely showing that dealing
with HZ-based values can be (maybe unexpectedly) more error prone.
--
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: [PATCH 0/10] ARM-PNX4008-clocking update patches (repost)

[Russell King - ARM Linux]

On Tue, Dec 22, 2009 at 12:21:12AM +0100, Kevin Wells wrote:
 These patches were originally submitted by Russell King to
 update the PNX4008 platform clock functions. These add
 better support for CLKDEV and I2C and WDT clocking updates
 needed for other platforms with this same IP.

Err, you're supposed to keep the signed-off-by's from people down-stream
of you if you're passing the patches through for merging.
--
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: [PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Russell King - ARM Linux]

On Wed, Nov 25, 2009 at 10:23:21PM +0100, Kevin Wells wrote:
 Remove suspend/resume functionality, I2C driver gates clock on
 only when an I2C transaction is in progress

Well, I'm happy with this (we have other drivers which don't do anything
spectacular in their suspend handling as well.)

Can you submit it to the patch system please?
--
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: [PATCH 07/10] ARM: PNX4008: move i2c suspend/resume callbacks into driver

[Russell King - ARM Linux]

On Fri, Nov 20, 2009 at 10:50:34AM +, Russell King - ARM Linux wrote:
 -static int i2c_pnx_suspend(struct platform_device *pdev, pm_message_t state)
 -{
 - int retval = 0;
 -#ifdef CONFIG_PM
 - retval = set_clock_run(pdev);
 -#endif

BTW, a comment from PNX folk (Vitaly/Kevin) would be appreciated.  Why
does PNX enable the clock when going into suspend?  Should I assume that
this should actually be disabling the clock?

Also, if Vitaly doesn't have anything to do with PNX4008 anymore, it
would be a good idea to update the MAINTAINERS file.
--
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

[PATCH] ARM: PNX4008: i2c-pnx: use the same dev_id for request_irq and free_irq

[Russell King - ARM Linux]

This allows i2c-pnx to free its interrupt handler when the module
is removed or if an error occurs; using the same dev_id for both
request_irq and free_irq is desirable.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 drivers/i2c/busses/i2c-pnx.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 1fca590..fbab684 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -650,7 +650,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
return 0;
 
 out_irq:
-   free_irq(alg_data-irq, alg_data);
+   free_irq(alg_data-irq, i2c_pnx-adapter);
 out_clock:
i2c_pnx-set_clock_stop(pdev);
 out_unmap:
@@ -669,7 +669,7 @@ static int __devexit i2c_pnx_remove(struct platform_device 
*pdev)
struct i2c_adapter *adap = i2c_pnx-adapter;
struct i2c_pnx_algo_data *alg_data = adap-algo_data;
 
-   free_irq(alg_data-irq, alg_data);
+   free_irq(alg_data-irq, i2c_pnx-adapter);
i2c_del_adapter(adap);
i2c_pnx-set_clock_stop(pdev);
iounmap((void *)alg_data-ioaddr);
-- 
1.6.2.5

--
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: [PATCH 07/10] ARM: PNX4008: move i2c suspend/resume callbacks into driver

[Russell King - ARM Linux]

On Sat, Nov 21, 2009 at 06:27:54PM +0100, Linus Walleij wrote:
 If you have this up for testing anyway, would it be advisable to take this
 opportunity to also switch i2c-pnx over to using struct dev_pm_ops?

These patches are only compile-tested, and that's partly why there's soo
many of them - each one does one transformation, which makes it easy to
review for correctness.

I'm also hoping that Kevin will test them on later PNX hardware so that
they can be submitted.
--
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

[PATCH 0/2] PNX fixes

[Russell King - ARM Linux]

The following patch series fixes compile bugs with current PNX drivers.
This is part of a larger patch set so I'd prefer to keep them in my
tree for merging with the next round of -rc fixes.

Only relevant patches will be cc'd to appropriate folk; the full series
will be posted on linux-arm-kernel.

Please provide acks for these two patches, thanks.
--
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: [PATCH 1/2] ARM: PNX4008: fix i2c-pnx.c build errors

[Russell King - ARM Linux]

... caused by a missing include.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 drivers/i2c/busses/i2c-pnx.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 6ff6c20..3ae5647 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -22,6 +22,7 @@
 #include mach/hardware.h
 #include asm/irq.h
 #include asm/uaccess.h
+#include mach/i2c.h
 
 #define I2C_PNX_TIMEOUT10 /* msec */
 #define I2C_PNX_SPEED_KHZ  100
-- 
1.6.2.5

--
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: [PATCH 1/2] ARM: PNX4008: fix i2c-pnx.c build errors

[Russell King - ARM Linux]

On Fri, Nov 20, 2009 at 02:21:29PM +, Ben Dooks wrote:
 On Fri, Nov 20, 2009 at 11:03:29AM +, Russell King - ARM Linux wrote:
  ... caused by a missing include.
 
 hi, thought this was fixed by the last fix set I sent which went in post
 2.6.32-rc8.

Ok, I'll rebase my master branch and drop this change.
--
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

[PATCH 0/10] PNX clock API fixes

[Russell King - ARM Linux]

Following on from the previous set of patches...

The PNX4008 clock API usage is extremely poor.  Nothing works as it was
intended to - clk_set_rate() is used to turn clocks on and off, or
set dividers.  I certainly doesn't take the desired clock rate in Hz.
clk_get_rate() doesn't report the clock rate in Hz either - it doesn't
even report any kind of rate.

clk_enable and clk_disable seem to be very weird too.

This patch set fixes the I2C and watchdog drivers so that they can be
used with clk API implementations which do actually conform with the
clk API.

As per the previous set, selective cc's.  Full set will be on
linux-arm-kernel.
--
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

[PATCH 06/10] ARM: PNX4008: convert i2c clocks to match by device only

[Russell King - ARM Linux]

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-pnx4008/clock.c |6 +++---
 arch/arm/mach-pnx4008/i2c.c   |8 ++--
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-pnx4008/clock.c b/arch/arm/mach-pnx4008/clock.c
index 270c254..26659cd 100644
--- a/arch/arm/mach-pnx4008/clock.c
+++ b/arch/arm/mach-pnx4008/clock.c
@@ -797,9 +797,9 @@ static struct clk_lookup onchip_clkreg[] = {
{ .clk = jpeg_ck,  .con_id = jpeg_ck },
{ .clk = ms_ck,.con_id = ms_ck   },
{ .clk = touch_ck, .con_id = touch_ck},
-   { .clk = i2c0_ck,  .con_id = i2c0_ck },
-   { .clk = i2c1_ck,  .con_id = i2c1_ck },
-   { .clk = i2c2_ck,  .con_id = i2c2_ck },
+   { .clk = i2c0_ck,  .dev_id = pnx-i2c.0   },
+   { .clk = i2c1_ck,  .dev_id = pnx-i2c.1   },
+   { .clk = i2c2_ck,  .dev_id = pnx-i2c.2   },
{ .clk = spi0_ck,  .con_id = spi0_ck },
{ .clk = spi1_ck,  .con_id = spi1_ck },
{ .clk = uart3_ck, .con_id = uart3_ck},
diff --git a/arch/arm/mach-pnx4008/i2c.c b/arch/arm/mach-pnx4008/i2c.c
index f3fea29..c986b3a 100644
--- a/arch/arm/mach-pnx4008/i2c.c
+++ b/arch/arm/mach-pnx4008/i2c.c
@@ -21,11 +21,9 @@
 static int set_clock_run(struct platform_device *pdev)
 {
struct clk *clk;
-   char name[10];
int retval = 0;
 
-   snprintf(name, 10, i2c%d_ck, pdev-id);
-   clk = clk_get(pdev-dev, name);
+   clk = clk_get(pdev-dev, NULL);
if (!IS_ERR(clk)) {
clk_set_rate(clk, 1);
clk_put(clk);
@@ -38,11 +36,9 @@ static int set_clock_run(struct platform_device *pdev)
 static int set_clock_stop(struct platform_device *pdev)
 {
struct clk *clk;
-   char name[10];
int retval = 0;
 
-   snprintf(name, 10, i2c%d_ck, pdev-id);
-   clk = clk_get(pdev-dev, name);
+   clk = clk_get(pdev-dev, NULL);
if (!IS_ERR(clk)) {
clk_set_rate(clk, 0);
clk_put(clk);
-- 
1.6.2.5

--
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

[PATCH 07/10] ARM: PNX4008: move i2c suspend/resume callbacks into driver

[Russell King - ARM Linux]

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-pnx4008/i2c.c  |   24 
 drivers/i2c/busses/i2c-pnx.c |9 +++--
 include/linux/i2c-pnx.h  |4 
 3 files changed, 7 insertions(+), 30 deletions(-)

diff --git a/arch/arm/mach-pnx4008/i2c.c b/arch/arm/mach-pnx4008/i2c.c
index c986b3a..707d819 100644
--- a/arch/arm/mach-pnx4008/i2c.c
+++ b/arch/arm/mach-pnx4008/i2c.c
@@ -48,24 +48,6 @@ static int set_clock_stop(struct platform_device *pdev)
return retval;
 }
 
-static int i2c_pnx_suspend(struct platform_device *pdev, pm_message_t state)
-{
-   int retval = 0;
-#ifdef CONFIG_PM
-   retval = set_clock_run(pdev);
-#endif
-   return retval;
-}
-
-static int i2c_pnx_resume(struct platform_device *pdev)
-{
-   int retval = 0;
-#ifdef CONFIG_PM
-   retval = set_clock_run(pdev);
-#endif
-   return retval;
-}
-
 static u32 calculate_input_freq(struct platform_device *pdev)
 {
return HCLK_MHZ;
@@ -102,8 +84,6 @@ static struct i2c_adapter pnx_adapter2 = {
 };
 
 static struct i2c_pnx_data i2c0_data = {
-   .suspend = i2c_pnx_suspend,
-   .resume = i2c_pnx_resume,
.calculate_input_freq = calculate_input_freq,
.set_clock_run = set_clock_run,
.set_clock_stop = set_clock_stop,
@@ -111,8 +91,6 @@ static struct i2c_pnx_data i2c0_data = {
 };
 
 static struct i2c_pnx_data i2c1_data = {
-   .suspend = i2c_pnx_suspend,
-   .resume = i2c_pnx_resume,
.calculate_input_freq = calculate_input_freq,
.set_clock_run = set_clock_run,
.set_clock_stop = set_clock_stop,
@@ -120,8 +98,6 @@ static struct i2c_pnx_data i2c1_data = {
 };
 
 static struct i2c_pnx_data i2c2_data = {
-   .suspend = i2c_pnx_suspend,
-   .resume = i2c_pnx_resume,
.calculate_input_freq = calculate_input_freq,
.set_clock_run = set_clock_run,
.set_clock_stop = set_clock_stop,
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 1fca590..8fe7de8 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -545,18 +545,23 @@ static struct i2c_algorithm pnx_algorithm = {
.functionality = i2c_pnx_func,
 };
 
+#ifdef CONFIG_PM
 static int i2c_pnx_controller_suspend(struct platform_device *pdev,
  pm_message_t state)
 {
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-   return i2c_pnx-suspend(pdev, state);
+   return i2c_pnx-set_clock_run(pdev);
 }
 
 static int i2c_pnx_controller_resume(struct platform_device *pdev)
 {
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-   return i2c_pnx-resume(pdev);
+   return i2c_pnx-set_clock_run(pdev);
 }
+#else
+#define i2c_pnx_controller_suspend NULL
+#define i2c_pnx_controller_resume  NULL
+#endif
 
 static int __devinit i2c_pnx_probe(struct platform_device *pdev)
 {
diff --git a/include/linux/i2c-pnx.h b/include/linux/i2c-pnx.h
index 9eb07bb..71de7f9 100644
--- a/include/linux/i2c-pnx.h
+++ b/include/linux/i2c-pnx.h
@@ -12,8 +12,6 @@
 #ifndef __I2C_PNX_H__
 #define __I2C_PNX_H__
 
-#include linux/pm.h
-
 struct platform_device;
 
 struct i2c_pnx_mif {
@@ -34,8 +32,6 @@ struct i2c_pnx_algo_data {
 };
 
 struct i2c_pnx_data {
-   int (*suspend) (struct platform_device *pdev, pm_message_t state);
-   int (*resume) (struct platform_device *pdev);
u32 (*calculate_input_freq) (struct platform_device *pdev);
int (*set_clock_run) (struct platform_device *pdev);
int (*set_clock_stop) (struct platform_device *pdev);
-- 
1.6.2.5

--
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

[PATCH 08/10] ARM: PNX4008: move i2c clock start/stop into driver

[Russell King - ARM Linux]

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-pnx4008/i2c.c  |   36 
 drivers/i2c/busses/i2c-pnx.c |   39 ++-
 include/linux/i2c-pnx.h  |4 ++--
 3 files changed, 32 insertions(+), 47 deletions(-)

diff --git a/arch/arm/mach-pnx4008/i2c.c b/arch/arm/mach-pnx4008/i2c.c
index 707d819..14b4906 100644
--- a/arch/arm/mach-pnx4008/i2c.c
+++ b/arch/arm/mach-pnx4008/i2c.c
@@ -18,36 +18,6 @@
 #include mach/irqs.h
 #include mach/i2c.h
 
-static int set_clock_run(struct platform_device *pdev)
-{
-   struct clk *clk;
-   int retval = 0;
-
-   clk = clk_get(pdev-dev, NULL);
-   if (!IS_ERR(clk)) {
-   clk_set_rate(clk, 1);
-   clk_put(clk);
-   } else
-   retval = -ENOENT;
-
-   return retval;
-}
-
-static int set_clock_stop(struct platform_device *pdev)
-{
-   struct clk *clk;
-   int retval = 0;
-
-   clk = clk_get(pdev-dev, NULL);
-   if (!IS_ERR(clk)) {
-   clk_set_rate(clk, 0);
-   clk_put(clk);
-   } else
-   retval = -ENOENT;
-
-   return retval;
-}
-
 static u32 calculate_input_freq(struct platform_device *pdev)
 {
return HCLK_MHZ;
@@ -85,22 +55,16 @@ static struct i2c_adapter pnx_adapter2 = {
 
 static struct i2c_pnx_data i2c0_data = {
.calculate_input_freq = calculate_input_freq,
-   .set_clock_run = set_clock_run,
-   .set_clock_stop = set_clock_stop,
.adapter = pnx_adapter0,
 };
 
 static struct i2c_pnx_data i2c1_data = {
.calculate_input_freq = calculate_input_freq,
-   .set_clock_run = set_clock_run,
-   .set_clock_stop = set_clock_stop,
.adapter = pnx_adapter1,
 };
 
 static struct i2c_pnx_data i2c2_data = {
.calculate_input_freq = calculate_input_freq,
-   .set_clock_run = set_clock_run,
-   .set_clock_stop = set_clock_stop,
.adapter = pnx_adapter2,
 };
 
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 8fe7de8..0a0ee4a 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -20,6 +20,9 @@
 #include linux/platform_device.h
 #include linux/i2c-pnx.h
 #include linux/io.h
+#include linux/err.h
+#include linux/clk.h
+
 #include mach/hardware.h
 #include mach/i2c.h
 #include asm/irq.h
@@ -550,13 +553,22 @@ static int i2c_pnx_controller_suspend(struct 
platform_device *pdev,
  pm_message_t state)
 {
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-   return i2c_pnx-set_clock_run(pdev);
+   struct i2c_pnx_algo_data *alg_data = i2c_pnx-adapter-algo_data;
+
+   /* FIXME: disable clock? */
+   clk_set_rate(alg_data-clk, 1);
+
+   return 0;
 }
 
 static int i2c_pnx_controller_resume(struct platform_device *pdev)
 {
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-   return i2c_pnx-set_clock_run(pdev);
+   struct i2c_pnx_algo_data *alg_data = i2c_pnx-adapter-algo_data;
+
+   clk_set_rate(alg_data-clk, 1);
+
+   return 0;
 }
 #else
 #define i2c_pnx_controller_suspend NULL
@@ -580,6 +592,15 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
 
platform_set_drvdata(pdev, i2c_pnx);
 
+   i2c_pnx-adapter-algo = pnx_algorithm;
+   alg_data = i2c_pnx-adapter-algo_data;
+
+   alg_data-clk = clk_get(pdev-dev, NULL);
+   if (IS_ERR(alg_data-clk)) {
+   ret = PTR_ERR(alg_data-clk);
+   goto out_drvdata;
+   }
+
if (i2c_pnx-calculate_input_freq)
freq_mhz = i2c_pnx-calculate_input_freq(pdev);
else {
@@ -588,9 +609,6 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
   %d MHz\n, freq_mhz);
}
 
-   i2c_pnx-adapter-algo = pnx_algorithm;
-
-   alg_data = i2c_pnx-adapter-algo_data;
init_timer(alg_data-mif.timer);
alg_data-mif.timer.function = i2c_pnx_timeout;
alg_data-mif.timer.data = (unsigned long)i2c_pnx-adapter;
@@ -602,7 +620,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
   I/O region 0x%08x for I2C already in use.\n,
   alg_data-base);
ret = -ENODEV;
-   goto out_drvdata;
+   goto out_clkget;
}
 
if (!(alg_data-ioaddr =
@@ -612,7 +630,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
goto out_release;
}
 
-   i2c_pnx-set_clock_run(pdev);
+   clk_set_rate(alg_data-clk, 1);
 
/*
 * Clock Divisor High This value is the number of system clocks
@@ -657,11 +675,13 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
 out_irq:
free_irq(alg_data-irq, alg_data);
 out_clock:
-   i2c_pnx-set_clock_stop(pdev);
+   clk_set_rate(alg_data-clk, 0);
 out_unmap:

[PATCH 10/10] ARM: PNX4008: get i2c clock rate from clk API

[Russell King - ARM Linux]

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-pnx4008/clock.c |9 ++---
 arch/arm/mach-pnx4008/i2c.c   |9 -
 drivers/i2c/busses/i2c-pnx.c  |   15 ---
 include/linux/i2c-pnx.h   |1 -
 4 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-pnx4008/clock.c b/arch/arm/mach-pnx4008/clock.c
index c60d798..a38ef66 100644
--- a/arch/arm/mach-pnx4008/clock.c
+++ b/arch/arm/mach-pnx4008/clock.c
@@ -638,9 +638,10 @@ static struct clk flash_ck = {
 static struct clk i2c0_ck = {
.name = i2c0_ck,
.parent = per_ck,
-   .flags = NEEDS_INITIALIZATION,
+   .flags = NEEDS_INITIALIZATION | FIXED_RATE,
.enable_shift = 0,
.enable_reg = I2CCLKCTRL_REG,
+   .rate = 1300,
.enable = clk_reg_enable,
.disable = clk_reg_disable,
 };
@@ -648,9 +649,10 @@ static struct clk i2c0_ck = {
 static struct clk i2c1_ck = {
.name = i2c1_ck,
.parent = per_ck,
-   .flags = NEEDS_INITIALIZATION,
+   .flags = NEEDS_INITIALIZATION | FIXED_RATE,
.enable_shift = 1,
.enable_reg = I2CCLKCTRL_REG,
+   .rate = 1300,
.enable = clk_reg_enable,
.disable = clk_reg_disable,
 };
@@ -658,9 +660,10 @@ static struct clk i2c1_ck = {
 static struct clk i2c2_ck = {
.name = i2c2_ck,
.parent = per_ck,
-   .flags = NEEDS_INITIALIZATION,
+   .flags = NEEDS_INITIALIZATION | FIXED_RATE,
.enable_shift = 2,
.enable_reg = USB_OTG_CLKCTRL_REG,
+   .rate = 1300,
.enable = clk_reg_enable,
.disable = clk_reg_disable,
 };
diff --git a/arch/arm/mach-pnx4008/i2c.c b/arch/arm/mach-pnx4008/i2c.c
index 14b4906..23ec335 100644
--- a/arch/arm/mach-pnx4008/i2c.c
+++ b/arch/arm/mach-pnx4008/i2c.c
@@ -18,12 +18,6 @@
 #include mach/irqs.h
 #include mach/i2c.h
 
-static u32 calculate_input_freq(struct platform_device *pdev)
-{
-   return HCLK_MHZ;
-}
-
-
 static struct i2c_pnx_algo_data pnx_algo_data0 = {
.base = PNX4008_I2C1_BASE,
.irq = I2C_1_INT,
@@ -54,17 +48,14 @@ static struct i2c_adapter pnx_adapter2 = {
 };
 
 static struct i2c_pnx_data i2c0_data = {
-   .calculate_input_freq = calculate_input_freq,
.adapter = pnx_adapter0,
 };
 
 static struct i2c_pnx_data i2c1_data = {
-   .calculate_input_freq = calculate_input_freq,
.adapter = pnx_adapter1,
 };
 
 static struct i2c_pnx_data i2c2_data = {
-   .calculate_input_freq = calculate_input_freq,
.adapter = pnx_adapter2,
 };
 
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 34da92d..a390ef3 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -31,7 +31,6 @@
 #define I2C_PNX_TIMEOUT10 /* msec */
 #define I2C_PNX_SPEED_KHZ  100
 #define I2C_PNX_REGION_SIZE0x100
-#define PNX_DEFAULT_FREQ   13 /* MHz */
 
 static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
 {
@@ -578,7 +577,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
unsigned long tmp;
int ret = 0;
struct i2c_pnx_algo_data *alg_data;
-   int freq_mhz;
+   unsigned long freq;
struct i2c_pnx_data *i2c_pnx = pdev-dev.platform_data;
 
if (!i2c_pnx || !i2c_pnx-adapter) {
@@ -599,14 +598,6 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
goto out_drvdata;
}
 
-   if (i2c_pnx-calculate_input_freq)
-   freq_mhz = i2c_pnx-calculate_input_freq(pdev);
-   else {
-   freq_mhz = PNX_DEFAULT_FREQ;
-   dev_info(pdev-dev, Setting bus frequency to default value: 
-  %d MHz\n, freq_mhz);
-   }
-
init_timer(alg_data-mif.timer);
alg_data-mif.timer.function = i2c_pnx_timeout;
alg_data-mif.timer.data = (unsigned long)i2c_pnx-adapter;
@@ -632,6 +623,8 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
if (ret)
goto out_unmap;
 
+   freq = clk_get_rate(alg_data-clk);
+
/*
 * Clock Divisor High This value is the number of system clocks
 * the serial clock (SCL) will be high.
@@ -643,7 +636,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
 * the deglitching filter length.
 */
 
-   tmp = ((freq_mhz * 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
+   tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
iowrite32(tmp, I2C_REG_CKH(alg_data));
iowrite32(tmp, I2C_REG_CKL(alg_data));
 
diff --git a/include/linux/i2c-pnx.h b/include/linux/i2c-pnx.h
index 688e292..9035711 100644
--- a/include/linux/i2c-pnx.h
+++ b/include/linux/i2c-pnx.h
@@ -34,7 +34,6 @@ struct i2c_pnx_algo_data {
 };
 
 struct i2c_pnx_data {
-   u32 (*calculate_input_freq) (struct platform_device *pdev);
struct i2c_adapter *adapter;
 };
 
-- 
1.6.2.5

--
To unsubscribe from this 

[PATCH 09/10] ARM: PNX4008: convert i2c-pnx to use clk API enable/disable calls

[Russell King - ARM Linux]

clk_set_rate() is not supposed to be used to turn clocks on and off.
That's what clk_enable/clk_disable is for.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-pnx4008/clock.c |   12 ++--
 drivers/i2c/busses/i2c-pnx.c  |   18 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-pnx4008/clock.c b/arch/arm/mach-pnx4008/clock.c
index 26659cd..c60d798 100644
--- a/arch/arm/mach-pnx4008/clock.c
+++ b/arch/arm/mach-pnx4008/clock.c
@@ -639,30 +639,30 @@ static struct clk i2c0_ck = {
.name = i2c0_ck,
.parent = per_ck,
.flags = NEEDS_INITIALIZATION,
-   .round_rate = on_off_round_rate,
-   .set_rate = on_off_set_rate,
.enable_shift = 0,
.enable_reg = I2CCLKCTRL_REG,
+   .enable = clk_reg_enable,
+   .disable = clk_reg_disable,
 };
 
 static struct clk i2c1_ck = {
.name = i2c1_ck,
.parent = per_ck,
.flags = NEEDS_INITIALIZATION,
-   .round_rate = on_off_round_rate,
-   .set_rate = on_off_set_rate,
.enable_shift = 1,
.enable_reg = I2CCLKCTRL_REG,
+   .enable = clk_reg_enable,
+   .disable = clk_reg_disable,
 };
 
 static struct clk i2c2_ck = {
.name = i2c2_ck,
.parent = per_ck,
.flags = NEEDS_INITIALIZATION,
-   .round_rate = on_off_round_rate,
-   .set_rate = on_off_set_rate,
.enable_shift = 2,
.enable_reg = USB_OTG_CLKCTRL_REG,
+   .enable = clk_reg_enable,
+   .disable = clk_reg_disable,
 };
 
 static struct clk spi0_ck = {
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 0a0ee4a..34da92d 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -555,8 +555,8 @@ static int i2c_pnx_controller_suspend(struct 
platform_device *pdev,
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
struct i2c_pnx_algo_data *alg_data = i2c_pnx-adapter-algo_data;
 
-   /* FIXME: disable clock? */
-   clk_set_rate(alg_data-clk, 1);
+   /* FIXME: shouldn't this be clk_disable? */
+   clk_enable(alg_data-clk);
 
return 0;
 }
@@ -566,9 +566,7 @@ static int i2c_pnx_controller_resume(struct platform_device 
*pdev)
struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
struct i2c_pnx_algo_data *alg_data = i2c_pnx-adapter-algo_data;
 
-   clk_set_rate(alg_data-clk, 1);
-
-   return 0;
+   return clk_enable(alg_data-clk);
 }
 #else
 #define i2c_pnx_controller_suspend NULL
@@ -630,7 +628,9 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
goto out_release;
}
 
-   clk_set_rate(alg_data-clk, 1);
+   ret = clk_enable(alg_data-clk);
+   if (ret)
+   goto out_unmap;
 
/*
 * Clock Divisor High This value is the number of system clocks
@@ -650,7 +650,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
ret = -ENODEV;
-   goto out_unmap;
+   goto out_clock;
}
init_completion(alg_data-mif.complete);
 
@@ -675,7 +675,7 @@ static int __devinit i2c_pnx_probe(struct platform_device 
*pdev)
 out_irq:
free_irq(alg_data-irq, alg_data);
 out_clock:
-   clk_set_rate(alg_data-clk, 0);
+   clk_disable(alg_data-clk);
 out_unmap:
iounmap((void *)alg_data-ioaddr);
 out_release:
@@ -696,7 +696,7 @@ static int __devexit i2c_pnx_remove(struct platform_device 
*pdev)
 
free_irq(alg_data-irq, alg_data);
i2c_del_adapter(adap);
-   clk_set_rate(alg_data-clk, 0);
+   clk_disable(alg_data-clk);
iounmap((void *)alg_data-ioaddr);
release_mem_region(alg_data-base, I2C_PNX_REGION_SIZE);
clk_put(alg_data-clk);
-- 
1.6.2.5

--
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: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro

[Russell King - ARM Linux]

On Tue, Nov 03, 2009 at 10:12:49PM +, Ben Dooks wrote:
 On Tue, Nov 03, 2009 at 09:01:15PM +, Russell King wrote:
  On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-K?nig wrote:
   Commit
   
 beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
  
  Oh great.  Some people may remember the LX code that I submitted to
  the mailing list a while back.  Removing the slave emulation buggers
  that platform up.  I do intend to submit that once the issue of where
  the PCON support should be located is resolved - which is still
  pending.
  
  Please revert this change, thanks.
 
 Would you like it reverted in the next merge window, or when the
 LX code is actually added?

Actually, the change was correct, and was part of the LX patchset - I just
misremembered where stuff was with it.

 I'll NAK any changes to remove the eedbg() for the moment.

That change is also fine, consider it acked:

Acked-by: Russell King rmk+ker...@arm.linux.org.uk

Sorry for the noise.
--
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: [RESUBMIT][PATCH][RFC] OMAP4: I2C Support for OMAP4430

[Russell King - ARM Linux]

On Tue, Jul 21, 2009 at 08:49:46PM +0530, Syed Rafiuddin wrote:
 This patch adds OMAP4 support to the I2C driver. All I2C register addresses
 are different between OMAP1/2/3 and OMAP4. In order to not have #ifdef's at
 various places in code, as well as to support multi-OMAP build, Array's are
 created to hold the register addresses.

Hmm, some comments follow.

 @@ -156,6 +162,12 @@
  #define SYSC_IDLEMODE_SMART  0x2
  #define SYSC_CLOCKACTIVITY_FCLK  0x2
 
 +#define maxvalue(x, y)   (x  y ? x : y)

We have a max() and max_t() functions in the kernel which are both
typesafe.  Please don't reintroduce the above buggy construct.

 +
 +struct reg_type {
 + u32 reg;
 + u32 offset;
 +};

I'm not sure what use 'reg' is here - since it's always identical to
the index into the array.  Just make this an array.

 
  struct omap_i2c_dev {
   struct device   *dev;
 @@ -165,6 +177,7 @@ struct omap_i2c_dev {
   struct clk  *fclk;  /* Functional clock */
   struct completion   cmd_complete;
   struct resource *ioarea;
 + struct reg_type *regs;

This could be const...

   u32 speed;  /* Speed of bus in Khz */
   u16 cmd_err;
   u8  *buf;
 @@ -180,15 +193,61 @@ struct omap_i2c_dev {
   u16 iestate;/* Saved interrupt register */
  };
 
 +static struct  __initdata reg_type reg_map[] = {
 + {OMAP_I2C_REV_REG, 0x00},
 + {OMAP_I2C_IE_REG, 0x04},
 + {OMAP_I2C_STAT_REG, 0x08},
 + {OMAP_I2C_IV_REG, 0x0c},
 + {OMAP_I2C_WE_REG, 0x0c},
 + {OMAP_I2C_SYSS_REG, 0x10},
 + {OMAP_I2C_BUF_REG, 0x14},
 + {OMAP_I2C_CNT_REG, 0x18},
 + {OMAP_I2C_DATA_REG, 0x1c},
 + {OMAP_I2C_SYSC_REG, 0x20},
 + {OMAP_I2C_CON_REG, 0x24},
 + {OMAP_I2C_OA_REG, 0x28},
 + {OMAP_I2C_SA_REG, 0x2c},
 + {OMAP_I2C_PSC_REG, 0x30},
 + {OMAP_I2C_SCLL_REG, 0x34},
 + {OMAP_I2C_SCLH_REG, 0x38},
 + {OMAP_I2C_SYSTEST_REG, 0x3C},
 + {OMAP_I2C_BUFSTAT_REG, 0x40},
 +};
 +
 +static struct  __initdata reg_type omap4_reg_map[] = {
 + {OMAP_I2C_REV_REG, 0x04},
 + {OMAP_I2C_IE_REG, 0x2c},
 + {OMAP_I2C_STAT_REG, 0x28},
 + {OMAP_I2C_IV_REG, 0x34},
 + {OMAP_I2C_WE_REG, 0x34},
 + {OMAP_I2C_SYSS_REG, 0x90},
 + {OMAP_I2C_BUF_REG, 0x94},
 + {OMAP_I2C_CNT_REG, 0x98},
 + {OMAP_I2C_DATA_REG, 0x9c},
 + {OMAP_I2C_SYSC_REG, 0x20},
 + {OMAP_I2C_CON_REG, 0xa4},
 + {OMAP_I2C_OA_REG, 0xa8},
 + {OMAP_I2C_SA_REG, 0xac},
 + {OMAP_I2C_PSC_REG, 0xb0},
 + {OMAP_I2C_SCLL_REG, 0xb4},
 + {OMAP_I2C_SCLH_REG, 0xb8},
 + {OMAP_I2C_SYSTEST_REG, 0xbC},
 + {OMAP_I2C_BUFSTAT_REG, 0xc0},
 + {OMAP_I2C_REVNB_LO, 0x00},
 + {OMAP_I2C_IRQSTATUS_RAW, 0x24},
 + {OMAP_I2C_IRQENABLE_SET, 0x2c},
 + {OMAP_I2C_IRQENABLE_CLR, 0x30},
 +};

These arrays could be of a well defined size (enough to hold all enum
values).  They're not going to be huge, and I suspect that the cost of
this code:

 + if (dev-regs == NULL) {
 + dev-regs = kmalloc(maxvalue(sizeof(omap4_reg_map),
 + sizeof(reg_map)), GFP_KERNEL);
 + if (dev-regs == NULL) {
 + r = -ENOMEM;
 + goto err_free_mem;
 + }
 + }
 +
 + if (cpu_is_omap44xx())
 + memcpy(dev-regs, omap4_reg_map, sizeof(omap4_reg_map));
 + else
 + memcpy(dev-regs, reg_map, sizeof(reg_map));
 +

is higher than just having the above be const arrays of 'u8' or maybe
'u16'.

Note that you can explicitly initialize array entries as follows:

static u8 omap4_reg_map[OMAP_I2C_MAX_REG] = {
[OMAP_I2C_REV_REG] = 0x04,
[OMAP_I2C_IE_REG]  = 0x2c,
...
};
--
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: I2C device board info

[Russell King - ARM Linux]

On Sat, Jul 18, 2009 at 03:08:03PM +0200, Jean Delvare wrote:
 Note that this is not an isolated case, although this one is worse
 because I2C_BOARD_INFO() and .type do not agree on the chip name. But
 there are several redundant .type definitions in arch/arm/mach-* and
 one in arch/blackfin/mach-bf527. Time to fix them?

That is my intention - I'm preparing patches for Realview and Versatile.
This will only leave the MX* series of platforms in arch/arm doing this,
which fall into Sascha's domain.
--
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: I2C device board info

[Russell King - ARM Linux]

On Sat, Jul 18, 2009 at 03:29:18PM +0200, Jean Delvare wrote:
 On Sat, 18 Jul 2009 14:12:37 +0100, Russell King - ARM Linux wrote:
  On Sat, Jul 18, 2009 at 03:08:03PM +0200, Jean Delvare wrote:
   Note that this is not an isolated case, although this one is worse
   because I2C_BOARD_INFO() and .type do not agree on the chip name. But
   there are several redundant .type definitions in arch/arm/mach-* and
   one in arch/blackfin/mach-bf527. Time to fix them?
  
  That is my intention - I'm preparing patches for Realview and Versatile.
  This will only leave the MX* series of platforms in arch/arm doing this,
  which fall into Sascha's domain.
 
 OK, thank you. I'll send a patch for the blackfin ones then.

Can I get an ack for this please?

Thanks.

Subject: ARM: Realview  Versatile: Fix i2c_board_info definitions

Fix i2c_board_info definitions - we were defining the 'type' field
of these structures twice since the first argument of I2C_BOARD_INFO
sets this field.  Move the second definition into I2C_BOARD_INFO().

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-realview/core.c  |3 +--
 arch/arm/mach-versatile/core.c |3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index ab615e7..dc3519c 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
@@ -208,8 +208,7 @@ struct platform_device realview_i2c_device = {
 
 static struct i2c_board_info realview_i2c_board_info[] = {
{
-   I2C_BOARD_INFO(rtc-ds1307, 0xd0  1),
-   .type = ds1338,
+   I2C_BOARD_INFO(ds1338, 0xd0  1),
},
 };
 
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index afc0f87..975eae4 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -343,8 +343,7 @@ static struct platform_device versatile_i2c_device = {
 
 static struct i2c_board_info versatile_i2c_board_info[] = {
{
-   I2C_BOARD_INFO(rtc-ds1307, 0xd0  1),
-   .type = ds1338,
+   I2C_BOARD_INFO(ds1338, 0xd0  1),
},
 };
 
--
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

[PATCH 0/5] PXA LX platform support

[Russell King - ARM Linux]

The following series adds basic support for the LX platform.  This
platform is a portable small netbook style machine, with support
for touch screen LCD, sound, MMC/SD, one serial port, CF, PCMCIA
and MMC.

The entire series is posted to the linux-arm-kernel list; individual
patches are only copied to relevent people/lists.
--
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