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