[Meego-kernel] [PATCH 1/6] apds9802als: add runtime PM support

[Hong Liu]

On Mon, 2010-09-27 at 15:57 +0800, Haicheng Li wrote:
> Hong Liu wrote:
> > On Sun, 2010-09-26 at 10:54 +0800, Haicheng Li wrote:
> >> Hong Liu wrote:
> >>> @@ -232,11 +219,13 @@ static int  apds9802als_probe(struct i2c_client *client,
> >>>  		dev_err(&client->dev, "device create file failed\n");
> >>>  		goto als_error1;
> >>>  	}
> >>> -	dev_info(&client->dev,
> >>> -		"%s apds9802als: ALS chip found\n", client->name);
> >>> +	dev_info(&client->dev, "ALS chip found\n");
> >>>  	als_set_default_config(client);
> >>> -	data->needresume = true;
> >>>  	mutex_init(&data->mutex);
> >>> +
> >>> +	pm_runtime_enable(&client->dev);
> >> missing a line here?
> >> +	pm_runtime_get(&client->dev);
> > 
> > It was there before, but it caused one problem (the 1st read get 0). 
> > 
> > Actually I always get 0 for the 1st measurement for this device. Before
> > it is not a problem since when we open the sysfs entry to read data, the
> > device already performs several measurements.
> > 
> > With runtime PM enabled, the first read on the sysfs entry actually is
> > the 1st measurement. I removed the pm_runtime_get() line and it seems
> > the problem was solved.
> > 
> > But your mail made me reread the runtime PM code, and found removing the
> > pm_runtime_get line actually makes the following pm_runtime_put do
> > nothing. 
> 
> In fact, pm_runtime_put() here will make *client->dev.power.usage_count* as -1.

I will add the pm_runtime_get() line back.

>  Logically it's wrong 
> since the value should be 0 in such case (pm_runtime_get_sync() in the rest code helps conceal this 
> potential issue).
> 
> > The device is powered on after probed if we don't read the
> > sysfs entry.
> > 
> > After some experiments, I find adding a delay (50ms) after
> > als_set_default_config solves the problem. This actually let the device
> > finish the 1st measurement when doing probe. The only concern is it will
> > introduce delay for the probe of this device.
> 
> It sounds like an ugly workaround. Why 50ms is OK? I suspect it might not work if you change to 
> another device:). A read back might help you decide if the 1st measurement is done or not.

It's just a workaround for the problem. 50ms is long enough for one
measurement. Just want to keep the workaround simple enough.

> 
> However, we'd better powered off the device after the 1st measurement. I think probably you need to 
> add such a line:
> +	als_set_power_state(client, false);
> ( *instead of pm_runtime_put()* )
> 

I think the pm_runtime_get and pm_runtime_put will do the same thing.

> > 
> > Thanks,
> > Hong
> > 
> >>> +	pm_runtime_put(&client->dev);
> >>> +
> >>>  	return res;
> >>>  als_error1:
> >>>  	i2c_set_clientdata(client, NULL);
> >>>  static int apds9802als_resume(struct i2c_client *client)
> >>>  {
> >>> -	struct als_data *data = i2c_get_clientdata(client);
> >>> +	als_set_default_config(client);
> >> ditto.
> >> +	pm_runtime_get(&client->dev);
> >>
> >>> +	pm_runtime_put(&client->dev);
> 
> same suggestion as above.
> 
> -haicheng