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

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

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