Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754740Ab0H0Q7h (ORCPT ); Fri, 27 Aug 2010 12:59:37 -0400 Received: from mgw-sa02.nokia.com ([147.243.1.48]:34616 "EHLO mgw-sa02.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751565Ab0H0Q7e (ORCPT ); Fri, 27 Aug 2010 12:59:34 -0400 Subject: Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver From: Onkalo Samu Reply-To: samu.p.onkalo@nokia.com To: ext Alan Cox , ext Dmitry Torokhov Cc: "linux-i2c@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "akpm@linux-foundation.org" , "linux-input@vger.kernel.org" In-Reply-To: <20100827133109.1eb974ed@lxorguk.ukuu.org.uk> References: <1282910083-8629-1-git-send-email-samu.p.onkalo@nokia.com> <1282910083-8629-2-git-send-email-samu.p.onkalo@nokia.com> <20100827133109.1eb974ed@lxorguk.ukuu.org.uk> Content-Type: text/plain; charset="UTF-8" Organization: Nokia Oyj Date: Fri, 27 Aug 2010 19:59:13 +0300 Message-ID: <1282928353.2194.27.camel@noppispoppis.nmp.nokia.com> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 Content-Transfer-Encoding: 7bit X-Nokia-AV: Clean Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5665 Lines: 194 Alan, Thank you for the comments. Dmitry, there is one question for you. -Samu On Fri, 2010-08-27 at 14:31 +0200, ext Alan Cox wrote: > > +static int ak8974_regulators_on(struct ak8974_chip *chip) > > +{ > > + int ret; > > + ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs); > > + if (ret == 0) > > + msleep(AK8974_POWERON_DELAY); > > + > > + return ret; > > +} > > + > > +static inline void ak8974_regulators_off(struct ak8974_chip *chip) > > +{ > > + regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs); > > +} > > That bit seems platform specific but in generic code ? > If the regulator frame work is not configured, this code does nothing. I have understood (hopefully correctly) that if the frame work is in use drivers could support that directly assuming that regulators are configured for that platform. If that is not the case, this should be platform specific. > > > +static ssize_t ak8974_misc_read(struct file *file, char __user *buf, > > + size_t count, loff_t *offset) > > +{ > > + struct ak8974_chip *chip = container_of(file->private_data, > > + struct ak8974_chip, > > + miscdev); > > + struct ak8974_data data; > > So we have a different API to the ak8975 just posted and to the other > existing devices. This needs sorting out across the devices before there > is a complete disaster. Right now we have a mix of submissions pending > which variously use > > misc + sysfs > sysfs > input (reporting X Y Z etc axes) > About year ago I send driver for the same chip with input-device interface. During that time I asked from Dmitry Torokhov that is that a correct interface for this kind of driver. I understood that input should not be used for this kind of sensors. sysfs is quite handy interface for small sensors. However, one problem is that the driver doesn't know when the interface is in use. I ended up to misc device to get information about the usercount for PM purposes. Dmitry, what is your opinion about using input device interface for this kind of sensors? > Someone needs to decide on a single API before it's too late. > That is definitely true. Could it be IIO? > > + > > + if (count < sizeof(data)) > > + return -EINVAL; > > + > > + if (*offset >= chip->offset) { > > + schedule_work(&chip->work); > > + if (file->f_flags & O_NONBLOCK) > > + return -EAGAIN; > > + if (wait_event_interruptible(chip->misc_wait, > > + (*offset < chip->offset))) > > + return -ERESTARTSYS; > > + } > > + > > + mutex_lock(&chip->lock); > > + data.x = chip->x; > > + data.y = chip->y; > > + data.z = chip->z; > > + data.valid = chip->valid; > > + *offset = chip->offset; > > + mutex_unlock(&chip->lock); > > What happens if you have two readers - is it fine they get copies of the > same event when it races ? Yes, it makes sense to report latest available information for all users. > > > > + > > + return copy_to_user(buf, &data, sizeof(data)) ? -EFAULT : sizeof(data); > > Pedantically if data is consumed and a partial copy occurs you should > return the bytes successfully copied. I need to check that. > > > +static DEVICE_ATTR(chip_id, S_IRUGO, ak8974_show_chip_id, NULL); > > + > > +static ssize_t ak8974_show_range(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct ak8974_chip *chip = dev_get_drvdata(dev); > > + return sprintf(buf, "%d\n", chip->max_range); > > +} > > Other devices make all of this sysfs or use input. We need to work out > what the right interface actually is. > As mentioned above, sysfs is not used to get information about the user count for PM purposes. > > + snprintf(chip->name, sizeof(chip->name), "ak8974%d", > > + atomic_add_return(1, &device_number) - 1); > > Surely this is serialized anyway ? Is it possible that when there are several chips on the system (in different bussed for example), probe functions are running in parallel? > > > > +#ifdef CONFIG_PM > > +static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg) > > +{ > > + struct ak8974_chip *chip = i2c_get_clientdata(client); > > + mutex_lock(&chip->users_lock); > > + if (chip->users > 0) > > + ak8974_enable(chip, AK8974_PWR_OFF); > > + mutex_unlock(&chip->users_lock); > > + return 0; > > +} > > + > > +static int ak8974_resume(struct i2c_client *client) > > +{ > > + struct ak8974_chip *chip = i2c_get_clientdata(client); > > + mutex_lock(&chip->users_lock); > > + if (chip->users > 0) > > + ak8974_enable(chip, AK8974_PWR_ON); > > + mutex_unlock(&chip->users_lock); > > + return 0; > > +} > > The whole chip->users thing you are implementing is basically a > reimplementation of the kernel runtime pm stuff - so can that be used > instead ? True. Most probably it can be used instead of own implementation. > > > > +#ifndef __LINUX_I2C_AK8974_H > > +#define __LINUX_I2C_AK8974_H > > + > > +#define AK8974_NO_MAP 0 > > +#define AK8974_DEV_X 1 > > +#define AK8974_DEV_Y 2 > > +#define AK8974_DEV_Z 3 > > +#define AK8974_INV_DEV_X -1 > > +#define AK8974_INV_DEV_Y -2 > > +#define AK8974_INV_DEV_Z -3 > > + > > +struct ak8974_platform_data { > > + s8 axis_x; > > + s8 axis_y; > > + s8 axis_z; > > +}; > > + > > +/* Device name: /dev/ak8974n, where n is a running number */ > > +struct ak8974_data { > > + __s16 x; > > + __s16 y; > > + __s16 z; > > + __u16 valid; > > +} __attribute__((packed)); > > This could go in the C file. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/