* [v1 0/3] media: ov8856: Add sensor modes & devicetree support @ 2020-03-10 13:46 Robert Foss 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Robert Foss @ 2020-03-10 13:46 UTC (permalink / raw) To: ben.kao, mchehab, robh+dt, mark.rutland, matthias.bgg, davem, gregkh, Jonathan.Cameron, andriy.shevchenko, linux-media, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Dongchun Zhu Cc: Robert Foss, Tomasz Figa 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 | 129 +++++++++++++++ MAINTAINERS | 1 + drivers/media/i2c/ov8856.c | 153 +++++++++++++++++- 3 files changed, 281 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] 18+ messages in thread
* [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-10 13:46 [v1 0/3] media: ov8856: Add sensor modes & devicetree support Robert Foss @ 2020-03-10 13:46 ` Robert Foss 2020-03-10 13:57 ` Fabio Estevam 2020-03-10 18:38 ` Rob Herring 2020-03-10 13:46 ` [v1 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-10 13:46 ` [v1 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2 siblings, 2 replies; 18+ messages in thread From: Robert Foss @ 2020-03-10 13:46 UTC (permalink / raw) To: ben.kao, mchehab, robh+dt, mark.rutland, matthias.bgg, davem, gregkh, Jonathan.Cameron, andriy.shevchenko, linux-media, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Dongchun Zhu Cc: Robert Foss, Tomasz Figa 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 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 | 129 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 130 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..c8099ccab1d9 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml @@ -0,0 +1,129 @@ +# 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. + + 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> + + ov8856: camera-sensor@10 { + compatible = "ovti,ov8856"; + reg = <0x10>; + reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&clk_24m_cam>; + + clocks = <&cru SCLK_TESTCLKOUT1>; + 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>; + 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] 18+ messages in thread
* Re: [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss @ 2020-03-10 13:57 ` Fabio Estevam 2020-03-10 15:51 ` Robert Foss 2020-03-10 18:38 ` Rob Herring 1 sibling, 1 reply; 18+ messages in thread From: Fabio Estevam @ 2020-03-10 13:57 UTC (permalink / raw) To: Robert Foss Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Andy Shevchenko, Greg Kroah-Hartman, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, Matthias Brugger, Mauro Carvalho Chehab, David S. Miller, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hi Robert, On Tue, Mar 10, 2020 at 10:46 AM Robert Foss <robert.foss@linaro.org> wrote: > + ov8856: camera-sensor@10 { > + compatible = "ovti,ov8856"; > + reg = <0x10>; > + reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>; Could you double check this is correct? Other OmniVision sensors have reset-gpios as active low. I suspect that the driver has also an inverted logic, so that's why it works. I don't have access to the datasheet though, so I am just guessing. _______________________________________________ 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] 18+ messages in thread
* Re: [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-10 13:57 ` Fabio Estevam @ 2020-03-10 15:51 ` Robert Foss 2020-03-12 10:13 ` Robert Foss 0 siblings, 1 reply; 18+ messages in thread From: Robert Foss @ 2020-03-10 15:51 UTC (permalink / raw) To: Fabio Estevam Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Andy Shevchenko, Greg Kroah-Hartman, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, Matthias Brugger, Mauro Carvalho Chehab, David S. Miller, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hey Fabio, Thanks for having a look at this series so quickly. On Tue, 10 Mar 2020 at 14:57, Fabio Estevam <festevam@gmail.com> wrote: > > Hi Robert, > > On Tue, Mar 10, 2020 at 10:46 AM Robert Foss <robert.foss@linaro.org> wrote: > > > + ov8856: camera-sensor@10 { > > + compatible = "ovti,ov8856"; > > + reg = <0x10>; > > + reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>; > > Could you double check this is correct? Other OmniVision sensors have > reset-gpios as active low. I have tested this, unfortunately I don't have access to a ov8856 datasheet that includes this level of detail. But I have tested this. > > I suspect that the driver has also an inverted logic, so that's why it works. That could explain it still working. Let me have a look into the driver and see what it does. > > I don't have access to the datasheet though, so I am just guessing. Me neither unfortunately, if anyone does have a link for it, I would very much appreciate it. _______________________________________________ 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] 18+ messages in thread
* Re: [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-10 15:51 ` Robert Foss @ 2020-03-12 10:13 ` Robert Foss 0 siblings, 0 replies; 18+ messages in thread From: Robert Foss @ 2020-03-12 10:13 UTC (permalink / raw) To: Fabio Estevam Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Andy Shevchenko, Greg Kroah-Hartman, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, Matthias Brugger, Mauro Carvalho Chehab, David S. Miller, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Tue, 10 Mar 2020 at 16:51, Robert Foss <robert.foss@linaro.org> wrote: > > Hey Fabio, > > Thanks for having a look at this series so quickly. > > On Tue, 10 Mar 2020 at 14:57, Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Robert, > > > > On Tue, Mar 10, 2020 at 10:46 AM Robert Foss <robert.foss@linaro.org> wrote: > > > > > + ov8856: camera-sensor@10 { > > > + compatible = "ovti,ov8856"; > > > + reg = <0x10>; > > > + reset-gpios = <&pio 111 GPIO_ACTIVE_HIGH>; > > > > Could you double check this is correct? Other OmniVision sensors have > > reset-gpios as active low. > > I have tested this, unfortunately I don't have access to a ov8856 > datasheet that includes > this level of detail. But I have tested this. > > > > > I suspect that the driver has also an inverted logic, so that's why it works. > > That could explain it still working. Let me have a look into the > driver and see what it does. > I had a look at some of OmniVision drivers, and there does seem to be a logical inversion in some of them, but not all of them. ov7251: - enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds to the hardware pin XSHUTDOWN which is physically active low. ov5640: - reset-gpios: reference to the GPIO connected to the reset pin, if any. This is an active low signal to the OV5640. I think the confusion stems from the XSHUTDOWN pin being mapped to a GPIO called reset, and the two being logically inverted. Currently this series does several mappings. XSHUTDOWN -> reset-gpio -> n_shutdn_gpio ^ ^ ^ Physical Pin DT Driver I think changing to what ov5640 does makes the most sense. XSHUTDOWN -> reset-gpio -> reset_gpio > > > > I don't have access to the datasheet though, so I am just guessing. > > Me neither unfortunately, if anyone does have a link for it, I would > very much appreciate it. _______________________________________________ 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] 18+ messages in thread
* Re: [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-10 13:57 ` Fabio Estevam @ 2020-03-10 18:38 ` Rob Herring 1 sibling, 0 replies; 18+ messages in thread From: Rob Herring @ 2020-03-10 18:38 UTC (permalink / raw) To: Robert Foss Cc: mark.rutland, devicetree, andriy.shevchenko, gregkh, linux-kernel, Robert Foss, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, mchehab, davem, linux-arm-kernel, linux-media On Tue, 10 Mar 2020 14:46:01 +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 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 | 129 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 130 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > My bot found errors running 'make dt_binding_check' on your patch: Error: Documentation/devicetree/bindings/media/i2c/ov8856.example.dts:26.28-29 syntax error FATAL ERROR: Unable to parse input tree scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml' failed make[1]: *** [Documentation/devicetree/bindings/media/i2c/ov8856.example.dt.yaml] Error 1 Makefile:1262: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1252173 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] 18+ messages in thread
* [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 13:46 [v1 0/3] media: ov8856: Add sensor modes & devicetree support Robert Foss 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss @ 2020-03-10 13:46 ` Robert Foss 2020-03-10 14:03 ` Fabio Estevam 2020-03-10 14:26 ` Andy Shevchenko 2020-03-10 13:46 ` [v1 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2 siblings, 2 replies; 18+ messages in thread From: Robert Foss @ 2020-03-10 13:46 UTC (permalink / raw) To: ben.kao, mchehab, robh+dt, mark.rutland, matthias.bgg, davem, gregkh, Jonathan.Cameron, andriy.shevchenko, linux-media, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Dongchun Zhu Cc: Robert Foss, Tomasz Figa Add devicetree match table, and enable ov8856_probe() to initialize power, clocks and reset pins. Signed-off-by: Robert Foss <robert.foss@linaro.org> --- drivers/media/i2c/ov8856.c | 105 ++++++++++++++++++++++++++++++++++++- 1 file changed, 103 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index 8655842af275..1769acdfaa44 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,14 @@ #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 */ +}; + +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) + enum { OV8856_LINK_FREQ_720MBPS, OV8856_LINK_FREQ_360MBPS, @@ -566,6 +579,10 @@ struct ov8856 { struct media_pad pad; struct v4l2_ctrl_handler ctrl_handler; + struct clk *xvclk; + struct gpio_desc *n_shutdn_gpio; + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES]; + /* V4L2 Controls */ struct v4l2_ctrl *link_freq; struct v4l2_ctrl *pixel_rate; @@ -908,6 +925,45 @@ 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->n_shutdn_gpio, GPIOD_OUT_LOW); + + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies); + if (ret < 0) { + dev_err(&client->dev, "failed to enable regulators\n"); + goto disable_clk; + } + + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); + + 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->n_shutdn_gpio, GPIOD_OUT_LOW); + regulator_bulk_disable(OV8856_NUM_SUPPLIES, 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 +1231,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 +1245,45 @@ 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 (IS_ERR(ov8856->xvclk)) { + dev_err(&client->dev, "failed to get xvclk\n"); + return -EINVAL; + } + + 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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset", + GPIOD_OUT_LOW); + if (IS_ERR(ov8856->n_shutdn_gpio)) { + dev_err(&client->dev, "failed to get reset-gpios\n"); + return -EINVAL; + } + + for (i = 0; i < OV8856_NUM_SUPPLIES; i++) + ov8856->supplies[i].supply = ov8856_supply_names[i]; + + ret = devm_regulator_bulk_get(&client->dev, OV8856_NUM_SUPPLIES, + 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 +1329,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 +1348,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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 13:46 ` [v1 2/3] media: ov8856: Add devicetree support Robert Foss @ 2020-03-10 14:03 ` Fabio Estevam 2020-03-10 15:46 ` Robert Foss 2020-03-10 14:26 ` Andy Shevchenko 1 sibling, 1 reply; 18+ messages in thread From: Fabio Estevam @ 2020-03-10 14:03 UTC (permalink / raw) To: Robert Foss Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Andy Shevchenko, Greg Kroah-Hartman, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, Matthias Brugger, Mauro Carvalho Chehab, David S. Miller, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Tue, Mar 10, 2020 at 10:47 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->n_shutdn_gpio, GPIOD_OUT_LOW); > + > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } > + > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); To power it up you probably only need: gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 0); And use reset-gpios as active low in your device tree. Assuming the reset-gpios is active low like other OmniVision sensors. > + > + 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->n_shutdn_gpio, GPIOD_OUT_LOW); > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > + clk_disable_unprepare(ov8856->xvclk); > +} > + > + Unneede extra blank line. > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > + if (IS_ERR(ov8856->xvclk)) { > + dev_err(&client->dev, "failed to get xvclk\n"); > + return -EINVAL; You should better return the real error insteald PTR_ERR(ov8856->xvclk). This way defer probe could work. > + } > + > + 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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(ov8856->n_shutdn_gpio)) { > + dev_err(&client->dev, "failed to get reset-gpios\n"); > + return -EINVAL; Please return the real error. _______________________________________________ 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 14:03 ` Fabio Estevam @ 2020-03-10 15:46 ` Robert Foss 0 siblings, 0 replies; 18+ messages in thread From: Robert Foss @ 2020-03-10 15:46 UTC (permalink / raw) To: Fabio Estevam Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Andy Shevchenko, Greg Kroah-Hartman, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, Matthias Brugger, Mauro Carvalho Chehab, David S. Miller, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Tue, 10 Mar 2020 at 15:03, Fabio Estevam <festevam@gmail.com> wrote: > > On Tue, Mar 10, 2020 at 10:47 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->n_shutdn_gpio, GPIOD_OUT_LOW); > > + > > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to enable regulators\n"); > > + goto disable_clk; > > + } > > + > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); > > To power it up you probably only need: > > gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 0); > > And use reset-gpios as active low in your device tree. Assuming the > reset-gpios is active low like other OmniVision sensors. Ack. > > > + > > + 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->n_shutdn_gpio, GPIOD_OUT_LOW); > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > + clk_disable_unprepare(ov8856->xvclk); > > +} > > + > > + > > Unneede extra blank line. Ack. > > > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops); > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > + if (IS_ERR(ov8856->xvclk)) { > > + dev_err(&client->dev, "failed to get xvclk\n"); > > + return -EINVAL; > > You should better return the real error insteald > PTR_ERR(ov8856->xvclk). This way defer probe could work. > Ack. > > + } > > + > > + 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->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(ov8856->n_shutdn_gpio)) { > > + dev_err(&client->dev, "failed to get reset-gpios\n"); > > + return -EINVAL; > > Please return the real error. Ack. _______________________________________________ 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 13:46 ` [v1 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-10 14:03 ` Fabio Estevam @ 2020-03-10 14:26 ` Andy Shevchenko 2020-03-10 15:55 ` Robert Foss 1 sibling, 1 reply; 18+ messages in thread From: Andy Shevchenko @ 2020-03-10 14:26 UTC (permalink / raw) To: Robert Foss Cc: mark.rutland, devicetree, gregkh, linux-kernel, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, mchehab, davem, linux-arm-kernel, linux-media On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. ... > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) Use ARRAY_SIZE() directly. Have you seen Sakari's comments? Sakari, do I have déjà vu or you indeed commented this driver? ... > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); Yes, seems this one is inverted. ... > +{ > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > + clk_disable_unprepare(ov8856->xvclk); > +} > + > + One blank line is enough. ... > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > + if (IS_ERR(ov8856->xvclk)) { > + dev_err(&client->dev, "failed to get xvclk\n"); > + return -EINVAL; > + } Previously it worked without clock provider, now you make a dependency. This won't work. -- 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 14:26 ` Andy Shevchenko @ 2020-03-10 15:55 ` Robert Foss 2020-03-11 9:05 ` Andy Shevchenko 2020-03-11 11:48 ` Sakari Ailus 0 siblings, 2 replies; 18+ messages in thread From: Robert Foss @ 2020-03-10 15:55 UTC (permalink / raw) To: Andy Shevchenko Cc: mark.rutland, devicetree, gregkh, linux-kernel, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, mchehab, davem, linux-arm-kernel, linux-media Hi Andy, On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: > > Add devicetree match table, and enable ov8856_probe() > > to initialize power, clocks and reset pins. > > ... > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) > > Use ARRAY_SIZE() directly. Ack. > > Have you seen Sakari's comments? > Sakari, do I have déją vu or you indeed commented this driver? Yes, I may have missed some part of it, so please tell me if I have. There is a patchset floating around that implements a larger chunk of functionality, including a couple of new modes. This is based on that series. > > ... > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); > > Yes, seems this one is inverted. > > ... > > > +{ > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > + clk_disable_unprepare(ov8856->xvclk); > > +} > > + > > + > > One blank line is enough. > > ... > > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > + if (IS_ERR(ov8856->xvclk)) { > > + dev_err(&client->dev, "failed to get xvclk\n"); > > + return -EINVAL; > > + } > > Previously it worked without clock provider, now you make a dependency. > > This won't work. So the ideal behavior would be to only use the xclk if it is provided? _______________________________________________ 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 15:55 ` Robert Foss @ 2020-03-11 9:05 ` Andy Shevchenko 2020-03-11 11:48 ` Sakari Ailus 1 sibling, 0 replies; 18+ messages in thread From: Andy Shevchenko @ 2020-03-11 9:05 UTC (permalink / raw) To: Robert Foss Cc: mark.rutland, devicetree, gregkh, linux-kernel, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, mchehab, davem, linux-arm-kernel, linux-media On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote: > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: ... > > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > > + if (IS_ERR(ov8856->xvclk)) { > > > + dev_err(&client->dev, "failed to get xvclk\n"); > > > + return -EINVAL; > > > + } > > > > Previously it worked without clock provider, now you make a dependency. > > > > This won't work. > > So the ideal behavior would be to only use the xclk if it is provided? Yes, make it optional. -- 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-10 15:55 ` Robert Foss 2020-03-11 9:05 ` Andy Shevchenko @ 2020-03-11 11:48 ` Sakari Ailus 2020-03-11 13:32 ` Robert Foss 1 sibling, 1 reply; 18+ messages in thread From: Sakari Ailus @ 2020-03-11 11:48 UTC (permalink / raw) To: Robert Foss Cc: mark.rutland, devicetree, mchehab, gregkh, linux-kernel, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, Andy Shevchenko, davem, linux-arm-kernel, linux-media Hi Robert, On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote: > Hi Andy, > > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: > > > Add devicetree match table, and enable ov8856_probe() > > > to initialize power, clocks and reset pins. > > > > ... > > > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) > > > > Use ARRAY_SIZE() directly. > > Ack. > > > > > Have you seen Sakari's comments? > > Sakari, do I have déją vu or you indeed commented this driver? > > Yes, I may have missed some part of it, so please tell me if I have. > > There is a patchset floating around that implements a larger chunk of > functionality, > including a couple of new modes. This is based on that series. Please see earlier comments given against an earlier variant of this set. They're on LMML. > > > > > ... > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); > > > > Yes, seems this one is inverted. > > > > ... > > > > > +{ > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > > + clk_disable_unprepare(ov8856->xvclk); > > > +} > > > + > > > + > > > > One blank line is enough. > > > > ... > > > > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > > + if (IS_ERR(ov8856->xvclk)) { > > > + dev_err(&client->dev, "failed to get xvclk\n"); > > > + return -EINVAL; > > > + } > > > > Previously it worked without clock provider, now you make a dependency. > > > > This won't work. > > So the ideal behavior would be to only use the xclk if it is provided? See e.g. the smiapp driver on how to do this so it continues to work on ACPI. I think it'd be also appropriate to add the usleep() after lifting reset only if the reset GPIO is defined for the device. Also do consider dropping some people from the distribution. For many this is just noise. -- 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-11 11:48 ` Sakari Ailus @ 2020-03-11 13:32 ` Robert Foss 2020-03-11 16:16 ` Sakari Ailus 0 siblings, 1 reply; 18+ messages in thread From: Robert Foss @ 2020-03-11 13:32 UTC (permalink / raw) To: Sakari Ailus Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hey Sakari, On Wed, 11 Mar 2020 at 12:49, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote: > > Hi Andy, > > > > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: > > > > Add devicetree match table, and enable ov8856_probe() > > > > to initialize power, clocks and reset pins. > > > > > > ... > > > > > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) > > > > > > Use ARRAY_SIZE() directly. > > > > Ack. > > > > > > > > Have you seen Sakari's comments? > > > Sakari, do I have déją vu or you indeed commented this driver? > > > > Yes, I may have missed some part of it, so please tell me if I have. > > > > There is a patchset floating around that implements a larger chunk of > > functionality, > > including a couple of new modes. This is based on that series. > > Please see earlier comments given against an earlier variant of this set. > They're on LMML. > > > > > > > > > ... > > > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); > > > > > > Yes, seems this one is inverted. > > > > > > ... > > > > > > > +{ > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > > > + clk_disable_unprepare(ov8856->xvclk); > > > > +} > > > > + > > > > + > > > > > > One blank line is enough. > > > > > > ... > > > > > > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > > > + if (IS_ERR(ov8856->xvclk)) { > > > > + dev_err(&client->dev, "failed to get xvclk\n"); > > > > + return -EINVAL; > > > > + } > > > > > > Previously it worked without clock provider, now you make a dependency. > > > > > > This won't work. > > > > So the ideal behavior would be to only use the xclk if it is provided? > > See e.g. the smiapp driver on how to do this so it continues to work on > ACPI. Thanks for the pointer! > > I think it'd be also appropriate to add the usleep() after lifting reset > only if the reset GPIO is defined for the device. Ack _______________________________________________ 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] 18+ messages in thread
* Re: [v1 2/3] media: ov8856: Add devicetree support 2020-03-11 13:32 ` Robert Foss @ 2020-03-11 16:16 ` Sakari Ailus 0 siblings, 0 replies; 18+ messages in thread From: Sakari Ailus @ 2020-03-11 16:16 UTC (permalink / raw) To: Robert Foss Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, ben.kao, Rob Herring, linux-mediatek, Dongchun Zhu, Andy Shevchenko, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media On Wed, Mar 11, 2020 at 02:32:30PM +0100, Robert Foss wrote: > Hey Sakari, > > On Wed, 11 Mar 2020 at 12:49, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Robert, > > > > On Tue, Mar 10, 2020 at 04:55:20PM +0100, Robert Foss wrote: > > > Hi Andy, > > > > > > On Tue, 10 Mar 2020 at 15:26, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Tue, Mar 10, 2020 at 02:46:02PM +0100, Robert Foss wrote: > > > > > Add devicetree match table, and enable ov8856_probe() > > > > > to initialize power, clocks and reset pins. > > > > > > > > ... > > > > > > > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) > > > > > > > > Use ARRAY_SIZE() directly. > > > > > > Ack. > > > > > > > > > > > Have you seen Sakari's comments? > > > > Sakari, do I have déją vu or you indeed commented this driver? > > > > > > Yes, I may have missed some part of it, so please tell me if I have. > > > > > > There is a patchset floating around that implements a larger chunk of > > > functionality, > > > including a couple of new modes. This is based on that series. > > > > Please see earlier comments given against an earlier variant of this set. > > They're on LMML. > > > > > > > > > > > > > ... > > > > > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > > > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH); > > > > > > > > Yes, seems this one is inverted. > > > > > > > > ... > > > > > > > > > +{ > > > > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW); > > > > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > > > > + clk_disable_unprepare(ov8856->xvclk); > > > > > +} > > > > > + > > > > > + > > > > > > > > One blank line is enough. > > > > > > > > ... > > > > > > > > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk"); > > > > > + if (IS_ERR(ov8856->xvclk)) { > > > > > + dev_err(&client->dev, "failed to get xvclk\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > Previously it worked without clock provider, now you make a dependency. > > > > > > > > This won't work. > > > > > > So the ideal behavior would be to only use the xclk if it is provided? > > > > See e.g. the smiapp driver on how to do this so it continues to work on > > ACPI. > > Thanks for the pointer! > > > > > I think it'd be also appropriate to add the usleep() after lifting reset > > only if the reset GPIO is defined for the device. > > Ack On second thought, that probably applies if any of the resources needed for powering the device on are defined. It could be that there's no reset GPIO but a regulator is still there, in which case a delay is needed. -- 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] 18+ messages in thread
* [v1 3/3] media: ov8856: Implement sensor module revision identification 2020-03-10 13:46 [v1 0/3] media: ov8856: Add sensor modes & devicetree support Robert Foss 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-10 13:46 ` [v1 2/3] media: ov8856: Add devicetree support Robert Foss @ 2020-03-10 13:46 ` Robert Foss 2020-03-10 14:30 ` Andy Shevchenko 2 siblings, 1 reply; 18+ messages in thread From: Robert Foss @ 2020-03-10 13:46 UTC (permalink / raw) To: ben.kao, mchehab, robh+dt, mark.rutland, matthias.bgg, davem, gregkh, Jonathan.Cameron, andriy.shevchenko, linux-media, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Dongchun Zhu Cc: Robert Foss, Tomasz Figa 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 | 48 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index 1769acdfaa44..48e8f4b997d6 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 @@ -713,6 +725,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, @@ -1145,6 +1176,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; } -- 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] 18+ messages in thread
* Re: [v1 3/3] media: ov8856: Implement sensor module revision identification 2020-03-10 13:46 ` [v1 3/3] media: ov8856: Implement sensor module revision identification Robert Foss @ 2020-03-10 14:30 ` Andy Shevchenko 2020-03-12 16:37 ` Robert Foss 0 siblings, 1 reply; 18+ messages in thread From: Andy Shevchenko @ 2020-03-10 14:30 UTC (permalink / raw) To: Robert Foss Cc: mark.rutland, devicetree, gregkh, linux-kernel, Tomasz Figa, ben.kao, robh+dt, linux-mediatek, Dongchun Zhu, Jonathan.Cameron, matthias.bgg, mchehab, davem, linux-arm-kernel, linux-media On Tue, Mar 10, 2020 at 02:46:03PM +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. Are you sure you send latest version? I have a déjà vu that I have seen this already and this one doesn't address any comment given. ... > + dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n", > + val, > + val == OV8856_1B_MODULE ? "1B" : "unknown revision", This is weird. Can you add a bit more general way of showing revision? > + client->addr); -- 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] 18+ messages in thread
* Re: [v1 3/3] media: ov8856: Implement sensor module revision identification 2020-03-10 14:30 ` Andy Shevchenko @ 2020-03-12 16:37 ` Robert Foss 0 siblings, 0 replies; 18+ messages in thread From: Robert Foss @ 2020-03-12 16:37 UTC (permalink / raw) To: Andy Shevchenko Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Tomasz Figa, ben.kao, linux-mediatek, Dongchun Zhu, Sakari Ailus, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, linux-media Hey Andy, On Tue, 10 Mar 2020 at 15:30, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 10, 2020 at 02:46:03PM +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. > > Are you sure you send latest version? > > I have a déją vu that I have seen this already and this one doesn't address any > comment given. I think pulled a series Dongchun Zhus earlier series apart and used some of it, I may have missed some of the feedback given to his v3. Sorry about that. > > ... > > > + dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n", > > + val, > > > + val == OV8856_1B_MODULE ? "1B" : "unknown revision", > > This is weird. Can you add a bit more general way of showing revision? This is modeled after the ov7251 driver, since that output came in handy during bringup. dev_info(dev, "OV7251 revision %x (%s) detected at address 0x%02x\n", chip_rev, chip_rev == 0x4 ? "1A / 1B" : chip_rev == 0x5 ? "1C / 1D" : chip_rev == 0x6 ? "1E" : chip_rev == 0x7 ? "1F" : "unknown", client->addr); To me this is pretty general approach, at least until this revision information is used in other places. I'm not quite sure what you had in mind. Maybe the current implementation is a little bit clunky in the case of ov8856 since there's only one revision number known currently. Either way, I'll happily change it. But I don't quite know what you have in mind. > > > + client->addr); > > -- > 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] 18+ messages in thread
end of thread, other threads:[~2020-03-12 16:37 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 13:46 [v1 0/3] media: ov8856: Add sensor modes & devicetree support Robert Foss 2020-03-10 13:46 ` [v1 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-10 13:57 ` Fabio Estevam 2020-03-10 15:51 ` Robert Foss 2020-03-12 10:13 ` Robert Foss 2020-03-10 18:38 ` Rob Herring 2020-03-10 13:46 ` [v1 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-10 14:03 ` Fabio Estevam 2020-03-10 15:46 ` Robert Foss 2020-03-10 14:26 ` Andy Shevchenko 2020-03-10 15:55 ` Robert Foss 2020-03-11 9:05 ` Andy Shevchenko 2020-03-11 11:48 ` Sakari Ailus 2020-03-11 13:32 ` Robert Foss 2020-03-11 16:16 ` Sakari Ailus 2020-03-10 13:46 ` [v1 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2020-03-10 14:30 ` Andy Shevchenko 2020-03-12 16:37 ` Robert Foss
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).