* [v2 0/3] media: ov8856: Add devicetree support
@ 2020-03-13 11:03 Robert Foss
2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Robert Foss @ 2020-03-13 11:03 UTC (permalink / raw)
To: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Sakari Ailus,
Tomasz Figa, linux-media, devicetree, linux-kernel,
linux-arm-kernel
Cc: Robert Foss
This adds devicetree support to the ov8856 driver.
In order to to aid debugging and enable future sensor
modes to be supported, module revision detection is also added.
Dongchun Zhu (1):
media: dt-bindings: ov8856: Document YAML bindings
Robert Foss (2):
media: ov8856: Add devicetree support
media: ov8856: Implement sensor module revision identification
.../devicetree/bindings/media/i2c/ov8856.yaml | 133 +++++++++++++++
MAINTAINERS | 1 +
drivers/media/i2c/ov8856.c | 151 +++++++++++++++++-
3 files changed, 283 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread* [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-13 11:03 [v2 0/3] media: ov8856: Add devicetree support Robert Foss @ 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:19 ` Sakari Ailus 2020-03-13 22:00 ` Rob Herring 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` [v2 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2 siblings, 2 replies; 17+ messages in thread From: Robert Foss @ 2020-03-13 11:03 UTC (permalink / raw) To: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Sakari Ailus, Tomasz Figa, linux-media, devicetree, linux-kernel, linux-arm-kernel Cc: Robert Foss From: Dongchun Zhu <dongchun.zhu@mediatek.com> This patch adds documentation of device tree in YAML schema for the OV8856 CMOS image sensor. Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> Signed-off-by: Robert Foss <robert.foss@linaro.org> --- - Changes since v4: * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description * Add clock-lanes property to example * robher: Fix syntax error in devicetree example - Changes since v3: * robher: Fix syntax error * robher: Removed maxItems * Fixes yaml 'make dt-binding-check' errors - Changes since v2: Fixes comments from from Andy, Tomasz, Sakari, Rob. * Convert text documentation to YAML schema. - Changes since v1: Fixes comments from Sakari, Tomasz * Add clock-frequency and link-frequencies in DT .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 134 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml new file mode 100644 index 000000000000..f5cb9add9277 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (c) 2019 MediaTek Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings + +maintainers: + - Ben Kao <ben.kao@intel.com> + - Dongchun Zhu <dongchun.zhu@mediatek.com> + +description: |- + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS + image sensor that delivers 3264x2448 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 4-lane). + +properties: + compatible: + const: ovti,ov8856 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + description: + Input clock for the sensor. + items: + - const: xvclk + + clock-frequency: + description: + Frequency of the xvclk clock in Hertz. + + dovdd-supply: + description: + Definition of the regulator used as interface power supply. + + avdd-supply: + description: + Definition of the regulator used as analog power supply. + + dvdd-supply: + description: + Definition of the regulator used as digital power supply. + + reset-gpios: + description: + The phandle and specifier for the GPIO that controls sensor reset. + This corresponds to the hardware pin XSHUTDOWN which is physically + active low. + + port: + type: object + additionalProperties: false + description: + A node containing input and output port nodes with endpoint definitions + as documented in + Documentation/devicetree/bindings/media/video-interfaces.txt + + properties: + endpoint: + type: object + + properties: + clock-lanes: + maxItems: 1 + + data-lanes: + maxItems: 1 + + remote-endpoint: true + + required: + - clock-lanes + - data-lanes + - remote-endpoint + + required: + - endpoint + +required: + - compatible + - reg + - clocks + - clock-names + - clock-frequency + - dovdd-supply + - avdd-supply + - dvdd-supply + - reset-gpios + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/clock/qcom,camcc-sdm845.h> + + ov8856: camera-sensor@10 { + compatible = "ovti,ov8856"; + reg = <0x10>; + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&clk_24m_cam>; + + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; + clock-names = "xvclk"; + clock-frequency = <19200000>; + + avdd-supply = <&mt6358_vcama2_reg>; + dvdd-supply = <&mt6358_vcamd_reg>; + dovdd-supply = <&mt6358_vcamio_reg>; + + port { + wcam_out: endpoint { + remote-endpoint = <&mipi_in_wcam>; + clock-lanes = <0>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <360000000 180000000>; + }; + }; + }; + +... \ No newline at end of file diff --git a/MAINTAINERS b/MAINTAINERS index a6fbdf354d34..0f99e863978a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov8856.c +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml OMNIVISION OV9650 SENSOR DRIVER M: Sakari Ailus <sakari.ailus@linux.intel.com> -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss @ 2020-03-13 12:19 ` Sakari Ailus 2020-03-13 22:00 ` Rob Herring 1 sibling, 0 replies; 17+ messages in thread From: Sakari Ailus @ 2020-03-13 12:19 UTC (permalink / raw) To: Robert Foss Cc: devicetree, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, linux-arm-kernel, linux-media Hi Robert, On Fri, Mar 13, 2020 at 12:03:48PM +0100, Robert Foss wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds documentation of device tree in YAML schema for the > OV8856 CMOS image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - Changes since v4: > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > * Add clock-lanes property to example > * robher: Fix syntax error in devicetree example > > - Changes since v3: > * robher: Fix syntax error > * robher: Removed maxItems > * Fixes yaml 'make dt-binding-check' errors > > - Changes since v2: > Fixes comments from from Andy, Tomasz, Sakari, Rob. > * Convert text documentation to YAML schema. > > - Changes since v1: > Fixes comments from Sakari, Tomasz > * Add clock-frequency and link-frequencies in DT > > .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 134 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > new file mode 100644 > index 000000000000..f5cb9add9277 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > @@ -0,0 +1,133 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2019 MediaTek Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > + > +maintainers: > + - Ben Kao <ben.kao@intel.com> Is Ben aware of this? > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > + > +description: |- > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > + image sensor that delivers 3264x2448 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 4-lane). > + > +properties: > + compatible: > + const: ovti,ov8856 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + description: > + Input clock for the sensor. > + items: > + - const: xvclk > + > + clock-frequency: > + description: > + Frequency of the xvclk clock in Hertz. > + > + dovdd-supply: > + description: > + Definition of the regulator used as interface power supply. > + > + avdd-supply: > + description: > + Definition of the regulator used as analog power supply. > + > + dvdd-supply: > + description: > + Definition of the regulator used as digital power supply. > + > + reset-gpios: > + description: > + The phandle and specifier for the GPIO that controls sensor reset. > + This corresponds to the hardware pin XSHUTDOWN which is physically > + active low. > + > + port: > + type: object > + additionalProperties: false > + description: > + A node containing input and output port nodes with endpoint definitions > + as documented in > + Documentation/devicetree/bindings/media/video-interfaces.txt > + > + properties: > + endpoint: > + type: object > + > + properties: > + clock-lanes: > + maxItems: 1 > + > + data-lanes: > + maxItems: 1 > + > + remote-endpoint: true > + > + required: > + - clock-lanes Do you need the clock-lanes property, i.e. does the device support lane reordering? If not, it should be removed. > + - data-lanes > + - remote-endpoint > + > + required: > + - endpoint > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - clock-frequency > + - dovdd-supply > + - avdd-supply > + - dvdd-supply > + - reset-gpios > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/clock/qcom,camcc-sdm845.h> > + > + ov8856: camera-sensor@10 { > + compatible = "ovti,ov8856"; > + reg = <0x10>; > + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; > + pinctrl-names = "default"; > + pinctrl-0 = <&clk_24m_cam>; > + > + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > + clock-names = "xvclk"; > + clock-frequency = <19200000>; > + > + avdd-supply = <&mt6358_vcama2_reg>; > + dvdd-supply = <&mt6358_vcamd_reg>; > + dovdd-supply = <&mt6358_vcamio_reg>; > + > + port { > + wcam_out: endpoint { > + remote-endpoint = <&mipi_in_wcam>; > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + link-frequencies = /bits/ 64 <360000000 180000000>; > + }; > + }; > + }; > + > +... > \ No newline at end of file > diff --git a/MAINTAINERS b/MAINTAINERS > index a6fbdf354d34..0f99e863978a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org > T: git git://linuxtv.org/media_tree.git > S: Maintained > F: drivers/media/i2c/ov8856.c > +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > OMNIVISION OV9650 SENSOR DRIVER > M: Sakari Ailus <sakari.ailus@linux.intel.com> -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-13 12:19 ` Sakari Ailus @ 2020-03-13 22:00 ` Rob Herring 1 sibling, 0 replies; 17+ messages in thread From: Rob Herring @ 2020-03-13 22:00 UTC (permalink / raw) To: Robert Foss Cc: devicetree, linux-kernel, Robert Foss, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, linux-arm-kernel, linux-media On Fri, 13 Mar 2020 12:03:48 +0100, Robert Foss wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds documentation of device tree in YAML schema for the > OV8856 CMOS image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - Changes since v4: > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > * Add clock-lanes property to example > * robher: Fix syntax error in devicetree example > > - Changes since v3: > * robher: Fix syntax error > * robher: Removed maxItems > * Fixes yaml 'make dt-binding-check' errors > > - Changes since v2: > Fixes comments from from Andy, Tomasz, Sakari, Rob. > * Convert text documentation to YAML schema. > > - Changes since v1: > Fixes comments from Sakari, Tomasz > * Add clock-frequency and link-frequencies in DT > > .../devicetree/bindings/media/i2c/ov8856.yaml | 133 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 134 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/media/i2c/ov8856.example.dts:22.13-26: Warning (reg_format): /example-0/camera-sensor@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1) Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format' See https://patchwork.ozlabs.org/patch/1254346 Please check and re-submit. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 11:03 [v2 0/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss @ 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:17 ` Sakari Ailus ` (2 more replies) 2020-03-13 11:03 ` [v2 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2 siblings, 3 replies; 17+ messages in thread From: Robert Foss @ 2020-03-13 11:03 UTC (permalink / raw) To: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Sakari Ailus, Tomasz Figa, linux-media, devicetree, linux-kernel, linux-arm-kernel Cc: Robert Foss Add devicetree match table, and enable ov8856_probe() to initialize power, clocks and reset pins. Signed-off-by: Robert Foss <robert.foss@linaro.org> --- - Changes since v1: * Fabio: Change n_shutdown_gpio name to reset_gpio * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change * Fabio: Remove empty line * Fabio: Remove real error from devm_gpiod_get() failures * Andy & Sakari: Make XVCLK optional since to not break ACPI * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES * Sakari: Use XVCLK rate as provided by DT drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index 8655842af275..db61eed223e8 100644 --- a/drivers/media/i2c/ov8856.c +++ b/drivers/media/i2c/ov8856.c @@ -3,10 +3,13 @@ #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/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> @@ -19,6 +22,8 @@ #define OV8856_LINK_FREQ_180MHZ 180000000ULL #define OV8856_SCLK 144000000ULL #define OV8856_MCLK 19200000 +#define OV8856_XVCLK_19_2 19200000 +#define OV8856_XVCLK_24 24000000 #define OV8856_DATA_LANES 4 #define OV8856_RGB_DEPTH 10 @@ -64,6 +69,12 @@ #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd) +static const char * const ov8856_supply_names[] = { + "dovdd", /* Digital I/O power */ + "avdd", /* Analog power */ + "dvdd", /* Digital core power */ +}; + enum { OV8856_LINK_FREQ_720MBPS, OV8856_LINK_FREQ_360MBPS, @@ -566,6 +577,10 @@ struct ov8856 { struct media_pad pad; struct v4l2_ctrl_handler ctrl_handler; + struct clk *xvclk; + struct gpio_desc *reset_gpio; + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)]; + /* V4L2 Controls */ struct v4l2_ctrl *link_freq; struct v4l2_ctrl *pixel_rate; @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) return ret; } +static int __ov8856_power_on(struct ov8856 *ov8856) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); + int ret; + + ret = clk_prepare_enable(ov8856->xvclk); + if (ret < 0) { + dev_err(&client->dev, "failed to enable xvclk\n"); + return ret; + } + + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); + + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), + ov8856->supplies); + if (ret < 0) { + dev_err(&client->dev, "failed to enable regulators\n"); + goto disable_clk; + } + + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); + + usleep_range(1500, 1800); + + return 0; + +disable_clk: + clk_disable_unprepare(ov8856->xvclk); + + return ret; +} + +static void __ov8856_power_off(struct ov8856 *ov8856) +{ + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names), + ov8856->supplies); + clk_disable_unprepare(ov8856->xvclk); +} + static int __maybe_unused ov8856_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client) static int ov8856_probe(struct i2c_client *client) { struct ov8856 *ov8856; - int ret; + int i, ret; ret = ov8856_check_hwcfg(&client->dev); if (ret) { @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client) return -ENOMEM; v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { + dev_info(&client->dev, "xvclk clock not defined, continuing...\n"); + ov8856->xvclk = NULL; + } else if (IS_ERR(ov8856->xvclk)) { + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", + PTR_ERR(ov8856->xvclk)); + return PTR_ERR(ov8856->xvclk); + } + + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); + if (ret < 0) { + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); + return ret; + } + + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(ov8856->reset_gpio)) { + dev_err(&client->dev, "failed to get reset-gpios\n"); + return PTR_ERR(ov8856->reset_gpio); + } + + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++) + ov8856->supplies[i].supply = ov8856_supply_names[i]; + + ret = devm_regulator_bulk_get(&client->dev, + ARRAY_SIZE(ov8856_supply_names), + ov8856->supplies); + if (ret) { + dev_warn(&client->dev, "failed to get regulators\n"); + return ret; + } + + ret = __ov8856_power_on(ov8856); + if (ret) { + dev_warn(&client->dev, "failed to power on\n"); + return ret; + } + ret = ov8856_identify_module(ov8856); if (ret) { dev_err(&client->dev, "failed to find sensor: %d", ret); - return ret; + goto probe_power_off; } mutex_init(&ov8856->mutex); @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client) v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler); mutex_destroy(&ov8856->mutex); +probe_power_off: + __ov8856_power_off(ov8856); + return ret; } @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = { MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids); #endif +static const struct of_device_id ov8856_of_match[] = { + { .compatible = "ovti,ov8856" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ov8856_of_match); + static struct i2c_driver ov8856_i2c_driver = { .driver = { .name = "ov8856", .pm = &ov8856_pm_ops, .acpi_match_table = ACPI_PTR(ov8856_acpi_ids), + .of_match_table = ov8856_of_match, }, .probe_new = ov8856_probe, .remove = ov8856_remove, -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss @ 2020-03-13 12:17 ` Sakari Ailus 2020-03-26 11:56 ` Robert Foss 2020-03-13 12:28 ` Andy Shevchenko 2020-03-13 13:15 ` Fabio Estevam 2 siblings, 1 reply; 17+ messages in thread From: Sakari Ailus @ 2020-03-13 12:17 UTC (permalink / raw) To: Robert Foss Cc: devicetree, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, linux-arm-kernel, linux-media Hi Robert, On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote: > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - Changes since v1: > * Fabio: Change n_shutdown_gpio name to reset_gpio > * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change > * Fabio: Remove empty line > * Fabio: Remove real error from devm_gpiod_get() failures > * Andy & Sakari: Make XVCLK optional since to not break ACPI > * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES > * Sakari: Use XVCLK rate as provided by DT > > drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > index 8655842af275..db61eed223e8 100644 > --- a/drivers/media/i2c/ov8856.c > +++ b/drivers/media/i2c/ov8856.c > @@ -3,10 +3,13 @@ > > #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/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> > @@ -19,6 +22,8 @@ > #define OV8856_LINK_FREQ_180MHZ 180000000ULL > #define OV8856_SCLK 144000000ULL > #define OV8856_MCLK 19200000 > +#define OV8856_XVCLK_19_2 19200000 Please use a single macro to refer to 19,2 MHz clock. > +#define OV8856_XVCLK_24 24000000 This doesn't seem to be needed > #define OV8856_DATA_LANES 4 > #define OV8856_RGB_DEPTH 10 > > @@ -64,6 +69,12 @@ > > #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd) > > +static const char * const ov8856_supply_names[] = { > + "dovdd", /* Digital I/O power */ > + "avdd", /* Analog power */ > + "dvdd", /* Digital core power */ > +}; > + > enum { > OV8856_LINK_FREQ_720MBPS, > OV8856_LINK_FREQ_360MBPS, > @@ -566,6 +577,10 @@ struct ov8856 { > struct media_pad pad; > struct v4l2_ctrl_handler ctrl_handler; > > + struct clk *xvclk; > + struct gpio_desc *reset_gpio; > + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)]; > + > /* V4L2 Controls */ > struct v4l2_ctrl *link_freq; > struct v4l2_ctrl *pixel_rate; > @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) > return ret; > } > > +static int __ov8856_power_on(struct ov8856 *ov8856) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + int ret; > + > + ret = clk_prepare_enable(ov8856->xvclk); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable xvclk\n"); > + return ret; > + } > + > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > + > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } > + > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > + > + usleep_range(1500, 1800); I think you could omit the delay on ACPI based systems. Or just bail out early in that case. > + > + return 0; > + > +disable_clk: How about the GPIO here? > + clk_disable_unprepare(ov8856->xvclk); > + > + return ret; > +} > + > +static void __ov8856_power_off(struct ov8856 *ov8856) > +{ > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + clk_disable_unprepare(ov8856->xvclk); > +} You'll need to call the two in the driver's suspend and resume functions. > + > static int __maybe_unused ov8856_suspend(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client) > static int ov8856_probe(struct i2c_client *client) > { > struct ov8856 *ov8856; > - int ret; > + int i, ret; unsigned int? > > ret = ov8856_check_hwcfg(&client->dev); > if (ret) { > @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client) > return -ENOMEM; > > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n"); How about dev_dbg()? > + ov8856->xvclk = NULL; > + } else if (IS_ERR(ov8856->xvclk)) { > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > + PTR_ERR(ov8856->xvclk)); > + return PTR_ERR(ov8856->xvclk); > + } > + > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); This should either come from platform data, or perhaps it'd be even better to get the clock rate and use assigned-clock-rates. I guess that's preferred nowadays. > + if (ret < 0) { > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); > + return ret; > + } > + > + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", > + GPIOD_OUT_HIGH); Indentation. What if no gpio is defined? > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_err(&client->dev, "failed to get reset-gpios\n"); > + return PTR_ERR(ov8856->reset_gpio); > + } > + > + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++) > + ov8856->supplies[i].supply = ov8856_supply_names[i]; > + > + ret = devm_regulator_bulk_get(&client->dev, > + ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); What happens if there are no regulators? > + if (ret) { > + dev_warn(&client->dev, "failed to get regulators\n"); > + return ret; > + } > + > + ret = __ov8856_power_on(ov8856); > + if (ret) { > + dev_warn(&client->dev, "failed to power on\n"); > + return ret; > + } > + > ret = ov8856_identify_module(ov8856); > if (ret) { > dev_err(&client->dev, "failed to find sensor: %d", ret); > - return ret; > + goto probe_power_off; > } > > mutex_init(&ov8856->mutex); > @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client) > v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler); > mutex_destroy(&ov8856->mutex); > > +probe_power_off: > + __ov8856_power_off(ov8856); > + Also remember to power off the device in remove(). > return ret; > } > > @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = { > MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids); > #endif > > +static const struct of_device_id ov8856_of_match[] = { > + { .compatible = "ovti,ov8856" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, ov8856_of_match); > + > static struct i2c_driver ov8856_i2c_driver = { > .driver = { > .name = "ov8856", > .pm = &ov8856_pm_ops, > .acpi_match_table = ACPI_PTR(ov8856_acpi_ids), > + .of_match_table = ov8856_of_match, > }, > .probe_new = ov8856_probe, > .remove = ov8856_remove, -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 12:17 ` Sakari Ailus @ 2020-03-26 11:56 ` Robert Foss 2020-03-26 14:47 ` Sakari Ailus 0 siblings, 1 reply; 17+ messages in thread From: Robert Foss @ 2020-03-26 11:56 UTC (permalink / raw) To: Sakari Ailus Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hey Sakari, On Fri, 13 Mar 2020 at 13:18, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote: > > Add devicetree match table, and enable ov8856_probe() > > to initialize power, clocks and reset pins. > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > --- > > > > - Changes since v1: > > * Fabio: Change n_shutdown_gpio name to reset_gpio > > * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change > > * Fabio: Remove empty line > > * Fabio: Remove real error from devm_gpiod_get() failures > > * Andy & Sakari: Make XVCLK optional since to not break ACPI > > * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES > > * Sakari: Use XVCLK rate as provided by DT > > > > drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 107 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > > index 8655842af275..db61eed223e8 100644 > > --- a/drivers/media/i2c/ov8856.c > > +++ b/drivers/media/i2c/ov8856.c > > @@ -3,10 +3,13 @@ > > > > #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/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> > > @@ -19,6 +22,8 @@ > > #define OV8856_LINK_FREQ_180MHZ 180000000ULL > > #define OV8856_SCLK 144000000ULL > > #define OV8856_MCLK 19200000 > > +#define OV8856_XVCLK_19_2 19200000 > > Please use a single macro to refer to 19,2 MHz clock. Alright, I'll combine the two into a macro for both. > > > +#define OV8856_XVCLK_24 24000000 > > This doesn't seem to be needed As long as we don't set the clock to 24Mhz we don't. I'm assuming that you're saying that the 24Mhz clock rate isn't needed for the modes used currently. Removing this macro in v3. > > > #define OV8856_DATA_LANES 4 > > #define OV8856_RGB_DEPTH 10 > > > > @@ -64,6 +69,12 @@ > > > > #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd) > > > > +static const char * const ov8856_supply_names[] = { > > + "dovdd", /* Digital I/O power */ > > + "avdd", /* Analog power */ > > + "dvdd", /* Digital core power */ > > +}; > > + > > enum { > > OV8856_LINK_FREQ_720MBPS, > > OV8856_LINK_FREQ_360MBPS, > > @@ -566,6 +577,10 @@ struct ov8856 { > > struct media_pad pad; > > struct v4l2_ctrl_handler ctrl_handler; > > > > + struct clk *xvclk; > > + struct gpio_desc *reset_gpio; > > + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)]; > > + > > /* V4L2 Controls */ > > struct v4l2_ctrl *link_freq; > > struct v4l2_ctrl *pixel_rate; > > @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) > > return ret; > > } > > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > + int ret; > > + > > + ret = clk_prepare_enable(ov8856->xvclk); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to enable xvclk\n"); > > + return ret; > > + } > > + > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > + ov8856->supplies); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to enable regulators\n"); > > + goto disable_clk; > > + } > > + > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > + > > + usleep_range(1500, 1800); > > I think you could omit the delay on ACPI based systems. Or just bail out > early in that case. I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > > + > > + return 0; > > + > > +disable_clk: > > How about the GPIO here? Ack > > > + clk_disable_unprepare(ov8856->xvclk); > > + > > + return ret; > > +} > > + > > +static void __ov8856_power_off(struct ov8856 *ov8856) > > +{ > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names), > > + ov8856->supplies); > > + clk_disable_unprepare(ov8856->xvclk); > > +} > > You'll need to call the two in the driver's suspend and resume functions. Ack > > > + > > static int __maybe_unused ov8856_suspend(struct device *dev) > > { > > struct i2c_client *client = to_i2c_client(dev); > > @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client) > > static int ov8856_probe(struct i2c_client *client) > > { > > struct ov8856 *ov8856; > > - int ret; > > + int i, ret; > > unsigned int? Ack > > > > > ret = ov8856_check_hwcfg(&client->dev); > > if (ret) { > > @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client) > > return -ENOMEM; > > > > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { > > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n"); > > How about dev_dbg()? Ack. > > > + ov8856->xvclk = NULL; > > + } else if (IS_ERR(ov8856->xvclk)) { > > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > > + PTR_ERR(ov8856->xvclk)); > > + return PTR_ERR(ov8856->xvclk); > > + } > > + > > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > > This should either come from platform data, or perhaps it'd be even better > to get the clock rate and use assigned-clock-rates. I guess that's > preferred nowadays. I'm a bit unsure about what this would look like. Are you thinking something like the way ext_clk is used in smiapp_core.c? I went ahead and implemented support for retrieving and storing 'clock-rates' during the ov8856_check_hwcfg() call, and then setting the rate to the configured rate during probing. > > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); > > + return ret; > > + } > > + > > + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", > > + GPIOD_OUT_HIGH); > > Indentation. Ack. > > What if no gpio is defined? As per Andys comments, I'll switch to the optional version of devm_gpiod_get(). > > > + if (IS_ERR(ov8856->reset_gpio)) { > > + dev_err(&client->dev, "failed to get reset-gpios\n"); > > + return PTR_ERR(ov8856->reset_gpio); > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++) > > + ov8856->supplies[i].supply = ov8856_supply_names[i]; > > + > > + ret = devm_regulator_bulk_get(&client->dev, > > + ARRAY_SIZE(ov8856_supply_names), > > + ov8856->supplies); > > What happens if there are no regulators? Like Andy mentioned, we should be alright, since devm_regulator_bulk_get() creates dummy regulators if one isn't found. > > > + if (ret) { > > + dev_warn(&client->dev, "failed to get regulators\n"); > > + return ret; > > + } > > + > > + ret = __ov8856_power_on(ov8856); > > + if (ret) { > > + dev_warn(&client->dev, "failed to power on\n"); > > + return ret; > > + } > > + > > ret = ov8856_identify_module(ov8856); > > if (ret) { > > dev_err(&client->dev, "failed to find sensor: %d", ret); > > - return ret; > > + goto probe_power_off; > > } > > > > mutex_init(&ov8856->mutex); > > @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client) > > v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler); > > mutex_destroy(&ov8856->mutex); > > > > +probe_power_off: > > + __ov8856_power_off(ov8856); > > + > > Also remember to power off the device in remove(). > Ack > > return ret; > > } > > > > @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = { > > MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids); > > #endif > > > > +static const struct of_device_id ov8856_of_match[] = { > > + { .compatible = "ovti,ov8856" }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, ov8856_of_match); > > + > > static struct i2c_driver ov8856_i2c_driver = { > > .driver = { > > .name = "ov8856", > > .pm = &ov8856_pm_ops, > > .acpi_match_table = ACPI_PTR(ov8856_acpi_ids), > > + .of_match_table = ov8856_of_match, > > }, > > .probe_new = ov8856_probe, > > .remove = ov8856_remove, > > -- > Regards, > > Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-26 11:56 ` Robert Foss @ 2020-03-26 14:47 ` Sakari Ailus 2020-03-27 10:32 ` Robert Foss 0 siblings, 1 reply; 17+ messages in thread From: Sakari Ailus @ 2020-03-26 14:47 UTC (permalink / raw) To: Robert Foss Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hi Robert, On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: ... > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > > +{ > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > > + int ret; > > > + > > > + ret = clk_prepare_enable(ov8856->xvclk); > > > + if (ret < 0) { > > > + dev_err(&client->dev, "failed to enable xvclk\n"); > > > + return ret; > > > + } > > > + > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > + > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > > + ov8856->supplies); > > > + if (ret < 0) { > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > > + goto disable_clk; > > > + } > > > + > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > > + > > > + usleep_range(1500, 1800); > > > > I think you could omit the delay on ACPI based systems. Or just bail out > > early in that case. > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. There could also be a regulator but no GPIO. I think if you don't have either, then certainly there's no need for a delay. ... > > > + ov8856->xvclk = NULL; > > > + } else if (IS_ERR(ov8856->xvclk)) { > > > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > > > + PTR_ERR(ov8856->xvclk)); > > > + return PTR_ERR(ov8856->xvclk); > > > + } > > > + > > > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > > > > This should either come from platform data, or perhaps it'd be even better > > to get the clock rate and use assigned-clock-rates. I guess that's > > preferred nowadays. > > I'm a bit unsure about what this would look like. > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > I went ahead and implemented support for retrieving and storing > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > the rate to the configured rate during probing. With assigned-clock-rates, you can simply use clk_get_rate(). As you get the actual rate, it could be somewhat off of the intended one. -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-26 14:47 ` Sakari Ailus @ 2020-03-27 10:32 ` Robert Foss 2020-03-27 13:37 ` Sakari Ailus 0 siblings, 1 reply; 17+ messages in thread From: Robert Foss @ 2020-03-27 10:32 UTC (permalink / raw) To: Sakari Ailus Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > ... > > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > > > +{ > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(ov8856->xvclk); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "failed to enable xvclk\n"); > > > > + return ret; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > > + > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > > > + ov8856->supplies); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > > > + goto disable_clk; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > > > + > > > > + usleep_range(1500, 1800); > > > > > > I think you could omit the delay on ACPI based systems. Or just bail out > > > early in that case. > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > There could also be a regulator but no GPIO. > > I think if you don't have either, then certainly there's no need for a > delay. Removing the delay if no action is taken makes sense, but I'm not sure how best to do it. If there are no regulators dummy ones are created automatically, which makes distinguishing between a little bit cumbersome. The regulator structs could of course all be inspected, and if all are dummy ones, the delay could be skipped. But is there a neater way of doing this? Manually inspecting the regs strikes me as a bit inelegant. > > ... > > > > > + ov8856->xvclk = NULL; > > > > + } else if (IS_ERR(ov8856->xvclk)) { > > > > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > > > > + PTR_ERR(ov8856->xvclk)); > > > > + return PTR_ERR(ov8856->xvclk); > > > > + } > > > > + > > > > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > > > > > > This should either come from platform data, or perhaps it'd be even better > > > to get the clock rate and use assigned-clock-rates. I guess that's > > > preferred nowadays. > > > > I'm a bit unsure about what this would look like. > > > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > > I went ahead and implemented support for retrieving and storing > > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > > the rate to the configured rate during probing. > > With assigned-clock-rates, you can simply use clk_get_rate(). Ah, I see. I'll switch to that approach then. > > As you get the actual rate, it could be somewhat off of the intended one. > > -- > Kind regards, > > Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-27 10:32 ` Robert Foss @ 2020-03-27 13:37 ` Sakari Ailus 0 siblings, 0 replies; 17+ messages in thread From: Sakari Ailus @ 2020-03-27 13:37 UTC (permalink / raw) To: Robert Foss Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hi Robert, On Fri, Mar 27, 2020 at 11:32:29AM +0100, Robert Foss wrote: > On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Robert, > > > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > > ... > > > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > > > > +{ > > > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > > > > + int ret; > > > > > + > > > > > + ret = clk_prepare_enable(ov8856->xvclk); > > > > > + if (ret < 0) { > > > > > + dev_err(&client->dev, "failed to enable xvclk\n"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > > > + > > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > > > > + ov8856->supplies); > > > > > + if (ret < 0) { > > > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > > > > + goto disable_clk; > > > > > + } > > > > > + > > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > > > > + > > > > > + usleep_range(1500, 1800); > > > > > > > > I think you could omit the delay on ACPI based systems. Or just bail out > > > > early in that case. > > > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > > > There could also be a regulator but no GPIO. > > > > I think if you don't have either, then certainly there's no need for a > > delay. > > Removing the delay if no action is taken makes sense, but I'm not sure > how best to do it. > If there are no regulators dummy ones are created automatically, which > makes distinguishing between a little bit cumbersome. The regulator > structs could of course all be inspected, and if all are dummy ones, > the delay could be skipped. But is there a neater way of doing this? > Manually inspecting the regs strikes me as a bit inelegant. I guess the cleanest, easy way to make this right, albeit slightly unoptimal in very rare cases where you have none of the above resources in a DT system, is to bail out if you're running on an ACPI based system. I.e. checking for e.g. is_acpi_node(dev->fwnode). -- Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 12:17 ` Sakari Ailus @ 2020-03-13 12:28 ` Andy Shevchenko 2020-03-13 13:15 ` Fabio Estevam 2 siblings, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2020-03-13 12:28 UTC (permalink / raw) To: Robert Foss Cc: devicetree, linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Fabio Estevam, linux-arm-kernel, linux-media On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote: > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. Thanks for an update. My comments below. ... > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); In many frameworks we have '_optional' variants of API. Please use it instead of open coded approach. > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) { > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n"); > + ov8856->xvclk = NULL; > + } else if (IS_ERR(ov8856->xvclk)) { > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > + PTR_ERR(ov8856->xvclk)); > + return PTR_ERR(ov8856->xvclk); > + } > + > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > + if (ret < 0) { > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); > + return ret; > + } > + > + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", > + GPIOD_OUT_HIGH); Same here. > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_err(&client->dev, "failed to get reset-gpios\n"); > + return PTR_ERR(ov8856->reset_gpio); > + } > + > + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++) > + ov8856->supplies[i].supply = ov8856_supply_names[i]; > + > + ret = devm_regulator_bulk_get(&client->dev, > + ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); Luckily regulator framework will create dummy ones if there is none found. > + if (ret) { > + dev_warn(&client->dev, "failed to get regulators\n"); > + return ret; > + } -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 12:17 ` Sakari Ailus 2020-03-13 12:28 ` Andy Shevchenko @ 2020-03-13 13:15 ` Fabio Estevam 2020-03-31 13:37 ` Robert Foss 2 siblings, 1 reply; 17+ messages in thread From: Fabio Estevam @ 2020-03-13 13:15 UTC (permalink / raw) To: Robert Foss Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hi Robert, On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote: > +static int __ov8856_power_on(struct ov8856 *ov8856) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + int ret; > + > + ret = clk_prepare_enable(ov8856->xvclk); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable xvclk\n"); > + return ret; > + } > + > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); The parameter of gpiod_set_value_cansleep() is typically 0 (inactive state) or 1 (active state), so: gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > + > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } > + > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); and here it should be: gpiod_set_value_cansleep(ov8856->reset_gpio, 0); Also, don't you need a reset period between the two? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-13 13:15 ` Fabio Estevam @ 2020-03-31 13:37 ` Robert Foss 2020-03-31 13:42 ` Fabio Estevam 0 siblings, 1 reply; 17+ messages in thread From: Robert Foss @ 2020-03-31 13:37 UTC (permalink / raw) To: Fabio Estevam Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Thu, 26 Mar 2020 at 13:18, Robert Foss <robert.foss@linaro.org> wrote: > > Hey Fabio, > > On Fri, 13 Mar 2020 at 14:15, Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Robert, > > > > On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> wrote: > > > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > > +{ > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > > + int ret; > > > + > > > + ret = clk_prepare_enable(ov8856->xvclk); > > > + if (ret < 0) { > > > + dev_err(&client->dev, "failed to enable xvclk\n"); > > > + return ret; > > > + } > > > + > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > > The parameter of gpiod_set_value_cansleep() is typically 0 (inactive > > state) or 1 (active state), so: > > > > gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > > Ack After testing this change, it breaks the driver during probing. I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they seem to never be 0 or 1. https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L38 GPIOD_ASIS = 0, GPIOD_IN = 1, GPIOD_OUT_LOW = 3 GPIOD_OUT_HIGH = 7 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-31 13:37 ` Robert Foss @ 2020-03-31 13:42 ` Fabio Estevam 2020-03-31 13:53 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Fabio Estevam @ 2020-03-31 13:42 UTC (permalink / raw) To: Robert Foss Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hi Robert, On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote: > After testing this change, it breaks the driver during probing. Why exactly does it break probing? Maybe the GPIO polarity defined in the device tree is wrong? > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they > seem to never be 0 or 1. If you do a grep in all gpiod_set_value_cansleep() usages in the kernel tree, there is not a single case where GPIOD_OUT_HIGH or GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep(). _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support 2020-03-31 13:42 ` Fabio Estevam @ 2020-03-31 13:53 ` Andy Shevchenko 0 siblings, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2020-03-31 13:53 UTC (permalink / raw) To: Fabio Estevam Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Robert Foss, Tomasz Figa, Sakari Ailus, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Tue, Mar 31, 2020 at 4:43 PM Fabio Estevam <festevam@gmail.com> wrote: > On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote: > > > After testing this change, it breaks the driver during probing. > > Why exactly does it break probing? Maybe the GPIO polarity defined in > the device tree is wrong? > > > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they > > seem to never be 0 or 1. > > If you do a grep in all gpiod_set_value_cansleep() usages in the > kernel tree, there is not a single case where GPIOD_OUT_HIGH or > GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep(). +1. It simple reveals the problem that is somewhere else. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [v2 3/3] media: ov8856: Implement sensor module revision identification 2020-03-13 11:03 [v2 0/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss @ 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:43 ` Sakari Ailus 2 siblings, 1 reply; 17+ messages in thread From: Robert Foss @ 2020-03-13 11:03 UTC (permalink / raw) To: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Sakari Ailus, Tomasz Figa, linux-media, devicetree, linux-kernel, linux-arm-kernel Cc: Robert Foss Query the sensor for its module revision, and compare it to known revisions. Currently only the '1B' revision has been added. Signed-off-by: Robert Foss <robert.foss@linaro.org> --- drivers/media/i2c/ov8856.c | 54 +++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index db61eed223e8..39662d3d86dd 100644 --- a/drivers/media/i2c/ov8856.c +++ b/drivers/media/i2c/ov8856.c @@ -34,6 +34,18 @@ #define OV8856_MODE_STANDBY 0x00 #define OV8856_MODE_STREAMING 0x01 +/* define 1B module revision */ +#define OV8856_1B_MODULE 0x02 + +/* the OTP read-out buffer is at 0x7000 and 0xf is the offset + * of the byte in the OTP that means the module revision + */ +#define OV8856_MODULE_REVISION 0x700f +#define OV8856_OTP_MODE_CTRL 0x3d84 +#define OV8856_OTP_LOAD_CTRL 0x3d81 +#define OV8856_OTP_MODE_AUTO 0x00 +#define OV8856_OTP_LOAD_CTRL_ENABLE BIT(0) + /* vertical-timings from sensor */ #define OV8856_REG_VTS 0x380e #define OV8856_VTS_MAX 0x7fff @@ -711,6 +723,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern) OV8856_REG_VALUE_08BIT, pattern); } +static int ov8856_check_revision(struct ov8856 *ov8856) +{ + int ret; + + ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT, + OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING); + if (ret) + return ret; + + ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL, + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO); + if (ret) + return ret; + + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL, + OV8856_REG_VALUE_08BIT, + OV8856_OTP_LOAD_CTRL_ENABLE); +} + static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl) { struct ov8856 *ov8856 = container_of(ctrl->handler, @@ -1144,6 +1175,23 @@ static int ov8856_identify_module(struct ov8856 *ov8856) return -ENXIO; } + /* check sensor hardware revision */ + ret = ov8856_check_revision(ov8856); + if (ret) { + dev_err(&client->dev, "failed to check sensor revision"); + return ret; + } + + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION, + OV8856_REG_VALUE_08BIT, &val); + if (ret) + return ret; + + dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n", + val, + val == OV8856_1B_MODULE ? "1B" : "unknown revision", + client->addr); + return 0; } @@ -1254,12 +1302,6 @@ static int ov8856_probe(struct i2c_client *client) return PTR_ERR(ov8856->xvclk); } - ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); - if (ret < 0) { - dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); - return ret; - } - ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(ov8856->reset_gpio)) { -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [v2 3/3] media: ov8856: Implement sensor module revision identification 2020-03-13 11:03 ` [v2 3/3] media: ov8856: Implement sensor module revision identification Robert Foss @ 2020-03-13 12:43 ` Sakari Ailus 0 siblings, 0 replies; 17+ messages in thread From: Sakari Ailus @ 2020-03-13 12:43 UTC (permalink / raw) To: Robert Foss Cc: devicetree, linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko, Fabio Estevam, linux-arm-kernel, linux-media Hi Robert, On Fri, Mar 13, 2020 at 12:03:50PM +0100, Robert Foss wrote: > Query the sensor for its module revision, and compare it > to known revisions. > Currently only the '1B' revision has been added. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > drivers/media/i2c/ov8856.c | 54 +++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c > index db61eed223e8..39662d3d86dd 100644 > --- a/drivers/media/i2c/ov8856.c > +++ b/drivers/media/i2c/ov8856.c > @@ -34,6 +34,18 @@ > #define OV8856_MODE_STANDBY 0x00 > #define OV8856_MODE_STREAMING 0x01 > > +/* define 1B module revision */ > +#define OV8856_1B_MODULE 0x02 > + > +/* the OTP read-out buffer is at 0x7000 and 0xf is the offset > + * of the byte in the OTP that means the module revision > + */ > +#define OV8856_MODULE_REVISION 0x700f > +#define OV8856_OTP_MODE_CTRL 0x3d84 > +#define OV8856_OTP_LOAD_CTRL 0x3d81 > +#define OV8856_OTP_MODE_AUTO 0x00 > +#define OV8856_OTP_LOAD_CTRL_ENABLE BIT(0) > + > /* vertical-timings from sensor */ > #define OV8856_REG_VTS 0x380e > #define OV8856_VTS_MAX 0x7fff > @@ -711,6 +723,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern) > OV8856_REG_VALUE_08BIT, pattern); > } > > +static int ov8856_check_revision(struct ov8856 *ov8856) There are no version checks being done here, nor apparently the version is read by this function. > +{ > + int ret; > + > + ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT, > + OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING); > + if (ret) > + return ret; > + > + ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL, > + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO); > + if (ret) > + return ret; > + > + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL, > + OV8856_REG_VALUE_08BIT, > + OV8856_OTP_LOAD_CTRL_ENABLE); If streaming is started to read the EEPROM, shouldn't it be stopped after reading it as well? > +} > + > static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl) > { > struct ov8856 *ov8856 = container_of(ctrl->handler, > @@ -1144,6 +1175,23 @@ static int ov8856_identify_module(struct ov8856 *ov8856) > return -ENXIO; > } > > + /* check sensor hardware revision */ > + ret = ov8856_check_revision(ov8856); > + if (ret) { > + dev_err(&client->dev, "failed to check sensor revision"); > + return ret; > + } > + > + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION, > + OV8856_REG_VALUE_08BIT, &val); > + if (ret) > + return ret; How about moving this inside the check_revision function above? It looks as if it's dependent on that. > + > + dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n", > + val, > + val == OV8856_1B_MODULE ? "1B" : "unknown revision", > + client->addr); > + > return 0; > } > > @@ -1254,12 +1302,6 @@ static int ov8856_probe(struct i2c_client *client) > return PTR_ERR(ov8856->xvclk); > } > > - ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); This seems like an unrelated change. > - if (ret < 0) { > - dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n"); > - return ret; > - } > - > ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset", > GPIOD_OUT_HIGH); > if (IS_ERR(ov8856->reset_gpio)) { -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-03-31 16:16 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 11:03 [v2 0/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-13 12:19 ` Sakari Ailus 2020-03-13 22:00 ` Rob Herring 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 12:17 ` Sakari Ailus 2020-03-26 11:56 ` Robert Foss 2020-03-26 14:47 ` Sakari Ailus 2020-03-27 10:32 ` Robert Foss 2020-03-27 13:37 ` Sakari Ailus 2020-03-13 12:28 ` Andy Shevchenko 2020-03-13 13:15 ` Fabio Estevam 2020-03-31 13:37 ` Robert Foss 2020-03-31 13:42 ` Fabio Estevam 2020-03-31 13:53 ` Andy Shevchenko 2020-03-13 11:03 ` [v2 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2020-03-13 12:43 ` Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).