| Message ID | 20220509143226.531117-1-foss+kernel@0leil.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/4] media: dt-bindings: ov5675: document YAML binding | expand |
Hi Quentin, On Mon, May 09, 2022 at 04:32:25PM +0200, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > Parse V4L2 device tree properties and register controls for them. > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > drivers/media/i2c/ov5675.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > index cee380196774..5544e1ae444e 100644 > --- a/drivers/media/i2c/ov5675.c > +++ b/drivers/media/i2c/ov5675.c > @@ -779,12 +779,14 @@ static const struct v4l2_ctrl_ops ov5675_ctrl_ops = { > > static int ov5675_init_controls(struct ov5675 *ov5675) > { > + struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd); > struct v4l2_ctrl_handler *ctrl_hdlr; > + struct v4l2_fwnode_device_properties props; You could move the line up and maintain the (in)famous reverse-xmas-tree ordering in variable declaration. Nit apart, looks good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > s64 exposure_max, h_blank; > int ret; > > ctrl_hdlr = &ov5675->ctrl_handler; > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); > if (ret) > return ret; > > @@ -838,9 +840,23 @@ static int ov5675_init_controls(struct ov5675 *ov5675) > if (ctrl_hdlr->error) > return ctrl_hdlr->error; > > + ret = v4l2_fwnode_device_parse(&client->dev, &props); > + if (ret) > + goto error; > + > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov5675_ctrl_ops, > + &props); > + if (ret) > + goto error; > + > ov5675->sd.ctrl_handler = ctrl_hdlr; > > return 0; > + > +error: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + > + return ret; > } > > static void ov5675_update_pad_format(const struct ov5675_mode *mode, > -- > 2.35.3 >
On 09/05/2022 16:32, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > This patch adds documentation of device tree in YAML schema for the > OV5675 CMOS image sensor from Omnivision. > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi all, On 5/10/22 11:46, Jacopo Mondi wrote: > Hi Quentin, > > On Mon, May 09, 2022 at 04:32:24PM +0200, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> Until now, this driver only supported ACPI. This adds support for >> Device Tree too while enabling clock and regulators in runtime PM. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > Thanks for addressing all comments on the previous version. > > Looks good to me! Unfortunately the sensor disagrees :/ For some reasons, the first three power-on + power-off are successful (sometimes only the first two) and then the sensor is not working until next cold boot. I got lucky when I tested the patch before sending, much less now. I'm looking into it, don't know how long it will take to get the runtime PM fixed. Please hold onto those patches (well.. technically patches 3/4 and 4/4 don't depend on anything from this patch, so feel free to merge those once reviewed). Cheers, Quentin > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> --- >> >> v3: >> - added linux/mod_devicetable.h include, >> - moved delay for reset pulse right after the regulators are enabled, >> - removed check on is_acpi_node in favor of checks on presence of OF >> properties (e.g. devm_clk_get_optional returns NULL), >> - moved power management out of system suspend/resume into runtime PM >> callbacks, >> - removed ACPI specific comment since it's not specific to this driver, >> - changed devm_clk_get to devm_clk_get_optional, >> - remove OF use of clock-frequency (handled by devm_clk_get_optional >> directly), >> - removed name of clock (only one, so no need for anything explicit) >> when requesting a clock from OF, >> - wrapped lines to 80 chars, >> >> v2: >> - fixed unused-const-variable warning by removing of_match_ptr in >> of_match_table, reported by kernel test robot, >> >> drivers/media/i2c/ov5675.c | 143 +++++++++++++++++++++++++++++++------ >> 1 file changed, 122 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c >> index 82ba9f56baec..cee380196774 100644 >> --- a/drivers/media/i2c/ov5675.c >> +++ b/drivers/media/i2c/ov5675.c >> @@ -3,10 +3,14 @@ >> >> #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> >> +#include <linux/mod_devicetable.h> >> #include <linux/module.h> >> #include <linux/pm_runtime.h> >> +#include <linux/regulator/consumer.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-device.h> >> #include <media/v4l2-fwnode.h> >> @@ -17,7 +21,7 @@ >> >> #define OV5675_LINK_FREQ_450MHZ 450000000ULL >> #define OV5675_SCLK 90000000LL >> -#define OV5675_MCLK 19200000 >> +#define OV5675_XVCLK_19_2 19200000 >> #define OV5675_DATA_LANES 2 >> #define OV5675_RGB_DEPTH 10 >> >> @@ -76,6 +80,14 @@ >> >> #define to_ov5675(_sd) container_of(_sd, struct ov5675, sd) >> >> +static const char * const ov5675_supply_names[] = { >> + "avdd", /* Analog power */ >> + "dovdd", /* Digital I/O power */ >> + "dvdd", /* Digital core power */ >> +}; >> + >> +#define OV5675_NUM_SUPPLIES ARRAY_SIZE(ov5675_supply_names) >> + >> enum { >> OV5675_LINK_FREQ_900MBPS, >> }; >> @@ -484,6 +496,9 @@ struct ov5675 { >> struct v4l2_subdev sd; >> struct media_pad pad; >> struct v4l2_ctrl_handler ctrl_handler; >> + struct clk *xvclk; >> + struct gpio_desc *reset_gpio; >> + struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES]; >> >> /* V4L2 Controls */ >> struct v4l2_ctrl *link_freq; >> @@ -944,6 +959,50 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable) >> return ret; >> } >> >> +static int ov5675_power_off(struct device *dev) >> +{ >> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >> + struct ov5675 *ov5675 = to_ov5675(sd); >> + >> + gpiod_set_value_cansleep(ov5675->reset_gpio, 1); >> + usleep_range(1000, 1200); >> + >> + regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies); >> + clk_disable_unprepare(ov5675->xvclk); >> + >> + return 0; >> +} >> + >> +static int ov5675_power_on(struct device *dev) >> +{ >> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >> + struct ov5675 *ov5675 = to_ov5675(sd); >> + int ret; >> + >> + ret = clk_prepare_enable(ov5675->xvclk); >> + if (ret < 0) { >> + dev_err(dev, "failed to enable xvclk: %d\n", ret); >> + return ret; >> + } >> + >> + gpiod_set_value_cansleep(ov5675->reset_gpio, 1); >> + >> + ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies); >> + if (ret) { >> + clk_disable_unprepare(ov5675->xvclk); >> + return ret; >> + } >> + >> + /* Reset pulse should be at least 2ms */ >> + usleep_range(2000, 2200); >> + >> + gpiod_set_value_cansleep(ov5675->reset_gpio, 0); >> + >> + usleep_range(1000, 1200); >> + >> + return 0; >> +} >> + >> static int __maybe_unused ov5675_suspend(struct device *dev) >> { >> struct v4l2_subdev *sd = dev_get_drvdata(dev); >> @@ -1106,32 +1165,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = { >> .open = ov5675_open, >> }; >> >> -static int ov5675_check_hwcfg(struct device *dev) >> +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev) >> { >> struct fwnode_handle *ep; >> struct fwnode_handle *fwnode = dev_fwnode(dev); >> struct v4l2_fwnode_endpoint bus_cfg = { >> .bus_type = V4L2_MBUS_CSI2_DPHY >> }; >> - u32 mclk; >> + u32 xvclk_rate; >> int ret; >> unsigned int i, j; >> >> if (!fwnode) >> return -ENXIO; >> >> - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); >> + ov5675->xvclk = devm_clk_get_optional(dev, NULL); >> + if (IS_ERR(ov5675->xvclk)) >> + return dev_err_probe(dev, PTR_ERR(ov5675->xvclk), >> + "failed to get xvclk: %ld\n", >> + PTR_ERR(ov5675->xvclk)); >> >> - if (ret) { >> - dev_err(dev, "can't get clock frequency"); >> - return ret; >> + if (ov5675->xvclk) { >> + xvclk_rate = clk_get_rate(ov5675->xvclk); >> + } else { >> + ret = fwnode_property_read_u32(fwnode, "clock-frequency", >> + &xvclk_rate); >> + >> + if (ret) { >> + dev_err(dev, "can't get clock frequency"); >> + return ret; >> + } >> } >> >> - if (mclk != OV5675_MCLK) { >> - dev_err(dev, "external clock %d is not supported", mclk); >> + if (xvclk_rate != OV5675_XVCLK_19_2) { >> + dev_err(dev, "external clock rate %u is unsupported", >> + xvclk_rate); >> return -EINVAL; >> } >> >> + ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset", >> + GPIOD_OUT_HIGH); >> + if (IS_ERR(ov5675->reset_gpio)) { >> + ret = PTR_ERR(ov5675->reset_gpio); >> + dev_err(dev, "failed to get reset-gpios: %d\n", ret); >> + return ret; >> + } >> + >> + for (i = 0; i < OV5675_NUM_SUPPLIES; i++) >> + ov5675->supplies[i].supply = ov5675_supply_names[i]; >> + >> + ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES, >> + ov5675->supplies); >> + if (ret) >> + return ret; >> + >> ep = fwnode_graph_get_next_endpoint(fwnode, NULL); >> if (!ep) >> return -ENXIO; >> @@ -1186,6 +1273,9 @@ static int ov5675_remove(struct i2c_client *client) >> pm_runtime_disable(&client->dev); >> mutex_destroy(&ov5675->mutex); >> >> + if (!pm_runtime_status_suspended(&client->dev)) >> + ov5675_power_off(&client->dev); >> + >> return 0; >> } >> >> @@ -1195,25 +1285,31 @@ static int ov5675_probe(struct i2c_client *client) >> bool full_power; >> int ret; >> >> - ret = ov5675_check_hwcfg(&client->dev); >> + ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL); >> + if (!ov5675) >> + return -ENOMEM; >> + >> + ret = ov5675_get_hwcfg(ov5675, &client->dev); >> if (ret) { >> - dev_err(&client->dev, "failed to check HW configuration: %d", >> + dev_err(&client->dev, "failed to get HW configuration: %d", >> ret); >> return ret; >> } >> >> - ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL); >> - if (!ov5675) >> - return -ENOMEM; >> - >> v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops); >> >> + ret = ov5675_power_on(&client->dev); >> + if (ret) { >> + dev_err(&client->dev, "failed to power on: %d\n", ret); >> + return ret; >> + } >> + >> full_power = acpi_dev_state_d0(&client->dev); >> if (full_power) { >> ret = ov5675_identify_module(ov5675); >> if (ret) { >> dev_err(&client->dev, "failed to find sensor: %d", ret); >> - return ret; >> + goto probe_power_off; >> } >> } >> >> @@ -1243,11 +1339,6 @@ static int ov5675_probe(struct i2c_client *client) >> goto probe_error_media_entity_cleanup; >> } >> >> - /* >> - * Device is already turned on by i2c-core with ACPI domain PM. >> - * Enable runtime PM and turn off the device. >> - */ >> - >> /* Set the device's state to active if it's in D0 state. */ >> if (full_power) >> pm_runtime_set_active(&client->dev); >> @@ -1262,12 +1353,15 @@ static int ov5675_probe(struct i2c_client *client) >> probe_error_v4l2_ctrl_handler_free: >> v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler); >> mutex_destroy(&ov5675->mutex); >> +probe_power_off: >> + ov5675_power_off(&client->dev); >> >> return ret; >> } >> >> static const struct dev_pm_ops ov5675_pm_ops = { >> SET_SYSTEM_SLEEP_PM_OPS(ov5675_suspend, ov5675_resume) >> + SET_RUNTIME_PM_OPS(ov5675_power_off, ov5675_power_on, NULL) >> }; >> >> #ifdef CONFIG_ACPI >> @@ -1279,11 +1373,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = { >> MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids); >> #endif >> >> +static const struct of_device_id ov5675_of_match[] = { >> + { .compatible = "ovti,ov5675", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, ov5675_of_match); >> + >> static struct i2c_driver ov5675_i2c_driver = { >> .driver = { >> .name = "ov5675", >> .pm = &ov5675_pm_ops, >> .acpi_match_table = ACPI_PTR(ov5675_acpi_ids), >> + .of_match_table = ov5675_of_match, >> }, >> .probe_new = ov5675_probe, >> .remove = ov5675_remove, >> -- >> 2.35.3 >>
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml new file mode 100644 index 000000000000..f0a48707bed7 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml @@ -0,0 +1,123 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5675.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OV5675 CMOS Sensor + +maintainers: + - Quentin Schulz <quentin.schulz@theobroma-systems.com> + +allOf: + - $ref: /schemas/media/video-interface-devices.yaml# + +description: | + The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS + image sensor that delivers 2592x1944 at 30fps. It provides full-frame, + sub-sampled, and windowed 10-bit MIPI images in various formats via the + Serial Camera Control Bus (SCCB) interface. + + This chip is programmable through I2C and two-wire SCCB. The sensor output + is available via CSI-2 serial data output (up to 2-lane). + +properties: + compatible: + const: ovti,ov5675 + + reg: + maxItems: 1 + + clocks: + description: + System input clock (aka XVCLK). From 6 to 27 MHz. + maxItems: 1 + + dovdd-supply: + description: + Digital I/O voltage supply, 1.8 volts. + + avdd-supply: + description: + Analog voltage supply, 2.8 volts. + + dvdd-supply: + description: + Digital core voltage supply, 1.2 volts. + + reset-gpios: + description: + The phandle and specifier for the GPIO that controls sensor reset. + This corresponds to the hardware pin XSHUTDN which is physically + active low. + maxItems: 1 + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + minItems: 1 + maxItems: 2 + + # Supports max data transfer of 900 Mbps per lane + link-frequencies: true + +required: + - compatible + - reg + - clocks + - dovdd-supply + - avdd-supply + - dvdd-supply + - port + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/px30-cru.h> + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/pinctrl/rockchip.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov5675: camera@36 { + compatible = "ovti,ov5675"; + reg = <0x36>; + + reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&cif_clkout_m0>; + + clocks = <&cru SCLK_CIF_OUT>; + assigned-clocks = <&cru SCLK_CIF_OUT>; + assigned-clock-rates = <19200000>; + + avdd-supply = <&vcc_1v8>; + dvdd-supply = <&vcc_1v2>; + dovdd-supply = <&vcc_2v8>; + + rotation = <90>; + orientation = <0>; + + port { + ucam_out: endpoint { + remote-endpoint = <&mipi_in_ucam>; + data-lanes = <1 2>; + link-frequencies = /bits/ 64 <450000000>; + }; + }; + }; + }; +... + diff --git a/MAINTAINERS b/MAINTAINERS index e8c52d0192a6..c7a2d9d3642e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14550,6 +14550,7 @@ M: Shawn Tu <shawnx.tu@intel.com> L: linux-media@vger.kernel.org S: Maintained T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml F: drivers/media/i2c/ov5675.c OMNIVISION OV5693 SENSOR DRIVER