| Message ID | 20220310130829.96001-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series | media: i2c: ov5670: OF support, runtime_pm, regulators | expand |
Hello, On Sat, Mar 12, 2022 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > On 11/03/2022 19:00, Jacopo Mondi wrote: > > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote: > >> On 11/03/2022 17:05, Jacopo Mondi wrote: > >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote: > >>>> On 10/03/2022 18:16, Jacopo Mondi wrote: > >>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote: > >>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote: > >>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor. > >>>>>>> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>>> --- > >>>>>>> .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++ > >>>>>> > >>>>>> Add the file to maintainers entry. > >>>>> > >>>>> Right > >>>>> > >>>>>>> 1 file changed, 93 insertions(+) > >>>>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>>> > >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>>> new file mode 100644 > >>>>>>> index 000000000000..dc4a3297bf6f > >>>>>>> --- /dev/null > >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>> > >>>>>> Missing vendor prefix in file name. > >>>>> > >>>>> Right x2 > >>>>> > >>>>>>> @@ -0,0 +1,93 @@ > >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>>>> +%YAML 1.2 > >>>>>>> +--- > >>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml# > >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>>>> + > >>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor > >>>>>>> + > >>>>>>> +maintainers: > >>>>>>> + - Jacopo Mondi <jacopo@jmondi.org> > >>>>>> > >>>>>> Please add also driver maintainer. > >>>>> > >>>>> I never got what the policy was, if the maintainer entries here only > >>>>> refer to the binding file or to the driver too > >>>> > >>>> It is a person responsible for the bindings, so indeed it might not feed > >>>> existing maintainer. > >>>> > >>>>>>> + > >>>>>>> +description: |- > >>>>>>> + The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits > >>>>>>> + RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is > >>>>>>> + controlled through an I2C compatible control bus. > >>>>>>> + > >>>>>>> +properties: > >>>>>>> + compatible: > >>>>>>> + const: ovti,ov5670 > >>>>>>> + > >>>>>>> + reg: > >>>>>>> + maxItems: 1 > >>>>>>> + > >>>>>>> + clock-frequency: > >>>>>>> + description: Frequency of the xclk clock. > >>>>>> > >>>>>> Is the xclk external clock coming to the sensor? If yes, there should be > >>>>>> a "clocks" property. > >>>>> > >>>>> To be honest I was not sure about this, as clock-frequency is already > >>>>> used by the driver for the ACPI part, but it seems to in DT bindings > >>>>> it is a property meant to be specified in the clock providers, even if > >>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't > >>>>> really clarify this > >>>>> > >>>>> Clock consumer should rather use 'clocks' and point to the provider's > >>>>> phandle if my understanding is right. > >>>> > >>>> This is a clock-frequency, not clock reference. For external clocks, a > >>> > >>> Yes, I was suggesting to replace clock-frequency with clocks, that > >>> accepts a phandle. > >>> > >>> The thing is, the driver parses 'clock-frequency' > >>> device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > >>> > >>> which I assume comes from ACPI (as the driver was developed for an > >>> ACPI platform). > >>> > >>> If in DTS we don't use it, I then need to > >>> > >>> #ifdef CONFIG_ACPI > >>> > >>> #elif defined CONFIG_OF > >>> > >>> #endif > >>> > >>> Which I would really like to avoid. > >>> > >>> Anyone with ACPI experience that knows where clock-frequency comes > >>> from ? > >> > >> I would assume that ACPI simply does not support common clock framework, > >> so it had to use clock-frequency. Several of such drivers were added by > >> folks from Intel which use ACPI, not Devicetree. > >> > >>>> clock phandles + assigned-clock-rates should be rather used. However for > >>>> internal clocks, this is a perfectly valid property. > >>>> > >>>> Therefore the question is - what is the "xclk"? > >>> > >>> xclk is the clock fed to the sensor, which which all its internal > >>> clocks are generated, so it's indeed an 'external' clock. As I've > >>> said, clock-frequency seems to be meant for clock providers, and > >>> the image sensor is a clock consumer. > >> > >> Regardless whether clock-frequency stays or not, you need the clocks > >> property in such case. > > > > Yes, I will have to ifdef in the driver if no better alternatives > > I do not see the need of ifdefs... BTW, imx258 has exactly that case - > clock-frequency coming from ACPI world but not added to DT bindings. The driver can call clk_get_rate() when a clock is provided, and use the clock-frequency property otherwise. I also don't think conditional compilation is needed.
Hi Jacopo, Thank you for the patch. On Thu, Mar 10, 2022 at 02:08:26PM +0100, Jacopo Mondi wrote: > The OV5670 has three power supplies (AVDD, DOVDD and DVDD). > > Probe them in the driver to prepare controlling with runtime_pm > operations. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5670.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > index 39786f3c9489..cba310aec011 100644 > --- a/drivers/media/i2c/ov5670.c > +++ b/drivers/media/i2c/ov5670.c > @@ -7,6 +7,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-event.h> > @@ -85,6 +86,14 @@ struct ov5670_link_freq_config { > const struct ov5670_reg_list reg_list; > }; > > +static const char * const ov5670_supply_names[] = { > + "avdd", /* Analog power */ > + "dvdd", /* Digital power */ > + "dovdd", /* Digital output power */ > +}; > + > +#define OV5670_NUM_SUPPLIES ARRAY_SIZE(ov5670_supply_names) > + > struct ov5670_mode { > /* Frame width in pixels */ > u32 width; > @@ -1830,6 +1839,9 @@ struct ov5670 { > /* Current mode */ > const struct ov5670_mode *cur_mode; > > + /* Regulators */ > + struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES]; > + > /* To serialize asynchronus callbacks */ > struct mutex mutex; > > @@ -2470,6 +2482,18 @@ static const struct v4l2_subdev_internal_ops ov5670_internal_ops = { > .open = ov5670_open, > }; > > +static int ov5670_regulators_probe(struct ov5670 *ov5670) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd); > + unsigned int i; > + > + for (i = 0; i < OV5670_NUM_SUPPLIES; i++) > + ov5670->supplies[i].supply = ov5670_supply_names[i]; > + > + return devm_regulator_bulk_get(&client->dev, OV5670_NUM_SUPPLIES, > + ov5670->supplies); > +} > + > static int ov5670_probe(struct i2c_client *client) > { > struct ov5670 *ov5670; > @@ -2492,6 +2516,12 @@ static int ov5670_probe(struct i2c_client *client) > /* Initialize subdev */ > v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > > + ret = ov5670_regulators_probe(ov5670); > + if (ret) { > + err_msg = "Regulators probe failed"; That's not common. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + goto error_print; > + } > + > full_power = acpi_dev_state_d0(&client->dev); > if (full_power) { > /* Check module identity */
Hi Jacopo, Thank you for the patch. On Thu, Mar 10, 2022 at 02:08:28PM +0100, Jacopo Mondi wrote: > Implement the power up and power down routines and install them as > runtime_pm handler for runtime_suspend and runtime_resume operations. > > Rework the runtime_pm enablement and the chip power handling during > probe, as calling pm_runtime_idle() in a driver that registers no > idle callback is a nop. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5670.c | 59 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 53 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c > index ca5191d043ce..c9f69ffef210 100644 > --- a/drivers/media/i2c/ov5670.c > +++ b/drivers/media/i2c/ov5670.c > @@ -2,6 +2,8 @@ > // Copyright (c) 2017 Intel Corporation. > > #include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> Isn't this header needed in the previous patch ? > #include <linux/i2c.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > @@ -2422,6 +2424,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable) > return ret; > } > > +static int __maybe_unused ov5670_power_on(struct device *dev) I'd name this ov5670_runtime_resume(), and the following function ov5670_runtime_suspend(). > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov5670 *ov5670 = to_ov5670(sd); > + int ret = 0; No need to initialize ret. > + > + ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies); > + if (ret) > + return ret; > + > + gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0); > + gpiod_set_value_cansleep(ov5670->reset_gpio, 0); > + > + /* 8192 * 2 clock pulses before the first SCCB transaction. */ > + usleep_range(1000, 1500); > + > + return 0; > +} > + > +static int __maybe_unused ov5670_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov5670 *ov5670 = to_ov5670(sd); > + > + gpiod_set_value_cansleep(ov5670->reset_gpio, 1); > + gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1); > + regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies); > + > + return 0; > +} > + > static int __maybe_unused ov5670_suspend(struct device *dev) > { > struct v4l2_subdev *sd = dev_get_drvdata(dev); > @@ -2549,14 +2584,25 @@ static int ov5670_probe(struct i2c_client *client) > goto error_print; > } > > + pm_runtime_enable(&client->dev); > + > full_power = acpi_dev_state_d0(&client->dev); That's peculiar. Sakari, is this needed, or is there a way to rely on runtime PM only ? Or on another API that would abstract the differences between ACPI and OF ? > if (full_power) { > + ret = pm_runtime_resume_and_get(&client->dev); > + if (ret) { > + err_msg = "Failed to power on"; > + goto error_print; > + } > + > /* Check module identity */ > ret = ov5670_identify_module(ov5670); > if (ret) { > err_msg = "ov5670_identify_module() error"; > - goto error_print; > + goto error_power_off; > } > + > + /* Set the device's state to active if it's in D0 state. */ > + pm_runtime_set_active(&client->dev); > } > > mutex_init(&ov5670->mutex); > @@ -2593,11 +2639,7 @@ static int ov5670_probe(struct i2c_client *client) > > ov5670->streaming = false; > > - /* Set the device's state to active if it's in D0 state. */ > - if (full_power) > - pm_runtime_set_active(&client->dev); > - pm_runtime_enable(&client->dev); > - pm_runtime_idle(&client->dev); > + pm_runtime_suspend(&client->dev); > > return 0; > > @@ -2610,6 +2652,9 @@ static int ov5670_probe(struct i2c_client *client) > error_mutex_destroy: > mutex_destroy(&ov5670->mutex); > > +error_power_off: > + pm_runtime_put(&client->dev); > + > error_print: > dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret); > > @@ -2626,6 +2671,7 @@ static int ov5670_remove(struct i2c_client *client) > v4l2_ctrl_handler_free(sd->ctrl_handler); > mutex_destroy(&ov5670->mutex); > > + pm_runtime_put(&client->dev); > pm_runtime_disable(&client->dev); > > return 0; > @@ -2633,6 +2679,7 @@ static int ov5670_remove(struct i2c_client *client) > > static const struct dev_pm_ops ov5670_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume) > + SET_RUNTIME_PM_OPS(ov5670_power_off, ov5670_power_on, NULL) > }; > > #ifdef CONFIG_ACPI