1. Patchew
  2. linux
  3. View series

[PATCH] media: dw9714: add support for powerdown pin

Matthias Fend posted 1 patch 4 months ago 
drivers/media/i2c/Kconfig  |  2 +-
drivers/media/i2c/dw9714.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
[PATCH] media: dw9714: add support for powerdown pin
Posted by Matthias Fend 4 months ago 
Add support for the powerdown pin (xSD), which can be used to put the VCM
driver into power down mode. This is useful, for example, if the VCM
driver's power supply cannot be controlled.
The use of the powerdown pin is optional.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/dw9714.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index e45ba127069fc0848f1a06ceb789efd3c222c008..e923daeec9c574c5b8c7014b9e83fcbad47866c0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -748,7 +748,7 @@ config VIDEO_AK7375
 
 config VIDEO_DW9714
 	tristate "DW9714 lens voice coil support"
-	depends on I2C && VIDEO_DEV
+	depends on GPIOLIB && I2C && VIDEO_DEV
 	select MEDIA_CONTROLLER
 	select VIDEO_V4L2_SUBDEV_API
 	select V4L2_ASYNC
diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
index 2ddd7daa79e28a2cde915b4173fa27e60d5a2b57..5b78c1848f80bc3e32df13d149f3865ff8defe6e 100644
--- a/drivers/media/i2c/dw9714.c
+++ b/drivers/media/i2c/dw9714.c
@@ -2,6 +2,7 @@
 // Copyright (c) 2015--2017 Intel Corporation.
 
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -38,6 +39,7 @@ struct dw9714_device {
 	struct v4l2_subdev sd;
 	u16 current_val;
 	struct regulator *vcc;
+	struct gpio_desc *powerdown_gpio;
 };
 
 static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
@@ -151,11 +153,20 @@ static int dw9714_probe(struct i2c_client *client)
 	if (IS_ERR(dw9714_dev->vcc))
 		return PTR_ERR(dw9714_dev->vcc);
 
+	dw9714_dev->powerdown_gpio = devm_gpiod_get_optional(&client->dev,
+							     "powerdown",
+							     GPIOD_OUT_LOW);
+	if (IS_ERR(dw9714_dev->powerdown_gpio))
+		return dev_err_probe(&client->dev,
+				     PTR_ERR(dw9714_dev->powerdown_gpio),
+				     "could not get powerdown gpio\n");
+
 	rval = regulator_enable(dw9714_dev->vcc);
 	if (rval < 0) {
 		dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
 		return rval;
 	}
+	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
 
 	usleep_range(1000, 2000);
 
@@ -185,6 +196,7 @@ static int dw9714_probe(struct i2c_client *client)
 	return 0;
 
 err_cleanup:
+	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
 	regulator_disable(dw9714_dev->vcc);
 	v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
 	media_entity_cleanup(&dw9714_dev->sd.entity);
@@ -200,6 +212,7 @@ static void dw9714_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	if (!pm_runtime_status_suspended(&client->dev)) {
+		gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
 		ret = regulator_disable(dw9714_dev->vcc);
 		if (ret) {
 			dev_err(&client->dev,
@@ -234,6 +247,7 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
 		usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
 	}
 
+	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
 	ret = regulator_disable(dw9714_dev->vcc);
 	if (ret)
 		dev_err(dev, "Failed to disable vcc: %d\n", ret);
@@ -262,6 +276,8 @@ static int  __maybe_unused dw9714_vcm_resume(struct device *dev)
 		dev_err(dev, "Failed to enable vcc: %d\n", ret);
 		return ret;
 	}
+	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
+
 	usleep_range(1000, 2000);
 
 	for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;

---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250611-dw9714-sd-fba5a354f936

Best regards,
-- 
Matthias Fend <matthias.fend@emfend.at>
Re: [PATCH] media: dw9714: add support for powerdown pin
Posted by Sakari Ailus 4 months ago 
Hi Matthias,

Thanks for the patch.

On Wed, Jun 11, 2025 at 09:13:33AM +0200, Matthias Fend wrote:
> Add support for the powerdown pin (xSD), which can be used to put the VCM
> driver into power down mode. This is useful, for example, if the VCM
> driver's power supply cannot be controlled.
> The use of the powerdown pin is optional.

Please rewrap. Most editors can do it for you.

> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  drivers/media/i2c/Kconfig  |  2 +-
>  drivers/media/i2c/dw9714.c | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e45ba127069fc0848f1a06ceb789efd3c222c008..e923daeec9c574c5b8c7014b9e83fcbad47866c0 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -748,7 +748,7 @@ config VIDEO_AK7375
>  
>  config VIDEO_DW9714
>  	tristate "DW9714 lens voice coil support"
> -	depends on I2C && VIDEO_DEV
> +	depends on GPIOLIB && I2C && VIDEO_DEV
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select V4L2_ASYNC
> diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> index 2ddd7daa79e28a2cde915b4173fa27e60d5a2b57..5b78c1848f80bc3e32df13d149f3865ff8defe6e 100644
> --- a/drivers/media/i2c/dw9714.c
> +++ b/drivers/media/i2c/dw9714.c
> @@ -2,6 +2,7 @@
>  // Copyright (c) 2015--2017 Intel Corporation.
>  
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> @@ -38,6 +39,7 @@ struct dw9714_device {
>  	struct v4l2_subdev sd;
>  	u16 current_val;
>  	struct regulator *vcc;
> +	struct gpio_desc *powerdown_gpio;
>  };
>  
>  static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
> @@ -151,11 +153,20 @@ static int dw9714_probe(struct i2c_client *client)
>  	if (IS_ERR(dw9714_dev->vcc))
>  		return PTR_ERR(dw9714_dev->vcc);
>  
> +	dw9714_dev->powerdown_gpio = devm_gpiod_get_optional(&client->dev,
> +							     "powerdown",
> +							     GPIOD_OUT_LOW);
> +	if (IS_ERR(dw9714_dev->powerdown_gpio))
> +		return dev_err_probe(&client->dev,
> +				     PTR_ERR(dw9714_dev->powerdown_gpio),
> +				     "could not get powerdown gpio\n");
> +
>  	rval = regulator_enable(dw9714_dev->vcc);
>  	if (rval < 0) {
>  		dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
>  		return rval;
>  	}
> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);

This seems to be redundant, you're already setting the GPIO low when
acquiring it. Typically the order is different, though: the regulator is
enabled first. Also related to the following comment.

>  
>  	usleep_range(1000, 2000);
>  
> @@ -185,6 +196,7 @@ static int dw9714_probe(struct i2c_client *client)
>  	return 0;
>  
>  err_cleanup:
> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>  	regulator_disable(dw9714_dev->vcc);

It'd be nice to have a single implementation of the power-on and power-off
sequences. Now there are two.

>  	v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
>  	media_entity_cleanup(&dw9714_dev->sd.entity);
> @@ -200,6 +212,7 @@ static void dw9714_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	if (!pm_runtime_status_suspended(&client->dev)) {
> +		gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>  		ret = regulator_disable(dw9714_dev->vcc);
>  		if (ret) {
>  			dev_err(&client->dev,
> @@ -234,6 +247,7 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
>  		usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>  	}
>  
> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>  	ret = regulator_disable(dw9714_dev->vcc);
>  	if (ret)
>  		dev_err(dev, "Failed to disable vcc: %d\n", ret);
> @@ -262,6 +276,8 @@ static int  __maybe_unused dw9714_vcm_resume(struct device *dev)
>  		dev_err(dev, "Failed to enable vcc: %d\n", ret);
>  		return ret;
>  	}
> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
> +
>  	usleep_range(1000, 2000);
>  
>  	for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
> 

-- 
Regards,

Sakari Ailus
Re: [PATCH] media: dw9714: add support for powerdown pin
Posted by Matthias Fend 4 months ago 
Hi Sakari,

thanks for your comments.

Am 11.06.2025 um 11:33 schrieb Sakari Ailus:
> Hi Matthias,
> 
> Thanks for the patch.
> 
> On Wed, Jun 11, 2025 at 09:13:33AM +0200, Matthias Fend wrote:
>> Add support for the powerdown pin (xSD), which can be used to put the VCM
>> driver into power down mode. This is useful, for example, if the VCM
>> driver's power supply cannot be controlled.
>> The use of the powerdown pin is optional.
> 
> Please rewrap. Most editors can do it for you.

To me the message body looks line wrapped at 75 columns. The last 
sentence on a new line. At least that is what was intended.
So I think I'm missing something here. Can you please tell me what 
exactly I should change?

> 
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   drivers/media/i2c/Kconfig  |  2 +-
>>   drivers/media/i2c/dw9714.c | 16 ++++++++++++++++
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index e45ba127069fc0848f1a06ceb789efd3c222c008..e923daeec9c574c5b8c7014b9e83fcbad47866c0 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -748,7 +748,7 @@ config VIDEO_AK7375
>>   
>>   config VIDEO_DW9714
>>   	tristate "DW9714 lens voice coil support"
>> -	depends on I2C && VIDEO_DEV
>> +	depends on GPIOLIB && I2C && VIDEO_DEV
>>   	select MEDIA_CONTROLLER
>>   	select VIDEO_V4L2_SUBDEV_API
>>   	select V4L2_ASYNC
>> diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
>> index 2ddd7daa79e28a2cde915b4173fa27e60d5a2b57..5b78c1848f80bc3e32df13d149f3865ff8defe6e 100644
>> --- a/drivers/media/i2c/dw9714.c
>> +++ b/drivers/media/i2c/dw9714.c
>> @@ -2,6 +2,7 @@
>>   // Copyright (c) 2015--2017 Intel Corporation.
>>   
>>   #include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/i2c.h>
>>   #include <linux/module.h>
>>   #include <linux/pm_runtime.h>
>> @@ -38,6 +39,7 @@ struct dw9714_device {
>>   	struct v4l2_subdev sd;
>>   	u16 current_val;
>>   	struct regulator *vcc;
>> +	struct gpio_desc *powerdown_gpio;
>>   };
>>   
>>   static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
>> @@ -151,11 +153,20 @@ static int dw9714_probe(struct i2c_client *client)
>>   	if (IS_ERR(dw9714_dev->vcc))
>>   		return PTR_ERR(dw9714_dev->vcc);
>>   
>> +	dw9714_dev->powerdown_gpio = devm_gpiod_get_optional(&client->dev,
>> +							     "powerdown",
>> +							     GPIOD_OUT_LOW);
>> +	if (IS_ERR(dw9714_dev->powerdown_gpio))
>> +		return dev_err_probe(&client->dev,
>> +				     PTR_ERR(dw9714_dev->powerdown_gpio),
>> +				     "could not get powerdown gpio\n");
>> +
>>   	rval = regulator_enable(dw9714_dev->vcc);
>>   	if (rval < 0) {
>>   		dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
>>   		return rval;
>>   	}
>> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
> 
> This seems to be redundant, you're already setting the GPIO low when
> acquiring it. Typically the order is different, though: the regulator is
> enabled first. Also related to the following comment.

Yes, that's right. I'll set the pin during initialization so that 
power-down mode is active. So at least the sequence is always the same.

> 
>>   
>>   	usleep_range(1000, 2000);
>>   
>> @@ -185,6 +196,7 @@ static int dw9714_probe(struct i2c_client *client)
>>   	return 0;
>>   
>>   err_cleanup:
>> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>>   	regulator_disable(dw9714_dev->vcc);
> 
> It'd be nice to have a single implementation of the power-on and power-off
> sequences. Now there are two.

Okay, will do so.

Thanks,
  ~Matthias

> 
>>   	v4l2_ctrl_handler_free(&dw9714_dev->ctrls_vcm);
>>   	media_entity_cleanup(&dw9714_dev->sd.entity);
>> @@ -200,6 +212,7 @@ static void dw9714_remove(struct i2c_client *client)
>>   
>>   	pm_runtime_disable(&client->dev);
>>   	if (!pm_runtime_status_suspended(&client->dev)) {
>> +		gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>>   		ret = regulator_disable(dw9714_dev->vcc);
>>   		if (ret) {
>>   			dev_err(&client->dev,
>> @@ -234,6 +247,7 @@ static int __maybe_unused dw9714_vcm_suspend(struct device *dev)
>>   		usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US + 10);
>>   	}
>>   
>> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
>>   	ret = regulator_disable(dw9714_dev->vcc);
>>   	if (ret)
>>   		dev_err(dev, "Failed to disable vcc: %d\n", ret);
>> @@ -262,6 +276,8 @@ static int  __maybe_unused dw9714_vcm_resume(struct device *dev)
>>   		dev_err(dev, "Failed to enable vcc: %d\n", ret);
>>   		return ret;
>>   	}
>> +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
>> +
>>   	usleep_range(1000, 2000);
>>   
>>   	for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS;
>>
>
Re: [PATCH] media: dw9714: add support for powerdown pin
Posted by Sakari Ailus 4 months ago 
Hi Matthias,

On Wed, Jun 11, 2025 at 02:15:10PM +0200, Matthias Fend wrote:
> Hi Sakari,
> 
> thanks for your comments.
> 
> Am 11.06.2025 um 11:33 schrieb Sakari Ailus:
> > Hi Matthias,
> > 
> > Thanks for the patch.
> > 
> > On Wed, Jun 11, 2025 at 09:13:33AM +0200, Matthias Fend wrote:
> > > Add support for the powerdown pin (xSD), which can be used to put the VCM
> > > driver into power down mode. This is useful, for example, if the VCM
> > > driver's power supply cannot be controlled.
> > > The use of the powerdown pin is optional.
> > 
> > Please rewrap. Most editors can do it for you.
> 
> To me the message body looks line wrapped at 75 columns. The last sentence
> on a new line. At least that is what was intended.
> So I think I'm missing something here. Can you please tell me what exactly I
> should change?

It should look like this (i.e. the lines shouldn't be longer than 75 but
also as long as that rule allows):

Add support for the powerdown pin (xSD), which can be used to put the VCM
driver into power down mode. This is useful, for example, if the VCM
driver's power supply cannot be controlled. The use of the powerdown pin is
optional.

> 
> > 
> > > 
> > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > ---
> > >   drivers/media/i2c/Kconfig  |  2 +-
> > >   drivers/media/i2c/dw9714.c | 16 ++++++++++++++++
> > >   2 files changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index e45ba127069fc0848f1a06ceb789efd3c222c008..e923daeec9c574c5b8c7014b9e83fcbad47866c0 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -748,7 +748,7 @@ config VIDEO_AK7375
> > >   config VIDEO_DW9714
> > >   	tristate "DW9714 lens voice coil support"
> > > -	depends on I2C && VIDEO_DEV
> > > +	depends on GPIOLIB && I2C && VIDEO_DEV
> > >   	select MEDIA_CONTROLLER
> > >   	select VIDEO_V4L2_SUBDEV_API
> > >   	select V4L2_ASYNC
> > > diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> > > index 2ddd7daa79e28a2cde915b4173fa27e60d5a2b57..5b78c1848f80bc3e32df13d149f3865ff8defe6e 100644
> > > --- a/drivers/media/i2c/dw9714.c
> > > +++ b/drivers/media/i2c/dw9714.c
> > > @@ -2,6 +2,7 @@
> > >   // Copyright (c) 2015--2017 Intel Corporation.
> > >   #include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > >   #include <linux/i2c.h>
> > >   #include <linux/module.h>
> > >   #include <linux/pm_runtime.h>
> > > @@ -38,6 +39,7 @@ struct dw9714_device {
> > >   	struct v4l2_subdev sd;
> > >   	u16 current_val;
> > >   	struct regulator *vcc;
> > > +	struct gpio_desc *powerdown_gpio;
> > >   };
> > >   static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
> > > @@ -151,11 +153,20 @@ static int dw9714_probe(struct i2c_client *client)
> > >   	if (IS_ERR(dw9714_dev->vcc))
> > >   		return PTR_ERR(dw9714_dev->vcc);
> > > +	dw9714_dev->powerdown_gpio = devm_gpiod_get_optional(&client->dev,
> > > +							     "powerdown",
> > > +							     GPIOD_OUT_LOW);
> > > +	if (IS_ERR(dw9714_dev->powerdown_gpio))
> > > +		return dev_err_probe(&client->dev,
> > > +				     PTR_ERR(dw9714_dev->powerdown_gpio),
> > > +				     "could not get powerdown gpio\n");
> > > +
> > >   	rval = regulator_enable(dw9714_dev->vcc);
> > >   	if (rval < 0) {
> > >   		dev_err(&client->dev, "failed to enable vcc: %d\n", rval);
> > >   		return rval;
> > >   	}
> > > +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 0);
> > 
> > This seems to be redundant, you're already setting the GPIO low when
> > acquiring it. Typically the order is different, though: the regulator is
> > enabled first. Also related to the following comment.
> 
> Yes, that's right. I'll set the pin during initialization so that power-down
> mode is active. So at least the sequence is always the same.
> 
> > 
> > >   	usleep_range(1000, 2000);
> > > @@ -185,6 +196,7 @@ static int dw9714_probe(struct i2c_client *client)
> > >   	return 0;
> > >   err_cleanup:
> > > +	gpiod_set_value_cansleep(dw9714_dev->powerdown_gpio, 1);
> > >   	regulator_disable(dw9714_dev->vcc);
> > 
> > It'd be nice to have a single implementation of the power-on and power-off
> > sequences. Now there are two.
> 
> Okay, will do so.

Please add a separate patch for this, before this one.

-- 
Regards,

Sakari Ailus

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.