| Message ID | 1394208873-23260-7-git-send-email-lars@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lars-Peter, See some comments below: On 03/07/2014 05:14 PM, Lars-Peter Clausen wrote: > The adv7180 has a low power mode in which the analog and the digital processing > section are shut down. Implement the s_power callback to let bridge drivers put > the part into low power mode when not needed. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/media/i2c/adv7180.c | 52 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c > index 623cec5..8271362 100644 > --- a/drivers/media/i2c/adv7180.c > +++ b/drivers/media/i2c/adv7180.c > @@ -127,6 +127,7 @@ struct adv7180_state { > int irq; > v4l2_std_id curr_norm; > bool autodetect; > + bool powered; > u8 input; > }; > #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler, \ > @@ -311,6 +312,39 @@ out: > return ret; > } > > +static int adv7180_set_power(struct adv7180_state *state, > + struct i2c_client *client, bool on) > +{ > + u8 val; > + > + if (on) > + val = ADV7180_PWR_MAN_ON; > + else > + val = ADV7180_PWR_MAN_OFF; > + > + return i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, val); > +} > + > +static int adv7180_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct adv7180_state *state = to_state(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + int ret; > + > + ret = mutex_lock_interruptible(&state->mutex); > + if (ret) > + return ret; > + > + ret = adv7180_set_power(state, client, on); > + if (ret) > + goto out; > + > + state->powered = on; > +out: I would change this to: if (!ret) state->powered = on; and drop the 'goto'. > + mutex_unlock(&state->mutex); > + return ret; > +} > + > static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = to_adv7180_sd(ctrl); > @@ -441,6 +475,7 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = { > > static const struct v4l2_subdev_core_ops adv7180_core_ops = { > .s_std = adv7180_s_std, > + .s_power = adv7180_s_power, > }; > > static const struct v4l2_subdev_ops adv7180_ops = { > @@ -640,13 +675,9 @@ static const struct i2c_device_id adv7180_id[] = { > static int adv7180_suspend(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > - int ret; > + struct adv7180_state *state = to_state(sd); > > - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, > - ADV7180_PWR_MAN_OFF); > - if (ret < 0) > - return ret; > - return 0; > + return adv7180_set_power(state, client, false); > } > > static int adv7180_resume(struct device *dev) > @@ -656,10 +687,11 @@ static int adv7180_resume(struct device *dev) > struct adv7180_state *state = to_state(sd); > int ret; > > - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, > - ADV7180_PWR_MAN_ON); > - if (ret < 0) > - return ret; > + if (state->powered) { > + ret = adv7180_set_power(state, client, true); > + if (ret) > + return ret; > + } > ret = init_device(client, state); > if (ret < 0) > return ret; > What is the initial state of the driver when loaded? Shouldn't probe() set the 'powered' variable to true initially? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2014 03:37 PM, Hans Verkuil wrote: [...] >> + >> +static int adv7180_s_power(struct v4l2_subdev *sd, int on) >> +{ >> + struct adv7180_state *state = to_state(sd); >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> + int ret; >> + >> + ret = mutex_lock_interruptible(&state->mutex); >> + if (ret) >> + return ret; >> + >> + ret = adv7180_set_power(state, client, on); >> + if (ret) >> + goto out; >> + >> + state->powered = on; >> +out: > > I would change this to: > > if (!ret) > state->powered = on; > > and drop the 'goto'. > ok. [...] >> static int adv7180_resume(struct device *dev) >> @@ -656,10 +687,11 @@ static int adv7180_resume(struct device *dev) >> struct adv7180_state *state = to_state(sd); >> int ret; >> >> - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, >> - ADV7180_PWR_MAN_ON); >> - if (ret < 0) >> - return ret; >> + if (state->powered) { >> + ret = adv7180_set_power(state, client, true); >> + if (ret) >> + return ret; >> + } >> ret = init_device(client, state); >> if (ret < 0) >> return ret; >> > > What is the initial state of the driver when loaded? Shouldn't probe() set the > 'powered' variable to true initially? Yep, st->powered should be set to true by default. What's your process in general, want me to resend the whole series, or are you going to apply the patches that are ok? Thanks for the quick review, - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2014 04:24 PM, Lars-Peter Clausen wrote: > On 03/10/2014 03:37 PM, Hans Verkuil wrote: > [...] >>> + >>> +static int adv7180_s_power(struct v4l2_subdev *sd, int on) >>> +{ >>> + struct adv7180_state *state = to_state(sd); >>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> + int ret; >>> + >>> + ret = mutex_lock_interruptible(&state->mutex); >>> + if (ret) >>> + return ret; >>> + >>> + ret = adv7180_set_power(state, client, on); >>> + if (ret) >>> + goto out; >>> + >>> + state->powered = on; >>> +out: >> >> I would change this to: >> >> if (!ret) >> state->powered = on; >> >> and drop the 'goto'. >> > > ok. > > [...] >>> static int adv7180_resume(struct device *dev) >>> @@ -656,10 +687,11 @@ static int adv7180_resume(struct device *dev) >>> struct adv7180_state *state = to_state(sd); >>> int ret; >>> >>> - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, >>> - ADV7180_PWR_MAN_ON); >>> - if (ret < 0) >>> - return ret; >>> + if (state->powered) { >>> + ret = adv7180_set_power(state, client, true); >>> + if (ret) >>> + return ret; >>> + } >>> ret = init_device(client, state); >>> if (ret < 0) >>> return ret; >>> >> >> What is the initial state of the driver when loaded? Shouldn't probe() set the >> 'powered' variable to true initially? > > Yep, st->powered should be set to true by default. > > What's your process in general, want me to resend the whole series, or are > you going to apply the patches that are ok? When do you think you can have a fixed version of this patch ready? If it is today/tomorrow, then I'll wait for that, otherwise I'll make a pull request for the other 6 patches (which look fine). In any case there is no need to resend the whole series. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/2014 04:28 PM, Hans Verkuil wrote: > On 03/10/2014 04:24 PM, Lars-Peter Clausen wrote: >> On 03/10/2014 03:37 PM, Hans Verkuil wrote: >> [...] >>>> + >>>> +static int adv7180_s_power(struct v4l2_subdev *sd, int on) >>>> +{ >>>> + struct adv7180_state *state = to_state(sd); >>>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>>> + int ret; >>>> + >>>> + ret = mutex_lock_interruptible(&state->mutex); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = adv7180_set_power(state, client, on); >>>> + if (ret) >>>> + goto out; >>>> + >>>> + state->powered = on; >>>> +out: >>> >>> I would change this to: >>> >>> if (!ret) >>> state->powered = on; >>> >>> and drop the 'goto'. >>> >> >> ok. >> >> [...] >>>> static int adv7180_resume(struct device *dev) >>>> @@ -656,10 +687,11 @@ static int adv7180_resume(struct device *dev) >>>> struct adv7180_state *state = to_state(sd); >>>> int ret; >>>> >>>> - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, >>>> - ADV7180_PWR_MAN_ON); >>>> - if (ret < 0) >>>> - return ret; >>>> + if (state->powered) { >>>> + ret = adv7180_set_power(state, client, true); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> ret = init_device(client, state); >>>> if (ret < 0) >>>> return ret; >>>> >>> >>> What is the initial state of the driver when loaded? Shouldn't probe() set the >>> 'powered' variable to true initially? >> >> Yep, st->powered should be set to true by default. >> >> What's your process in general, want me to resend the whole series, or are >> you going to apply the patches that are ok? > > When do you think you can have a fixed version of this patch ready? If it is > today/tomorrow, then I'll wait for that, otherwise I'll make a pull request > for the other 6 patches (which look fine). I already have the updated version of the patch, will send it out soon. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c index 623cec5..8271362 100644 --- a/drivers/media/i2c/adv7180.c +++ b/drivers/media/i2c/adv7180.c @@ -127,6 +127,7 @@ struct adv7180_state { int irq; v4l2_std_id curr_norm; bool autodetect; + bool powered; u8 input; }; #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler, \ @@ -311,6 +312,39 @@ out: return ret; } +static int adv7180_set_power(struct adv7180_state *state, + struct i2c_client *client, bool on) +{ + u8 val; + + if (on) + val = ADV7180_PWR_MAN_ON; + else + val = ADV7180_PWR_MAN_OFF; + + return i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, val); +} + +static int adv7180_s_power(struct v4l2_subdev *sd, int on) +{ + struct adv7180_state *state = to_state(sd); + struct i2c_client *client = v4l2_get_subdevdata(sd); + int ret; + + ret = mutex_lock_interruptible(&state->mutex); + if (ret) + return ret; + + ret = adv7180_set_power(state, client, on); + if (ret) + goto out; + + state->powered = on; +out: + mutex_unlock(&state->mutex); + return ret; +} + static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl) { struct v4l2_subdev *sd = to_adv7180_sd(ctrl); @@ -441,6 +475,7 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = { static const struct v4l2_subdev_core_ops adv7180_core_ops = { .s_std = adv7180_s_std, + .s_power = adv7180_s_power, }; static const struct v4l2_subdev_ops adv7180_ops = { @@ -640,13 +675,9 @@ static const struct i2c_device_id adv7180_id[] = { static int adv7180_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); - int ret; + struct adv7180_state *state = to_state(sd); - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, - ADV7180_PWR_MAN_OFF); - if (ret < 0) - return ret; - return 0; + return adv7180_set_power(state, client, false); } static int adv7180_resume(struct device *dev) @@ -656,10 +687,11 @@ static int adv7180_resume(struct device *dev) struct adv7180_state *state = to_state(sd); int ret; - ret = i2c_smbus_write_byte_data(client, ADV7180_PWR_MAN_REG, - ADV7180_PWR_MAN_ON); - if (ret < 0) - return ret; + if (state->powered) { + ret = adv7180_set_power(state, client, true); + if (ret) + return ret; + } ret = init_device(client, state); if (ret < 0) return ret;
The adv7180 has a low power mode in which the analog and the digital processing section are shut down. Implement the s_power callback to let bridge drivers put the part into low power mode when not needed. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/media/i2c/adv7180.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)