Re: [Bugme-new] [Bug 35572] New: eeprom module: modprobing hangs

2011-06-01 Thread Andrew Morton
On Wed, 1 Jun 2011 14:42:40 -0700
Andrew Morton  wrote:

> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
> 
> On Sat, 21 May 2011 20:17:32 GMT
> bugzilla-dae...@bugzilla.kernel.org wrote:
> 
> > https://bugzilla.kernel.org/show_bug.cgi?id=35572

whoa, that's creepy.  The report is ten days old, but in the two-minute
window between me reading it and then sending this email, Jean
responded in bugzilla.

Still, the backtrace might be useful.
--
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: [Bugme-new] [Bug 35572] New: eeprom module: modprobing hangs

2011-06-01 Thread Andrew Morton

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Sat, 21 May 2011 20:17:32 GMT
bugzilla-dae...@bugzilla.kernel.org wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=35572
> 
>Summary: eeprom module: modprobing hangs
>Product: Other
>Version: 2.5
> Kernel Version: 2.6.39
>   Platform: All
> OS/Version: Linux
>   Tree: Mainline
> Status: NEW
>   Severity: normal
>   Priority: P1
>  Component: Modules
> AssignedTo: other_modu...@kernel-bugs.osdl.org
> ReportedBy: markota...@gmail.com
> Regression: No

This is drivers/misc/eeprom/eeprom.c?

That driver doesn't really have a maintainer.  Perhaps the i2c
developers can help debug this?

> 
> modbrobing eeprom module hangs, but does not crash the kernel. Occurs with
> 2.6.39, with 2.6.38 and prev works ok.

So 2.6.38 did not have this bug?

The driver didn't change between .38 and .39.

> Confirmed by other user, the bug
> reported in Arch can be found here: 
> https://bugs.archlinux.org/24376
> 
> my HW: 
> i686, archlinux kernel (no relevant patch, almost same as mainline :
> ftp://ftp.archlinux.org/other/kernel26/patch-2.6.39-1-ARCH.bz2 ), 
> 
> 
> part from dmesg when modprobing eeprom:
>  57.222509] tg3 :02:00.0: eth0: Flow control is on for TX and on for RX
> [   57.223449] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [   67.936739] eth0: no IPv6 routers present
> [   93.797133] ip_tables: (C) 2000-2006 Netfilter Core Team
> [   94.057661] nf_conntrack version 0.5.0 (16384 buckets, 65536 max)
> [  106.272909] EXT4-fs (sda4): re-mounted. Opts: commit=0
> [  106.957966] EXT4-fs (sda9): re-mounted. Opts: commit=0
> [  107.181605] EXT4-fs (sda2): re-mounted. Opts: commit=0
> [  107.331670] EXT4-fs (sda12): re-mounted. Opts: commit=0
> [  136.933416] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  136.986739] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.043440] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.096738] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.150067] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.203415] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.256761] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.310101] [drm] GMBUS timed out, falling back to bit banging on pin 0
> [i915 gmbus disabled]
> [  137.523400] [drm] GMBUS timed out, falling back to bit banging on pin 4
> [i915 gmbus dpc]
> [  178.299024] kwin_opengl_tes[3439]: segfault at 2c ip b70a5c48 sp bfb90520
> error 4 in i915_dri.so[b706c000+35b000]
> [  213.183783] process `skype' is using obsolete setsockopt SO_BSDCOMPAT
> 
> 
> 
> 
> lspci -vvv
> 00:00.0 Host bridge: Intel Corporation Mobile 945GME Express Memory Controller
> Hub (rev 03)
> Subsystem: Lenovo Device 386f
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR+ FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- 
>  SERR-  Latency: 0
> Capabilities: [e0] Vendor Specific Information: Len=09 
> Kernel driver in use: agpgart-intel
> Kernel modules: intel-agp
> 
> 00:02.0 VGA compatible controller: Intel Corporation Mobile 945GME Express
> Integrated Graphics Controller (rev 03) (prog-if 00 [VGA controller])
> Subsystem: Lenovo Device 3870
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- 
>  SERR-  Latency: 0
> Interrupt: pin A routed to IRQ 16
> Region 0: Memory at f030 (32-bit, non-prefetchable) [size=512K]
> Region 1: I/O ports at 1800 [size=8]
> Region 2: Memory at d000 (32-bit, prefetchable) [size=256M]
> Region 3: Memory at f040 (32-bit, non-prefetchable) [size=256K]
> Expansion ROM at  [disabled]
> Capabilities: [90] MSI: Enable- Count=1/1 Maskable- 64bit-
> Address:   Data: 
> Capabilities: [d0] Power Management version 2
> Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA
> PME(D0-,D1-,D2-,D3hot-,D3cold-)
> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> Kernel driver in use: i915
> Kernel modules: i915
> 
> 00:02.1 Display controller: Intel Corporation Mobile 945GM/GMS/GME, 943/940GML
> Express Integrated Graphics Controller (rev 03)
> Subsystem: Lenovo Device 3870
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
> 

Re: [PATCH v2 1/1] NFC: Driver for NXP Semiconductors PN544 NFC chip.

2010-10-29 Thread Andrew Morton
On Fri, 29 Oct 2010 09:26:09 +0300
"Matti J. Aaltonen"  wrote:

> +static void __exit pn544_exit(void)
> +{
> + flush_scheduled_work();

You said this had been removed.  Did you send the correct version?

> + i2c_del_driver(&pn544_driver);
> + pr_info(DRIVER_DESC ", Exiting.\n");
> +}
--
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 0/1] PN544 NFC driver.

2010-10-29 Thread Andrew Morton
On Fri, 29 Oct 2010 09:26:08 +0300
"Matti J. Aaltonen"  wrote:

> Hello.
> 
> And thanks to Andrew for the comments.
> 
> >> include/linux/pn544.h |   99 ++
> > 
> > Is drivers/misc/ the best place for this?
> > 
> > Don't be afraid to create a new drivers/nfc/ even if it has only one
> > driver in it.  If someone later comes in and adds a new NFC driver then
> > things will all fall into place.
> 
> OK. Now I've created directories drivers/nfc, include/linux/nfc and 
> Documentation/nfc.

I beefed up the changelog a bit, telling people what "nfc" is.

We really should tell more people that we're adding a new subsystem. 
Can you please resend the patchset, cc'ing linux-kernel?

> >> +/* sysfs interface */
> > 
> > OK, this is more serious.
> > 
> > You're proposing a permanent addition to the kernel ABI.  This is the
> > most important part of the driver because it is something we can never
> > change.  This interface is the very first thing we'll want to
> > understand and review, before we even look at the implementation.
> > 
> > And it isn't even described!  Not in the changelog, not in a
> > documentation file, not even in code comments.
> > 
> > Please, provide a description of this proposed interface.  Sufficient
> > for reviewers to understand it and for users to use it.  Pobably this
> > will require some description of the hardware functions as well.
> > 
> > Please also consider updating Documentation/ABI/
> 
> I've added a documentation file. But I didn't make changes to the interface
> yet.

So we can expect some updates?

> > 
> > And an ioctl interface as well!  An undocmented one.
> > 
> > ioctls are pretty unpopular for various reasons.  To a large extent,
> > people have been using sysfs interfaces as a repalcement for ioctl()s.
> > But this driver has both!
> 
> Some explanatory text written into the documentation file.

So, the sysfs interface is purely for running a device test?

And primary communication is via the /dev node, and that node supports
an ioctl() which changes the meaning of reads and writes to that node?

If so, that's a bit of a peculiar interface.  Perhaps there should have
been two /dev nodes.  Certainly it would be most popular if the ioctl()
interface were to simply disappear.

Please, do spell all of this out in some detail within the changelog
when resending the code for wider review.  There are people out there
(eg Greg, Alan) who are better at these things than I and we should
provide them with all the details up-front without going through
another question-and-answer session or expecting them to grovel through
code to reverse-engineer the interfaces.

Another consideration here is that if we do expect more NFC devices and
drivers for them, then we should aim for some standardisation of the
interface, from day one.  Some discussion of this would also be helpful
for reviewers.


One other thing: these messages which flow between userspace and the
device.  Are they documented or sufficiently well understood so that
non-Nokia people can use this driver?

Because we had a driver come up a couple of weeks ago.  It was a
simple, clean driver but all it did was to shuffle opaque data between
userspace and the device, and the contents of that data was not
publicly available.  I discussed the driver with Linus and he said "no".

--
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 RFC] Misc: Driver for NXP Semiconductors PN544 NFC chip.

2010-10-25 Thread Andrew Morton
On Mon, 25 Oct 2010 17:44:18 +0300
"Matti J. Aaltonen"  wrote:

> This is a driver for the pn544 NFC device. The driver transfers
> ETSI messages between the device and the user space.

err, this reader at least doesn't know what an NFC is, nor what an ETSI
message is.  I could google them, but it wouldn't kill you to spell
these things out a little more completely please.

> Signed-off-by: Matti J. Aaltonen 
> ---
>  drivers/misc/Kconfig  |   12 +
>  drivers/misc/Makefile |1 +
>  drivers/misc/pn544.c  |  912 
> +
>  include/linux/pn544.h |   99 ++

Is drivers/misc/ the best place for this?

Don't be afraid to create a new drivers/nfc/ even if it has only one
driver in it.  If someone later comes in and adds a new NFC driver then
things will all fall into place.

>  4 files changed, 1024 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/pn544.c
>  create mode 100644 include/linux/pn544.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 1f69743..23977ff 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -402,6 +402,18 @@ config PCH_PHUB
> To compile this driver as a module, choose M here: the module will
> be called pch_phub.
>  
> +config PN544_NFC
> + tristate "PN544 NFC driver"
> + depends on I2C
> + select CRC_CCITT
> + default n
> + ---help---
> +   Say yes if you want PN544 Near Field Communication driver.

OK, I did google it ;)  Interesting.

> +   This is for i2c connected version. If unsure, say N here.
> +
> +   To compile this driver as a module, choose M here. The module will
> +   be called pn544.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
>
> ...
>
> +static void pn544_print_buf(char *msg, u8 *buf, int len)
> +{
> +#ifdef DEBUG
> + int i;
> +
> + pr_debug(PN544_DRIVER_NAME ": Got %d at %s:", len, msg);
> + for (i = 0; i < len; i++)
> + pr_info(" %x", buf[i]);
> +
> + pr_info("\n");

You might be able to use print_hex_dump() here, but I wouldn't blame
you if you didn't ;)

> +#endif
> +}
> +
> +/* sysfs interface */

OK, this is more serious.

You're proposing a permanent addition to the kernel ABI.  This is the
most important part of the driver because it is something we can never
change.  This interface is the very first thing we'll want to
understand and review, before we even look at the implementation.

And it isn't even described!  Not in the changelog, not in a
documentation file, not even in code comments.

Please, provide a description of this proposed interface.  Sufficient
for reviewers to understand it and for users to use it.  Pobably this
will require some description of the hardware functions as well.

Please also consider updating Documentation/ABI/

> +static ssize_t pn544_test(struct device *dev,
> +   struct device_attribute *attr, char *buf)
> +{
> + struct pn544_info *info = dev_get_drvdata(dev);
> + struct i2c_client *client = info->i2c_dev;
> + struct pn544_nfc_platform_data *pdata = client->dev.platform_data;
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", pdata->test());
> +}
> +
> +static int pn544_enable(struct pn544_info *info, int mode)
> +{
> + struct pn544_nfc_platform_data *pdata;
> + struct i2c_client *client = info->i2c_dev;
> +
> + int r;
> +
> + r = regulator_bulk_enable(ARRAY_SIZE(info->regs), info->regs);
> + if (r < 0)
> + return r;
> +
> + pdata = client->dev.platform_data;
> + info->read_irq = PN544_NONE;
> + if (pdata->enable)
> + pdata->enable(mode);
> +
> + if (mode) {
> + if (!info->fw_buf)
> + info->fw_buf = kmalloc(PN544_MAX_I2C_TRANSFER,
> +GFP_KERNEL);

>From my reading, later code will crash the kernel if this allocation failed.

Also, is there a race here between reading info->fw_buf and setting it?
Can two CPUs get into thei function at the same time?  If so, there
should be locking.  Say, a mutex local to this function.


> + info->state = PN544_ST_FW_READY;
> + dev_dbg(&client->dev, "now in FW-mode\n");
> + } else {
> + kfree(info->fw_buf);
> + info->fw_buf = NULL;
> + info->state = PN544_ST_READY;
> + dev_dbg(&client->dev, "now in HCI-mode\n");
> + }
> +
> + msleep(PN544_BOOT_TIME);
> +
> + return 0;
> +}
> +
>
> ...
>
> +static int pn544_i2c_write(struct i2c_client *client, u8 *buf, int len)
> +{
> + int r;
> +
> + if (len < 4 || len != (buf[0] + 1) || check_crc(buf, len))
> + return -EBADMSG;

EBADMSG is a unix streams errno.  It's quite inappropriate that a
device driver be using it.  Imagine the poor user's confution if he
sees this message pop up on his screen.

> + msleep(PN544_I2C_IO_TIME

Re: [PATCHv4 0/5] BH1770GLC, SFH7770 and APDS990x als / proximity sensor drivers

2010-10-19 Thread Andrew Morton
On Tue, 19 Oct 2010 10:09:43 +0300
Samu Onkalo  wrote:

> Patch set contains two drivers. One for BH1770GLC / SFH7770 chips and
> one for APDS990X chip.
> 
> Both drivers have similar sysfs based interface. Both supports
> pm_runtime and regulator frame work. There is short documentation
> for both drivers.
> 
> Unfortunately I can't promise data sheets to public access.
> 
> Changes since patch set version 3:
> 
> - acked-by added to patches 1, 3 and 5
> - Documentation clean up (extra lines removed, LUX changed to lux)
> 
> Patches 2 and 4 are not directly acked or I have missed something.
> However, all requested cleanups have been done to those patches.

That all looks quite nice.

I'll fold patch 2 into patch 1 and patch 4 into patch 3 before sending
the patches onwards - there isn't much point in separating the code
patch from the wire-it-up-to-kbuild patch.

--
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: [PATCHv2 0/4] Led driver support for LP5521 and LP5523 chips

2010-10-05 Thread Andrew Morton
On Wed, 29 Sep 2010 12:10:01 +0300
Samu Onkalo  wrote:

> Documentation is provided as a part of the patch set.
> I created "leds" sub directory to Documentation.
> Perhaps rest of the leds* documentation should be moved
> there.

That sounds like a good idea.
--
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: [PATCHv2 1/4] leds: driver for National Semiconductor LP5521 chip

2010-10-05 Thread Andrew Morton
On Wed, 29 Sep 2010 12:10:02 +0300
Samu Onkalo  wrote:

> +#define cdev_to_led(c)   container_of(c, struct lp5521_led, cdev)
> +#define engine_to_lp5521(eng)container_of((eng), struct lp5521_chip, 
> \
> + engines[(eng)->id - 1])
> +#define led_to_lp5521(led)   container_of((led), struct lp5521_chip, \
> + leds[(led)->id])

It would be better if these were implemented as C functions.  That would
make them cqearer, cleaner and more type-safe (container_of uses
typecasts in a way which makes it possible to misuse).

Also that would fix the bug which cdev_to_led(foo++) adds.

Similar improvements can be made to the other driver.  Also
SHIFT_MASK() and LE_ACTIVE() could become regular old lower-case C
functions, I think.

--
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/4] Led driver support for LP5521 and LP5523 chips

2010-09-28 Thread Andrew Morton
On Tue, 28 Sep 2010 11:00:20 +0200
Linus Walleij  wrote:

> 2010/9/27  :
> 
> > I haven't got any mails or comments from Richard about this driver.
> > Also led tree here:
> > http://git.o-hand.com/cgit.cgi/linux-rpurdie-leds/
> >
> > is not updated in 4 months.
> >
> > I'm just wondering should I send this to somebody else than Richard Purdie?
> >
> > I'll update the driver based on other comments and send v2 patch set.
> 
> We (ST-Ericsson) need this driver in as well.
> 
> If Richard is busy maybe Andrew can pick it up for the time being?

I looked.  There are a few valid-looking review comments which remain
unaddressed and un-replied-to.

> Looking forward to the v2 version!

yup, please cc me on v2.
--
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: idr_get_new_exact ?

2010-09-20 Thread Andrew Morton
On Mon, 20 Sep 2010 16:11:31 +0200
Ohad Ben-Cohen  wrote:

> Occasionally, drivers care about the value that idr associates with
> their pointers.
> 
> Today we have idr_get_new_above() which allocates a new idr entry
> above or equal to a given starting id, but sometimes drivers need to
> force an exact value.
> 
> To overcome this small API gap, drivers are wrapping idr_get_new_above
> and then either BUG_ON() or just call idr_remove() and returns -EBUSY
> when idr allocates them an id which is different than their requested
> value.
> 
> There are only a handful of users who need this (see below. especially
> note the i2c comment :), but it might be nice to have such an API (a
> bit less of code, and a bit less error prone).
> 
> Would something like the below be desirable/acceptable ?

It seems OK to me - it's an improvement over what we have now.

> (untested. and i just picked the simplest and straight-forward way to
> implement this; obviously it's not optimal since there's no reason to
> even allocate an id if we know it's not the id we're looking for. but
> it's enough to get the idea, it's not a hot path, and it's what
> drivers are doing today)

Sure, we can speed it up later if that appears to be necessary.

--
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] misc: adds support the FSA9480 USB Switch

2010-09-17 Thread Andrew Morton
(catching up!)

On Tue, 31 Aug 2010 19:28:42 +0900
Minkyu Kang  wrote:

> The FSA9480 is a USB port accessory detector and switch.
> This patch adds support the FSA9480 USB Switch.
> 

What a strange device.

Is there a data sheet available?

> +config USB_SWITCH_FSA9480
> + tristate "FSA9480 USB Switch"
> + depends on I2C
> + help
> +   The FSA9480 is a USB port accessory detector and switch.
> +   The FSA9480 is fully controlled using I2C and enables USB data,
> +   stereo and mono audio, video, microphone and UART data to use
> +   a common connector port.

So if I'm understanding it correctly, it's an i2c-controlled device
which turns USB devices on and off, multiplexing them into a single USB
port?  So if I switch from "USB data" over to "microphone", the USB
subsystem will see the "USB data" device get unplugged and will see a
"microphone" get plugged in?

Or something else.  Am I even vaguely understanding this thing?

It would help if the changelog were to contain a paragraph
describing the overall behaviour of this device.

>
> ...
>
> +void fsa9480_set_switch(const char *buf)
> +{
> + struct fsa9480_usbsw *usbsw = chip;
> + struct i2c_client *client = usbsw->client;
> + unsigned int value;
> + unsigned int path = 0;
> +
> + value = fsa9480_read_reg(client, FSA9480_REG_CTRL);
> +
> + if (!strncmp(buf, "VAUDIO", 6)) {
> + path = SW_VAUDIO;
> + value &= ~CON_MANUAL_SW;
> + } else if (!strncmp(buf, "UART", 4)) {
> + path = SW_UART;
> + value &= ~CON_MANUAL_SW;
> + } else if (!strncmp(buf, "AUDIO", 5)) {
> + path = SW_AUDIO;
> + value &= ~CON_MANUAL_SW;
> + } else if (!strncmp(buf, "DHOST", 5)) {
> + path = SW_DHOST;
> + value &= ~CON_MANUAL_SW;
> + } else if (!strncmp(buf, "AUTO", 4)) {
> + path = SW_AUTO;
> + value |= CON_MANUAL_SW;
> + } else {
> + printk(KERN_ERR "Wrong command\n");
> + return;
> + }
> +
> + usbsw->mansw = path;
> + fsa9480_write_reg(client, FSA9480_REG_MANSW1, path);
> + fsa9480_write_reg(client, FSA9480_REG_CTRL, value);
> +}
> +EXPORT_SYMBOL_GPL(fsa9480_set_switch);

Why was this exported?

> +ssize_t fsa9480_get_switch(char *buf)
> +{
> + struct fsa9480_usbsw *usbsw = chip;
> + struct i2c_client *client = usbsw->client;
> + unsigned int value;
> +
> + value = fsa9480_read_reg(client, FSA9480_REG_MANSW1);
> +
> + if (value == SW_VAUDIO)
> + return sprintf(buf, "VAUDIO\n");
> + else if (value == SW_UART)
> + return sprintf(buf, "UART\n");
> + else if (value == SW_AUDIO)
> + return sprintf(buf, "AUDIO\n");
> + else if (value == SW_DHOST)
> + return sprintf(buf, "DHOST\n");
> + else if (value == SW_AUTO)
> + return sprintf(buf, "AUTO\n");
> + else
> + return sprintf(buf, "%x", value);
> +}
> +EXPORT_SYMBOL_GPL(fsa9480_get_switch);

This export also has no callers?

These functions implement a userspace API.  Userspace APIs are
important.  But the patch provided no documentation for that API. 
Please always fully, exhaustively document userspace APIs!  For they
are the one part of the driver which we can never change.

Documenting them in a documentation file is OK.  Also there's
Documentation/ABI/.  And they can be nicely described in the changelog
too.

But providing us with no description at all makes review harder and
less effective than we'd like it to be, and results in a driver which
is harder for our users to use.

OK?

> +static ssize_t fsa9480_show_status(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + struct fsa9480_usbsw *usbsw = dev_get_drvdata(dev);
> + struct i2c_client *client = usbsw->client;
> + int devid, ctrl, adc, dev1, dev2, intr,
> + intmask1, intmask2, time1, time2, mansw1;
> +
> + devid = fsa9480_read_reg(client, FSA9480_REG_DEVID);
> + ctrl = fsa9480_read_reg(client, FSA9480_REG_CTRL);
> + adc = fsa9480_read_reg(client, FSA9480_REG_ADC);
> + intmask1 = fsa9480_read_reg(client, FSA9480_REG_INT1_MASK);
> + intmask2 = fsa9480_read_reg(client, FSA9480_REG_INT2_MASK);
> + dev1 = fsa9480_read_reg(client, FSA9480_REG_DEV_T1);
> + dev2 = fsa9480_read_reg(client, FSA9480_REG_DEV_T2);
> + time1 = fsa9480_read_reg(client, FSA9480_REG_TIMING1);
> + time2 = fsa9480_read_reg(client, FSA9480_REG_TIMING2);
> + mansw1 = fsa9480_read_reg(client, FSA9480_REG_MANSW1);
> +
> + fsa9480_read_irq(client, &intr);
> +
> + return sprintf(buf, "Device ID(%02x), CTRL(%02x)\n"
> + "ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n"
> + "INT(%04x), INTMASK(%02x, %02x)\n"
> + "TIMING(%02x, %02x), MANSW1(%02x)\n",
> + devid,

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

2010-09-03 Thread Andrew Morton
On Thu,  2 Sep 2010 16:40:37 -0700
ac...@nvidia.com wrote:

> This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
> the magnetometer section of the IIO subsystem, and implements the raw
> magn_x,y,z attributes, as well as x,y,z factory calibration attributes.
> 
> Signed-off-by: Andrew Chew 
> ---
>  drivers/staging/iio/magnetometer/Kconfig  |   11 +
>  drivers/staging/iio/magnetometer/Makefile |1 +
>  drivers/staging/iio/magnetometer/ak8975.c |  521 
> +
>  3 files changed, 533 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/magnetometer/ak8975.c

This is version 2, yes?

It would be nice to provide some accounting of how it differs from
version 1, for those who already reviewed version 1.  The v1->v2 diff
is below.

--- a/drivers/staging/iio/magnetometer/ak8975.c~a
+++ a/drivers/staging/iio/magnetometer/ak8975.c
@@ -192,14 +192,14 @@ static int ak8975_setup(struct i2c_clien
   AK8975_REG_CNTL_MODE_SHIFT);
if (!status) {
dev_err(&client->dev, "Error in setting fuse access mode\n");
-   return false;
+   return -EINVAL;
}
 
/* Get asa data and store in the device data. */
status = ak8975_read_data(client, AK8975_REG_ASAX, 3, buffer);
if (!status) {
dev_err(&client->dev, "Not able to read asa data\n");
-   return -ENODEV;
+   return -EINVAL;
}
 
data->asa[0] = buffer[0] & 0xFF;
@@ -236,10 +236,10 @@ static ssize_t store_mode(struct device 
 
/* Convert mode string and do some basic sanity checking on it.
   only 0 or 1 are valid. */
-   if (strict_strtol(buf, 10, &oval))
+   if (strict_strtoul(buf, 10, &oval))
return -EINVAL;
 
-   if ((oval < 0) || (oval > 1)) {
+   if (oval > 1) {
dev_err(dev, "mode value is not supported\n");
return -EINVAL;
}
@@ -320,7 +320,7 @@ static ssize_t show_raw(struct device *d
   AK8975_REG_CNTL_MODE_SHIFT);
if (!status) {
dev_err(&client->dev, "Error in setting operating mode\n");
-   return false;
+   return -EINVAL;
}
 
/* Wait for the conversion to complete. */
@@ -333,13 +333,13 @@ static ssize_t show_raw(struct device *d
}
if (!timeout_ms) {
dev_err(&client->dev, "Conversion timeout happend\n");
-   return false;
+   return -EINVAL;
}
 
status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
if (!status) {
dev_err(&client->dev, "Error in reading ST1\n");
-   return false;
+   return -EINVAL;
}
 
if (read_status & AK8975_REG_ST1_DRDY_MASK) {
@@ -347,13 +347,13 @@ static ssize_t show_raw(struct device *d
  1, &read_status);
if (!status) {
dev_err(&client->dev, "Error in reading ST2\n");
-   return false;
+   return -EINVAL;
}
if (read_status & (AK8975_REG_ST2_DERR_MASK |
   AK8975_REG_ST2_HOFL_MASK)) {
dev_err(&client->dev, "ST2 status error 0x%x\n",
read_status);
-   return false;
+   return -EINVAL;
}
}
 
@@ -363,7 +363,7 @@ static ssize_t show_raw(struct device *d
  (u8 *)&meas_reg);
if (!status) {
dev_err(&client->dev, "Read axis data fails\n");
-   return false;
+   return -EINVAL;
}
 
/* Endian conversion of the measured values */
@@ -418,6 +418,11 @@ static int ak8975_probe(struct i2c_clien
data->eoc_irq = client->irq;
data->eoc_gpio = irq_to_gpio(client->irq);
 
+   if (!data->eoc_gpio) {
+   dev_err(&client->dev, "failed, no valid GPIO\n");
+   goto exit_free;
+   }
+
err = gpio_request(data->eoc_gpio, "ak_8975");
if (err < 0) {
dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
@@ -429,7 +434,6 @@ static int ak8975_probe(struct i2c_clien
if (err < 0) {
dev_err(&client->dev, "Failed to configure input direction for"
" GPIO %d, error %d\n", data->eoc_gpio, err);
-   gpio_free(data->eoc_gpio);
goto exit_gpio;
}
 
_

--
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] drivers/misc/akm8975: Add compass sensor driver

2010-08-27 Thread Andrew Morton
On Fri, 27 Aug 2010 09:16:07 +0200 Jean Delvare  wrote:

> > +static int akm_aot_open(struct inode *inode, struct file *file)
> > +{
> > +   int ret = -1;
> 
> Useless and dangerous initialization.
> 
> > +
> > +   FUNCDBG("called");
> > +   if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> > +   wake_up(&open_wq);
> > +   ret = 0;

this doesn't do anything either.

> > +   }
> > +
> > +   ret = nonseekable_open(inode, file);
> > +   if (ret)
> > +   return ret;
> > +
> > +   file->private_data = akmd_data;
> > +
> > +   return ret;
> > +}
--
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] drivers/misc/akm8975: Add compass sensor driver

2010-08-26 Thread Andrew Morton

> From: andrew.c...@smtp1.linux-foundation.org

Your email setup is doing strange things.

On Thu, 26 Aug 2010 18:31:57 -0700 andrew.c...@smtp1.linux-foundation.org wrote:

> From: Andrew Chew 
> 
> This is for the Asahi Kasei's I2C compass sensor AK8975.
> 
> Signed-off-by: Andrew Chew 
> ---
>  drivers/misc/Kconfig|8 +
>  drivers/misc/Makefile   |1 +
>  drivers/misc/akm8975.c  |  670 
> +++
>  include/linux/akm8975.h |   87 ++
>  4 files changed, 766 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/akm8975.c
>  create mode 100644 include/linux/akm8975.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9bb3d03 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -304,6 +304,14 @@ config SENSORS_TSL2550
> This driver can also be built as a module.  If so, the module
> will be called tsl2550.
>  
> +config SENSORS_AK8975
> + tristate "AK8975 compass support"
> + default n
> + depends on I2C
> + help
> +   If you say yes here you get support for Asahi Kasei's
> +   orientation sensor AK8975.

Should it be in drivers/hwmon?

Perhaps not, given that drivers/misc/hmc6352.c is in drivers/misc/

Did you review the userspace interface in hmc6352.c?  Is the interface
for this driver identical to that one (I hope so).

Your changelog didn't describe this driver's interface at all, and the
patch provided no documentation for the interface.  But the interface
is the most important part of the driver, because it is the one thing
we can never change.

>  config EP93XX_P
>   tristate "EP93xx PWM support"
>   depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..366791b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP)  += iwmc3200top/
>  obj-y+= eeprom/
>  obj-y+= cb710/
>  obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
> +obj-$(CONFIG_SENSORS_AK8975) += akm8975.o
> diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c
> new file mode 100644
> index 000..17a096e
> --- /dev/null
> +++ b/drivers/misc/akm8975.c
> @@ -0,0 +1,670 @@
> +/* drivers/misc/akm8975.c - akm8975 compass driver
> + *
> + * Copyright (C) 2007-2008 HTC Corporation.
> + * Author: Hou-Kun Chen 

It would be nice to get this person's signed-off-by.

> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +/*
> + * Revised by AKM 2009/04/02
> + * Revised by Motorola 2010/05/27

And even per...@motorola's, if possible?

> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define AK8975DRV_CALL_DBG 0
> +#if AK8975DRV_CALL_DBG
> +#define FUNCDBG(msg) pr_err("%s:%s\n", __func__, msg);
> +#else
> +#define FUNCDBG(msg)
> +#endif
> +
> +#define AK8975DRV_DATA_DBG 0
> +#define MAX_FAILURE_COUNT 10
> +
> +struct akm8975_data {
> + struct i2c_client *this_client;
> + struct akm8975_platform_data *pdata;
> + struct input_dev *input_dev;
> + struct work_struct work;
> + struct mutex flags_lock;
> +};
> +
> +/*
> +* Because misc devices can not carry a pointer from driver register to
> +* open, we keep this global. This limits the driver to a single instance.
> +*/
> +struct akm8975_data *akmd_data;

That has changed.  See the post-2.6.35 patch

commit fa1f68db6ca7ebb6fc4487ac215bffba06c01c28
Author: Samu Onkalo 
Date:   Mon May 24 14:33:10 2010 -0700

drivers: misc: pass miscdevice pointer via file private data


> +static DECLARE_WAIT_QUEUE_HEAD(open_wq);
> +
> +static atomic_t open_flag;
> +
> +static short m_flag;
> +static short a_flag;
> +static short t_flag;
> +static short mv_flag;
> +
> +static short akmd_delay;

It would bee great to add comments describing the above globals.

The driver assumes that I will only ever have one device in the
machine.  That's surely a reasonable assumption, unless I'm making a
compass-testing workstation.  But it's a bit sad from a general driver
design point of view.

> +static ssize_t akm8975_show(struct device *dev, struct device_attribute 
> *attr,
> +  char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client,
> +  AK8975_REG_CNT

Re: [PATCH v2] gpio: sx150x: Add Semtech I2C sx150x gpio expander driver.

2010-07-19 Thread Andrew Morton
On Mon, 19 Jul 2010 16:31:42 -0700
Andrew Morton  wrote:

> On Thu,  8 Jul 2010 12:37:14 -0700
> Gregory Bean  wrote:
> 
> > Add support for Semtech SX150-series I2C GPIO expanders.
> 
> It doesn't work when compiled as a module:
> 
> ERROR: "irq_to_desc" [drivers/gpio/sx150x.ko] undefined!
> ERROR: "handle_edge_irq" [drivers/gpio/sx150x.ko] undefined!
> ERROR: "set_irq_chip_and_handler" [drivers/gpio/sx150x.ko] undefined!
> ERROR: "set_irq_noprobe" [drivers/gpio/sx150x.ko] undefined!
> 
> I think there's a reason why some of those things aren't exported to
> modules.  Perhaps Thomas can remind us?
> 
> Meanwhile I'll do s/tristate/bool/.

Which of course didn't work because i2c is a module.


drivers/built-in.o: In function `sx150x_i2c_read':
sx150x.c:(.text+0x5362): undefined reference to `i2c_smbus_read_byte_data'
drivers/built-in.o: In function `sx150x_i2c_write':
sx150x.c:(.text+0x53e4): undefined reference to `i2c_smbus_write_byte_data'
drivers/built-in.o: In function `sx150x_init':
sx150x.c:(.init.text+0x418): undefined reference to `i2c_register_driver'
drivers/built-in.o: In function `sx150x_probe':
sx150x.c:(.devinit.text+0xda1): undefined reference to 
`i2c_smbus_write_word_data'
drivers/built-in.o: In function `sx150x_exit':
sx150x.c:(.exit.text+0x7c): undefined reference to `i2c_del_driver'
--
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] gpio: sx150x: Add Semtech I2C sx150x gpio expander driver.

2010-07-19 Thread Andrew Morton
On Thu,  8 Jul 2010 12:37:14 -0700
Gregory Bean  wrote:

> Add support for Semtech SX150-series I2C GPIO expanders.

It doesn't work when compiled as a module:

ERROR: "irq_to_desc" [drivers/gpio/sx150x.ko] undefined!
ERROR: "handle_edge_irq" [drivers/gpio/sx150x.ko] undefined!
ERROR: "set_irq_chip_and_handler" [drivers/gpio/sx150x.ko] undefined!
ERROR: "set_irq_noprobe" [drivers/gpio/sx150x.ko] undefined!

I think there's a reason why some of those things aren't exported to
modules.  Perhaps Thomas can remind us?

Meanwhile I'll do s/tristate/bool/.
--
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] gpio: sx150x: Add Semtech I2C sx150x gpio expander driver.

2010-07-12 Thread Andrew Morton
On Mon, 12 Jul 2010 14:25:24 +0530
Trilok Soni  wrote:

> Hi Jean, Andrew,
> 
> On 7/9/2010 5:27 PM, Trilok Soni wrote:
> > On 7/9/2010 1:07 AM, Gregory Bean wrote:
> >> Add support for Semtech SX150-series I2C GPIO expanders.
> >> Compatible models include:
> >>
> >> 8 bits:  sx1508q
> >> 16 bits: sx1509q
> >>
> >> Signed-off-by: Gregory Bean 
> >> ---
> >>  drivers/gpio/Kconfig   |   11 +
> >>  drivers/gpio/Makefile  |1 +
> >>  drivers/gpio/sx150x.c  |  646 
> >> 
> >>  include/linux/i2c/sx150x.h |   78 ++
> >>  4 files changed, 736 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/gpio/sx150x.c
> >>  create mode 100644 include/linux/i2c/sx150x.h
> >>
> > 
> > Looks good to me. 
> > 
> > Reviewed-by: Trilok Soni 
> > 
> 
> Any review comments on this new version of the driver?

It looked OK to me.  You should have received an email when I merged it
into my tree?

--
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] hwmon: Add support for JEDEC JC 42.4 compliant temperature sensors

2010-07-12 Thread Andrew Morton
On Sun, 11 Jul 2010 07:53:04 -0700
Guenter Roeck  wrote:

> +static int jc42_probe(struct i2c_client *new_client,
> +   const struct i2c_device_id *id)
> +{
> + struct jc42_data *data;
> + int config, cap, err;
> +
> + data = kzalloc(sizeof(struct jc42_data), GFP_KERNEL);
> + if (!data) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + i2c_set_clientdata(new_client, data);
> + data->valid = false;

I think it would be acceptable to assume that memset(..., 0, ...) sets
a bool to `false' ;)


> + mutex_init(&data->update_lock);
--
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: Synaptics touchscreen for HTC Dream: check that smbus is available

2009-08-17 Thread Andrew Morton
On Sat, 8 Aug 2009 15:40:50 +0200
Pavel Machek  wrote:

> > >> Because this driver is using smbus i2c apis, it will be good to add
> > >> that check too.
> > >
> > > So I should do something like
> > >
> > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > > __ __ __ __...
> > >
> > > in addition?
> > 
> > Yes.
> 
> Ok, here it is.
> 
> --- 
> 
> Check that SMBUS APIs are available in touchscreen driver.

Why?

> Signed-off-by: Pavel Machek 
> 
> diff --git a/drivers/staging/dream/synaptics_i2c_rmi.c 
> b/drivers/staging/dream/synaptics_i2c_rmi.c
> index dc1e3c7..6e23276 100644
> --- a/drivers/staging/dream/synaptics_i2c_rmi.c
> +++ b/drivers/staging/dream/synaptics_i2c_rmi.c
> @@ -373,6 +373,12 @@ static int __devinit synaptics_ts_probe(
>   goto err_check_functionality_failed;
>   }
>  
> + if (!i2c_check_functionality(client->adapter, 
> I2C_FUNC_SMBUS_WORD_DATA)) {
> + pr_err("synaptics_ts_probe: need I2C_FUNC_SMBUS_WORD_DATA\n");
> + ret = -ENODEV;
> + goto err_check_functionality_failed;
> + }
> +
>   ts = kzalloc(sizeof(*ts), GFP_KERNEL);
>   if (ts == NULL) {
>   ret = -ENOMEM;

--
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-pxa.c: timeouts off by 1

2009-04-23 Thread Andrew Morton
On Thu, 23 Apr 2009 17:02:18 +0200
Jean Delvare  wrote:

> On Thu, 23 Apr 2009 16:27:39 +0200, Roel Kluin wrote:
> > Ok, here's for drivers/i2c/busses/i2c-pxa.c. Note that I found another,
> > the last hunk.
> > --->8-8<--
> > With `while (timeout--)' timeout reaches -1 after the loop, so the tests
> > below are off by one.
> > 
> > Signed-off-by: Roel Kluin 
> > ---
> 
> Ben, Wolfram, I'll let you handle this one as it's an arm driver.
> 

The patch looks OK, but the original code is weird.

: static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
: {
:   int timeout = DEF_TIMEOUT;
: 
:   while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB)) {
:   if ((readl(_ISR(i2c)) & ISR_SAD) != 0)
:   timeout += 4;
: 
:   msleep(2);
:   show_state(i2c);
:   }
: 
:   if (timeout < 0)
:   show_state(i2c);
: 
:   return timeout < 0 ? I2C_RETRY : 0;
: }

The timeout+=4 inside the loop makes my brain hurt.  It makes the loop
potentially almost-infinite.  By effectively doing timeout+=3 each time
we'll break out of the loop after we've wrapped through 0x1
three times.  Or something.  Help!



Also, i2c_pxa_pio_set_master() does

long timeout = 2 * DEF_TIMEOUT;

whereas i2c_pxa_wait_bus_not_bus() does

int timeout = DEF_TIMEOUT;


`int' seems an appropriate choice.

--
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] Added driver for ISL29003 ambient light sensor

2009-03-11 Thread Andrew Morton
On Wed, 11 Mar 2009 09:32:52 +0100 Daniel Mack  wrote:

> On Tue, Mar 10, 2009 at 09:28:26PM -0700, Andrew Morton wrote:
> > On Wed, 11 Mar 2009 01:31:55 +0100 Daniel Mack  wrote:
> > > > All looks fine to me.
> > > > 
> > > > Acked-by: Jonathan Cameron 
> > 
> > I cannot find this review which Jonathan performed.  Fumbled
> > reply-to-all, or did vger break?
> 
> Jonathan replied to the posting on linux-i2c and linux-kernel wasn't in
> the loop, sorry. Find his posting here:
> 
>   http://marc.info/?l=linux-i2c&m=123669064909549&w=2

OK.  Please always use reply-to-all, guys.

> > I can take care of it.
> > 
> > Was any thought given to the CONFIG_SYSFS=n situation?
> 
> You're right, it should depend on that. You want me so send a patch on
> top of the other or one that replaces it?

Either way is OK by me at this time.
--
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] Added driver for ISL29003 ambient light sensor

2009-03-10 Thread Andrew Morton
On Wed, 11 Mar 2009 01:31:55 +0100 Daniel Mack  wrote:

> On Tue, Mar 10, 2009 at 05:49:55PM +, Jonathan Cameron wrote:
> > >>From 8ee5021834900f312ff26cd2586c18e99af31a5d Mon Sep 17 00:00:00 2001
> > > From: Daniel Mack 
> > > Date: Tue, 10 Mar 2009 16:13:07 +0100
> > > Subject: [PATCH] Added driver for ISL29003 ambient light sensor
> > > 
> > > This patch adds a driver for Intersil's ISL29003 ambient light sensor
> > > device plus some documentation. Inspired by tsl2550.c, a driver for a
> > > similar device.
> > > 
> > > It is put to drivers/misc for now until the industrial I/O framework
> > > gets merged.
> > > 
> > > Signed-off-by: Daniel Mack 
> 
> [...]
> 
> > All looks fine to me.
> > 
> > Acked-by: Jonathan Cameron 

I cannot find this review which Jonathan performed.  Fumbled
reply-to-all, or did vger break?

> Ok, wonderful :)
> 
> What's the merge path for that, who'll queue it?

I can take care of it.

Was any thought given to the CONFIG_SYSFS=n situation?
--
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] Incremental i2c-mpc driver fix for multi-master i2c busses.

2009-01-12 Thread Andrew Morton
On Mon, 5 Jan 2009 14:21:02 +0100
Clifford Wolf  wrote:

> Incremental i2c-mpc driver fix for multi-master i2c busses.
> 
> This is an incremental bugfix for the i2c-mpc driver. It is based
> on the bugfix I've sent on 2008-12-22:
> 
>   http://lkml.org/lkml/2008/12/22/99
> 
> There still was a remaining problem with multi-master i2c busses
> when an i2c bus access is interrupted by a unix signal while
> waiting for bus arbitration.
> 
> This is an extreamly rare case but I managed to stumble over it in
> multi master i2c performance tests.
> 
> Tested with a freescale MPC8349E host cpu.

That's not a very good changelog - it has basically no information,
apart from the linked-to original changelog.

And the linked-to changelog has no description of the bug which is
being fixed.

> --- drivers/i2c/busses/i2c-mpc.c  (revision 2216)
> +++ drivers/i2c/busses/i2c-mpc.c  (working copy)

Please prepare patches in `patch -p1' form:

--- a/drivers/i2c/busses/i2c-mpc.c
+++ a/drivers/i2c/busses/i2c-mpc.c

--
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 from interrupt context?

2008-11-10 Thread Andrew Morton
On Fri, 07 Nov 2008 17:05:35 +0100 Krzysztof Halasa <[EMAIL PROTECTED]> wrote:

> Jean Delvare <[EMAIL PROTECTED]> writes:
> 
> > The situation is far from perfect though. For one thing, I seem to
> > recall that Andrew Morton didn't like the approach taken in
> > i2c_transfer(). For another, i2c_smbus_xfer() was not yet modified so
> > at this point only I2C-level transactions can be non-sleeping,
> > SMBus-level transactions can't. But all this could be fixed by anyone
> > who cares about these specific issues.
> 
> Thanks, I'll look at it.

The problem (well: bug) is that in_atomic() returns false inside a
spinlock when CONFIG_PREEMPT=n.  The code as it stands can sleep
inside a spinlock, which is deadlockable if a scheduled-to task
tries to take the same spinlock.

There is no means like this by which a piece of code can determine
whether it can call schedule().  The pattern which we use in many many
places (most especially GFP_KERNEL/GFP_ATOMIC) is to pass a flag down
to callees telling them in some manner which context they were called from.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html