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

[Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>]

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