[RFC PATCH 0/9] lis3 accelerator feature update

[RFC PATCH 0/9] lis3 accelerator feature update

[Samu Onkalo]

This patch set is done to top of 2.6.36-RC5

Changes are tested only with I2C interface using 8bit sensor since I don't
have other possibilities.

I send this as RFC since changes may affect functionalities or use cases
which I can't test or I don't know to exist. 

Description about the changes:

0001:
Add support for pm_runtime framework for lis3 core driver and lis3-i2c
interface code. hp_accel and spi should not be affected by the changes.
When either input device or freefall device is opened, chip is powered up.
When a sysfs entry is accessed, chip is powered up and scheduled power down
is requested to happen within 5 seconds.
I'm not familiar enough how pm_runtime works with spi or acpi interfaces, so
those are not changed. And I don't have any change to test changes to those
areas.

0002:
Regulator frame work support added. Regulators are controlled based on
pm_runtime state transitions. Chip register context is stored at power down
and restored at power up. Regulators are controlled only when I2C interface is
used since the implementation requires pm_runtime support.

0003:
Interrupt control is cleaned up little bit. The need for threaded interrupt
is controlled by the input layer open / close function. In the interrupt,
only one check is needed instead of several one.

Also platform information is copied to lis3_dev structure to shorten access
in interrupt code.

Interrupt handling for the WU blocks doesn't check interrupt source anymore.
It is not necessary since interrupt handler is called only when WU unit causes
an interrupt.

0004:
It is possible to use input interface purely under threshold interrupt control
by setting polling interval to 0. In that case, it may take a long time
when coordinates are updated after the input device open. Applications can see
totally outdated information. Coordinates are now refreshed at the device open.

0005:
We have faced sometimes situation where lis3 powerup fails somehow. BOOT
bit is set at device init to force boot sequence. This way lis3 recovers
from the failed boot.
Also data ready interrupts are enabled at init (reset default value). They
have been removed if the module is unloaded and loaded back.

0006:
Add some missing parameters to 8bit chip platform data.
Chip interrupt generation features can now be controlled better. It
is also possible to configure interrupts at both rising and falling edges.
With some fancy interrupt configuration, this gives fresh coordinates
at the beginning and end of the acceleration event.

0007:
Set lower default fuzziness for 8bit device. Original value was from 12bit
device which is much more sensitive than the 8 bit device.

0008:
Interface driver can provide block read function to reduce number of individual
bus operations. With I2C this reduces number of interrupts about 60-70%
and makes access little bit faster due to reduced amount transactions.

0009:
Selftest is enhanced to test also IRQ lines in 8bit device.
This is done by enabling data_ready interrupt during selftest and
calculating number of received interrupts during the test.

Samu Onkalo (9):
  hwmon: lis3: pm_runtime support
  hwmon: lis3: regulator control
  hwmon: lis3: Cleanup interrupt handling
  hwmon: lis3: Update coordinates at polled device open
  hwmon: lis3: Power on corrections
  hwmon: lis3: New parameters to platform data
  hwmon: lis3: Adjust fuzziness for 8 bit device
  hwmon: lis3: use block read to access data registers
  hwmon: lis3: Enhance lis3 selftest with IRQ line test

 drivers/hwmon/lis3lv02d.c     |  303 ++++++++++++++++++++++++++++++++++-------
 drivers/hwmon/lis3lv02d.h     |   25 ++++
 drivers/hwmon/lis3lv02d_i2c.c |  102 ++++++++++++--
 drivers/hwmon/lis3lv02d_spi.c |    2 +-
 include/linux/lis3lv02d.h     |    7 +-
 5 files changed, 375 insertions(+), 64 deletions(-)

[RFC PATCH 1/9] hwmon: lis3: pm_runtime support

[Samu Onkalo]

Add pm_runtime support to lis3 core driver.
Add pm_runtime support to lis3 i2c driver.

spi and hp_accel drivers are not yet supported. Old always
on functionality remains for those.

For sysfs there is 5 second delay before turning off the
chip to avoid long ramp up delay.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c     |   59 +++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/lis3lv02d.h     |    1 +
 drivers/hwmon/lis3lv02d_i2c.c |   48 +++++++++++++++++++++++++--------
 3 files changed, 96 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index fc591ae..eaa5bf0 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -34,6 +34,7 @@
 #include <linux/freezer.h>
 #include <linux/uaccess.h>
 #include <linux/miscdevice.h>
+#include <linux/pm_runtime.h>
 #include <asm/atomic.h>
 #include "lis3lv02d.h"
 
@@ -43,6 +44,9 @@
 #define MDPS_POLL_INTERVAL 50
 #define MDPS_POLL_MIN	   0
 #define MDPS_POLL_MAX	   2000
+
+#define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
+
 /*
  * The sensor can also generate interrupts (DRDY) but it's pretty pointless
  * because they are generated even if the data do not change. So it's better
@@ -262,6 +266,18 @@ static void lis3lv02d_joystick_poll(struct input_polled_dev *pidev)
 	mutex_unlock(&lis3_dev.mutex);
 }
 
+static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
+{
+	if (lis3_dev.pm_dev)
+		pm_runtime_get_sync(lis3_dev.pm_dev);
+}
+
+static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
+{
+	if (lis3_dev.pm_dev)
+		pm_runtime_put(lis3_dev.pm_dev);
+}
+
 static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
 {
 	if (!test_bit(0, &lis3_dev.misc_opened))
@@ -356,6 +372,9 @@ static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(0, &lis3_dev.misc_opened))
 		return -EBUSY; /* already open */
 
+	if (lis3_dev.pm_dev)
+		pm_runtime_get_sync(lis3_dev.pm_dev);
+
 	atomic_set(&lis3_dev.count, 0);
 	return 0;
 }
@@ -364,6 +383,8 @@ static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
 {
 	fasync_helper(-1, file, 0, &lis3_dev.async_queue);
 	clear_bit(0, &lis3_dev.misc_opened); /* release the device */
+	if (lis3_dev.pm_dev)
+		pm_runtime_put(lis3_dev.pm_dev);
 	return 0;
 }
 
@@ -460,6 +481,8 @@ int lis3lv02d_joystick_enable(void)
 		return -ENOMEM;
 
 	lis3_dev.idev->poll = lis3lv02d_joystick_poll;
+	lis3_dev.idev->open = lis3lv02d_joystick_open;
+	lis3_dev.idev->close = lis3lv02d_joystick_close;
 	lis3_dev.idev->poll_interval = MDPS_POLL_INTERVAL;
 	lis3_dev.idev->poll_interval_min = MDPS_POLL_MIN;
 	lis3_dev.idev->poll_interval_max = MDPS_POLL_MAX;
@@ -512,12 +535,30 @@ void lis3lv02d_joystick_disable(void)
 EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
 
 /* Sysfs stuff */
+static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
+{
+	/*
+	 * SYSFS functions are fast visitors so put-call
+	 * immediately after the get-call. However, keep
+	 * chip running for a while and schedule delayed
+	 * suspend. This way periodic sysfs calls doesn't
+	 * suffer from relatively long power up time.
+	 */
+
+	if (lis3_dev.pm_dev) {
+		pm_runtime_get_sync(lis3->pm_dev);
+		pm_runtime_put_noidle(lis3->pm_dev);
+		pm_schedule_suspend(lis3->pm_dev, SYSFS_POWERDOWN_DELAY);
+	}
+}
+
 static ssize_t lis3lv02d_selftest_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int result;
 	s16 values[3];
 
+	lis3lv02d_sysfs_poweron(&lis3_dev);
 	result = lis3lv02d_selftest(&lis3_dev, values);
 	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
 		values[0], values[1], values[2]);
@@ -528,6 +569,7 @@ static ssize_t lis3lv02d_position_show(struct device *dev,
 {
 	int x, y, z;
 
+	lis3lv02d_sysfs_poweron(&lis3_dev);
 	mutex_lock(&lis3_dev.mutex);
 	lis3lv02d_get_xyz(&lis3_dev, &x, &y, &z);
 	mutex_unlock(&lis3_dev.mutex);
@@ -549,6 +591,7 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
 	if (strict_strtoul(buf, 0, &rate))
 		return -EINVAL;
 
+	lis3lv02d_sysfs_poweron(&lis3_dev);
 	if (lis3lv02d_set_odr(rate))
 		return -EINVAL;
 
@@ -585,6 +628,17 @@ int lis3lv02d_remove_fs(struct lis3lv02d *lis3)
 {
 	sysfs_remove_group(&lis3->pdev->dev.kobj, &lis3lv02d_attribute_group);
 	platform_device_unregister(lis3->pdev);
+	if (lis3_dev.pm_dev) {
+		/* Barrier after the sysfs remove */
+		pm_runtime_barrier(lis3->pm_dev);
+
+		/* SYSFS may have left chip running. Turn off if necessary */
+		if (!pm_runtime_suspended(lis3->pm_dev))
+			lis3lv02d_poweroff(&lis3_dev);
+
+		pm_runtime_disable(lis3->pm_dev);
+		pm_runtime_set_suspended(lis3->pm_dev);
+	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
@@ -685,6 +739,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 	lis3lv02d_add_fs(dev);
 	lis3lv02d_poweron(dev);
 
+	if (lis3_dev.pm_dev) {
+		pm_runtime_set_active(dev->pm_dev);
+		pm_runtime_enable(dev->pm_dev);
+	}
+
 	if (lis3lv02d_joystick_enable())
 		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
 
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 8540913..3e8a208 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -214,6 +214,7 @@ struct axis_conversion {
 
 struct lis3lv02d {
 	void			*bus_priv; /* used by the bus layer only */
+	struct device		*pm_dev; /* for pm_runtime purposes */
 	int (*init) (struct lis3lv02d *lis3);
 	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
 	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index 8e5933b..b9ed1fb 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -29,6 +29,7 @@
 #include <linux/init.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
+#include <linux/pm_runtime.h>
 #include "lis3lv02d.h"
 
 #define DRV_NAME 	"lis3lv02d_i2c"
@@ -95,6 +96,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.write	  = lis3_i2c_write;
 	lis3_dev.irq	  = client->irq;
 	lis3_dev.ac	  = lis3lv02d_axis_map;
+	lis3_dev.pm_dev	  = &client->dev;
 
 	i2c_set_clientdata(client, &lis3_dev);
 	ret = lis3lv02d_init_device(&lis3_dev);
@@ -111,14 +113,14 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
 		pdata->release_resources();
 
 	lis3lv02d_joystick_disable();
-	lis3lv02d_poweroff(lis3);
 
 	return lis3lv02d_remove_fs(&lis3_dev);
 }
 
 #ifdef CONFIG_PM
-static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
+static int lis3lv02d_i2c_suspend(struct device *dev)
 {
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
 	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
 
 	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
@@ -126,18 +128,16 @@ static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
 	return 0;
 }
 
-static int lis3lv02d_i2c_resume(struct i2c_client *client)
+static int lis3lv02d_i2c_resume(struct device *dev)
 {
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
 	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
 
-	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
+	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
+		pm_runtime_suspended(dev))
 		lis3lv02d_poweron(lis3);
-	return 0;
-}
 
-static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
-{
-	lis3lv02d_i2c_suspend(client, PMSG_SUSPEND);
+	return 0;
 }
 #else
 #define lis3lv02d_i2c_suspend	NULL
@@ -145,6 +145,24 @@ static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
 #define lis3lv02d_i2c_shutdown	NULL
 #endif
 
+static int lis3_i2c_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
+
+	lis3lv02d_poweroff(lis3);
+	return 0;
+}
+
+static int lis3_i2c_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
+
+	lis3lv02d_poweron(lis3);
+	return 0;
+}
+
 static const struct i2c_device_id lis3lv02d_id[] = {
 	{"lis3lv02d", 0 },
 	{}
@@ -152,14 +170,20 @@ static const struct i2c_device_id lis3lv02d_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, lis3lv02d_id);
 
+static const struct dev_pm_ops lis3_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(lis3lv02d_i2c_suspend,
+				lis3lv02d_i2c_resume)
+	SET_RUNTIME_PM_OPS(lis3_i2c_runtime_suspend,
+			   lis3_i2c_runtime_resume,
+			   NULL)
+};
+
 static struct i2c_driver lis3lv02d_i2c_driver = {
 	.driver	 = {
 		.name   = DRV_NAME,
 		.owner  = THIS_MODULE,
+		.pm     = &lis3_pm_ops,
 	},
-	.suspend = lis3lv02d_i2c_suspend,
-	.shutdown = lis3lv02d_i2c_shutdown,
-	.resume = lis3lv02d_i2c_resume,
 	.probe	= lis3lv02d_i2c_probe,
 	.remove	= __devexit_p(lis3lv02d_i2c_remove),
 	.id_table = lis3lv02d_id,
-- 
1.6.0.4

Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support

[Jonathan Cameron]

On 10/01/10 12:46, Samu Onkalo wrote:
> Add pm_runtime support to lis3 core driver.
> Add pm_runtime support to lis3 i2c driver.
> 
> spi and hp_accel drivers are not yet supported. Old always
> on functionality remains for those.
> 
> For sysfs there is 5 second delay before turning off the
> chip to avoid long ramp up delay.
I'm not overly familiar with the runtime stuff so looking at this based
on a quick read of their documentation.

Note I'm also not that familiar with the driver :)

One real query about the logic in lis3lv02d_i2c_resume. My reading is
that if we are runtime suspended when coming out of a system suspend
then we don't want to power up the chip?  However I'm not entirely
sure how the two types of power management interact so sorry if I
have completely misunderstood this!

Jonathan
> 
> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/hwmon/lis3lv02d.c     |   59 +++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/lis3lv02d.h     |    1 +
>  drivers/hwmon/lis3lv02d_i2c.c |   48 +++++++++++++++++++++++++--------
>  3 files changed, 96 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index fc591ae..eaa5bf0 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -34,6 +34,7 @@
>  #include <linux/freezer.h>
>  #include <linux/uaccess.h>
>  #include <linux/miscdevice.h>
> +#include <linux/pm_runtime.h>
>  #include <asm/atomic.h>
>  #include "lis3lv02d.h"
>  
> @@ -43,6 +44,9 @@
>  #define MDPS_POLL_INTERVAL 50
>  #define MDPS_POLL_MIN	   0
>  #define MDPS_POLL_MAX	   2000
> +
> +#define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
Name is rather generic...
> +
>  /*
>   * The sensor can also generate interrupts (DRDY) but it's pretty pointless
>   * because they are generated even if the data do not change. So it's better
> @@ -262,6 +266,18 @@ static void lis3lv02d_joystick_poll(struct input_polled_dev *pidev)
>  	mutex_unlock(&lis3_dev.mutex);
>  }
>  
> +static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
> +{
> +	if (lis3_dev.pm_dev)
> +		pm_runtime_get_sync(lis3_dev.pm_dev);
> +}
> +
> +static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
> +{
> +	if (lis3_dev.pm_dev)
> +		pm_runtime_put(lis3_dev.pm_dev);
> +}
> +
>  static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
>  {
>  	if (!test_bit(0, &lis3_dev.misc_opened))
> @@ -356,6 +372,9 @@ static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
>  	if (test_and_set_bit(0, &lis3_dev.misc_opened))
>  		return -EBUSY; /* already open */
>  
> +	if (lis3_dev.pm_dev)
> +		pm_runtime_get_sync(lis3_dev.pm_dev);
> +
>  	atomic_set(&lis3_dev.count, 0);
>  	return 0;
>  }
> @@ -364,6 +383,8 @@ static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
>  {
>  	fasync_helper(-1, file, 0, &lis3_dev.async_queue);
>  	clear_bit(0, &lis3_dev.misc_opened); /* release the device */
> +	if (lis3_dev.pm_dev)
> +		pm_runtime_put(lis3_dev.pm_dev);
>  	return 0;
>  }
>  
> @@ -460,6 +481,8 @@ int lis3lv02d_joystick_enable(void)
>  		return -ENOMEM;
>  
>  	lis3_dev.idev->poll = lis3lv02d_joystick_poll;
> +	lis3_dev.idev->open = lis3lv02d_joystick_open;
> +	lis3_dev.idev->close = lis3lv02d_joystick_close;
>  	lis3_dev.idev->poll_interval = MDPS_POLL_INTERVAL;
>  	lis3_dev.idev->poll_interval_min = MDPS_POLL_MIN;
>  	lis3_dev.idev->poll_interval_max = MDPS_POLL_MAX;
> @@ -512,12 +535,30 @@ void lis3lv02d_joystick_disable(void)
>  EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
>  
>  /* Sysfs stuff */
> +static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
> +{
> +	/*
> +	 * SYSFS functions are fast visitors so put-call
> +	 * immediately after the get-call. However, keep
> +	 * chip running for a while and schedule delayed
> +	 * suspend. This way periodic sysfs calls doesn't
> +	 * suffer from relatively long power up time.
> +	 */
> +
> +	if (lis3_dev.pm_dev) {
> +		pm_runtime_get_sync(lis3->pm_dev);
> +		pm_runtime_put_noidle(lis3->pm_dev);
> +		pm_schedule_suspend(lis3->pm_dev, SYSFS_POWERDOWN_DELAY);
> +	}
> +}
> +
>  static ssize_t lis3lv02d_selftest_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
>  	int result;
>  	s16 values[3];
>  
> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>  	result = lis3lv02d_selftest(&lis3_dev, values);
>  	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
>  		values[0], values[1], values[2]);
> @@ -528,6 +569,7 @@ static ssize_t lis3lv02d_position_show(struct device *dev,
>  {
>  	int x, y, z;
>  
> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>  	mutex_lock(&lis3_dev.mutex);
>  	lis3lv02d_get_xyz(&lis3_dev, &x, &y, &z);
>  	mutex_unlock(&lis3_dev.mutex);
> @@ -549,6 +591,7 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
>  	if (strict_strtoul(buf, 0, &rate))
>  		return -EINVAL;
>  
> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>  	if (lis3lv02d_set_odr(rate))
>  		return -EINVAL;
>  
> @@ -585,6 +628,17 @@ int lis3lv02d_remove_fs(struct lis3lv02d *lis3)
>  {
>  	sysfs_remove_group(&lis3->pdev->dev.kobj, &lis3lv02d_attribute_group);
>  	platform_device_unregister(lis3->pdev);
> +	if (lis3_dev.pm_dev) {
> +		/* Barrier after the sysfs remove */
> +		pm_runtime_barrier(lis3->pm_dev);
> +
> +		/* SYSFS may have left chip running. Turn off if necessary */
> +		if (!pm_runtime_suspended(lis3->pm_dev))
> +			lis3lv02d_poweroff(&lis3_dev);
> +
> +		pm_runtime_disable(lis3->pm_dev);
> +		pm_runtime_set_suspended(lis3->pm_dev);
> +	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
> @@ -685,6 +739,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  	lis3lv02d_add_fs(dev);
>  	lis3lv02d_poweron(dev);
>  
> +	if (lis3_dev.pm_dev) {
> +		pm_runtime_set_active(dev->pm_dev);
> +		pm_runtime_enable(dev->pm_dev);
> +	}
> +
>  	if (lis3lv02d_joystick_enable())
>  		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
>  
> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> index 8540913..3e8a208 100644
> --- a/drivers/hwmon/lis3lv02d.h
> +++ b/drivers/hwmon/lis3lv02d.h
> @@ -214,6 +214,7 @@ struct axis_conversion {
>  
>  struct lis3lv02d {
>  	void			*bus_priv; /* used by the bus layer only */
> +	struct device		*pm_dev; /* for pm_runtime purposes */
>  	int (*init) (struct lis3lv02d *lis3);
>  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
>  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> index 8e5933b..b9ed1fb 100644
> --- a/drivers/hwmon/lis3lv02d_i2c.c
> +++ b/drivers/hwmon/lis3lv02d_i2c.c
> @@ -29,6 +29,7 @@
>  #include <linux/init.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
>  #include "lis3lv02d.h"
>  
>  #define DRV_NAME 	"lis3lv02d_i2c"
> @@ -95,6 +96,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>  	lis3_dev.write	  = lis3_i2c_write;
>  	lis3_dev.irq	  = client->irq;
>  	lis3_dev.ac	  = lis3lv02d_axis_map;
> +	lis3_dev.pm_dev	  = &client->dev;
>  
>  	i2c_set_clientdata(client, &lis3_dev);
>  	ret = lis3lv02d_init_device(&lis3_dev);
> @@ -111,14 +113,14 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
>  		pdata->release_resources();
>  
>  	lis3lv02d_joystick_disable();
> -	lis3lv02d_poweroff(lis3);
>  
>  	return lis3lv02d_remove_fs(&lis3_dev);
>  }
>  
>  #ifdef CONFIG_PM
> -static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
> +static int lis3lv02d_i2c_suspend(struct device *dev)
>  {
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>  
>  	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> @@ -126,18 +128,16 @@ static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
>  	return 0;
>  }
>  
> -static int lis3lv02d_i2c_resume(struct i2c_client *client)
> +static int lis3lv02d_i2c_resume(struct device *dev)
>  {
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>  
> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> +	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
> +		pm_runtime_suspended(dev))
I may be misunderstanding the docs on this, but I would have thought
we only want to power it up if it isn't in suspended state?
>  		lis3lv02d_poweron(lis3);
> -	return 0;
> -}
>  
> -static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
> -{
> -	lis3lv02d_i2c_suspend(client, PMSG_SUSPEND);
> +	return 0;
>  }
>  #else
>  #define lis3lv02d_i2c_suspend	NULL
> @@ -145,6 +145,24 @@ static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
>  #define lis3lv02d_i2c_shutdown	NULL
>  #endif
>  
> +static int lis3_i2c_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> +
> +	lis3lv02d_poweroff(lis3);
> +	return 0;
> +}
> +
> +static int lis3_i2c_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> +
> +	lis3lv02d_poweron(lis3);
> +	return 0;
> +}
> +
>  static const struct i2c_device_id lis3lv02d_id[] = {
>  	{"lis3lv02d", 0 },
>  	{}
> @@ -152,14 +170,20 @@ static const struct i2c_device_id lis3lv02d_id[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, lis3lv02d_id);
>  
> +static const struct dev_pm_ops lis3_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(lis3lv02d_i2c_suspend,
> +				lis3lv02d_i2c_resume)
> +	SET_RUNTIME_PM_OPS(lis3_i2c_runtime_suspend,
> +			   lis3_i2c_runtime_resume,
> +			   NULL)
> +};
> +
>  static struct i2c_driver lis3lv02d_i2c_driver = {
>  	.driver	 = {
>  		.name   = DRV_NAME,
>  		.owner  = THIS_MODULE,
> +		.pm     = &lis3_pm_ops,
>  	},
> -	.suspend = lis3lv02d_i2c_suspend,
> -	.shutdown = lis3lv02d_i2c_shutdown,
> -	.resume = lis3lv02d_i2c_resume,
>  	.probe	= lis3lv02d_i2c_probe,
>  	.remove	= __devexit_p(lis3lv02d_i2c_remove),
>  	.id_table = lis3lv02d_id,

Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support

[Onkalo Samu]

Thanks for comments.

On Sat, 2010-10-02 at 19:14 +0200, ext Jonathan Cameron wrote:
> On 10/01/10 12:46, Samu Onkalo wrote:
> > Add pm_runtime support to lis3 core driver.
> > Add pm_runtime support to lis3 i2c driver.
> > 
> > spi and hp_accel drivers are not yet supported. Old always
> > on functionality remains for those.
> > 
> > For sysfs there is 5 second delay before turning off the
> > chip to avoid long ramp up delay.
> I'm not overly familiar with the runtime stuff so looking at this based
> on a quick read of their documentation.
> 
> Note I'm also not that familiar with the driver :)
> 
> One real query about the logic in lis3lv02d_i2c_resume. My reading is
> that if we are runtime suspended when coming out of a system suspend
> then we don't want to power up the chip?  However I'm not entirely
> sure how the two types of power management interact so sorry if I
> have completely misunderstood this!
> 

It took some time understand how this system works over the system level
suspend. And depending on the use, lis3 chip can be kept active or
powered down over the system standby :)


> Jonathan
> > 
> > Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/hwmon/lis3lv02d.c     |   59 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/hwmon/lis3lv02d.h     |    1 +
> >  drivers/hwmon/lis3lv02d_i2c.c |   48 +++++++++++++++++++++++++--------
> >  3 files changed, 96 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> > index fc591ae..eaa5bf0 100644
> > --- a/drivers/hwmon/lis3lv02d.c
> > +++ b/drivers/hwmon/lis3lv02d.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/freezer.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/miscdevice.h>
> > +#include <linux/pm_runtime.h>
> >  #include <asm/atomic.h>
> >  #include "lis3lv02d.h"
> >  
> > @@ -43,6 +44,9 @@
> >  #define MDPS_POLL_INTERVAL 50
> >  #define MDPS_POLL_MIN	   0
> >  #define MDPS_POLL_MAX	   2000
> > +
> > +#define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
> Name is rather generic...
> > +
> >  /*
> >   * The sensor can also generate interrupts (DRDY) but it's pretty pointless
> >   * because they are generated even if the data do not change. So it's better
> > @@ -262,6 +266,18 @@ static void lis3lv02d_joystick_poll(struct input_polled_dev *pidev)
> >  	mutex_unlock(&lis3_dev.mutex);
> >  }
> >  
> > +static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
> > +{
> > +	if (lis3_dev.pm_dev)
> > +		pm_runtime_get_sync(lis3_dev.pm_dev);
> > +}
> > +
> > +static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
> > +{
> > +	if (lis3_dev.pm_dev)
> > +		pm_runtime_put(lis3_dev.pm_dev);
> > +}
> > +
> >  static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
> >  {
> >  	if (!test_bit(0, &lis3_dev.misc_opened))
> > @@ -356,6 +372,9 @@ static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
> >  	if (test_and_set_bit(0, &lis3_dev.misc_opened))
> >  		return -EBUSY; /* already open */
> >  
> > +	if (lis3_dev.pm_dev)
> > +		pm_runtime_get_sync(lis3_dev.pm_dev);
> > +
> >  	atomic_set(&lis3_dev.count, 0);
> >  	return 0;
> >  }
> > @@ -364,6 +383,8 @@ static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
> >  {
> >  	fasync_helper(-1, file, 0, &lis3_dev.async_queue);
> >  	clear_bit(0, &lis3_dev.misc_opened); /* release the device */
> > +	if (lis3_dev.pm_dev)
> > +		pm_runtime_put(lis3_dev.pm_dev);
> >  	return 0;
> >  }
> >  
> > @@ -460,6 +481,8 @@ int lis3lv02d_joystick_enable(void)
> >  		return -ENOMEM;
> >  
> >  	lis3_dev.idev->poll = lis3lv02d_joystick_poll;
> > +	lis3_dev.idev->open = lis3lv02d_joystick_open;
> > +	lis3_dev.idev->close = lis3lv02d_joystick_close;
> >  	lis3_dev.idev->poll_interval = MDPS_POLL_INTERVAL;
> >  	lis3_dev.idev->poll_interval_min = MDPS_POLL_MIN;
> >  	lis3_dev.idev->poll_interval_max = MDPS_POLL_MAX;
> > @@ -512,12 +535,30 @@ void lis3lv02d_joystick_disable(void)
> >  EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
> >  
> >  /* Sysfs stuff */
> > +static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
> > +{
> > +	/*
> > +	 * SYSFS functions are fast visitors so put-call
> > +	 * immediately after the get-call. However, keep
> > +	 * chip running for a while and schedule delayed
> > +	 * suspend. This way periodic sysfs calls doesn't
> > +	 * suffer from relatively long power up time.
> > +	 */
> > +
> > +	if (lis3_dev.pm_dev) {
> > +		pm_runtime_get_sync(lis3->pm_dev);
> > +		pm_runtime_put_noidle(lis3->pm_dev);
> > +		pm_schedule_suspend(lis3->pm_dev, SYSFS_POWERDOWN_DELAY);
> > +	}
> > +}
> > +
> >  static ssize_t lis3lv02d_selftest_show(struct device *dev,
> >  				struct device_attribute *attr, char *buf)
> >  {
> >  	int result;
> >  	s16 values[3];
> >  
> > +	lis3lv02d_sysfs_poweron(&lis3_dev);
> >  	result = lis3lv02d_selftest(&lis3_dev, values);
> >  	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
> >  		values[0], values[1], values[2]);
> > @@ -528,6 +569,7 @@ static ssize_t lis3lv02d_position_show(struct device *dev,
> >  {
> >  	int x, y, z;
> >  
> > +	lis3lv02d_sysfs_poweron(&lis3_dev);
> >  	mutex_lock(&lis3_dev.mutex);
> >  	lis3lv02d_get_xyz(&lis3_dev, &x, &y, &z);
> >  	mutex_unlock(&lis3_dev.mutex);
> > @@ -549,6 +591,7 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
> >  	if (strict_strtoul(buf, 0, &rate))
> >  		return -EINVAL;
> >  
> > +	lis3lv02d_sysfs_poweron(&lis3_dev);
> >  	if (lis3lv02d_set_odr(rate))
> >  		return -EINVAL;
> >  
> > @@ -585,6 +628,17 @@ int lis3lv02d_remove_fs(struct lis3lv02d *lis3)
> >  {
> >  	sysfs_remove_group(&lis3->pdev->dev.kobj, &lis3lv02d_attribute_group);
> >  	platform_device_unregister(lis3->pdev);
> > +	if (lis3_dev.pm_dev) {
> > +		/* Barrier after the sysfs remove */
> > +		pm_runtime_barrier(lis3->pm_dev);
> > +
> > +		/* SYSFS may have left chip running. Turn off if necessary */
> > +		if (!pm_runtime_suspended(lis3->pm_dev))
> > +			lis3lv02d_poweroff(&lis3_dev);
> > +
> > +		pm_runtime_disable(lis3->pm_dev);
> > +		pm_runtime_set_suspended(lis3->pm_dev);
> > +	}
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
> > @@ -685,6 +739,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
> >  	lis3lv02d_add_fs(dev);
> >  	lis3lv02d_poweron(dev);
> >  
> > +	if (lis3_dev.pm_dev) {
> > +		pm_runtime_set_active(dev->pm_dev);
> > +		pm_runtime_enable(dev->pm_dev);
> > +	}
> > +
> >  	if (lis3lv02d_joystick_enable())
> >  		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
> >  
> > diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> > index 8540913..3e8a208 100644
> > --- a/drivers/hwmon/lis3lv02d.h
> > +++ b/drivers/hwmon/lis3lv02d.h
> > @@ -214,6 +214,7 @@ struct axis_conversion {
> >  
> >  struct lis3lv02d {
> >  	void			*bus_priv; /* used by the bus layer only */
> > +	struct device		*pm_dev; /* for pm_runtime purposes */
> >  	int (*init) (struct lis3lv02d *lis3);
> >  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
> >  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
> > diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> > index 8e5933b..b9ed1fb 100644
> > --- a/drivers/hwmon/lis3lv02d_i2c.c
> > +++ b/drivers/hwmon/lis3lv02d_i2c.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/init.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> > +#include <linux/pm_runtime.h>
> >  #include "lis3lv02d.h"
> >  
> >  #define DRV_NAME 	"lis3lv02d_i2c"
> > @@ -95,6 +96,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
> >  	lis3_dev.write	  = lis3_i2c_write;
> >  	lis3_dev.irq	  = client->irq;
> >  	lis3_dev.ac	  = lis3lv02d_axis_map;
> > +	lis3_dev.pm_dev	  = &client->dev;
> >  
> >  	i2c_set_clientdata(client, &lis3_dev);
> >  	ret = lis3lv02d_init_device(&lis3_dev);
> > @@ -111,14 +113,14 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
> >  		pdata->release_resources();
> >  
> >  	lis3lv02d_joystick_disable();
> > -	lis3lv02d_poweroff(lis3);
> >  
> >  	return lis3lv02d_remove_fs(&lis3_dev);
> >  }
> >  
> >  #ifdef CONFIG_PM
> > -static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
> > +static int lis3lv02d_i2c_suspend(struct device *dev)
> >  {
> > +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> >  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> >  
> >  	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> > @@ -126,18 +128,16 @@ static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
> >  	return 0;
> >  }
> >  
> > -static int lis3lv02d_i2c_resume(struct i2c_client *client)
> > +static int lis3lv02d_i2c_resume(struct device *dev)
> >  {
> > +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> >  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> >  
> > -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
> > +	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
> > +		pm_runtime_suspended(dev))
> I may be misunderstanding the docs on this, but I would have thought
> we only want to power it up if it isn't in suspended state?

That was what I also though, but ...

runtime documentation says:

"During system resume, devices generally should be brought back to full power,
even if they were suspended before the system sleep began.  There are several
reasons for this, including:"

And there was quite good reasons in the doc to do that :)

I2C core code turns suspended child devices on. However, there are no users
and pm_runtime notices that. Pm_runtime calls runtime_suspend-cb soon after the 
system level resume. If the chip is not powered up when that happens, it is possible
that regulators are off and all the accesses towards the chip fails.





> >  		lis3lv02d_poweron(lis3);
> > -	return 0;
> > -}
> >  
> > -static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
> > -{
> > -	lis3lv02d_i2c_suspend(client, PMSG_SUSPEND);
> > +	return 0;
> >  }
> >  #else
> >  #define lis3lv02d_i2c_suspend	NULL
> > @@ -145,6 +145,24 @@ static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
> >  #define lis3lv02d_i2c_shutdown	NULL
> >  #endif
> >  
> > +static int lis3_i2c_runtime_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> > +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> > +
> > +	lis3lv02d_poweroff(lis3);
> > +	return 0;
> > +}
> > +
> > +static int lis3_i2c_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> > +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
> > +
> > +	lis3lv02d_poweron(lis3);
> > +	return 0;
> > +}
> > +
> >  static const struct i2c_device_id lis3lv02d_id[] = {
> >  	{"lis3lv02d", 0 },
> >  	{}
> > @@ -152,14 +170,20 @@ static const struct i2c_device_id lis3lv02d_id[] = {
> >  
> >  MODULE_DEVICE_TABLE(i2c, lis3lv02d_id);
> >  
> > +static const struct dev_pm_ops lis3_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(lis3lv02d_i2c_suspend,
> > +				lis3lv02d_i2c_resume)
> > +	SET_RUNTIME_PM_OPS(lis3_i2c_runtime_suspend,
> > +			   lis3_i2c_runtime_resume,
> > +			   NULL)
> > +};
> > +
> >  static struct i2c_driver lis3lv02d_i2c_driver = {
> >  	.driver	 = {
> >  		.name   = DRV_NAME,
> >  		.owner  = THIS_MODULE,
> > +		.pm     = &lis3_pm_ops,
> >  	},
> > -	.suspend = lis3lv02d_i2c_suspend,
> > -	.shutdown = lis3lv02d_i2c_shutdown,
> > -	.resume = lis3lv02d_i2c_resume,
> >  	.probe	= lis3lv02d_i2c_probe,
> >  	.remove	= __devexit_p(lis3lv02d_i2c_remove),
> >  	.id_table = lis3lv02d_id,
> 

Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support

[Jonathan Cameron]

On 10/03/10 06:03, Onkalo Samu wrote:
> 
> Thanks for comments.
> 
> On Sat, 2010-10-02 at 19:14 +0200, ext Jonathan Cameron wrote:
>> On 10/01/10 12:46, Samu Onkalo wrote:
>>> Add pm_runtime support to lis3 core driver.
>>> Add pm_runtime support to lis3 i2c driver.
>>>
>>> spi and hp_accel drivers are not yet supported. Old always
>>> on functionality remains for those.
>>>
>>> For sysfs there is 5 second delay before turning off the
>>> chip to avoid long ramp up delay.
>> I'm not overly familiar with the runtime stuff so looking at this based
>> on a quick read of their documentation.
>>
>> Note I'm also not that familiar with the driver :)
>>
>> One real query about the logic in lis3lv02d_i2c_resume. My reading is
>> that if we are runtime suspended when coming out of a system suspend
>> then we don't want to power up the chip?  However I'm not entirely
>> sure how the two types of power management interact so sorry if I
>> have completely misunderstood this!
>>
> 
> It took some time understand how this system works over the system level
> suspend. And depending on the use, lis3 chip can be kept active or
> powered down over the system standby :)
The answer below covers my query (and is a handy thing to know about in
general so thanks for that!)  I'll certainly be copying this code for
some of my drivers when I have the time to play with runtime pm.

Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> 
> 
>> Jonathan
>>>
>>> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/hwmon/lis3lv02d.c     |   59 +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/hwmon/lis3lv02d.h     |    1 +
>>>  drivers/hwmon/lis3lv02d_i2c.c |   48 +++++++++++++++++++++++++--------
>>>  3 files changed, 96 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>>> index fc591ae..eaa5bf0 100644
>>> --- a/drivers/hwmon/lis3lv02d.c
>>> +++ b/drivers/hwmon/lis3lv02d.c
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/freezer.h>
>>>  #include <linux/uaccess.h>
>>>  #include <linux/miscdevice.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include <asm/atomic.h>
>>>  #include "lis3lv02d.h"
>>>  
>>> @@ -43,6 +44,9 @@
>>>  #define MDPS_POLL_INTERVAL 50
>>>  #define MDPS_POLL_MIN	   0
>>>  #define MDPS_POLL_MAX	   2000
>>> +
>>> +#define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
>> Name is rather generic...
>>> +
>>>  /*
>>>   * The sensor can also generate interrupts (DRDY) but it's pretty pointless
>>>   * because they are generated even if the data do not change. So it's better
>>> @@ -262,6 +266,18 @@ static void lis3lv02d_joystick_poll(struct input_polled_dev *pidev)
>>>  	mutex_unlock(&lis3_dev.mutex);
>>>  }
>>>  
>>> +static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
>>> +{
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_get_sync(lis3_dev.pm_dev);
>>> +}
>>> +
>>> +static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
>>> +{
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_put(lis3_dev.pm_dev);
>>> +}
>>> +
>>>  static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
>>>  {
>>>  	if (!test_bit(0, &lis3_dev.misc_opened))
>>> @@ -356,6 +372,9 @@ static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
>>>  	if (test_and_set_bit(0, &lis3_dev.misc_opened))
>>>  		return -EBUSY; /* already open */
>>>  
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_get_sync(lis3_dev.pm_dev);
>>> +
>>>  	atomic_set(&lis3_dev.count, 0);
>>>  	return 0;
>>>  }
>>> @@ -364,6 +383,8 @@ static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
>>>  {
>>>  	fasync_helper(-1, file, 0, &lis3_dev.async_queue);
>>>  	clear_bit(0, &lis3_dev.misc_opened); /* release the device */
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_put(lis3_dev.pm_dev);
>>>  	return 0;
>>>  }
>>>  
>>> @@ -460,6 +481,8 @@ int lis3lv02d_joystick_enable(void)
>>>  		return -ENOMEM;
>>>  
>>>  	lis3_dev.idev->poll = lis3lv02d_joystick_poll;
>>> +	lis3_dev.idev->open = lis3lv02d_joystick_open;
>>> +	lis3_dev.idev->close = lis3lv02d_joystick_close;
>>>  	lis3_dev.idev->poll_interval = MDPS_POLL_INTERVAL;
>>>  	lis3_dev.idev->poll_interval_min = MDPS_POLL_MIN;
>>>  	lis3_dev.idev->poll_interval_max = MDPS_POLL_MAX;
>>> @@ -512,12 +535,30 @@ void lis3lv02d_joystick_disable(void)
>>>  EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
>>>  
>>>  /* Sysfs stuff */
>>> +static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
>>> +{
>>> +	/*
>>> +	 * SYSFS functions are fast visitors so put-call
>>> +	 * immediately after the get-call. However, keep
>>> +	 * chip running for a while and schedule delayed
>>> +	 * suspend. This way periodic sysfs calls doesn't
>>> +	 * suffer from relatively long power up time.
>>> +	 */
>>> +
>>> +	if (lis3_dev.pm_dev) {
>>> +		pm_runtime_get_sync(lis3->pm_dev);
>>> +		pm_runtime_put_noidle(lis3->pm_dev);
>>> +		pm_schedule_suspend(lis3->pm_dev, SYSFS_POWERDOWN_DELAY);
>>> +	}
>>> +}
>>> +
>>>  static ssize_t lis3lv02d_selftest_show(struct device *dev,
>>>  				struct device_attribute *attr, char *buf)
>>>  {
>>>  	int result;
>>>  	s16 values[3];
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	result = lis3lv02d_selftest(&lis3_dev, values);
>>>  	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
>>>  		values[0], values[1], values[2]);
>>> @@ -528,6 +569,7 @@ static ssize_t lis3lv02d_position_show(struct device *dev,
>>>  {
>>>  	int x, y, z;
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	mutex_lock(&lis3_dev.mutex);
>>>  	lis3lv02d_get_xyz(&lis3_dev, &x, &y, &z);
>>>  	mutex_unlock(&lis3_dev.mutex);
>>> @@ -549,6 +591,7 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
>>>  	if (strict_strtoul(buf, 0, &rate))
>>>  		return -EINVAL;
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	if (lis3lv02d_set_odr(rate))
>>>  		return -EINVAL;
>>>  
>>> @@ -585,6 +628,17 @@ int lis3lv02d_remove_fs(struct lis3lv02d *lis3)
>>>  {
>>>  	sysfs_remove_group(&lis3->pdev->dev.kobj, &lis3lv02d_attribute_group);
>>>  	platform_device_unregister(lis3->pdev);
>>> +	if (lis3_dev.pm_dev) {
>>> +		/* Barrier after the sysfs remove */
>>> +		pm_runtime_barrier(lis3->pm_dev);
>>> +
>>> +		/* SYSFS may have left chip running. Turn off if necessary */
>>> +		if (!pm_runtime_suspended(lis3->pm_dev))
>>> +			lis3lv02d_poweroff(&lis3_dev);
>>> +
>>> +		pm_runtime_disable(lis3->pm_dev);
>>> +		pm_runtime_set_suspended(lis3->pm_dev);
>>> +	}
>>>  	return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
>>> @@ -685,6 +739,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>>>  	lis3lv02d_add_fs(dev);
>>>  	lis3lv02d_poweron(dev);
>>>  
>>> +	if (lis3_dev.pm_dev) {
>>> +		pm_runtime_set_active(dev->pm_dev);
>>> +		pm_runtime_enable(dev->pm_dev);
>>> +	}
>>> +
>>>  	if (lis3lv02d_joystick_enable())
>>>  		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
>>>  
>>> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
>>> index 8540913..3e8a208 100644
>>> --- a/drivers/hwmon/lis3lv02d.h
>>> +++ b/drivers/hwmon/lis3lv02d.h
>>> @@ -214,6 +214,7 @@ struct axis_conversion {
>>>  
>>>  struct lis3lv02d {
>>>  	void			*bus_priv; /* used by the bus layer only */
>>> +	struct device		*pm_dev; /* for pm_runtime purposes */
>>>  	int (*init) (struct lis3lv02d *lis3);
>>>  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
>>>  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
>>> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
>>> index 8e5933b..b9ed1fb 100644
>>> --- a/drivers/hwmon/lis3lv02d_i2c.c
>>> +++ b/drivers/hwmon/lis3lv02d_i2c.c
>>> @@ -29,6 +29,7 @@
>>>  #include <linux/init.h>
>>>  #include <linux/err.h>
>>>  #include <linux/i2c.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include "lis3lv02d.h"
>>>  
>>>  #define DRV_NAME 	"lis3lv02d_i2c"
>>> @@ -95,6 +96,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>>>  	lis3_dev.write	  = lis3_i2c_write;
>>>  	lis3_dev.irq	  = client->irq;
>>>  	lis3_dev.ac	  = lis3lv02d_axis_map;
>>> +	lis3_dev.pm_dev	  = &client->dev;
>>>  
>>>  	i2c_set_clientdata(client, &lis3_dev);
>>>  	ret = lis3lv02d_init_device(&lis3_dev);
>>> @@ -111,14 +113,14 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
>>>  		pdata->release_resources();
>>>  
>>>  	lis3lv02d_joystick_disable();
>>> -	lis3lv02d_poweroff(lis3);
>>>  
>>>  	return lis3lv02d_remove_fs(&lis3_dev);
>>>  }
>>>  
>>>  #ifdef CONFIG_PM
>>> -static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
>>> +static int lis3lv02d_i2c_suspend(struct device *dev)
>>>  {
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>>  
>>>  	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
>>> @@ -126,18 +128,16 @@ static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
>>>  	return 0;
>>>  }
>>>  
>>> -static int lis3lv02d_i2c_resume(struct i2c_client *client)
>>> +static int lis3lv02d_i2c_resume(struct device *dev)
>>>  {
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>>  
>>> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
>>> +	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
>>> +		pm_runtime_suspended(dev))
>> I may be misunderstanding the docs on this, but I would have thought
>> we only want to power it up if it isn't in suspended state?
> 
> That was what I also though, but ...
> 
> runtime documentation says:
> 
> "During system resume, devices generally should be brought back to full power,
> even if they were suspended before the system sleep began.  There are several
> reasons for this, including:"
> 
> And there was quite good reasons in the doc to do that :)
> 
> I2C core code turns suspended child devices on. However, there are no users
> and pm_runtime notices that. Pm_runtime calls runtime_suspend-cb soon after the 
> system level resume. If the chip is not powered up when that happens, it is possible
> that regulators are off and all the accesses towards the chip fails.
I guess that makes sense in a kind of roundabout way!
> 
> 
> 
> 
> 
>>>  		lis3lv02d_poweron(lis3);
>>> -	return 0;
>>> -}
>>>  
>>> -static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
>>> -{
>>> -	lis3lv02d_i2c_suspend(client, PMSG_SUSPEND);
>>> +	return 0;
>>>  }
>>>  #else
>>>  #define lis3lv02d_i2c_suspend	NULL
>>> @@ -145,6 +145,24 @@ static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
>>>  #define lis3lv02d_i2c_shutdown	NULL
>>>  #endif
>>>  
>>> +static int lis3_i2c_runtime_suspend(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>> +
>>> +	lis3lv02d_poweroff(lis3);
>>> +	return 0;
>>> +}
>>> +
>>> +static int lis3_i2c_runtime_resume(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>> +
>>> +	lis3lv02d_poweron(lis3);
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct i2c_device_id lis3lv02d_id[] = {
>>>  	{"lis3lv02d", 0 },
>>>  	{}
>>> @@ -152,14 +170,20 @@ static const struct i2c_device_id lis3lv02d_id[] = {
>>>  
>>>  MODULE_DEVICE_TABLE(i2c, lis3lv02d_id);
>>>  
>>> +static const struct dev_pm_ops lis3_pm_ops = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(lis3lv02d_i2c_suspend,
>>> +				lis3lv02d_i2c_resume)
>>> +	SET_RUNTIME_PM_OPS(lis3_i2c_runtime_suspend,
>>> +			   lis3_i2c_runtime_resume,
>>> +			   NULL)
>>> +};
>>> +
>>>  static struct i2c_driver lis3lv02d_i2c_driver = {
>>>  	.driver	 = {
>>>  		.name   = DRV_NAME,
>>>  		.owner  = THIS_MODULE,
>>> +		.pm     = &lis3_pm_ops,
>>>  	},
>>> -	.suspend = lis3lv02d_i2c_suspend,
>>> -	.shutdown = lis3lv02d_i2c_shutdown,
>>> -	.resume = lis3lv02d_i2c_resume,
>>>  	.probe	= lis3lv02d_i2c_probe,
>>>  	.remove	= __devexit_p(lis3lv02d_i2c_remove),
>>>  	.id_table = lis3lv02d_id,
>>
> 
> 
> 

[RFC PATCH 2/9] hwmon: lis3: regulator control

[Samu Onkalo]

Based on pm_runtime control, turn lis3 regulators on and off.
Perform context save and restore on transitions.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c     |   48 +++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/lis3lv02d.h     |   19 ++++++++++++++++
 drivers/hwmon/lis3lv02d_i2c.c |   43 +++++++++++++++++++++++++++++++++++-
 3 files changed, 109 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index eaa5bf0..23d47ad 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -223,10 +223,46 @@ fail:
 	return ret;
 }
 
+/*
+ * Order of registers in the list affects to order of the restore process.
+ * Perhaps it is a good idea to set interrupt enable register as a last one
+ * after all other configurations
+ */
+static u8 lis3_wai8_regs[] = { FF_WU_CFG_1, FF_WU_THS_1, FF_WU_DURATION_1,
+			       FF_WU_CFG_2, FF_WU_THS_2, FF_WU_DURATION_2,
+			       CLICK_CFG, CLICK_SRC, CLICK_THSY_X, CLICK_THSZ,
+			       CLICK_TIMELIMIT, CLICK_LATENCY, CLICK_WINDOW,
+			       CTRL_REG1, CTRL_REG2, CTRL_REG3};
+
+static u8 lis3_wai12_regs[] = {FF_WU_CFG, FF_WU_THS_L, FF_WU_THS_H,
+			       FF_WU_DURATION, DD_CFG, DD_THSI_L, DD_THSI_H,
+			       DD_THSE_L, DD_THSE_H,
+			       CTRL_REG1, CTRL_REG3, CTRL_REG2};
+
+static inline void lis3_context_save(struct lis3lv02d *lis3)
+{
+	int i;
+	for (i = 0; i < lis3->regs_size; i++)
+		lis3->read(lis3, lis3->regs[i], &lis3->reg_cache[i]);
+	lis3->regs_stored = true;
+}
+
+static inline void lis3_context_restore(struct lis3lv02d *lis3)
+{
+	int i;
+	if (lis3->regs_stored)
+		for (i = 0; i < lis3->regs_size; i++)
+			lis3->write(lis3, lis3->regs[i], lis3->reg_cache[i]);
+}
+
 void lis3lv02d_poweroff(struct lis3lv02d *lis3)
 {
+	if (lis3->reg_ctrl)
+		lis3_context_save(lis3);
 	/* disable X,Y,Z axis and power down */
 	lis3->write(lis3, CTRL_REG1, 0x00);
+	if (lis3->reg_ctrl)
+		lis3->reg_ctrl(lis3, LIS3_REG_OFF);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
 
@@ -249,6 +285,8 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
 		reg |= CTRL2_BDU;
 		lis3->write(lis3, CTRL_REG2, reg);
 	}
+	if (lis3->reg_ctrl)
+		lis3_context_restore(lis3);
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
 
@@ -718,6 +756,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 		dev->odrs = lis3_12_rates;
 		dev->odr_mask = CTRL1_DF0 | CTRL1_DF1;
 		dev->scale = LIS3_SENSITIVITY_12B;
+		dev->regs = lis3_wai12_regs;
+		dev->regs_size = ARRAY_SIZE(lis3_wai12_regs);
 		break;
 	case WAI_8B:
 		printk(KERN_INFO DRIVER_NAME ": 8 bits sensor found\n");
@@ -727,6 +767,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 		dev->odrs = lis3_8_rates;
 		dev->odr_mask = CTRL1_DR;
 		dev->scale = LIS3_SENSITIVITY_8B;
+		dev->regs = lis3_wai8_regs;
+		dev->regs_size = ARRAY_SIZE(lis3_wai8_regs);
 		break;
 	default:
 		printk(KERN_ERR DRIVER_NAME
@@ -734,6 +776,12 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 		return -EINVAL;
 	}
 
+	if (dev->regs_size > LIS3_NUM_MAX_REG) {
+		printk(KERN_ERR DRIVER_NAME
+			": register cache area is too small");
+		return -EINVAL;
+	}
+
 	mutex_init(&dev->mutex);
 
 	lis3lv02d_add_fs(dev);
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 3e8a208..caf3ed1 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -20,6 +20,7 @@
  */
 #include <linux/platform_device.h>
 #include <linux/input-polldev.h>
+#include <linux/regulator/consumer.h>
 
 /*
  * This driver tries to support the "digital" accelerometer chips from
@@ -97,6 +98,15 @@ enum lis3_who_am_i {
 	WAI_8B		= 0x3B, /* 8 bits: LIS[23]02D[LQ]... */
 	WAI_6B		= 0x52, /* 6 bits: LIS331DLF - not supported */
 };
+/* Number of RW registers in each device for register caching purposes */
+#define NUM_RW_REGS_12B 21
+#define NUM_RW_REGS_8B  15
+
+#if NUM_RW_REGS_8B > NUM_RW_REGS_12B
+#define LIS3_NUM_MAX_REG NUM_RW_REGS_8B
+#else
+#define LIS3_NUM_MAX_REG NUM_RW_REGS_12B
+#endif
 
 enum lis3lv02d_ctrl1_12b {
 	CTRL1_Xen	= 0x01,
@@ -206,6 +216,9 @@ enum lis3lv02d_click_src_8b {
 	CLICK_IA	= 0x40,
 };
 
+#define LIS3_REG_OFF	0x00
+#define LIS3_REG_ON	0x01
+
 struct axis_conversion {
 	s8	x;
 	s8	y;
@@ -218,8 +231,13 @@ struct lis3lv02d {
 	int (*init) (struct lis3lv02d *lis3);
 	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
 	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
+	int (*reg_ctrl) (struct lis3lv02d *lis3, bool state);
 
 	int                     *odrs;     /* Supported output data rates */
+	u8			*regs;	   /* Regs to store / restore */
+	int			regs_size;
+	bool			regs_stored;
+	u8                      reg_cache[LIS3_NUM_MAX_REG];
 	u8                      odr_mask;  /* ODR bit mask */
 	u8			whoami;    /* indicates measurement precision */
 	s16 (*read_data) (struct lis3lv02d *lis3, int reg);
@@ -232,6 +250,7 @@ struct lis3lv02d {
 
 	struct input_polled_dev	*idev;     /* input device */
 	struct platform_device	*pdev;     /* platform device */
+	struct regulator_bulk_data regulators[2];
 	atomic_t		count;     /* interrupt count after last read */
 	struct axis_conversion	ac;        /* hw -> logical axis */
 	int			mapped_btns[3];
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index b9ed1fb..0852bed 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -30,10 +30,29 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/pm_runtime.h>
+#include <linux/delay.h>
 #include "lis3lv02d.h"
 
 #define DRV_NAME 	"lis3lv02d_i2c"
 
+static const char reg_vdd[]    = "Vdd";
+static const char reg_vdd_io[] = "Vdd_IO";
+
+static int lis3_reg_ctrl(struct lis3lv02d *lis3, bool state)
+{
+	int ret;
+	if (state == LIS3_REG_OFF) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(lis3->regulators),
+					lis3->regulators);
+	} else {
+		ret = regulator_bulk_enable(ARRAY_SIZE(lis3->regulators),
+					lis3->regulators);
+		/* Chip needs time to wakeup. Not mentioned in datasheet */
+		usleep_range(5000, 10000);
+	}
+	return ret;
+}
+
 static inline s32 lis3_i2c_write(struct lis3lv02d *lis3, int reg, u8 value)
 {
 	struct i2c_client *c = lis3->bus_priv;
@@ -52,6 +71,12 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
 	u8 reg;
 	int ret;
 
+	lis3_reg_ctrl(lis3, LIS3_REG_ON);
+
+	lis3->read(lis3, WHO_AM_I, &reg);
+	if (lis3->whoami != 0 && reg != lis3->whoami)
+		printk(KERN_ERR "lis3: power on failure\n");
+
 	/* power up the device */
 	ret = lis3->read(lis3, CTRL_REG1, &reg);
 	if (ret < 0)
@@ -89,16 +114,29 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 			goto fail;
 	}
 
+	lis3_dev.regulators[0].supply = reg_vdd;
+	lis3_dev.regulators[1].supply = reg_vdd_io;
+	ret = regulator_bulk_get(&client->dev, ARRAY_SIZE(lis3_dev.regulators),
+				lis3_dev.regulators);
+	if (ret < 0)
+		goto fail;
+
 	lis3_dev.pdata	  = pdata;
 	lis3_dev.bus_priv = client;
 	lis3_dev.init	  = lis3_i2c_init;
 	lis3_dev.read	  = lis3_i2c_read;
 	lis3_dev.write	  = lis3_i2c_write;
+	lis3_dev.reg_ctrl = lis3_reg_ctrl;
 	lis3_dev.irq	  = client->irq;
 	lis3_dev.ac	  = lis3lv02d_axis_map;
 	lis3_dev.pm_dev	  = &client->dev;
 
 	i2c_set_clientdata(client, &lis3_dev);
+
+	/* Provide power over the init call */
+	lis3_reg_ctrl(&lis3_dev, LIS3_REG_ON);
+	lis3_reg_ctrl(&lis3_dev, LIS3_REG_OFF);
+
 	ret = lis3lv02d_init_device(&lis3_dev);
 fail:
 	return ret;
@@ -113,8 +151,11 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
 		pdata->release_resources();
 
 	lis3lv02d_joystick_disable();
+	lis3lv02d_remove_fs(lis3);
 
-	return lis3lv02d_remove_fs(&lis3_dev);
+	regulator_bulk_free(ARRAY_SIZE(lis3->regulators),
+			lis3_dev.regulators);
+	return 0;
 }
 
 #ifdef CONFIG_PM
-- 
1.6.0.4

Re: [lm-sensors] [RFC PATCH 2/9] hwmon: lis3: regulator control

[Jonathan Cameron]

On 10/01/10 12:46, Samu Onkalo wrote:
> Based on pm_runtime control, turn lis3 regulators on and off.
> Perform context save and restore on transitions.

As this is a simple save and restore state patch I'm happy to comment on it.

Mostly fine, though I have a couple of minor questions.
> 
> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/hwmon/lis3lv02d.c     |   48 +++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/lis3lv02d.h     |   19 ++++++++++++++++
>  drivers/hwmon/lis3lv02d_i2c.c |   43 +++++++++++++++++++++++++++++++++++-
>  3 files changed, 109 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index eaa5bf0..23d47ad 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -223,10 +223,46 @@ fail:
>  	return ret;
>  }
>  
> +/*
> + * Order of registers in the list affects to order of the restore process.
> + * Perhaps it is a good idea to set interrupt enable register as a last one
> + * after all other configurations
> + */
> +static u8 lis3_wai8_regs[] = { FF_WU_CFG_1, FF_WU_THS_1, FF_WU_DURATION_1,
> +			       FF_WU_CFG_2, FF_WU_THS_2, FF_WU_DURATION_2,
> +			       CLICK_CFG, CLICK_SRC, CLICK_THSY_X, CLICK_THSZ,
> +			       CLICK_TIMELIMIT, CLICK_LATENCY, CLICK_WINDOW,
> +			       CTRL_REG1, CTRL_REG2, CTRL_REG3};
> +
> +static u8 lis3_wai12_regs[] = {FF_WU_CFG, FF_WU_THS_L, FF_WU_THS_H,
> +			       FF_WU_DURATION, DD_CFG, DD_THSI_L, DD_THSI_H,
> +			       DD_THSE_L, DD_THSE_H,
> +			       CTRL_REG1, CTRL_REG3, CTRL_REG2};
> +
> +static inline void lis3_context_save(struct lis3lv02d *lis3)
> +{
> +	int i;
> +	for (i = 0; i < lis3->regs_size; i++)
> +		lis3->read(lis3, lis3->regs[i], &lis3->reg_cache[i]);
> +	lis3->regs_stored = true;
> +}
> +
> +static inline void lis3_context_restore(struct lis3lv02d *lis3)
> +{
> +	int i;
> +	if (lis3->regs_stored)
> +		for (i = 0; i < lis3->regs_size; i++)
> +			lis3->write(lis3, lis3->regs[i], lis3->reg_cache[i]);
> +}
> +
>  void lis3lv02d_poweroff(struct lis3lv02d *lis3)
>  {
> +	if (lis3->reg_ctrl)
> +		lis3_context_save(lis3);
>  	/* disable X,Y,Z axis and power down */
>  	lis3->write(lis3, CTRL_REG1, 0x00);
> +	if (lis3->reg_ctrl)
> +		lis3->reg_ctrl(lis3, LIS3_REG_OFF);
>  }
>  EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
>  
> @@ -249,6 +285,8 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
>  		reg |= CTRL2_BDU;
>  		lis3->write(lis3, CTRL_REG2, reg);
>  	}
> +	if (lis3->reg_ctrl)
> +		lis3_context_restore(lis3);
>  }
>  EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
>  
> @@ -718,6 +756,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  		dev->odrs = lis3_12_rates;
>  		dev->odr_mask = CTRL1_DF0 | CTRL1_DF1;
>  		dev->scale = LIS3_SENSITIVITY_12B;
> +		dev->regs = lis3_wai12_regs;
> +		dev->regs_size = ARRAY_SIZE(lis3_wai12_regs);
>  		break;
>  	case WAI_8B:
>  		printk(KERN_INFO DRIVER_NAME ": 8 bits sensor found\n");
> @@ -727,6 +767,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  		dev->odrs = lis3_8_rates;
>  		dev->odr_mask = CTRL1_DR;
>  		dev->scale = LIS3_SENSITIVITY_8B;
> +		dev->regs = lis3_wai8_regs;
> +		dev->regs_size = ARRAY_SIZE(lis3_wai8_regs);
>  		break;
>  	default:
>  		printk(KERN_ERR DRIVER_NAME
> @@ -734,6 +776,12 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  		return -EINVAL;
>  	}
>  
This is a little odd as runtime checks go.  Surely it can only occur in event
of a clear driver bug?  Personally I'd just go with dynamically allocating
the reg_cache to be the right size...
> +	if (dev->regs_size > LIS3_NUM_MAX_REG) {
> +		printk(KERN_ERR DRIVER_NAME
> +			": register cache area is too small");
> +		return -EINVAL;
> +	}
> +
>  	mutex_init(&dev->mutex);
>  
>  	lis3lv02d_add_fs(dev);
> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> index 3e8a208..caf3ed1 100644
> --- a/drivers/hwmon/lis3lv02d.h
> +++ b/drivers/hwmon/lis3lv02d.h
> @@ -20,6 +20,7 @@
>   */
>  #include <linux/platform_device.h>
>  #include <linux/input-polldev.h>
> +#include <linux/regulator/consumer.h>
>  
>  /*
>   * This driver tries to support the "digital" accelerometer chips from
> @@ -97,6 +98,15 @@ enum lis3_who_am_i {
>  	WAI_8B		= 0x3B, /* 8 bits: LIS[23]02D[LQ]... */
>  	WAI_6B		= 0x52, /* 6 bits: LIS331DLF - not supported */
>  };
> +/* Number of RW registers in each device for register caching purposes */
You could just enforce this through review, but I guess it doesn't hurt
to have sanity checks... 

> +#define NUM_RW_REGS_12B 21
> +#define NUM_RW_REGS_8B  15
> +
> +#if NUM_RW_REGS_8B > NUM_RW_REGS_12B
> +#define LIS3_NUM_MAX_REG NUM_RW_REGS_8B
> +#else
> +#define LIS3_NUM_MAX_REG NUM_RW_REGS_12B
> +#endif
>  
>  enum lis3lv02d_ctrl1_12b {
>  	CTRL1_Xen	= 0x01,
> @@ -206,6 +216,9 @@ enum lis3lv02d_click_src_8b {
>  	CLICK_IA	= 0x40,
>  };
>  
> +#define LIS3_REG_OFF	0x00
> +#define LIS3_REG_ON	0x01
I think the rest of these are done as enums.  Worth doing that here for
consistency?
> +
>  struct axis_conversion {
>  	s8	x;
>  	s8	y;
> @@ -218,8 +231,13 @@ struct lis3lv02d {
>  	int (*init) (struct lis3lv02d *lis3);
>  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
>  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
> +	int (*reg_ctrl) (struct lis3lv02d *lis3, bool state);
>  
>  	int                     *odrs;     /* Supported output data rates */
> +	u8			*regs;	   /* Regs to store / restore */
> +	int			regs_size;
> +	bool			regs_stored;
> +	u8                      reg_cache[LIS3_NUM_MAX_REG];
>  	u8                      odr_mask;  /* ODR bit mask */
>  	u8			whoami;    /* indicates measurement precision */
>  	s16 (*read_data) (struct lis3lv02d *lis3, int reg);
> @@ -232,6 +250,7 @@ struct lis3lv02d {
>  
>  	struct input_polled_dev	*idev;     /* input device */
>  	struct platform_device	*pdev;     /* platform device */
> +	struct regulator_bulk_data regulators[2];
>  	atomic_t		count;     /* interrupt count after last read */
>  	struct axis_conversion	ac;        /* hw -> logical axis */
>  	int			mapped_btns[3];
> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> index b9ed1fb..0852bed 100644
> --- a/drivers/hwmon/lis3lv02d_i2c.c
> +++ b/drivers/hwmon/lis3lv02d_i2c.c
> @@ -30,10 +30,29 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/delay.h>
>  #include "lis3lv02d.h"
>  
>  #define DRV_NAME 	"lis3lv02d_i2c"
>  
> +static const char reg_vdd[]    = "Vdd";
> +static const char reg_vdd_io[] = "Vdd_IO";
> +
> +static int lis3_reg_ctrl(struct lis3lv02d *lis3, bool state)
> +{
> +	int ret;
> +	if (state == LIS3_REG_OFF) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(lis3->regulators),
> +					lis3->regulators);
> +	} else {
> +		ret = regulator_bulk_enable(ARRAY_SIZE(lis3->regulators),
> +					lis3->regulators);
> +		/* Chip needs time to wakeup. Not mentioned in datasheet */
> +		usleep_range(5000, 10000);
> +	}
> +	return ret;
> +}
> +
>  static inline s32 lis3_i2c_write(struct lis3lv02d *lis3, int reg, u8 value)
>  {
>  	struct i2c_client *c = lis3->bus_priv;
> @@ -52,6 +71,12 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
>  	u8 reg;
>  	int ret;
>  
> +	lis3_reg_ctrl(lis3, LIS3_REG_ON);
> +
> +	lis3->read(lis3, WHO_AM_I, &reg);
> +	if (lis3->whoami != 0 && reg != lis3->whoami)
What is the purpose of the first test? How can we get here without that being set?
> +		printk(KERN_ERR "lis3: power on failure\n");
> +
>  	/* power up the device */
>  	ret = lis3->read(lis3, CTRL_REG1, &reg);
>  	if (ret < 0)
> @@ -89,16 +114,29 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>  			goto fail;
>  	}
>  
> +	lis3_dev.regulators[0].supply = reg_vdd;
> +	lis3_dev.regulators[1].supply = reg_vdd_io;
> +	ret = regulator_bulk_get(&client->dev, ARRAY_SIZE(lis3_dev.regulators),
> +				lis3_dev.regulators);
> +	if (ret < 0)
> +		goto fail;
> +
>  	lis3_dev.pdata	  = pdata;
>  	lis3_dev.bus_priv = client;
>  	lis3_dev.init	  = lis3_i2c_init;
>  	lis3_dev.read	  = lis3_i2c_read;
>  	lis3_dev.write	  = lis3_i2c_write;
> +	lis3_dev.reg_ctrl = lis3_reg_ctrl;
>  	lis3_dev.irq	  = client->irq;
>  	lis3_dev.ac	  = lis3lv02d_axis_map;
>  	lis3_dev.pm_dev	  = &client->dev;
>  
>  	i2c_set_clientdata(client, &lis3_dev);
> +
> +	/* Provide power over the init call */
> +	lis3_reg_ctrl(&lis3_dev, LIS3_REG_ON);
> +	lis3_reg_ctrl(&lis3_dev, LIS3_REG_OFF);
> +
>  	ret = lis3lv02d_init_device(&lis3_dev);
>  fail:
>  	return ret;
> @@ -113,8 +151,11 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
>  		pdata->release_resources();
>  
>  	lis3lv02d_joystick_disable();
> +	lis3lv02d_remove_fs(lis3);
subtle change here... Out of intererst, why did the top level lis3_dev
structure ever exist?  (you can tell I haven't looked closely at this driver
before!) Can remove_fs return an error? 
>  
> -	return lis3lv02d_remove_fs(&lis3_dev);
> +	regulator_bulk_free(ARRAY_SIZE(lis3->regulators),
> +			lis3_dev.regulators);
> +	return 0;
>  }
>  
>  #ifdef CONFIG_PM

Re: [lm-sensors] [RFC PATCH 2/9] hwmon: lis3: regulator control

[Onkalo Samu]

On Sat, 2010-10-02 at 19:33 +0200, ext Jonathan Cameron wrote:
> On 10/01/10 12:46, Samu Onkalo wrote:
> > Based on pm_runtime control, turn lis3 regulators on and off.
> > Perform context save and restore on transitions.
> 
> As this is a simple save and restore state patch I'm happy to comment on it.
> 
> Mostly fine, though I have a couple of minor questions.
> > 
> > Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/hwmon/lis3lv02d.c     |   48 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/hwmon/lis3lv02d.h     |   19 ++++++++++++++++
> >  drivers/hwmon/lis3lv02d_i2c.c |   43 +++++++++++++++++++++++++++++++++++-
> >  3 files changed, 109 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> > index eaa5bf0..23d47ad 100644
> > --- a/drivers/hwmon/lis3lv02d.c
> > +++ b/drivers/hwmon/lis3lv02d.c
> > @@ -223,10 +223,46 @@ fail:
> >  	return ret;
> >  }
> >  
> > +/*
> > + * Order of registers in the list affects to order of the restore process.
> > + * Perhaps it is a good idea to set interrupt enable register as a last one
> > + * after all other configurations
> > + */
> > +static u8 lis3_wai8_regs[] = { FF_WU_CFG_1, FF_WU_THS_1, FF_WU_DURATION_1,
> > +			       FF_WU_CFG_2, FF_WU_THS_2, FF_WU_DURATION_2,
> > +			       CLICK_CFG, CLICK_SRC, CLICK_THSY_X, CLICK_THSZ,
> > +			       CLICK_TIMELIMIT, CLICK_LATENCY, CLICK_WINDOW,
> > +			       CTRL_REG1, CTRL_REG2, CTRL_REG3};
> > +
> > +static u8 lis3_wai12_regs[] = {FF_WU_CFG, FF_WU_THS_L, FF_WU_THS_H,
> > +			       FF_WU_DURATION, DD_CFG, DD_THSI_L, DD_THSI_H,
> > +			       DD_THSE_L, DD_THSE_H,
> > +			       CTRL_REG1, CTRL_REG3, CTRL_REG2};
> > +
> > +static inline void lis3_context_save(struct lis3lv02d *lis3)
> > +{
> > +	int i;
> > +	for (i = 0; i < lis3->regs_size; i++)
> > +		lis3->read(lis3, lis3->regs[i], &lis3->reg_cache[i]);
> > +	lis3->regs_stored = true;
> > +}
> > +
> > +static inline void lis3_context_restore(struct lis3lv02d *lis3)
> > +{
> > +	int i;
> > +	if (lis3->regs_stored)
> > +		for (i = 0; i < lis3->regs_size; i++)
> > +			lis3->write(lis3, lis3->regs[i], lis3->reg_cache[i]);
> > +}
> > +
> >  void lis3lv02d_poweroff(struct lis3lv02d *lis3)
> >  {
> > +	if (lis3->reg_ctrl)
> > +		lis3_context_save(lis3);
> >  	/* disable X,Y,Z axis and power down */
> >  	lis3->write(lis3, CTRL_REG1, 0x00);
> > +	if (lis3->reg_ctrl)
> > +		lis3->reg_ctrl(lis3, LIS3_REG_OFF);
> >  }
> >  EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
> >  
> > @@ -249,6 +285,8 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
> >  		reg |= CTRL2_BDU;
> >  		lis3->write(lis3, CTRL_REG2, reg);
> >  	}
> > +	if (lis3->reg_ctrl)
> > +		lis3_context_restore(lis3);
> >  }
> >  EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
> >  
> > @@ -718,6 +756,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
> >  		dev->odrs = lis3_12_rates;
> >  		dev->odr_mask = CTRL1_DF0 | CTRL1_DF1;
> >  		dev->scale = LIS3_SENSITIVITY_12B;
> > +		dev->regs = lis3_wai12_regs;
> > +		dev->regs_size = ARRAY_SIZE(lis3_wai12_regs);
> >  		break;
> >  	case WAI_8B:
> >  		printk(KERN_INFO DRIVER_NAME ": 8 bits sensor found\n");
> > @@ -727,6 +767,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
> >  		dev->odrs = lis3_8_rates;
> >  		dev->odr_mask = CTRL1_DR;
> >  		dev->scale = LIS3_SENSITIVITY_8B;
> > +		dev->regs = lis3_wai8_regs;
> > +		dev->regs_size = ARRAY_SIZE(lis3_wai8_regs);
> >  		break;
> >  	default:
> >  		printk(KERN_ERR DRIVER_NAME
> > @@ -734,6 +776,12 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
> >  		return -EINVAL;
> >  	}
> >  
> This is a little odd as runtime checks go.  Surely it can only occur in event
> of a clear driver bug?  Personally I'd just go with dynamically allocating
> the reg_cache to be the right size...

Makes sense.

> > +	if (dev->regs_size > LIS3_NUM_MAX_REG) {
> > +		printk(KERN_ERR DRIVER_NAME
> > +			": register cache area is too small");
> > +		return -EINVAL;
> > +	}
> > +
> >  	mutex_init(&dev->mutex);
> >  
> >  	lis3lv02d_add_fs(dev);
> > diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> > index 3e8a208..caf3ed1 100644
> > --- a/drivers/hwmon/lis3lv02d.h
> > +++ b/drivers/hwmon/lis3lv02d.h
> > @@ -20,6 +20,7 @@
> >   */
> >  #include <linux/platform_device.h>
> >  #include <linux/input-polldev.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  /*
> >   * This driver tries to support the "digital" accelerometer chips from
> > @@ -97,6 +98,15 @@ enum lis3_who_am_i {
> >  	WAI_8B		= 0x3B, /* 8 bits: LIS[23]02D[LQ]... */
> >  	WAI_6B		= 0x52, /* 6 bits: LIS331DLF - not supported */
> >  };
> > +/* Number of RW registers in each device for register caching purposes */
> You could just enforce this through review, but I guess it doesn't hurt
> to have sanity checks... 
> 

With runtime allocation these are not needed at all.

> > +#define NUM_RW_REGS_12B 21
> > +#define NUM_RW_REGS_8B  15
> > +
> > +#if NUM_RW_REGS_8B > NUM_RW_REGS_12B
> > +#define LIS3_NUM_MAX_REG NUM_RW_REGS_8B
> > +#else
> > +#define LIS3_NUM_MAX_REG NUM_RW_REGS_12B
> > +#endif
> >  
> >  enum lis3lv02d_ctrl1_12b {
> >  	CTRL1_Xen	= 0x01,
> > @@ -206,6 +216,9 @@ enum lis3lv02d_click_src_8b {
> >  	CLICK_IA	= 0x40,
> >  };
> >  
> > +#define LIS3_REG_OFF	0x00
> > +#define LIS3_REG_ON	0x01
> I think the rest of these are done as enums.  Worth doing that here for
> consistency?

True

> > +
> >  struct axis_conversion {
> >  	s8	x;
> >  	s8	y;
> > @@ -218,8 +231,13 @@ struct lis3lv02d {
> >  	int (*init) (struct lis3lv02d *lis3);
> >  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
> >  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
> > +	int (*reg_ctrl) (struct lis3lv02d *lis3, bool state);
> >  
> >  	int                     *odrs;     /* Supported output data rates */
> > +	u8			*regs;	   /* Regs to store / restore */
> > +	int			regs_size;
> > +	bool			regs_stored;
> > +	u8                      reg_cache[LIS3_NUM_MAX_REG];
> >  	u8                      odr_mask;  /* ODR bit mask */
> >  	u8			whoami;    /* indicates measurement precision */
> >  	s16 (*read_data) (struct lis3lv02d *lis3, int reg);
> > @@ -232,6 +250,7 @@ struct lis3lv02d {
> >  
> >  	struct input_polled_dev	*idev;     /* input device */
> >  	struct platform_device	*pdev;     /* platform device */
> > +	struct regulator_bulk_data regulators[2];
> >  	atomic_t		count;     /* interrupt count after last read */
> >  	struct axis_conversion	ac;        /* hw -> logical axis */
> >  	int			mapped_btns[3];
> > diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> > index b9ed1fb..0852bed 100644
> > --- a/drivers/hwmon/lis3lv02d_i2c.c
> > +++ b/drivers/hwmon/lis3lv02d_i2c.c
> > @@ -30,10 +30,29 @@
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/delay.h>
> >  #include "lis3lv02d.h"
> >  
> >  #define DRV_NAME 	"lis3lv02d_i2c"
> >  
> > +static const char reg_vdd[]    = "Vdd";
> > +static const char reg_vdd_io[] = "Vdd_IO";
> > +
> > +static int lis3_reg_ctrl(struct lis3lv02d *lis3, bool state)
> > +{
> > +	int ret;
> > +	if (state == LIS3_REG_OFF) {
> > +		ret = regulator_bulk_disable(ARRAY_SIZE(lis3->regulators),
> > +					lis3->regulators);
> > +	} else {
> > +		ret = regulator_bulk_enable(ARRAY_SIZE(lis3->regulators),
> > +					lis3->regulators);
> > +		/* Chip needs time to wakeup. Not mentioned in datasheet */
> > +		usleep_range(5000, 10000);
> > +	}
> > +	return ret;
> > +}
> > +
> >  static inline s32 lis3_i2c_write(struct lis3lv02d *lis3, int reg, u8 value)
> >  {
> >  	struct i2c_client *c = lis3->bus_priv;
> > @@ -52,6 +71,12 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
> >  	u8 reg;
> >  	int ret;
> >  
> > +	lis3_reg_ctrl(lis3, LIS3_REG_ON);
> > +
> > +	lis3->read(lis3, WHO_AM_I, &reg);
> > +	if (lis3->whoami != 0 && reg != lis3->whoami)
> What is the purpose of the first test? How can we get here without that being set?

uumm.... there is no way. Init function is called first time after 
setting up the whoami value. 

> > +		printk(KERN_ERR "lis3: power on failure\n");
> > +
> >  	/* power up the device */
> >  	ret = lis3->read(lis3, CTRL_REG1, &reg);
> >  	if (ret < 0)
> > @@ -89,16 +114,29 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
> >  			goto fail;
> >  	}
> >  
> > +	lis3_dev.regulators[0].supply = reg_vdd;
> > +	lis3_dev.regulators[1].supply = reg_vdd_io;
> > +	ret = regulator_bulk_get(&client->dev, ARRAY_SIZE(lis3_dev.regulators),
> > +				lis3_dev.regulators);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> >  	lis3_dev.pdata	  = pdata;
> >  	lis3_dev.bus_priv = client;
> >  	lis3_dev.init	  = lis3_i2c_init;
> >  	lis3_dev.read	  = lis3_i2c_read;
> >  	lis3_dev.write	  = lis3_i2c_write;
> > +	lis3_dev.reg_ctrl = lis3_reg_ctrl;
> >  	lis3_dev.irq	  = client->irq;
> >  	lis3_dev.ac	  = lis3lv02d_axis_map;
> >  	lis3_dev.pm_dev	  = &client->dev;
> >  
> >  	i2c_set_clientdata(client, &lis3_dev);
> > +
> > +	/* Provide power over the init call */
> > +	lis3_reg_ctrl(&lis3_dev, LIS3_REG_ON);
> > +	lis3_reg_ctrl(&lis3_dev, LIS3_REG_OFF);
> > +
> >  	ret = lis3lv02d_init_device(&lis3_dev);
> >  fail:
> >  	return ret;
> > @@ -113,8 +151,11 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
> >  		pdata->release_resources();
> >  
> >  	lis3lv02d_joystick_disable();
> > +	lis3lv02d_remove_fs(lis3);
> subtle change here... Out of intererst, why did the top level lis3_dev
> structure ever exist?  (you can tell I haven't looked closely at this driver
> before!) Can remove_fs return an error? 

Remove fs returns always 0. 

There are couple of bigger changes which somebody should do to this
driver:
- Change static lis3_dev structure to a dynamically allocated one
- Add proper error handling to the driver.



> >  
> > -	return lis3lv02d_remove_fs(&lis3_dev);
> > +	regulator_bulk_free(ARRAY_SIZE(lis3->regulators),
> > +			lis3_dev.regulators);
> > +	return 0;
> >  }
> >  
> >  #ifdef CONFIG_PM
> 

Re: [lm-sensors] [RFC PATCH 2/9] hwmon: lis3: regulator control

[Jonathan Cameron]

...
>> subtle change here... Out of intererst, why did the top level lis3_dev
>> structure ever exist?  (you can tell I haven't looked closely at this driver
>> before!) Can remove_fs return an error? 
> 
> Remove fs returns always 0. 
> 
> There are couple of bigger changes which somebody should do to this
> driver:
> - Change static lis3_dev structure to a dynamically allocated one//generalize
Definitely.
> - Add proper error handling to the driver.

Agreed, hopefully someone will step up and do it (good job for a starting out
kernel dev perhaps?)

Re: [RFC PATCH 2/9] hwmon: lis3: regulator control

[David Lutolf]

>> There are couple of bigger changes which somebody should do to this
>> driver:
>> - Change static lis3_dev structure to a dynamically allocated one//generalize
>Definitely.
>> - Add proper error handling to the driver.
>
>Agreed, hopefully someone will step up and do it (good job for a starting out
>kernel dev perhaps?)

If I'm given some guidance on how to do it I'll be glad to start. I don't have much experience in C but I should have the required knowledge..

what's the procedure now? should I just take the patch, fix it (hopefuly) and test inclusion in the latest kernel?

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[RFC PATCH 3/9] hwmon: lis3: Cleanup interrupt handling

[Samu Onkalo]

Irqcfg moved to chip data instead of platform data.
This simplifies access in interrupt handler little bit.

Input device open and close functions set status for
interrupt threaded handler once.

Unnecessary check for interrupt source removed since
it is enough that active interrupt line indicates that
there was an interrupt.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c |   37 +++++++++++++------------------------
 drivers/hwmon/lis3lv02d.h |    2 ++
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 23d47ad..483204a 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -308,10 +308,14 @@ static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
 {
 	if (lis3_dev.pm_dev)
 		pm_runtime_get_sync(lis3_dev.pm_dev);
+
+	if (lis3_dev.pdata && lis3_dev.whoami == WAI_8B && lis3_dev.idev)
+		atomic_set(&lis3_dev.wake_thread, 1);
 }
 
 static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
 {
+	atomic_set(&lis3_dev.wake_thread, 0);
 	if (lis3_dev.pm_dev)
 		pm_runtime_put(lis3_dev.pm_dev);
 }
@@ -331,8 +335,7 @@ static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
 	wake_up_interruptible(&lis3_dev.misc_wait);
 	kill_fasync(&lis3_dev.async_queue, SIGIO, POLL_IN);
 out:
-	if (lis3_dev.pdata && lis3_dev.whoami == WAI_8B && lis3_dev.idev &&
-	    lis3_dev.idev->input->users)
+	if (atomic_read(&lis3_dev.wake_thread))
 		return IRQ_WAKE_THREAD;
 	return IRQ_HANDLED;
 }
@@ -363,31 +366,15 @@ static void lis302dl_interrupt_handle_click(struct lis3lv02d *lis3)
 	mutex_unlock(&lis3->mutex);
 }
 
-static void lis302dl_interrupt_handle_ff_wu(struct lis3lv02d *lis3)
-{
-	u8 wu1_src;
-	u8 wu2_src;
-
-	lis3->read(lis3, FF_WU_SRC_1, &wu1_src);
-	lis3->read(lis3, FF_WU_SRC_2, &wu2_src);
-
-	wu1_src = wu1_src & FF_WU_SRC_IA ? wu1_src : 0;
-	wu2_src = wu2_src & FF_WU_SRC_IA ? wu2_src : 0;
-
-	/* joystick poll is internally protected by the lis3->mutex. */
-	if (wu1_src || wu2_src)
-		lis3lv02d_joystick_poll(lis3_dev.idev);
-}
-
 static irqreturn_t lis302dl_interrupt_thread1_8b(int irq, void *data)
 {
 
 	struct lis3lv02d *lis3 = data;
 
-	if ((lis3->pdata->irq_cfg & LIS3_IRQ1_MASK) == LIS3_IRQ1_CLICK)
+	if ((lis3->irq_cfg & LIS3_IRQ1_MASK) == LIS3_IRQ1_CLICK)
 		lis302dl_interrupt_handle_click(lis3);
 	else
-		lis302dl_interrupt_handle_ff_wu(lis3);
+		lis3lv02d_joystick_poll(lis3_dev.idev);
 
 	return IRQ_HANDLED;
 }
@@ -397,10 +384,10 @@ static irqreturn_t lis302dl_interrupt_thread2_8b(int irq, void *data)
 
 	struct lis3lv02d *lis3 = data;
 
-	if ((lis3->pdata->irq_cfg & LIS3_IRQ2_MASK) == LIS3_IRQ2_CLICK)
+	if ((lis3->irq_cfg & LIS3_IRQ2_MASK) == LIS3_IRQ2_CLICK)
 		lis302dl_interrupt_handle_click(lis3);
 	else
-		lis302dl_interrupt_handle_ff_wu(lis3);
+		lis3lv02d_joystick_poll(lis3_dev.idev);
 
 	return IRQ_HANDLED;
 }
@@ -783,6 +770,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 	}
 
 	mutex_init(&dev->mutex);
+	atomic_set(&dev->wake_thread, 0);
 
 	lis3lv02d_add_fs(dev);
 	lis3lv02d_poweron(dev);
@@ -795,14 +783,15 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 	if (lis3lv02d_joystick_enable())
 		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
 
-	/* passing in platform specific data is purely optional and only
-	 * used by the SPI transport layer at the moment */
+	/* passing in platform specific data is purely optional and
+	 * used by the SPI & I2C transport layer at the moment */
 	if (dev->pdata) {
 		struct lis3lv02d_platform_data *p = dev->pdata;
 
 		if (dev->whoami == WAI_8B)
 			lis3lv02d_8b_configure(dev, p);
 
+		dev->irq_cfg = p->irq_cfg;
 		if (p->irq_cfg)
 			dev->write(dev, CTRL_REG3, p->irq_cfg);
 	}
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index caf3ed1..8c19185 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -259,6 +259,8 @@ struct lis3lv02d {
 	struct fasync_struct	*async_queue; /* queue for the misc device */
 	wait_queue_head_t	misc_wait; /* Wait queue for the misc device */
 	unsigned long		misc_opened; /* bit0: whether the device is open */
+	atomic_t                wake_thread;
+	unsigned char           irq_cfg;
 
 	struct lis3lv02d_platform_data *pdata;	/* for passing board config */
 	struct mutex		mutex;     /* Serialize poll and selftest */
-- 
1.6.0.4

[RFC PATCH 4/9] hwmon: lis3: Update coordinates at polled device open

[Samu Onkalo]

Call input device poll function at device open to refresh coordinates
immediately. This is needed for the case where poll interval is set to
zero and coordinate updates happens purely under interrupt control.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 483204a..dc777d2 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -311,6 +311,11 @@ static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
 
 	if (lis3_dev.pdata && lis3_dev.whoami == WAI_8B && lis3_dev.idev)
 		atomic_set(&lis3_dev.wake_thread, 1);
+	/*
+	 * Update coordinates for the case where poll interval is 0 and
+	 * the chip in running purely under interrupt control
+	 */
+	lis3lv02d_joystick_poll(pidev);
 }
 
 static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
-- 
1.6.0.4

[RFC PATCH 5/9] hwmon: lis3: Power on corrections

[Samu Onkalo]

Sometimes lis3 chip seems to fail to setup factory tuning at boot up.
This probably happens if there is some odd power ramp down ramp up sequence
for example in device restart. Set boot bit in control2 register to
trig boot sequence manually and wait until it is finished.
Also restore axis enable bits in init.

Signed-off-by: Samu O kalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c     |   19 +++++++++++--------
 drivers/hwmon/lis3lv02d.h     |    1 +
 drivers/hwmon/lis3lv02d_i2c.c |    2 +-
 drivers/hwmon/lis3lv02d_spi.c |    2 +-
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index dc777d2..81e2313 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -272,19 +272,22 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
 
 	lis3->init(lis3);
 
-	/* LIS3 power on delay is quite long */
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
-
 	/*
 	 * Common configuration
 	 * BDU: (12 bits sensors only) LSB and MSB values are not updated until
 	 *      both have been read. So the value read will always be correct.
+	 * Set BOOT bit to refresh factory tuning values.
 	 */
-	if (lis3->whoami ==  WAI_12B) {
-		lis3->read(lis3, CTRL_REG2, &reg);
-		reg |= CTRL2_BDU;
-		lis3->write(lis3, CTRL_REG2, reg);
-	}
+	lis3->read(lis3, CTRL_REG2, &reg);
+	if (lis3->whoami ==  WAI_12B)
+		reg |= CTRL2_BDU | CTRL2_BOOT;
+	else
+		reg |= CTRL2_BOOT_8B;
+	lis3->write(lis3, CTRL_REG2, reg);
+
+	/* LIS3 power on delay is quite long */
+	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+
 	if (lis3->reg_ctrl)
 		lis3_context_restore(lis3);
 }
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 8c19185..1522855 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -142,6 +142,7 @@ enum lis3lv02d_ctrl2 {
 enum lis302d_ctrl2 {
 	HP_FF_WU2	= 0x08,
 	HP_FF_WU1	= 0x04,
+	CTRL2_BOOT_8B   = 0x40,
 };
 
 enum lis3lv02d_ctrl3 {
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index 0852bed..c5a60e4 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -82,7 +82,7 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
 	if (ret < 0)
 		return ret;
 
-	reg |= CTRL1_PD0;
+	reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
 	return lis3->write(lis3, CTRL_REG1, reg);
 }
 
diff --git a/drivers/hwmon/lis3lv02d_spi.c b/drivers/hwmon/lis3lv02d_spi.c
index b9be5e3..778885d 100644
--- a/drivers/hwmon/lis3lv02d_spi.c
+++ b/drivers/hwmon/lis3lv02d_spi.c
@@ -50,7 +50,7 @@ static int lis3_spi_init(struct lis3lv02d *lis3)
 	if (ret < 0)
 		return ret;
 
-	reg |= CTRL1_PD0;
+	reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
 	return lis3->write(lis3, CTRL_REG1, reg);
 }
 
-- 
1.6.0.4

Re: [lm-sensors] [RFC PATCH 5/9] hwmon: lis3: Power on corrections

[Jonathan Cameron]

On 10/01/10 12:46, Samu Onkalo wrote:
> Sometimes lis3 chip seems to fail to setup factory tuning at boot up.
> This probably happens if there is some odd power ramp down ramp up sequence
> for example in device restart. Set boot bit in control2 register to
> trig boot sequence manually and wait until it is finished.
> Also restore axis enable bits in init.
This really ought to be two separate patches.  The issues are completely
unconnected.  I've noticed you tend to fix other things in individual
patches as you go along. This makes the harder to review so please
break out each conceptual change.

Both look fine to me, so feel free to add

Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
To a broken out pair of patches.
> 
> Signed-off-by: Samu O kalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/hwmon/lis3lv02d.c     |   19 +++++++++++--------
>  drivers/hwmon/lis3lv02d.h     |    1 +
>  drivers/hwmon/lis3lv02d_i2c.c |    2 +-
>  drivers/hwmon/lis3lv02d_spi.c |    2 +-
>  4 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index dc777d2..81e2313 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -272,19 +272,22 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
>  
>  	lis3->init(lis3);
>  
> -	/* LIS3 power on delay is quite long */
> -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> -
>  	/*
>  	 * Common configuration
>  	 * BDU: (12 bits sensors only) LSB and MSB values are not updated until
>  	 *      both have been read. So the value read will always be correct.
> +	 * Set BOOT bit to refresh factory tuning values.
>  	 */
> -	if (lis3->whoami ==  WAI_12B) {
> -		lis3->read(lis3, CTRL_REG2, &reg);
> -		reg |= CTRL2_BDU;
> -		lis3->write(lis3, CTRL_REG2, reg);
> -	}
> +	lis3->read(lis3, CTRL_REG2, &reg);
> +	if (lis3->whoami ==  WAI_12B)
> +		reg |= CTRL2_BDU | CTRL2_BOOT;
> +	else
> +		reg |= CTRL2_BOOT_8B;
> +	lis3->write(lis3, CTRL_REG2, reg);
> +
> +	/* LIS3 power on delay is quite long */
> +	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> +
>  	if (lis3->reg_ctrl)
>  		lis3_context_restore(lis3);
>  }
> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> index 8c19185..1522855 100644
> --- a/drivers/hwmon/lis3lv02d.h
> +++ b/drivers/hwmon/lis3lv02d.h
> @@ -142,6 +142,7 @@ enum lis3lv02d_ctrl2 {
>  enum lis302d_ctrl2 {
>  	HP_FF_WU2	= 0x08,
>  	HP_FF_WU1	= 0x04,
> +	CTRL2_BOOT_8B   = 0x40,
>  };
>  
>  enum lis3lv02d_ctrl3 {
> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> index 0852bed..c5a60e4 100644
> --- a/drivers/hwmon/lis3lv02d_i2c.c
> +++ b/drivers/hwmon/lis3lv02d_i2c.c
> @@ -82,7 +82,7 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
>  	if (ret < 0)
>  		return ret;
>  
> -	reg |= CTRL1_PD0;
> +	reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
>  	return lis3->write(lis3, CTRL_REG1, reg);
>  }
>  
> diff --git a/drivers/hwmon/lis3lv02d_spi.c b/drivers/hwmon/lis3lv02d_spi.c
> index b9be5e3..778885d 100644
> --- a/drivers/hwmon/lis3lv02d_spi.c
> +++ b/drivers/hwmon/lis3lv02d_spi.c
> @@ -50,7 +50,7 @@ static int lis3_spi_init(struct lis3lv02d *lis3)
>  	if (ret < 0)
>  		return ret;
>  
> -	reg |= CTRL1_PD0;
> +	reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen;
>  	return lis3->write(lis3, CTRL_REG1, reg);
>  }
>  

[RFC PATCH 7/9] hwmon: lis3: Adjust fuzziness for 8 bit device

[Samu Onkalo]

Default fuziness is set smaller for 8 device.
In 12 bit device LSB is quite close to 1 mg.
In 8bit device LSB is about 18 mg. This add
default sensitivity for 8 bit device.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 0a46a0e..b740024 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -70,8 +70,10 @@
 #define LIS3_SENSITIVITY_12B		((LIS3_ACCURACY * 1000) / 1024)
 #define LIS3_SENSITIVITY_8B		(18 * LIS3_ACCURACY)
 
-#define LIS3_DEFAULT_FUZZ		3
-#define LIS3_DEFAULT_FLAT		3
+#define LIS3_DEFAULT_FUZZ_12B		3
+#define LIS3_DEFAULT_FLAT_12B		3
+#define LIS3_DEFAULT_FUZZ_8B		1
+#define LIS3_DEFAULT_FLAT_8B		1
 
 struct lis3lv02d lis3_dev = {
 	.misc_wait   = __WAIT_QUEUE_HEAD_INITIALIZER(lis3_dev.misc_wait),
@@ -529,8 +531,16 @@ int lis3lv02d_joystick_enable(void)
 
 	set_bit(EV_ABS, input_dev->evbit);
 	max_val = (lis3_dev.mdps_max_val * lis3_dev.scale) / LIS3_ACCURACY;
-	fuzz = (LIS3_DEFAULT_FUZZ * lis3_dev.scale) / LIS3_ACCURACY;
-	flat = (LIS3_DEFAULT_FLAT * lis3_dev.scale) / LIS3_ACCURACY;
+	if (lis3_dev.whoami == WAI_12B) {
+		fuzz = LIS3_DEFAULT_FUZZ_12B;
+		flat = LIS3_DEFAULT_FLAT_12B;
+	} else {
+		fuzz = LIS3_DEFAULT_FUZZ_8B;
+		flat = LIS3_DEFAULT_FLAT_8B;
+	}
+	fuzz = (fuzz * lis3_dev.scale) / LIS3_ACCURACY;
+	flat = (flat * lis3_dev.scale) / LIS3_ACCURACY;
+
 	input_set_abs_params(input_dev, ABS_X, -max_val, max_val, fuzz, flat);
 	input_set_abs_params(input_dev, ABS_Y, -max_val, max_val, fuzz, flat);
 	input_set_abs_params(input_dev, ABS_Z, -max_val, max_val, fuzz, flat);
-- 
1.6.0.4

[RFC PATCH 8/9] hwmon: lis3: use block read to access data registers

[Samu Onkalo]

Add optional blockread function to interface driver. If available
the chip driver uses it for data register access. For 12 bit device
it reads 6 bytes to get 3*16bit data. For 8 bit device it reads out
5 bytes since every second byte is dummy.
This optimizes bus usage and reduces number of operations and
interrupts needed for one data update.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c     |   21 ++++++++++++++++++---
 drivers/hwmon/lis3lv02d.h     |    1 +
 drivers/hwmon/lis3lv02d_i2c.c |    9 +++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index b740024..469f251 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -129,9 +129,24 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
 	int position[3];
 	int i;
 
-	position[0] = lis3->read_data(lis3, OUTX);
-	position[1] = lis3->read_data(lis3, OUTY);
-	position[2] = lis3->read_data(lis3, OUTZ);
+	if (lis3->blkread) {
+		if (lis3_dev.whoami == WAI_12B) {
+			u16 data[3];
+			lis3->blkread(lis3, OUTX_L, 6, (u8 *)data);
+			for (i = 0; i < 3; i++)
+				position[i] = (s16)le16_to_cpu(data[i]);
+		} else {
+			u8 data[5];
+			/* Data: x, dummy, y, dummy, z */
+			lis3->blkread(lis3, OUTX, 5, data);
+			for (i = 0; i < 3; i++)
+				position[i] = (s8)data[i * 2];
+		}
+	} else {
+		position[0] = lis3->read_data(lis3, OUTX);
+		position[1] = lis3->read_data(lis3, OUTY);
+		position[2] = lis3->read_data(lis3, OUTZ);
+	}
 
 	for (i = 0; i < 3; i++)
 		position[i] = (position[i] * lis3->scale) / LIS3_ACCURACY;
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 1522855..cfff63c 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -232,6 +232,7 @@ struct lis3lv02d {
 	int (*init) (struct lis3lv02d *lis3);
 	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
 	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
+	int (*blkread) (struct lis3lv02d *lis3, int reg, int len, u8 *ret);
 	int (*reg_ctrl) (struct lis3lv02d *lis3, bool state);
 
 	int                     *odrs;     /* Supported output data rates */
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index c5a60e4..1e0673b 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -66,6 +66,14 @@ static inline s32 lis3_i2c_read(struct lis3lv02d *lis3, int reg, u8 *v)
 	return 0;
 }
 
+static inline s32 lis3_i2c_blockread(struct lis3lv02d *lis3, int reg, int len,
+				u8 *v)
+{
+	struct i2c_client *c = lis3->bus_priv;
+	reg |= (1 << 7); /* 7th bit enables address auto incrementation */
+	return i2c_smbus_read_i2c_block_data(c, reg, len, v);
+}
+
 static int lis3_i2c_init(struct lis3lv02d *lis3)
 {
 	u8 reg;
@@ -125,6 +133,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.bus_priv = client;
 	lis3_dev.init	  = lis3_i2c_init;
 	lis3_dev.read	  = lis3_i2c_read;
+	lis3_dev.blkread  = lis3_i2c_blockread;
 	lis3_dev.write	  = lis3_i2c_write;
 	lis3_dev.reg_ctrl = lis3_reg_ctrl;
 	lis3_dev.irq	  = client->irq;
-- 
1.6.0.4

Re: [lm-sensors] [RFC PATCH 8/9] hwmon: lis3: use block read to access data registers

[Jonathan Cameron]

On 10/01/10 12:46, Samu Onkalo wrote:
> Add optional blockread function to interface driver. If available
> the chip driver uses it for data register access. For 12 bit device
> it reads 6 bytes to get 3*16bit data. For 8 bit device it reads out
> 5 bytes since every second byte is dummy.
> This optimizes bus usage and reduces number of operations and
> interrupts needed for one data update.

Do we need to query if the i2c bus supports block reading or are they
all guaranteed to do so?

I'm guessing not seeing as i2c.h has a functionality bit for it...
I2C_FUNC_SMBUS_READ_BLOCK_DATA

Otherwise looks good.  Either justify that all i2c buses will work
or add the functionality check and you can add.

Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>

> 
> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/hwmon/lis3lv02d.c     |   21 ++++++++++++++++++---
>  drivers/hwmon/lis3lv02d.h     |    1 +
>  drivers/hwmon/lis3lv02d_i2c.c |    9 +++++++++
>  3 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index b740024..469f251 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -129,9 +129,24 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
>  	int position[3];
>  	int i;
>  
> -	position[0] = lis3->read_data(lis3, OUTX);
> -	position[1] = lis3->read_data(lis3, OUTY);
> -	position[2] = lis3->read_data(lis3, OUTZ);
> +	if (lis3->blkread) {
> +		if (lis3_dev.whoami == WAI_12B) {
> +			u16 data[3];
> +			lis3->blkread(lis3, OUTX_L, 6, (u8 *)data);
> +			for (i = 0; i < 3; i++)
> +				position[i] = (s16)le16_to_cpu(data[i]);
> +		} else {
> +			u8 data[5];
> +			/* Data: x, dummy, y, dummy, z */
> +			lis3->blkread(lis3, OUTX, 5, data);
> +			for (i = 0; i < 3; i++)
> +				position[i] = (s8)data[i * 2];
> +		}
> +	} else {
> +		position[0] = lis3->read_data(lis3, OUTX);
> +		position[1] = lis3->read_data(lis3, OUTY);
> +		position[2] = lis3->read_data(lis3, OUTZ);
> +	}
>  
>  	for (i = 0; i < 3; i++)
>  		position[i] = (position[i] * lis3->scale) / LIS3_ACCURACY;
> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
> index 1522855..cfff63c 100644
> --- a/drivers/hwmon/lis3lv02d.h
> +++ b/drivers/hwmon/lis3lv02d.h
> @@ -232,6 +232,7 @@ struct lis3lv02d {
>  	int (*init) (struct lis3lv02d *lis3);
>  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
>  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
> +	int (*blkread) (struct lis3lv02d *lis3, int reg, int len, u8 *ret);
>  	int (*reg_ctrl) (struct lis3lv02d *lis3, bool state);
>  
>  	int                     *odrs;     /* Supported output data rates */
> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> index c5a60e4..1e0673b 100644
> --- a/drivers/hwmon/lis3lv02d_i2c.c
> +++ b/drivers/hwmon/lis3lv02d_i2c.c
> @@ -66,6 +66,14 @@ static inline s32 lis3_i2c_read(struct lis3lv02d *lis3, int reg, u8 *v)
>  	return 0;
>  }
>  
> +static inline s32 lis3_i2c_blockread(struct lis3lv02d *lis3, int reg, int len,
> +				u8 *v)
> +{
> +	struct i2c_client *c = lis3->bus_priv;
> +	reg |= (1 << 7); /* 7th bit enables address auto incrementation */
> +	return i2c_smbus_read_i2c_block_data(c, reg, len, v);
> +}
> +
>  static int lis3_i2c_init(struct lis3lv02d *lis3)
>  {
>  	u8 reg;
> @@ -125,6 +133,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>  	lis3_dev.bus_priv = client;
>  	lis3_dev.init	  = lis3_i2c_init;
>  	lis3_dev.read	  = lis3_i2c_read;
> +	lis3_dev.blkread  = lis3_i2c_blockread;
>  	lis3_dev.write	  = lis3_i2c_write;
>  	lis3_dev.reg_ctrl = lis3_reg_ctrl;
>  	lis3_dev.irq	  = client->irq;

Re: [lm-sensors] [RFC PATCH 8/9] hwmon: lis3: use block read to access data registers

[Guenter Roeck]

On Mon, Oct 04, 2010 at 07:41:34AM -0400, Jonathan Cameron wrote:
> On 10/01/10 12:46, Samu Onkalo wrote:
> > Add optional blockread function to interface driver. If available
> > the chip driver uses it for data register access. For 12 bit device
> > it reads 6 bytes to get 3*16bit data. For 8 bit device it reads out
> > 5 bytes since every second byte is dummy.
> > This optimizes bus usage and reduces number of operations and
> > interrupts needed for one data update.
> 
> Do we need to query if the i2c bus supports block reading or are they
> all guaranteed to do so?
> 
Not really. It may be true that block reads happen to be supported on 
all i2c controllers used on all boards using this chip, but that
is pretty much a coincidence.

> I'm guessing not seeing as i2c.h has a functionality bit for it...
> I2C_FUNC_SMBUS_READ_BLOCK_DATA
> 
> Otherwise looks good.  Either justify that all i2c buses will work
> or add the functionality check and you can add.
> 
Should really have the check. And, for completeness, the driver should
check if BYTE_DATA is supported as well. Besides, adding the check
seems to be much less effort than trying to explain why it isn't needed.

Guenter

[RFC PATCH 9/9] hwmon: lis3: Enhance lis3 selftest with IRQ line test

[Samu Onkalo]

Configure chip to data ready mode in selftest and count received
interrupts to see that interrupt line(s) are working.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/lis3lv02d.c |   87 ++++++++++++++++++++++++++++++++++++++++----
 drivers/hwmon/lis3lv02d.h |    3 +-
 2 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 469f251..cf1b4db 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -47,6 +47,13 @@
 
 #define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
 
+#define SELFTEST_OK	       0
+#define SELFTEST_FAIL	       -1
+#define SELFTEST_IRQ	       -2
+
+#define IRQ_LINE0	       0
+#define IRQ_LINE1	       1
+
 /*
  * The sensor can also generate interrupts (DRDY) but it's pretty pointless
  * because they are generated even if the data do not change. So it's better
@@ -197,6 +204,8 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 	s16 x, y, z;
 	u8 selftest;
 	int ret;
+	u8 ctrl_reg_data;
+	unsigned char irq_cfg;
 
 	mutex_lock(&lis3->mutex);
 	if (lis3_dev.whoami == WAI_12B)
@@ -204,6 +213,20 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 	else
 		selftest = CTRL1_STP;
 
+	irq_cfg = lis3->irq_cfg;
+	if (lis3_dev.whoami == WAI_8B) {
+		lis3->data_ready_count[IRQ_LINE0] = 0;
+		lis3->data_ready_count[IRQ_LINE1] = 0;
+
+		/* Change interrupt cfg to data ready for selftest */
+		atomic_inc(&lis3_dev.wake_thread);
+		lis3->irq_cfg = LIS3_IRQ1_DATA_READY | LIS3_IRQ2_DATA_READY;
+		lis3->read(lis3, CTRL_REG3, &ctrl_reg_data);
+		lis3->write(lis3, CTRL_REG3, (ctrl_reg_data &
+				~(LIS3_IRQ1_MASK | LIS3_IRQ2_MASK)) |
+				(LIS3_IRQ1_DATA_READY | LIS3_IRQ2_DATA_READY));
+	}
+
 	lis3->read(lis3, CTRL_REG1, &reg);
 	lis3->write(lis3, CTRL_REG1, (reg | selftest));
 	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
@@ -222,13 +245,33 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 	results[2] = z - lis3->read_data(lis3, OUTZ);
 
 	ret = 0;
+
+	if (lis3_dev.whoami == WAI_8B) {
+		/* Restore original interrupt configuration */
+		atomic_dec(&lis3_dev.wake_thread);
+		lis3->write(lis3, CTRL_REG3, ctrl_reg_data);
+		lis3->irq_cfg = irq_cfg;
+
+		if ((irq_cfg & LIS3_IRQ1_MASK) &&
+			lis3->data_ready_count[IRQ_LINE0] < 2) {
+			ret = SELFTEST_IRQ;
+			goto fail;
+		}
+
+		if ((irq_cfg & LIS3_IRQ2_MASK) &&
+			lis3->data_ready_count[IRQ_LINE1] < 2) {
+			ret = SELFTEST_IRQ;
+			goto fail;
+		}
+	}
+
 	if (lis3->pdata) {
 		int i;
 		for (i = 0; i < 3; i++) {
 			/* Check against selftest acceptance limits */
 			if ((results[i] < lis3->pdata->st_min_limits[i]) ||
 			    (results[i] > lis3->pdata->st_max_limits[i])) {
-				ret = -EIO;
+				ret = SELFTEST_FAIL;
 				goto fail;
 			}
 		}
@@ -391,13 +434,24 @@ static void lis302dl_interrupt_handle_click(struct lis3lv02d *lis3)
 	mutex_unlock(&lis3->mutex);
 }
 
-static irqreturn_t lis302dl_interrupt_thread1_8b(int irq, void *data)
+static inline void lis302dl_data_ready(struct lis3lv02d *lis3, int index)
 {
+	int dummy;
 
+	/* Dummy read to ack interrupt */
+	lis3lv02d_get_xyz(lis3, &dummy, &dummy, &dummy);
+	lis3->data_ready_count[index]++;
+}
+
+static irqreturn_t lis302dl_interrupt_thread1_8b(int irq, void *data)
+{
 	struct lis3lv02d *lis3 = data;
+	u8 irq_cfg = lis3->irq_cfg & LIS3_IRQ1_MASK;
 
-	if ((lis3->irq_cfg & LIS3_IRQ1_MASK) == LIS3_IRQ1_CLICK)
+	if (irq_cfg == LIS3_IRQ1_CLICK)
 		lis302dl_interrupt_handle_click(lis3);
+	else if (unlikely(irq_cfg == LIS3_IRQ1_DATA_READY))
+		lis302dl_data_ready(lis3, IRQ_LINE0);
 	else
 		lis3lv02d_joystick_poll(lis3_dev.idev);
 
@@ -406,11 +460,13 @@ static irqreturn_t lis302dl_interrupt_thread1_8b(int irq, void *data)
 
 static irqreturn_t lis302dl_interrupt_thread2_8b(int irq, void *data)
 {
-
 	struct lis3lv02d *lis3 = data;
+	u8 irq_cfg = lis3->irq_cfg & LIS3_IRQ2_MASK;
 
-	if ((lis3->irq_cfg & LIS3_IRQ2_MASK) == LIS3_IRQ2_CLICK)
+	if (irq_cfg == LIS3_IRQ2_CLICK)
 		lis302dl_interrupt_handle_click(lis3);
+	else if (unlikely(irq_cfg == LIS3_IRQ2_DATA_READY))
+		lis302dl_data_ready(lis3, IRQ_LINE1);
 	else
 		lis3lv02d_joystick_poll(lis3_dev.idev);
 
@@ -613,12 +669,27 @@ static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
 static ssize_t lis3lv02d_selftest_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	int result;
 	s16 values[3];
 
+	static const char ok[] = "OK";
+	static const char fail[] = "FAIL";
+	static const char irq[] = "FAIL_IRQ";
+	const char *res;
+
 	lis3lv02d_sysfs_poweron(&lis3_dev);
-	result = lis3lv02d_selftest(&lis3_dev, values);
-	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
+	switch (lis3lv02d_selftest(&lis3_dev, values)) {
+	case SELFTEST_FAIL:
+		res = fail;
+		break;
+	case SELFTEST_IRQ:
+		res = irq;
+		break;
+	case SELFTEST_OK:
+	default:
+		res = ok;
+		break;
+	}
+	return sprintf(buf, "%s %d %d %d\n", res,
 		values[0], values[1], values[2]);
 }
 
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index cfff63c..3b739ec 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -261,7 +261,8 @@ struct lis3lv02d {
 	struct fasync_struct	*async_queue; /* queue for the misc device */
 	wait_queue_head_t	misc_wait; /* Wait queue for the misc device */
 	unsigned long		misc_opened; /* bit0: whether the device is open */
-	atomic_t                wake_thread;
+	int                     data_ready_count[2];
+	atomic_t		wake_thread;
 	unsigned char           irq_cfg;
 
 	struct lis3lv02d_platform_data *pdata;	/* for passing board config */
-- 
1.6.0.4

Re: [RFC PATCH 0/9] lis3 accelerator feature update

[Guenter Roeck]

On Fri, Oct 01, 2010 at 07:46:47AM -0400, Samu Onkalo wrote:
> This patch set is done to top of 2.6.36-RC5
> 
> Changes are tested only with I2C interface using 8bit sensor since I don't
> have other possibilities.
> 
> I send this as RFC since changes may affect functionalities or use cases
> which I can't test or I don't know to exist. 
> 
Wonder if there is a better mailing list to get good (or even any) feedback
for this patchset. Even though the driver resides in the hwmon directory,
it isn't really a hardware monitoring driver. Neither i2c nor hwmon fits well.

Ideas, anyone ?

Guenter

Re: [RFC PATCH 0/9] lis3 accelerator feature update

[Jean Delvare]

Hi Guenter,

On Fri, 1 Oct 2010 19:53:11 -0700, Guenter Roeck wrote:
> On Fri, Oct 01, 2010 at 07:46:47AM -0400, Samu Onkalo wrote:
> > This patch set is done to top of 2.6.36-RC5
> > 
> > Changes are tested only with I2C interface using 8bit sensor since I don't
> > have other possibilities.
> > 
> > I send this as RFC since changes may affect functionalities or use cases
> > which I can't test or I don't know to exist. 
> > 
> Wonder if there is a better mailing list to get good (or even any) feedback
> for this patchset. Even though the driver resides in the hwmon directory,
> it isn't really a hardware monitoring driver. Neither i2c nor hwmon fits well.
> 
> Ideas, anyone ?

In an ideal world, people interested in accelerometers would create a
subsystem for it, declare themselves maintainers of it, and all
accelerometer drivers which currently leave under drivers/hwmon (hdaps,
hp_accel, lis3lv02d and maybe ams) would be moved there.

The current state is very bad, because these drivers are located in a
maintained subsystem but they don't belong there. This is what
motivated my request to move these drivers away [1]. But as you can see
nobody replied.

[1] http://www.spinics.net/lists/linux-input/msg11411.html

-- 
Jean Delvare

Re: [lm-sensors] [RFC PATCH 0/9] lis3 accelerator feature update

[Jonathan Cameron]

On 10/02/10 09:25, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri, 1 Oct 2010 19:53:11 -0700, Guenter Roeck wrote:
>> On Fri, Oct 01, 2010 at 07:46:47AM -0400, Samu Onkalo wrote:
>>> This patch set is done to top of 2.6.36-RC5
>>>
>>> Changes are tested only with I2C interface using 8bit sensor since I don't
>>> have other possibilities.
>>>
>>> I send this as RFC since changes may affect functionalities or use cases
>>> which I can't test or I don't know to exist. 
>>>
>> Wonder if there is a better mailing list to get good (or even any) feedback
>> for this patchset. Even though the driver resides in the hwmon directory,
>> it isn't really a hardware monitoring driver. Neither i2c nor hwmon fits well.
>>
>> Ideas, anyone ?
cc lkml?  These are very much device specific so only people who can really
review are those who actually have the part or don't mind spending a fair bit
of time familiarizing themselves with a fairly complex driver.

Perhaps runtime pm ones want to go to the runtime pm guys? That bit isn't
really subsystem specific. I'm quite keen to drive that stuff into some of
my drivers, but haven't done it yet so am not familiar enough with the
infrastructure.

> 
> In an ideal world, people interested in accelerometers would create a
> subsystem for it, declare themselves maintainers of it, and all
> accelerometer drivers which currently leave under drivers/hwmon (hdaps,
> hp_accel, lis3lv02d and maybe ams) would be moved there.
Agreed.
> 
> The current state is very bad, because these drivers are located in a
> maintained subsystem but they don't belong there. This is what
> motivated my request to move these drivers away [1]. But as you can see
> nobody replied.
Dmitry often takes a little while to reply, so I wouldn't give up hope yet.

Sadly, despite numerous people wading in from time to time to tell use we
are doing it all wrong, those who actually are interested day to day in
these sorts of devices are far too thin on the ground.

Personally, if no one responds from input, I'd just push this driver
into misc asap.  Can't seen anyone objecting to that.
> 
> [1] http://www.spinics.net/lists/linux-input/msg11411.html
> 

Re: [lm-sensors] [RFC PATCH 0/9] lis3 accelerator feature update

[Guenter Roeck]

On Sat, Oct 02, 2010 at 08:27:05AM -0400, Jonathan Cameron wrote:
> On 10/02/10 09:25, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Fri, 1 Oct 2010 19:53:11 -0700, Guenter Roeck wrote:
> >> On Fri, Oct 01, 2010 at 07:46:47AM -0400, Samu Onkalo wrote:
> >>> This patch set is done to top of 2.6.36-RC5
> >>>
> >>> Changes are tested only with I2C interface using 8bit sensor since I don't
> >>> have other possibilities.
> >>>
> >>> I send this as RFC since changes may affect functionalities or use cases
> >>> which I can't test or I don't know to exist. 
> >>>
> >> Wonder if there is a better mailing list to get good (or even any) feedback
> >> for this patchset. Even though the driver resides in the hwmon directory,
> >> it isn't really a hardware monitoring driver. Neither i2c nor hwmon fits well.
> >>
> >> Ideas, anyone ?
> cc lkml?  These are very much device specific so only people who can really
> review are those who actually have the part or don't mind spending a fair bit
> of time familiarizing themselves with a fairly complex driver.
> 
> Perhaps runtime pm ones want to go to the runtime pm guys? That bit isn't
> really subsystem specific. I'm quite keen to drive that stuff into some of
> my drivers, but haven't done it yet so am not familiar enough with the
> infrastructure.
> 
> > 
> > In an ideal world, people interested in accelerometers would create a
> > subsystem for it, declare themselves maintainers of it, and all
> > accelerometer drivers which currently leave under drivers/hwmon (hdaps,
> > hp_accel, lis3lv02d and maybe ams) would be moved there.
> Agreed.
> > 
> > The current state is very bad, because these drivers are located in a
> > maintained subsystem but they don't belong there. This is what
> > motivated my request to move these drivers away [1]. But as you can see
> > nobody replied.

Yes, I noticed. 

> Dmitry often takes a little while to reply, so I wouldn't give up hope yet.
> 
> Sadly, despite numerous people wading in from time to time to tell use we
> are doing it all wrong, those who actually are interested day to day in
> these sorts of devices are far too thin on the ground.
> 
> Personally, if no one responds from input, I'd just push this driver
> into misc asap.  Can't seen anyone objecting to that.

Makes sense to me. Drivers there seem to be maintained by individuals,
as is this driver, so it makes kind of sense. Maybe Eric has some comments ?

Guenter

[RFC PATCH 6/9] hwmon: lis3: New parameters to platform data

[Samu Onkalo]

Added control for duration via platform data.
Added possibility to configure interrupts to trig on
both rising and falling edge. The lis3 WU unit can be
configured quite many ways and with some configurations it
is quite handy to get coordinate refresh when some
event trigs and when it reason goes away.

Signed-off-by: Samu Onkalo <samu.p.onkalo@nokia.com>
---
 drivers/hwmon/lis3lv02d.c |   21 ++++++++++++++-------
 include/linux/lis3lv02d.h |    7 ++++++-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 81e2313..0a46a0e 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -681,6 +681,7 @@ static void lis3lv02d_8b_configure(struct lis3lv02d *dev,
 {
 	int err;
 	int ctrl2 = p->hipass_ctrl;
+	int irq_flags = IRQF_TRIGGER_RISING | IRQF_ONESHOT;
 
 	if (p->click_flags) {
 		dev->write(dev, CLICK_CFG, p->click_flags);
@@ -703,27 +704,29 @@ static void lis3lv02d_8b_configure(struct lis3lv02d *dev,
 	if (p->wakeup_flags) {
 		dev->write(dev, FF_WU_CFG_1, p->wakeup_flags);
 		dev->write(dev, FF_WU_THS_1, p->wakeup_thresh & 0x7f);
-		/* default to 2.5ms for now */
-		dev->write(dev, FF_WU_DURATION_1, 1);
+		/* pdata value + 1 to keep this backward compatible*/
+		dev->write(dev, FF_WU_DURATION_1, p->duration1 + 1);
 		ctrl2 ^= HP_FF_WU1; /* Xor to keep compatible with old pdata*/
 	}
 
 	if (p->wakeup_flags2) {
 		dev->write(dev, FF_WU_CFG_2, p->wakeup_flags2);
 		dev->write(dev, FF_WU_THS_2, p->wakeup_thresh2 & 0x7f);
-		/* default to 2.5ms for now */
-		dev->write(dev, FF_WU_DURATION_2, 1);
+		/* pdata value + 1 to keep this backward compatible*/
+		dev->write(dev, FF_WU_DURATION_2, p->duration2 + 1);
 		ctrl2 ^= HP_FF_WU2; /* Xor to keep compatible with old pdata*/
 	}
 	/* Configure hipass filters */
 	dev->write(dev, CTRL_REG2, ctrl2);
 
+	if (p->irq_flags & LIS3_IRQ2_USE_BOTH_EDGES)
+		irq_flags |= IRQF_TRIGGER_FALLING;
+
 	if (p->irq2) {
 		err = request_threaded_irq(p->irq2,
 					NULL,
 					lis302dl_interrupt_thread2_8b,
-					IRQF_TRIGGER_RISING |
-					IRQF_ONESHOT,
+					irq_flags,
 					DRIVER_NAME, &lis3_dev);
 		if (err < 0)
 			printk(KERN_ERR DRIVER_NAME
@@ -739,6 +742,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 {
 	int err;
 	irq_handler_t thread_fn;
+	int irq_flags = IRQF_TRIGGER_RISING | IRQF_ONESHOT;
 
 	dev->whoami = lis3lv02d_read_8(dev, WHO_AM_I);
 
@@ -799,6 +803,9 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 		if (dev->whoami == WAI_8B)
 			lis3lv02d_8b_configure(dev, p);
 
+		if (dev->pdata->irq_flags & LIS3_IRQ1_USE_BOTH_EDGES)
+			irq_flags |= IRQF_TRIGGER_FALLING;
+
 		dev->irq_cfg = p->irq_cfg;
 		if (p->irq_cfg)
 			dev->write(dev, CTRL_REG3, p->irq_cfg);
@@ -829,7 +836,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 
 	err = request_threaded_irq(dev->irq, lis302dl_interrupt,
 				thread_fn,
-				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+				irq_flags,
 				DRIVER_NAME, &lis3_dev);
 
 	if (err < 0) {
diff --git a/include/linux/lis3lv02d.h b/include/linux/lis3lv02d.h
index 0e8a346..167f947 100644
--- a/include/linux/lis3lv02d.h
+++ b/include/linux/lis3lv02d.h
@@ -36,13 +36,18 @@ struct lis3lv02d_platform_data {
 #define LIS3_IRQ_OPEN_DRAIN	(1 << 6)
 #define LIS3_IRQ_ACTIVE_LOW	(1 << 7)
 	unsigned char irq_cfg;
-
+#define LIS3_IRQ1_USE_BOTH_EDGES 1
+#define LIS3_IRQ2_USE_BOTH_EDGES 2
+	unsigned char irq_flags;
+	unsigned char duration1;
+	unsigned char duration2;
 #define LIS3_WAKEUP_X_LO	(1 << 0)
 #define LIS3_WAKEUP_X_HI	(1 << 1)
 #define LIS3_WAKEUP_Y_LO	(1 << 2)
 #define LIS3_WAKEUP_Y_HI	(1 << 3)
 #define LIS3_WAKEUP_Z_LO	(1 << 4)
 #define LIS3_WAKEUP_Z_HI	(1 << 5)
+#define LIS3_WAKEUP_AOI		(1 << 7) /* 'AND' combination of interrupts */
 	unsigned char wakeup_flags;
 	unsigned char wakeup_thresh;
 	unsigned char wakeup_flags2;
-- 
1.6.0.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 6/9] hwmon: lis3: New parameters to platform data

[Jonathan Cameron]

On 10/01/10 12:46, Samu Onkalo wrote:
> Added control for duration via platform data.
> Added possibility to configure interrupts to trig on
> both rising and falling edge. The lis3 WU unit can be
> configured quite many ways and with some configurations it
> is quite handy to get coordinate refresh when some
> event trigs and when it reason goes away.
There are probably cleaner ways of doing this.

For example rather than creating your own data types, why not just
have platform data eleemnts that are exactly the irq_flags you
want?  Or perhaps use resource structures instead? Looks like there
is some irq type support in ioport.h and a few users in tree...

Still, patch is sound except for the random last addition that
has nothing to do with this.  I'd like to seem some explanatory
documentation in the platform data talking about why you would
want to do this though... (actual examples please)

> 
> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/hwmon/lis3lv02d.c |   21 ++++++++++++++-------
>  include/linux/lis3lv02d.h |    7 ++++++-
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index 81e2313..0a46a0e 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -681,6 +681,7 @@ static void lis3lv02d_8b_configure(struct lis3lv02d *dev,
>  {
>  	int err;
>  	int ctrl2 = p->hipass_ctrl;
> +	int irq_flags = IRQF_TRIGGER_RISING | IRQF_ONESHOT;
>  
>  	if (p->click_flags) {
>  		dev->write(dev, CLICK_CFG, p->click_flags);
> @@ -703,27 +704,29 @@ static void lis3lv02d_8b_configure(struct lis3lv02d *dev,
>  	if (p->wakeup_flags) {
>  		dev->write(dev, FF_WU_CFG_1, p->wakeup_flags);
>  		dev->write(dev, FF_WU_THS_1, p->wakeup_thresh & 0x7f);
> -		/* default to 2.5ms for now */
> -		dev->write(dev, FF_WU_DURATION_1, 1);
> +		/* pdata value + 1 to keep this backward compatible*/
> +		dev->write(dev, FF_WU_DURATION_1, p->duration1 + 1);
>  		ctrl2 ^= HP_FF_WU1; /* Xor to keep compatible with old pdata*/
>  	}
>  
>  	if (p->wakeup_flags2) {
>  		dev->write(dev, FF_WU_CFG_2, p->wakeup_flags2);
>  		dev->write(dev, FF_WU_THS_2, p->wakeup_thresh2 & 0x7f);
> -		/* default to 2.5ms for now */
> -		dev->write(dev, FF_WU_DURATION_2, 1);
> +		/* pdata value + 1 to keep this backward compatible*/
> +		dev->write(dev, FF_WU_DURATION_2, p->duration2 + 1);

>  		ctrl2 ^= HP_FF_WU2; /* Xor to keep compatible with old pdata*/
>  	}
>  	/* Configure hipass filters */
>  	dev->write(dev, CTRL_REG2, ctrl2);
>  
> +	if (p->irq_flags & LIS3_IRQ2_USE_BOTH_EDGES)
> +		irq_flags |= IRQF_TRIGGER_FALLING;
> +
>  	if (p->irq2) {
>  		err = request_threaded_irq(p->irq2,
>  					NULL,
>  					lis302dl_interrupt_thread2_8b,
> -					IRQF_TRIGGER_RISING |
> -					IRQF_ONESHOT,
> +					irq_flags,
>  					DRIVER_NAME, &lis3_dev);
>  		if (err < 0)
>  			printk(KERN_ERR DRIVER_NAME
> @@ -739,6 +742,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  {
>  	int err;
>  	irq_handler_t thread_fn;
> +	int irq_flags = IRQF_TRIGGER_RISING | IRQF_ONESHOT;
>  
>  	dev->whoami = lis3lv02d_read_8(dev, WHO_AM_I);
>  
> @@ -799,6 +803,9 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  		if (dev->whoami == WAI_8B)
>  			lis3lv02d_8b_configure(dev, p);
>  
> +		if (dev->pdata->irq_flags & LIS3_IRQ1_USE_BOTH_EDGES)
> +			irq_flags |= IRQF_TRIGGER_FALLING;
> +
>  		dev->irq_cfg = p->irq_cfg;
>  		if (p->irq_cfg)
>  			dev->write(dev, CTRL_REG3, p->irq_cfg);
> @@ -829,7 +836,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  
>  	err = request_threaded_irq(dev->irq, lis302dl_interrupt,
>  				thread_fn,
> -				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +				irq_flags,
>  				DRIVER_NAME, &lis3_dev);
>  
>  	if (err < 0) {
> diff --git a/include/linux/lis3lv02d.h b/include/linux/lis3lv02d.h
> index 0e8a346..167f947 100644
> --- a/include/linux/lis3lv02d.h
> +++ b/include/linux/lis3lv02d.h
> @@ -36,13 +36,18 @@ struct lis3lv02d_platform_data {
>  #define LIS3_IRQ_OPEN_DRAIN	(1 << 6)
>  #define LIS3_IRQ_ACTIVE_LOW	(1 << 7)
>  	unsigned char irq_cfg;
> -
> +#define LIS3_IRQ1_USE_BOTH_EDGES 1
> +#define LIS3_IRQ2_USE_BOTH_EDGES 2
> +	unsigned char irq_flags;
> +	unsigned char duration1;
> +	unsigned char duration2;
>  #define LIS3_WAKEUP_X_LO	(1 << 0)
>  #define LIS3_WAKEUP_X_HI	(1 << 1)
>  #define LIS3_WAKEUP_Y_LO	(1 << 2)
>  #define LIS3_WAKEUP_Y_HI	(1 << 3)
>  #define LIS3_WAKEUP_Z_LO	(1 << 4)
>  #define LIS3_WAKEUP_Z_HI	(1 << 5)
> +#define LIS3_WAKEUP_AOI		(1 << 7) /* 'AND' combination of interrupts */
This change has nothing to do with the patch so shouldn't be here.

>  	unsigned char wakeup_flags;
>  	unsigned char wakeup_thresh;
>  	unsigned char wakeup_flags2;