[PATCH 0/2] media: ov2740: Add support for reset GPIO and external clock

[PATCH 0/2] media: ov2740: Add support for reset GPIO and external clock

[Hans de Goede]

Hi All,

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various ThinkPad models with
IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
and the sensor's clock itself.

This series adds support for having the driver control
an optional reset GPIO and an optional external clock.

Regards,

Hans


Hans de Goede (2):
  media: ov2740: Add support for reset GPIO
  media: ov2740: Add support for external clock

 drivers/media/i2c/ov2740.c | 60 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH 1/2] media: ov2740: Add support for reset GPIO

[Hans de Goede]

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various ThinkPad models with
IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
and the sensor's clock itself.

Add support for having the driver control an optional reset GPIO.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2740.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 24e468485fbf..e5f9569a229d 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -4,6 +4,7 @@
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -333,6 +334,9 @@ struct ov2740 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
+	/* GPIOs, clocks */
+	struct gpio_desc *reset_gpio;
+
 	/* Current mode */
 	const struct ov2740_mode *cur_mode;
 
@@ -1058,6 +1062,26 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 	return 0;
 }
 
+static int ov2740_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov2740 *ov2740 = to_ov2740(sd);
+
+	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
+	return 0;
+}
+
+static int ov2740_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov2740 *ov2740 = to_ov2740(sd);
+
+	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
+	msleep(20);
+
+	return 0;
+}
+
 static int ov2740_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1073,12 +1097,24 @@ static int ov2740_probe(struct i2c_client *client)
 	if (!ov2740)
 		return -ENOMEM;
 
+	ov2740->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ov2740->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
+				     "failed to get reset GPIO\n");
+
 	v4l2_i2c_subdev_init(&ov2740->sd, client, &ov2740_subdev_ops);
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
-		ret = ov2740_identify_module(ov2740);
+		/* ACPI does not always clear the reset GPIO / enable the clock */
+		ret = ov2740_resume(dev);
 		if (ret)
-			return dev_err_probe(dev, ret, "failed to find sensor\n");
+			return dev_err_probe(dev, ret, "failed to power on sensor\n");
+
+		ret = ov2740_identify_module(ov2740);
+		if (ret) {
+			dev_err_probe(dev, ret, "failed to find sensor\n");
+			goto probe_error_power_off;
+		}
 	}
 
 	ov2740->cur_mode = &supported_modes[0];
@@ -1132,9 +1168,16 @@ static int ov2740_probe(struct i2c_client *client)
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
 
+probe_error_power_off:
+	if (full_power)
+		ov2740_suspend(dev);
+
 	return ret;
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(ov2740_pm_ops, ov2740_suspend, ov2740_resume,
+				 NULL);
+
 static const struct acpi_device_id ov2740_acpi_ids[] = {
 	{"INT3474"},
 	{}
@@ -1146,6 +1189,7 @@ static struct i2c_driver ov2740_i2c_driver = {
 	.driver = {
 		.name = "ov2740",
 		.acpi_match_table = ov2740_acpi_ids,
+		.pm = pm_sleep_ptr(&ov2740_pm_ops),
 	},
 	.probe = ov2740_probe,
 	.remove = ov2740_remove,
-- 
2.41.0

Re: [PATCH 1/2] media: ov2740: Add support for reset GPIO

[Bingbu Cao]

Hans,

Thanks for your patch.

On 11/15/23 8:38 PM, Hans de Goede wrote:
> On some ACPI platforms, such as Chromebooks the ACPI methods to
> change the power-state (_PS0 and _PS3) fully take care of powering
> on/off the sensor.
> 
> On other ACPI platforms, such as e.g. various ThinkPad models with
> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
> and the sensor's clock itself.
> 
> Add support for having the driver control an optional reset GPIO.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2740.c | 48 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 24e468485fbf..e5f9569a229d 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -4,6 +4,7 @@
>  #include <asm/unaligned.h>
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> @@ -333,6 +334,9 @@ struct ov2740 {
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *exposure;
>  
> +	/* GPIOs, clocks */

It looks like the 'clock' should be in another one (2/2), :).

> +	struct gpio_desc *reset_gpio;
> +
>  	/* Current mode */
>  	const struct ov2740_mode *cur_mode;
>  
> @@ -1058,6 +1062,26 @@ static int ov2740_register_nvmem(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int ov2740_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov2740 *ov2740 = to_ov2740(sd);
> +
> +	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
> +	return 0;
> +}
> +
> +static int ov2740_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov2740 *ov2740 = to_ov2740(sd);
> +
> +	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
> +	msleep(20);

I remember that usleep_range() is prefered for <=20ms.

> +
> +	return 0;
> +}
> +
>  static int ov2740_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1073,12 +1097,24 @@ static int ov2740_probe(struct i2c_client *client)
>  	if (!ov2740)
>  		return -ENOMEM;
>  
> +	ov2740->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov2740->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
> +				     "failed to get reset GPIO\n");
> +
>  	v4l2_i2c_subdev_init(&ov2740->sd, client, &ov2740_subdev_ops);
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> -		ret = ov2740_identify_module(ov2740);
> +		/* ACPI does not always clear the reset GPIO / enable the clock */
> +		ret = ov2740_resume(dev);
>  		if (ret)
> -			return dev_err_probe(dev, ret, "failed to find sensor\n");
> +			return dev_err_probe(dev, ret, "failed to power on sensor\n");
> +
> +		ret = ov2740_identify_module(ov2740);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "failed to find sensor\n");
> +			goto probe_error_power_off;
> +		}
>  	}
>  
>  	ov2740->cur_mode = &supported_modes[0];
> @@ -1132,9 +1168,16 @@ static int ov2740_probe(struct i2c_client *client)
>  probe_error_v4l2_ctrl_handler_free:
>  	v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
>  
> +probe_error_power_off:
> +	if (full_power)
> +		ov2740_suspend(dev);
> +
>  	return ret;
>  }
>  
> +static DEFINE_RUNTIME_DEV_PM_OPS(ov2740_pm_ops, ov2740_suspend, ov2740_resume,
> +				 NULL);
> +
>  static const struct acpi_device_id ov2740_acpi_ids[] = {
>  	{"INT3474"},
>  	{}
> @@ -1146,6 +1189,7 @@ static struct i2c_driver ov2740_i2c_driver = {
>  	.driver = {
>  		.name = "ov2740",
>  		.acpi_match_table = ov2740_acpi_ids,
> +		.pm = pm_sleep_ptr(&ov2740_pm_ops),
>  	},
>  	.probe = ov2740_probe,
>  	.remove = ov2740_remove,
>

Except the minor comment.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>




-- 
Best regards,
Bingbu Cao

Re: [PATCH 1/2] media: ov2740: Add support for reset GPIO

[Hans de Goede]

Hi Bingbu,

Thank you for the review!

On 11/20/23 05:04, Bingbu Cao wrote:
> 
> Hans,
> 
> Thanks for your patch.
> 
> On 11/15/23 8:38 PM, Hans de Goede wrote:
>> On some ACPI platforms, such as Chromebooks the ACPI methods to
>> change the power-state (_PS0 and _PS3) fully take care of powering
>> on/off the sensor.
>>
>> On other ACPI platforms, such as e.g. various ThinkPad models with
>> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
>> and the sensor's clock itself.
>>
>> Add support for having the driver control an optional reset GPIO.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2740.c | 48 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>> index 24e468485fbf..e5f9569a229d 100644
>> --- a/drivers/media/i2c/ov2740.c
>> +++ b/drivers/media/i2c/ov2740.c
>> @@ -4,6 +4,7 @@
>>  #include <asm/unaligned.h>
>>  #include <linux/acpi.h>
>>  #include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/pm_runtime.h>
>> @@ -333,6 +334,9 @@ struct ov2740 {
>>  	struct v4l2_ctrl *hblank;
>>  	struct v4l2_ctrl *exposure;
>>  
>> +	/* GPIOs, clocks */
> 
> It looks like the 'clock' should be in another one (2/2), :).

This was intentional to avoid churn in the form of
immediately changing the comment in the second patch :)

>> +	struct gpio_desc *reset_gpio;
>> +
>>  	/* Current mode */
>>  	const struct ov2740_mode *cur_mode;
>>  
>> @@ -1058,6 +1062,26 @@ static int ov2740_register_nvmem(struct i2c_client *client,
>>  	return 0;
>>  }
>>  
>> +static int ov2740_suspend(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov2740 *ov2740 = to_ov2740(sd);
>> +
>> +	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>> +	return 0;
>> +}
>> +
>> +static int ov2740_resume(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov2740 *ov2740 = to_ov2740(sd);
>> +
>> +	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>> +	msleep(20);
> 
> I remember that usleep_range() is prefered for <=20ms.

I think that only applies to msleep <= 10ms, at least
check-patch is happy with this and I know it complains
about too short msleep() calls.

Regards,

Hans

[PATCH 2/2] media: ov2740: Add support for external clock

[Hans de Goede]

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various ThinkPad models with
IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
and the sensor's clock itself.

Add support for having the driver control an optional clock.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2740.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index e5f9569a229d..0a87d0920eb8 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -3,6 +3,7 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -336,6 +337,7 @@ struct ov2740 {
 
 	/* GPIOs, clocks */
 	struct gpio_desc *reset_gpio;
+	struct clk *clk;
 
 	/* Current mode */
 	const struct ov2740_mode *cur_mode;
@@ -1068,6 +1070,7 @@ static int ov2740_suspend(struct device *dev)
 	struct ov2740 *ov2740 = to_ov2740(sd);
 
 	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
+	clk_disable_unprepare(ov2740->clk);
 	return 0;
 }
 
@@ -1075,6 +1078,11 @@ static int ov2740_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2740 *ov2740 = to_ov2740(sd);
+	int ret;
+
+	ret = clk_prepare_enable(ov2740->clk);
+	if (ret)
+		return ret;
 
 	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
 	msleep(20);
@@ -1102,6 +1110,10 @@ static int ov2740_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
 				     "failed to get reset GPIO\n");
 
+	ov2740->clk = devm_clk_get_optional(dev, "clk");
+	if (IS_ERR(ov2740->clk))
+		return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n");
+
 	v4l2_i2c_subdev_init(&ov2740->sd, client, &ov2740_subdev_ops);
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
-- 
2.41.0

Re: [PATCH 2/2] media: ov2740: Add support for external clock

[Bingbu Cao]

Hans,

On 11/15/23 8:38 PM, Hans de Goede wrote:
> On some ACPI platforms, such as Chromebooks the ACPI methods to
> change the power-state (_PS0 and _PS3) fully take care of powering
> on/off the sensor.
> 
> On other ACPI platforms, such as e.g. various ThinkPad models with
> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
> and the sensor's clock itself.
> 
> Add support for having the driver control an optional clock.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2740.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index e5f9569a229d..0a87d0920eb8 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -3,6 +3,7 @@
>  
>  #include <asm/unaligned.h>
>  #include <linux/acpi.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> @@ -336,6 +337,7 @@ struct ov2740 {
>  
>  	/* GPIOs, clocks */
>  	struct gpio_desc *reset_gpio;
> +	struct clk *clk;
>  
>  	/* Current mode */
>  	const struct ov2740_mode *cur_mode;
> @@ -1068,6 +1070,7 @@ static int ov2740_suspend(struct device *dev)
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
> +	clk_disable_unprepare(ov2740->clk);
>  	return 0;
>  }
>  
> @@ -1075,6 +1078,11 @@ static int ov2740_resume(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct ov2740 *ov2740 = to_ov2740(sd);
> +	int ret;
> +
> +	ret = clk_prepare_enable(ov2740->clk);
> +	if (ret)
> +		return ret;
>  
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>  	msleep(20);
> @@ -1102,6 +1110,10 @@ static int ov2740_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
>  				     "failed to get reset GPIO\n");
>  
> +	ov2740->clk = devm_clk_get_optional(dev, "clk");
> +	if (IS_ERR(ov2740->clk))
> +		return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n");
> +

I am not very sure that the 80-char rule is still valid for checkpatch.pl.


>  	v4l2_i2c_subdev_init(&ov2740->sd, client, &ov2740_subdev_ops);
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

-- 
Best regards,
Bingbu Cao

Re: [PATCH 2/2] media: ov2740: Add support for external clock

[Hans de Goede]

Hi Bingbu,

On 11/20/23 05:06, Bingbu Cao wrote:
> 
> Hans,
> 
> On 11/15/23 8:38 PM, Hans de Goede wrote:
>> On some ACPI platforms, such as Chromebooks the ACPI methods to
>> change the power-state (_PS0 and _PS3) fully take care of powering
>> on/off the sensor.
>>
>> On other ACPI platforms, such as e.g. various ThinkPad models with
>> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
>> and the sensor's clock itself.
>>
>> Add support for having the driver control an optional clock.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2740.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>> index e5f9569a229d..0a87d0920eb8 100644
>> --- a/drivers/media/i2c/ov2740.c
>> +++ b/drivers/media/i2c/ov2740.c
>> @@ -3,6 +3,7 @@
>>  
>>  #include <asm/unaligned.h>
>>  #include <linux/acpi.h>
>> +#include <linux/clk.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>> @@ -336,6 +337,7 @@ struct ov2740 {
>>  
>>  	/* GPIOs, clocks */
>>  	struct gpio_desc *reset_gpio;
>> +	struct clk *clk;
>>  
>>  	/* Current mode */
>>  	const struct ov2740_mode *cur_mode;
>> @@ -1068,6 +1070,7 @@ static int ov2740_suspend(struct device *dev)
>>  	struct ov2740 *ov2740 = to_ov2740(sd);
>>  
>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>> +	clk_disable_unprepare(ov2740->clk);
>>  	return 0;
>>  }
>>  
>> @@ -1075,6 +1078,11 @@ static int ov2740_resume(struct device *dev)
>>  {
>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>  	struct ov2740 *ov2740 = to_ov2740(sd);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(ov2740->clk);
>> +	if (ret)
>> +		return ret;
>>  
>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>>  	msleep(20);
>> @@ -1102,6 +1110,10 @@ static int ov2740_probe(struct i2c_client *client)
>>  		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
>>  				     "failed to get reset GPIO\n");
>>  
>> +	ov2740->clk = devm_clk_get_optional(dev, "clk");
>> +	if (IS_ERR(ov2740->clk))
>> +		return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n");
>> +
> 
> I am not very sure that the 80-char rule is still valid for checkpatch.pl.

checkpatch.pl defaults to allowing longer lines (<100 chars) now,
but you are right that the linux-media maintainers prefer 80.

Still there is an exception to not split strings running
over the limit and this line ends with a string,
so I think that this is fine.

Regards,

Hans

Re: [PATCH 2/2] media: ov2740: Add support for external clock

[Sakari Ailus]

Hi Hans,

On Mon, Nov 20, 2023 at 11:00:14AM +0100, Hans de Goede wrote:
> Hi Bingbu,
> 
> On 11/20/23 05:06, Bingbu Cao wrote:
> > 
> > Hans,
> > 
> > On 11/15/23 8:38 PM, Hans de Goede wrote:
> >> On some ACPI platforms, such as Chromebooks the ACPI methods to
> >> change the power-state (_PS0 and _PS3) fully take care of powering
> >> on/off the sensor.
> >>
> >> On other ACPI platforms, such as e.g. various ThinkPad models with
> >> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
> >> and the sensor's clock itself.
> >>
> >> Add support for having the driver control an optional clock.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/media/i2c/ov2740.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> >> index e5f9569a229d..0a87d0920eb8 100644
> >> --- a/drivers/media/i2c/ov2740.c
> >> +++ b/drivers/media/i2c/ov2740.c
> >> @@ -3,6 +3,7 @@
> >>  
> >>  #include <asm/unaligned.h>
> >>  #include <linux/acpi.h>
> >> +#include <linux/clk.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/gpio/consumer.h>
> >>  #include <linux/i2c.h>
> >> @@ -336,6 +337,7 @@ struct ov2740 {
> >>  
> >>  	/* GPIOs, clocks */
> >>  	struct gpio_desc *reset_gpio;
> >> +	struct clk *clk;
> >>  
> >>  	/* Current mode */
> >>  	const struct ov2740_mode *cur_mode;
> >> @@ -1068,6 +1070,7 @@ static int ov2740_suspend(struct device *dev)
> >>  	struct ov2740 *ov2740 = to_ov2740(sd);
> >>  
> >>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
> >> +	clk_disable_unprepare(ov2740->clk);
> >>  	return 0;
> >>  }
> >>  
> >> @@ -1075,6 +1078,11 @@ static int ov2740_resume(struct device *dev)
> >>  {
> >>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >>  	struct ov2740 *ov2740 = to_ov2740(sd);
> >> +	int ret;
> >> +
> >> +	ret = clk_prepare_enable(ov2740->clk);
> >> +	if (ret)
> >> +		return ret;
> >>  
> >>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
> >>  	msleep(20);
> >> @@ -1102,6 +1110,10 @@ static int ov2740_probe(struct i2c_client *client)
> >>  		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
> >>  				     "failed to get reset GPIO\n");
> >>  
> >> +	ov2740->clk = devm_clk_get_optional(dev, "clk");
> >> +	if (IS_ERR(ov2740->clk))
> >> +		return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n");
> >> +
> > 
> > I am not very sure that the 80-char rule is still valid for checkpatch.pl.
> 
> checkpatch.pl defaults to allowing longer lines (<100 chars) now,
> but you are right that the linux-media maintainers prefer 80.
> 
> Still there is an exception to not split strings running
> over the limit and this line ends with a string,
> so I think that this is fine.

The rule is not to split strings in order to satisfy alignment rules. IOW
the line should be wrapped before the string. :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: ov2740: Add support for external clock

[Hans de Goede]

Hi,

On 11/20/23 17:32, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Nov 20, 2023 at 11:00:14AM +0100, Hans de Goede wrote:
>> Hi Bingbu,
>>
>> On 11/20/23 05:06, Bingbu Cao wrote:
>>>
>>> Hans,
>>>
>>> On 11/15/23 8:38 PM, Hans de Goede wrote:
>>>> On some ACPI platforms, such as Chromebooks the ACPI methods to
>>>> change the power-state (_PS0 and _PS3) fully take care of powering
>>>> on/off the sensor.
>>>>
>>>> On other ACPI platforms, such as e.g. various ThinkPad models with
>>>> IPU6 + ov2740 sensor, the sensor driver must control the reset GPIO
>>>> and the sensor's clock itself.
>>>>
>>>> Add support for having the driver control an optional clock.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/media/i2c/ov2740.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>>>> index e5f9569a229d..0a87d0920eb8 100644
>>>> --- a/drivers/media/i2c/ov2740.c
>>>> +++ b/drivers/media/i2c/ov2740.c
>>>> @@ -3,6 +3,7 @@
>>>>  
>>>>  #include <asm/unaligned.h>
>>>>  #include <linux/acpi.h>
>>>> +#include <linux/clk.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/gpio/consumer.h>
>>>>  #include <linux/i2c.h>
>>>> @@ -336,6 +337,7 @@ struct ov2740 {
>>>>  
>>>>  	/* GPIOs, clocks */
>>>>  	struct gpio_desc *reset_gpio;
>>>> +	struct clk *clk;
>>>>  
>>>>  	/* Current mode */
>>>>  	const struct ov2740_mode *cur_mode;
>>>> @@ -1068,6 +1070,7 @@ static int ov2740_suspend(struct device *dev)
>>>>  	struct ov2740 *ov2740 = to_ov2740(sd);
>>>>  
>>>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>>>> +	clk_disable_unprepare(ov2740->clk);
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -1075,6 +1078,11 @@ static int ov2740_resume(struct device *dev)
>>>>  {
>>>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>>>  	struct ov2740 *ov2740 = to_ov2740(sd);
>>>> +	int ret;
>>>> +
>>>> +	ret = clk_prepare_enable(ov2740->clk);
>>>> +	if (ret)
>>>> +		return ret;
>>>>  
>>>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>>>>  	msleep(20);
>>>> @@ -1102,6 +1110,10 @@ static int ov2740_probe(struct i2c_client *client)
>>>>  		return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio),
>>>>  				     "failed to get reset GPIO\n");
>>>>  
>>>> +	ov2740->clk = devm_clk_get_optional(dev, "clk");
>>>> +	if (IS_ERR(ov2740->clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n");
>>>> +
>>>
>>> I am not very sure that the 80-char rule is still valid for checkpatch.pl.
>>
>> checkpatch.pl defaults to allowing longer lines (<100 chars) now,
>> but you are right that the linux-media maintainers prefer 80.
>>
>> Still there is an exception to not split strings running
>> over the limit and this line ends with a string,
>> so I think that this is fine.
> 
> The rule is not to split strings in order to satisfy alignment rules. IOW
> the line should be wrapped before the string. :-)

Ok, will fix for v2.

Regards,

Hans