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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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