| Message ID | 20231115123817.196252-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov2740: Add support for reset GPIO and external clock | expand |
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>
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
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,
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(-)