Hi Hans,
Thank you for the patch.
On Wednesday 17 Aug 2016 08:29:41 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> Add DT support. Use it to get the reset and pwdn pins (if there are any).
> Tested with one sensor requiring reset/pwdn and one sensor that doesn't
> have reset/pwdn pins.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> .../devicetree/bindings/media/i2c/ov7670.txt | 44 +++++++++++++++++
> MAINTAINERS | 1 +
> drivers/media/i2c/ov7670.c | 51 +++++++++++++++++++
> 3 files changed, 96 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7670.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt new file mode
> 100644
> index 0000000..3231c47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> @@ -0,0 +1,44 @@
> +* Omnivision OV7670 CMOS sensor
> +
> +The Omnivision OV7670 sensor support multiple resolutions output, such as
s/support/supports/
> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.
s/format/formats/ (and possibly s/can support/can support the/)
> +
> +Required Properties:
> +- compatible: should be "ovti,ov7670"
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".
> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + i2c1: i2c@f0018000 {
> + status = "okay";
> +
> + ov7670: camera@0x21 {
> + compatible = "ovti,ov7670";
> + reg = <0x21>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_pck0_as_isi_mck
> &pinctrl_sensor_power
> &pinctrl_sensor_reset>;
The pinctrl properties should be part of the clock provider DT node.
> + resetb-gpios = <&pioE 11 GPIO_ACTIVE_LOW>;
> + pwdn-gpios = <&pioE 13 GPIO_ACTIVE_HIGH>;
> + clocks = <&pck0>;
> + clock-names = "xvclk";
> + assigned-clocks = <&pck0>;
> + assigned-clock-rates = <24000000>;
You should compute and set the clock rate dynamically in the driver, not
hardcode it in DT.
> + port {
> + ov7670_0: endpoint {
> + remote-endpoint = <&isi_0>;
> + bus-width = <8>;
> + };
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 20bb1d0..1fec3a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8615,6 +8615,7 @@ L: linux-media@xxxxxxxxxxxxxxx
> T: git git://linuxtv.org/media_tree.git
> S: Maintained
> F: drivers/media/i2c/ov7670.c
> +F: Documentation/devicetree/bindings/media/i2c/ov7670.txt
>
> ONENAND FLASH DRIVER
> M: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 57adf3d..a401b99 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -17,6 +17,9 @@
> #include <linux/i2c.h>
> #include <linux/delay.h>
> #include <linux/videodev2.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_gpio.h>
I don't think you need of_gpio.h.
> #include <media/v4l2-device.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-clk.h>
> @@ -231,6 +234,8 @@ struct ov7670_info {
> };
> struct ov7670_format_struct *fmt; /* Current format */
> struct v4l2_clk *clk;
> + struct gpio_desc *resetb_gpio;
> + struct gpio_desc *pwdn_gpio;
> int min_width; /* Filter out smaller sizes */
> int min_height; /* Filter out smaller sizes */
> int clock_speed; /* External clock speed (MHz) */
> @@ -1551,6 +1556,40 @@ static const struct ov7670_devtype ov7670_devdata[] =
> { },
> };
>
> +static int ov7670_probe_dt(struct i2c_client *client,
The function doesn't actually depend on DT, how about calling it
ov7670_init_gpio() or something similar ?
> + struct ov7670_info *info)
> +{
> + /* Request the reset GPIO deasserted */
> + info->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> + GPIOD_OUT_LOW);
> + if (!info->resetb_gpio)
> + dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
I don't think the debug message is really worth it, but that's up to you.
> + else if (IS_ERR(info->resetb_gpio)) {
> + dev_info(&client->dev, "no resetb\n");
If you write this "no %s\n", "resetb" and do the same for the "no pwdn\n"
string below you will save a bit of memory as all strings will be shared. I
would also write it as "can't get %s GPIO\n" (or something similar), the "no
%s\n" message seems a bit confusing to me.
> + return PTR_ERR(info->resetb_gpio);
> + }
> +
> + /* Request the power down GPIO asserted */
> + info->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> + GPIOD_OUT_LOW);
> + if (!info->pwdn_gpio)
> + dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
> + else if (IS_ERR(info->pwdn_gpio)) {
> + dev_info(&client->dev, "no pwdn\n");
> + return PTR_ERR(info->pwdn_gpio);
> + }
> +
> + if (info->resetb_gpio) {
> + /* Active the resetb pin to perform a reset pulse */
> + gpiod_direction_output(info->resetb_gpio, 1);
> + usleep_range(3000, 5000);
> + gpiod_direction_output(info->resetb_gpio, 0);
> + }
> + usleep_range(3000, 5000);
Do you need to sleep if you can't reset the sensor ?
> +
> + return 0;
> +}
> +
> static int ov7670_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1596,6 +1635,10 @@ static int ov7670_probe(struct i2c_client *client,
> return -EPROBE_DEFER;
> v4l2_clk_enable(info->clk);
>
> + ret = ov7670_probe_dt(client, info);
> + if (ret)
> + return ret;
> +
> info->clock_speed = v4l2_clk_get_rate(info->clk) / 1000000;
> if (info->clock_speed < 12 ||
> info->clock_speed > 48)
> @@ -1707,6 +1750,14 @@ static const struct i2c_device_id ov7670_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, ov7670_id);
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov7670_of_match[] = {
> + { .compatible = "ovti,ov7670", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov7670_of_match);
> +#endif
> +
> static struct i2c_driver ov7670_driver = {
> .driver = {
> .name = "ov7670",
Shouldn't you set .of_match_table() here ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
