[PATCH 1/7] [media] adv7180: Fix remove order

[PATCH 1/7] [media] adv7180: Fix remove order

[Lars-Peter Clausen]

The mutex is used in the subdev callbacks, so unregister the subdev before the
mutex is destroyed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index d7d99f1..1a3622a 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -616,8 +616,8 @@ static int adv7180_probe(struct i2c_client *client,
 err_free_ctrl:
 	adv7180_exit_controls(state);
 err_unreg_subdev:
-	mutex_destroy(&state->mutex);
 	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&state->mutex);
 err:
 	printk(KERN_ERR KBUILD_MODNAME ": Failed to probe: %d\n", ret);
 	return ret;
@@ -640,8 +640,8 @@ static int adv7180_remove(struct i2c_client *client)
 		}
 	}
 
-	mutex_destroy(&state->mutex);
 	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&state->mutex);
 	return 0;
 }
 
-- 
1.8.0

[PATCH 2/7] [media] adv7180: Free control handler on remove()

[Lars-Peter Clausen]

Make sure to free the control handler when the device is removed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 1a3622a..2359fd8 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -641,6 +641,7 @@ static int adv7180_remove(struct i2c_client *client)
 	}
 
 	v4l2_device_unregister_subdev(sd);
+	adv7180_exit_controls(state);
 	mutex_destroy(&state->mutex);
 	return 0;
 }
-- 
1.8.0

[PATCH 3/7] [media] adv7180: Remove unnecessary v4l2_device_unregister_subdev() from probe error path

[Lars-Peter Clausen]

The device can't possibly be registered at this point, so no need to to call
v4l2_device_unregister_subdev().

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 2359fd8..85cb4e9 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -616,7 +616,6 @@ static int adv7180_probe(struct i2c_client *client,
 err_free_ctrl:
 	adv7180_exit_controls(state);
 err_unreg_subdev:
-	v4l2_device_unregister_subdev(sd);
 	mutex_destroy(&state->mutex);
 err:
 	printk(KERN_ERR KBUILD_MODNAME ": Failed to probe: %d\n", ret);
-- 
1.8.0

[PATCH 4/7] [media] adv7180: Remove duplicated probe error message

[Lars-Peter Clausen]

The device driver core already prints out a very similar message when a driver
fails to probe. No need to print one in the driver itself.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 85cb4e9..98a3ff1 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -618,7 +618,6 @@ err_free_ctrl:
 err_unreg_subdev:
 	mutex_destroy(&state->mutex);
 err:
-	printk(KERN_ERR KBUILD_MODNAME ": Failed to probe: %d\n", ret);
 	return ret;
 }
 
-- 
1.8.0

[PATCH 5/7] [media] adv7180: Use threaded IRQ instead of IRQ + workqueue

[Lars-Peter Clausen]

The proper way to handle IRQs that need to be able to sleep in their IRQ handler
is to use a threaded IRQ.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 33 +++++----------------------------
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 98a3ff1..c750aae 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -123,7 +123,6 @@
 struct adv7180_state {
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct v4l2_subdev	sd;
-	struct work_struct	work;
 	struct mutex		mutex; /* mutual excl. when accessing chip */
 	int			irq;
 	v4l2_std_id		curr_norm;
@@ -449,10 +448,9 @@ static const struct v4l2_subdev_ops adv7180_ops = {
 	.video = &adv7180_video_ops,
 };
 
-static void adv7180_work(struct work_struct *work)
+static irqreturn_t adv7180_irq(int irq, void *devid)
 {
-	struct adv7180_state *state = container_of(work, struct adv7180_state,
-						   work);
+	struct adv7180_state *state = devid;
 	struct i2c_client *client = v4l2_get_subdevdata(&state->sd);
 	u8 isr3;
 
@@ -468,17 +466,6 @@ static void adv7180_work(struct work_struct *work)
 		__adv7180_status(client, NULL, &state->curr_norm);
 	mutex_unlock(&state->mutex);
 
-	enable_irq(state->irq);
-}
-
-static irqreturn_t adv7180_irq(int irq, void *devid)
-{
-	struct adv7180_state *state = devid;
-
-	schedule_work(&state->work);
-
-	disable_irq_nosync(state->irq);
-
 	return IRQ_HANDLED;
 }
 
@@ -533,8 +520,8 @@ static int init_device(struct i2c_client *client, struct adv7180_state *state)
 
 	/* register for interrupts */
 	if (state->irq > 0) {
-		ret = request_irq(state->irq, adv7180_irq, 0, KBUILD_MODNAME,
-				  state);
+		ret = request_threaded_irq(state->irq, NULL, adv7180_irq,
+					   IRQF_ONESHOT, KBUILD_MODNAME, state);
 		if (ret)
 			return ret;
 
@@ -598,7 +585,6 @@ static int adv7180_probe(struct i2c_client *client,
 	}
 
 	state->irq = client->irq;
-	INIT_WORK(&state->work, adv7180_work);
 	mutex_init(&state->mutex);
 	state->autodetect = true;
 	state->input = 0;
@@ -626,17 +612,8 @@ static int adv7180_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct adv7180_state *state = to_state(sd);
 
-	if (state->irq > 0) {
+	if (state->irq > 0)
 		free_irq(client->irq, state);
-		if (cancel_work_sync(&state->work)) {
-			/*
-			 * Work was pending, therefore we need to enable
-			 * IRQ here to balance the disable_irq() done in the
-			 * interrupt handler.
-			 */
-			enable_irq(state->irq);
-		}
-	}
 
 	v4l2_device_unregister_subdev(sd);
 	adv7180_exit_controls(state);
-- 
1.8.0

[PATCH 6/7] [media] adv7180: Add support for async device registration

[Lars-Peter Clausen]

Add support for async device registration to the adv7180 driver.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index c750aae..623cec5 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -597,8 +597,16 @@ static int adv7180_probe(struct i2c_client *client,
 	ret = init_device(client, state);
 	if (ret)
 		goto err_free_ctrl;
+
+	ret = v4l2_async_register_subdev(sd);
+	if (ret)
+		goto err_free_irq;
+
 	return 0;
 
+err_free_irq:
+	if (state->irq > 0)
+		free_irq(client->irq, state);
 err_free_ctrl:
 	adv7180_exit_controls(state);
 err_unreg_subdev:
@@ -612,6 +620,8 @@ static int adv7180_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct adv7180_state *state = to_state(sd);
 
+	v4l2_async_unregister_subdev(sd);
+
 	if (state->irq > 0)
 		free_irq(client->irq, state);
 
-- 
1.8.0

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

[Lars-Peter Clausen]

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:
+	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;
-- 
1.8.0

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

[Hans Verkuil]

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

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

[Lars-Peter Clausen]

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

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

[Hans Verkuil]

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

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

[Lars-Peter Clausen]

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