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.onk...@nokia.com>
> > ---
> > 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, ®);
> > + 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, ®);
> > 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
