Re: [PATCH 7/7] [media] adv7180: Add support for power down

[Lars-Peter Clausen <lars@metafoo.de>]

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