[{"id":1770392,"web_url":"http://patchwork.ozlabs.org/comment/1770392/","msgid":"<87k20vswdv.fsf@anholt.net>","list_archive_url":null,"date":"2017-09-18T18:18:52","subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","submitter":{"id":608,"url":"http://patchwork.ozlabs.org/api/people/608/","name":"Eric Anholt","email":"eric@anholt.net"},"content":"Dave Stevenson writes:\n> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n> new file mode 100644\n> index 0000000..5b1adc3\n> --- /dev/null\n> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n> @@ -0,0 +1,2192 @@\n> +/*\n> + * BCM2835 Unicam capture Driver\n> + *\n> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.\n> + *\n> + * Dave Stevenson \n> + *\n> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and\n> + * TI CAL camera interface driver by Benoit Parrot.\n> + *\n\nPossible future improvement: this description of the driver is really\nnice and could be turned into kernel-doc.\n\n> + * There are two camera drivers in the kernel for BCM283x - this one\n> + * and bcm2835-camera (currently in staging).\n> + *\n> + * This driver is purely the kernel control the Unicam peripheral - there\n\nMaybe \"This driver directly controls...\"?\n\n> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2\n> + * or CCP2 data and writes it into SDRAM. The only processing options are\n> + * to repack Bayer data into an alternate format, and applying windowing\n> + * (currently not implemented).\n> + * It should be possible to connect it to any sensor with a\n> + * suitable output interface and V4L2 subdevice driver.\n> + *\n> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,\n\n\"uses the\"\n\n> + * Unicam, ISP, and all tuner control loops. Fully processed frames are\n> + * delivered to the driver by the firmware. It only has sensor drivers\n> + * for Omnivision OV5647, and Sony IMX219 sensors.\n> + *\n> + * The two drivers are mutually exclusive for the same Unicam instance.\n> + * The VideoCore firmware checks the device tree configuration during boot.\n> + * If it finds device tree nodes called csi0 or csi1 it will block the\n> + * firmware from accessing the peripheral, and bcm2835-camera will\n> + * not be able to stream data.\n\nThanks for describing this here!\n\n> +/*\n> + * The peripheral can unpack and repack between several of\n> + * the Bayer raw formats, so any Bayer format can be advertised\n> + * as the same Bayer order in each of the supported bit depths.\n> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8\n> + * formats.\n> + */\n> +#define PIX_FMT_ALL_BGGR v4l2_fourcc('b', 'g', 'g', 'r')\n> +#define PIX_FMT_ALL_RGGB v4l2_fourcc('r', 'g', 'g', 'b')\n> +#define PIX_FMT_ALL_GBRG v4l2_fourcc('g', 'b', 'r', 'g')\n> +#define PIX_FMT_ALL_GRBG v4l2_fourcc('g', 'r', 'b', 'g')\n\nShould thes fourccs be defined in a common v4l2 header, to reserve it\nfrom clashing with others later?\n\nThis is really the only question I have about this driver before seeing\nit merged. As far as me wearing my platform maintainer hat, I'm happy\nwith the driver, and my other little notes are optional.\n\n> +static int unicam_probe(struct platform_device *pdev)\n> +{\n> +\tstruct unicam_cfg *unicam_cfg;\n> +\tstruct unicam_device *unicam;\n> +\tstruct v4l2_ctrl_handler *hdl;\n> +\tstruct resource\t*res;\n> +\tint ret;\n> +\n> +\tunicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);\n> +\tif (!unicam)\n> +\t\treturn -ENOMEM;\n> +\n> +\tunicam->pdev = pdev;\n> +\tunicam_cfg = &unicam->cfg;\n> +\n> +\tres = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n> +\tunicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);\n> +\tif (IS_ERR(unicam_cfg->base)) {\n> +\t\tunicam_err(unicam, \"Failed to get main io block\\n\");\n> +\t\treturn PTR_ERR(unicam_cfg->base);\n> +\t}\n> +\n> +\tres = platform_get_resource(pdev, IORESOURCE_MEM, 1);\n> +\tunicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);\n> +\tif (IS_ERR(unicam_cfg->clk_gate_base)) {\n> +\t\tunicam_err(unicam, \"Failed to get 2nd io block\\n\");\n> +\t\treturn PTR_ERR(unicam_cfg->clk_gate_base);\n> +\t}\n> +\n> +\tunicam->clock = devm_clk_get(&pdev->dev, \"lp_clock\");\n> +\tif (IS_ERR(unicam->clock)) {\n> +\t\tunicam_err(unicam, \"Failed to get clock\\n\");\n> +\t\treturn PTR_ERR(unicam->clock);\n> +\t}\n> +\n> +\tret = platform_get_irq(pdev, 0);\n> +\tif (ret <= 0) {\n> +\t\tdev_err(&pdev->dev, \"No IRQ resource\\n\");\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tunicam_cfg->irq = ret;\n> +\n> +\tret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,\n> +\t\t\t \"unicam_capture0\", unicam);\n\nLooks like there's no need to keep \"irq\" in the device private struct.\n\n> +\tif (ret) {\n> +\t\tdev_err(&pdev->dev, \"Unable to request interrupt\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);\n> +\tif (ret) {\n> +\t\tunicam_err(unicam,\n> +\t\t\t \"Unable to register v4l2 device.\\n\");\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Reserve space for the controls */\n> +\thdl = &unicam->ctrl_handler;\n> +\tret = v4l2_ctrl_handler_init(hdl, 16);\n> +\tif (ret < 0)\n> +\t\tgoto probe_out_v4l2_unregister;\n> +\tunicam->v4l2_dev.ctrl_handler = hdl;\n> +\n> +\t/* set the driver data in platform device */\n> +\tplatform_set_drvdata(pdev, unicam);\n> +\n> +\tret = of_unicam_connect_subdevs(unicam);\n> +\tif (ret) {\n> +\t\tdev_err(&pdev->dev, \"Failed to connect subdevs\\n\");\n> +\t\tgoto free_hdl;\n> +\t}\n> +\n> +\t/* Enabling module functional clock */\n> +\tpm_runtime_enable(&pdev->dev);\n\nI think pm_runtime is only controlling the power domain for the PHY, not\nthe clock (which you're handling manually).","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwvQp0y3Wz9s7m\n\tfor ;\n\tTue, 19 Sep 2017 04:18:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752741AbdIRSS4 (ORCPT\n\t);\n\tMon, 18 Sep 2017 14:18:56 -0400","from anholt.net ([50.246.234.109]:42682 \"EHLO anholt.net\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752046AbdIRSSz (ORCPT );\n\tMon, 18 Sep 2017 14:18:55 -0400","from localhost (localhost [127.0.0.1])\n\tby anholt.net (Postfix) with ESMTP id 4CAC710A13F0;\n\tMon, 18 Sep 2017 11:18:55 -0700 (PDT)","from anholt.net ([127.0.0.1])\n\tby localhost (kingsolver.anholt.net [127.0.0.1]) (amavisd-new,\n\tport 10024)\n\twith LMTP id R-orN-15ngdP; Mon, 18 Sep 2017 11:18:53 -0700 (PDT)","from eliezer.anholt.net (localhost [127.0.0.1])\n\tby anholt.net (Postfix) with ESMTP id 8E35810A05DD;\n\tMon, 18 Sep 2017 11:18:53 -0700 (PDT)","by eliezer.anholt.net (Postfix, from userid 1000)\n\tid 242BF2FE2179; Mon, 18 Sep 2017 11:18:53 -0700 (PDT)"],"X-Virus-Scanned":"Debian amavisd-new at anholt.net","From":"Eric Anholt ","To":"Dave Stevenson ,\n\tMauro Carvalho Chehab ,\n\tRob Herring , Hans Verkuil , \n\tStefan Wahren ,\n\tSakari Ailus , \n\tlinux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,\n\tdevicetree@vger.kernel.org","Cc":"Dave Stevenson ","Subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","In-Reply-To":"<0d4dc119a2ba4917e0fecfe5084425830ec53ccb.1505314390.git.dave.stevenson@raspberrypi.org>","References":"\n\t<0d4dc119a2ba4917e0fecfe5084425830ec53ccb.1505314390.git.dave.stevenson@raspberrypi.org>","User-Agent":"Notmuch/0.22.2+1~gb0bcfaa (http://notmuchmail.org) Emacs/25.2.2\n\t(x86_64-pc-linux-gnu)","Date":"Mon, 18 Sep 2017 11:18:52 -0700","Message-ID":"<87k20vswdv.fsf@anholt.net>","MIME-Version":"1.0","Content-Type":"multipart/signed; boundary=\"=-=-=\";\n\tmicalg=pgp-sha512; protocol=\"application/pgp-signature\"","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1770800,"web_url":"http://patchwork.ozlabs.org/comment/1770800/","msgid":"","list_archive_url":null,"date":"2017-09-19T09:50:50","subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","submitter":{"id":72357,"url":"http://patchwork.ozlabs.org/api/people/72357/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.org"},"content":"Hi Eric.\n\nThanks for the review.\n\nOn 18 September 2017 at 19:18, Eric Anholt wrote:\n> Dave Stevenson writes:\n>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>> new file mode 100644\n>> index 0000000..5b1adc3\n>> --- /dev/null\n>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>> @@ -0,0 +1,2192 @@\n>> +/*\n>> + * BCM2835 Unicam capture Driver\n>> + *\n>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.\n>> + *\n>> + * Dave Stevenson \n>> + *\n>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and\n>> + * TI CAL camera interface driver by Benoit Parrot.\n>> + *\n>\n> Possible future improvement: this description of the driver is really\n> nice and could be turned into kernel-doc.\n\nDocumentation?! Surely not :-)\nFor now I'll leave it as a task for another day.\n\n>> + * There are two camera drivers in the kernel for BCM283x - this one\n>> + * and bcm2835-camera (currently in staging).\n>> + *\n>> + * This driver is purely the kernel control the Unicam peripheral - there\n>\n> Maybe \"This driver directly controls...\"?\n\nWill do in v3.\n\n>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2\n>> + * or CCP2 data and writes it into SDRAM. The only processing options are\n>> + * to repack Bayer data into an alternate format, and applying windowing\n>> + * (currently not implemented).\n>> + * It should be possible to connect it to any sensor with a\n>> + * suitable output interface and V4L2 subdevice driver.\n>> + *\n>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,\n>\n> \"uses the\"\n\nWill do in v3.\n\n>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are\n>> + * delivered to the driver by the firmware. It only has sensor drivers\n>> + * for Omnivision OV5647, and Sony IMX219 sensors.\n>> + *\n>> + * The two drivers are mutually exclusive for the same Unicam instance.\n>> + * The VideoCore firmware checks the device tree configuration during boot.\n>> + * If it finds device tree nodes called csi0 or csi1 it will block the\n>> + * firmware from accessing the peripheral, and bcm2835-camera will\n>> + * not be able to stream data.\n>\n> Thanks for describing this here!\n>\n>> +/*\n>> + * The peripheral can unpack and repack between several of\n>> + * the Bayer raw formats, so any Bayer format can be advertised\n>> + * as the same Bayer order in each of the supported bit depths.\n>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8\n>> + * formats.\n>> + */\n>> +#define PIX_FMT_ALL_BGGR v4l2_fourcc('b', 'g', 'g', 'r')\n>> +#define PIX_FMT_ALL_RGGB v4l2_fourcc('r', 'g', 'g', 'b')\n>> +#define PIX_FMT_ALL_GBRG v4l2_fourcc('g', 'b', 'r', 'g')\n>> +#define PIX_FMT_ALL_GRBG v4l2_fourcc('g', 'r', 'b', 'g')\n>\n> Should thes fourccs be defined in a common v4l2 header, to reserve it\n> from clashing with others later?\n\nI'm only using them as flags and probably in a manner that nothing\nelse is likely to copy, so it seems a little excessive to put them in\na common header.\nPerhaps it's better to switch to 0xFFFFFFF0 to 0xFFFFFFF3 or other\nvalue that won't come up as a fourcc under any normal circumstance.\nAny thoughts from other people?\n\n> This is really the only question I have about this driver before seeing\n> it merged. As far as me wearing my platform maintainer hat, I'm happy\n> with the driver, and my other little notes are optional.\n>\n>> +static int unicam_probe(struct platform_device *pdev)\n>> +{\n>> + struct unicam_cfg *unicam_cfg;\n>> + struct unicam_device *unicam;\n>> + struct v4l2_ctrl_handler *hdl;\n>> + struct resource *res;\n>> + int ret;\n>> +\n>> + unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);\n>> + if (!unicam)\n>> + return -ENOMEM;\n>> +\n>> + unicam->pdev = pdev;\n>> + unicam_cfg = &unicam->cfg;\n>> +\n>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n>> + unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);\n>> + if (IS_ERR(unicam_cfg->base)) {\n>> + unicam_err(unicam, \"Failed to get main io block\\n\");\n>> + return PTR_ERR(unicam_cfg->base);\n>> + }\n>> +\n>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);\n>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);\n>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {\n>> + unicam_err(unicam, \"Failed to get 2nd io block\\n\");\n>> + return PTR_ERR(unicam_cfg->clk_gate_base);\n>> + }\n>> +\n>> + unicam->clock = devm_clk_get(&pdev->dev, \"lp_clock\");\n>> + if (IS_ERR(unicam->clock)) {\n>> + unicam_err(unicam, \"Failed to get clock\\n\");\n>> + return PTR_ERR(unicam->clock);\n>> + }\n>> +\n>> + ret = platform_get_irq(pdev, 0);\n>> + if (ret <= 0) {\n>> + dev_err(&pdev->dev, \"No IRQ resource\\n\");\n>> + return -ENODEV;\n>> + }\n>> + unicam_cfg->irq = ret;\n>> +\n>> + ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,\n>> + \"unicam_capture0\", unicam);\n>\n> Looks like there's no need to keep \"irq\" in the device private struct.\n\nAgreed. I'll remove in v3.\n\n>> + if (ret) {\n>> + dev_err(&pdev->dev, \"Unable to request interrupt\\n\");\n>> + return -EINVAL;\n>> + }\n>> +\n>> + ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);\n>> + if (ret) {\n>> + unicam_err(unicam,\n>> + \"Unable to register v4l2 device.\\n\");\n>> + return ret;\n>> + }\n>> +\n>> + /* Reserve space for the controls */\n>> + hdl = &unicam->ctrl_handler;\n>> + ret = v4l2_ctrl_handler_init(hdl, 16);\n>> + if (ret < 0)\n>> + goto probe_out_v4l2_unregister;\n>> + unicam->v4l2_dev.ctrl_handler = hdl;\n>> +\n>> + /* set the driver data in platform device */\n>> + platform_set_drvdata(pdev, unicam);\n>> +\n>> + ret = of_unicam_connect_subdevs(unicam);\n>> + if (ret) {\n>> + dev_err(&pdev->dev, \"Failed to connect subdevs\\n\");\n>> + goto free_hdl;\n>> + }\n>> +\n>> + /* Enabling module functional clock */\n>> + pm_runtime_enable(&pdev->dev);\n>\n> I think pm_runtime is only controlling the power domain for the PHY, not\n> the clock (which you're handling manually).\n\nYou're right. Copy and paste from the driver I'd based this on.\nWill correct in v3.\n\n Dave\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tsecure) header.d=raspberrypi.org header.i=@raspberrypi.org\n\theader.b=\"xSePZbip\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi-org.20150623.gappssmtp.com\n\theader.i=@raspberrypi-org.20150623.gappssmtp.com header.b=\"ctd1wv5M\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxJ6C3M6Cz9s7F\n\tfor ;\n\tTue, 19 Sep 2017 19:50:59 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751367AbdISJu4 (ORCPT\n\t);\n\tTue, 19 Sep 2017 05:50:56 -0400","from mx07-00252a01.pphosted.com ([62.209.51.214]:15499 \"EHLO\n\tmx07-00252a01.pphosted.com\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751053AbdISJuz (ORCPT\n\t); Tue, 19 Sep 2017 05:50:55 -0400","from pps.filterd (m0102628.ppops.net [127.0.0.1])\n\tby mx07-00252a01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8J9mKax017285\n\tfor ; Tue, 19 Sep 2017 10:50:54 +0100","from mail-pg0-f72.google.com (mail-pg0-f72.google.com\n\t[74.125.83.72])\n\tby mx07-00252a01.pphosted.com with ESMTP id 2d0sc01e8y-1\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128\n\tverify=OK)\n\tfor ; Tue, 19 Sep 2017 10:50:53 +0100","by mail-pg0-f72.google.com with SMTP id 6so5680632pgh.0\n\tfor ;\n\tTue, 19 Sep 2017 02:50:53 -0700 (PDT)","by 10.100.185.135 with HTTP; Tue, 19 Sep 2017 02:50:50 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.org;\n\th=mime-version :\n\tin-reply-to : references : from : date : message-id : subject : to :\n\tcc :\n\tcontent-type; s=pp; bh=6v+D7TYWidxolV1DTP6abo22PaHdTvqtksqCim0nzQc=; \n\tb=xSePZbip2yNnc+PJo0nsvnLX2Tk31kcdhUFSbGRhp3ud0/knuUwwJT6+mLws9/JkTnmX\n\trao4CCVWw8zOj3wGdZ1enQdxe0y8w/u4ZBeN0wrjQ8f6K6JgccKNRuh+1U0xKTiScryH\n\tjibEQPw72IAwxnzrLuKFzqqBUPUrH9wPyEvpFhTn5gsCnvsPVwxM/ayel269Z1Q2wsC5\n\ts9vrFrP1kpZD0gA8yfTpJH2YxAyoRTbOga4ZtAW26f26veszeLSJ9smO7NluedxnSTh/\n\tBm0zWXCYNArmlBemfe2drBbrI1+UuITICzJkx+bN6WbhcCbm7AF1iIh9O+CB+shP44t0\n\tcQ== ","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi-org.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=6v+D7TYWidxolV1DTP6abo22PaHdTvqtksqCim0nzQc=;\n\tb=ctd1wv5MBa4vIS87fAvI7K1IyYj0CQunw7P/PirQE99gSUB94z/3mxU3Sf4E1rV0gR\n\t51y5gsRsTgMIiDYVOyIVIOHCf87Tiqvx9+mMpK2XtDT9AaLt6TDsCm++xhh5FxeRB7MA\n\tD1isjaPh9oedtNRpkHS6irxst5syYacfECP6sRzt2wKjId6FjBxYtyfR8HIdhcCVnScH\n\tv42U/oeILt+EOGWKwidUTzsHGHiUfy3QjNeg7IDUcSFGANciOqsyWVcMYFFRPs/5ZZW9\n\t6phx8IesISskD3kebG0M48YMXrkYItAlM1wPF+wXZ5FSyto+uFbA1DUlXS43cT40fO2l\n\tfKvQ=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=6v+D7TYWidxolV1DTP6abo22PaHdTvqtksqCim0nzQc=;\n\tb=MzhUbDxJJY2FckHhmxhNKya3UUx6NGMWqXfGk84k1WcbakdsI3lXXtsBs6YJg4OQts\n\t6WUCM1lvxzthU6frpBzo378qksoEc5RnTmVdVjuyiSw0dN6N6J0oSW1dn5IpAgQMXSk1\n\t3irDuUQIkiGa4qyZaHGhvbKflSq4C7l06PsIvV0GjRY3eh3fZg6Jio/+lzwoKe61+AcC\n\tEumt3+17bXvikHhGlCBCoLIYcumuuqCS4aoWlm9UlS78KadoRjFf8cUFucnxeSvxPY4f\n\tO4wSuzXXAWfU+gasR+vAL5VUR1ucwbuFPyhWCgY9g7sy8zNU1JV7teQBTNkpzaPrRIlj\n\tYoxA==","X-Gm-Message-State":"AHPjjUgi5CMgzlbQ/jKGv9q5QQ/LqytLItfwnWj4v0epuZG9qwkuBHpb\n\tQ4RK/8iWAh9I50WZr9smz/O8MBP9PPh69MfbvsSDRM1tKYXEqcsCx/F+KPztBWcpmMT11rP0Nij\n\tRomtjKdkSo3TF9sHWrurCSNu27gQkSyDtM4Gi","X-Received":["by 10.99.175.14 with SMTP id w14mr779135pge.365.1505814651543;\n\tTue, 19 Sep 2017 02:50:51 -0700 (PDT)","by 10.99.175.14 with SMTP id w14mr779120pge.365.1505814651120;\n\tTue, 19 Sep 2017 02:50:51 -0700 (PDT)"],"X-Google-Smtp-Source":"AOwi7QCYfHE+Hx3oWJjzEdhJzFQcbwpvlNOUmpkmp5cttWOiS1F+dcu0/7A9VARRJP0MLLwx+h9hToqP/UUlP/GfE20=","MIME-Version":"1.0","In-Reply-To":"<87k20vswdv.fsf@anholt.net>","References":"\n\t<0d4dc119a2ba4917e0fecfe5084425830ec53ccb.1505314390.git.dave.stevenson@raspberrypi.org>\n\t<87k20vswdv.fsf@anholt.net>","From":"Dave Stevenson ","Date":"Tue, 19 Sep 2017 10:50:50 +0100","Message-ID":"","Subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","To":"Eric Anholt ","Cc":"Mauro Carvalho Chehab ,\n\tRob Herring , Hans Verkuil , \n\tStefan Wahren ,\n\tSakari Ailus , \n\tlinux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,\n\tdevicetree@vger.kernel.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_04:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_spam_notspam policy=outbound_spam\n\tscore=0 priorityscore=1501\n\tmalwarescore=0 suspectscore=5 phishscore=0 bulkscore=0 spamscore=0\n\tclxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0\n\tclassifier=spam adjust=0 reason=mlx scancount=1\n\tengine=8.0.1-1707230000\n\tdefinitions=main-1709190139","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1770831,"web_url":"http://patchwork.ozlabs.org/comment/1770831/","msgid":"","list_archive_url":null,"date":"2017-09-19T10:20:17","subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","submitter":{"id":723,"url":"http://patchwork.ozlabs.org/api/people/723/","name":"Hans Verkuil","email":"hverkuil@xs4all.nl"},"content":"On 09/19/17 11:50, Dave Stevenson wrote:\n> Hi Eric.\n> \n> Thanks for the review.\n> \n> On 18 September 2017 at 19:18, Eric Anholt wrote:\n>> Dave Stevenson writes:\n>>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>>> new file mode 100644\n>>> index 0000000..5b1adc3\n>>> --- /dev/null\n>>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>>> @@ -0,0 +1,2192 @@\n>>> +/*\n>>> + * BCM2835 Unicam capture Driver\n>>> + *\n>>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.\n>>> + *\n>>> + * Dave Stevenson \n>>> + *\n>>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and\n>>> + * TI CAL camera interface driver by Benoit Parrot.\n>>> + *\n>>\n>> Possible future improvement: this description of the driver is really\n>> nice and could be turned into kernel-doc.\n> \n> Documentation?! Surely not :-)\n> For now I'll leave it as a task for another day.\n> \n>>> + * There are two camera drivers in the kernel for BCM283x - this one\n>>> + * and bcm2835-camera (currently in staging).\n>>> + *\n>>> + * This driver is purely the kernel control the Unicam peripheral - there\n>>\n>> Maybe \"This driver directly controls...\"?\n> \n> Will do in v3.\n> \n>>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2\n>>> + * or CCP2 data and writes it into SDRAM. The only processing options are\n>>> + * to repack Bayer data into an alternate format, and applying windowing\n>>> + * (currently not implemented).\n>>> + * It should be possible to connect it to any sensor with a\n>>> + * suitable output interface and V4L2 subdevice driver.\n>>> + *\n>>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,\n>>\n>> \"uses the\"\n> \n> Will do in v3.\n> \n>>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are\n>>> + * delivered to the driver by the firmware. It only has sensor drivers\n>>> + * for Omnivision OV5647, and Sony IMX219 sensors.\n>>> + *\n>>> + * The two drivers are mutually exclusive for the same Unicam instance.\n>>> + * The VideoCore firmware checks the device tree configuration during boot.\n>>> + * If it finds device tree nodes called csi0 or csi1 it will block the\n>>> + * firmware from accessing the peripheral, and bcm2835-camera will\n>>> + * not be able to stream data.\n>>\n>> Thanks for describing this here!\n>>\n>>> +/*\n>>> + * The peripheral can unpack and repack between several of\n>>> + * the Bayer raw formats, so any Bayer format can be advertised\n>>> + * as the same Bayer order in each of the supported bit depths.\n>>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8\n>>> + * formats.\n>>> + */\n>>> +#define PIX_FMT_ALL_BGGR v4l2_fourcc('b', 'g', 'g', 'r')\n>>> +#define PIX_FMT_ALL_RGGB v4l2_fourcc('r', 'g', 'g', 'b')\n>>> +#define PIX_FMT_ALL_GBRG v4l2_fourcc('g', 'b', 'r', 'g')\n>>> +#define PIX_FMT_ALL_GRBG v4l2_fourcc('g', 'r', 'b', 'g')\n>>\n>> Should thes fourccs be defined in a common v4l2 header, to reserve it\n>> from clashing with others later?\n> \n> I'm only using them as flags and probably in a manner that nothing\n> else is likely to copy, so it seems a little excessive to put them in\n> a common header.\n> Perhaps it's better to switch to 0xFFFFFFF0 to 0xFFFFFFF3 or other\n> value that won't come up as a fourcc under any normal circumstance.\n> Any thoughts from other people?\n\nI think that's better, yes.\n\n> \n>> This is really the only question I have about this driver before seeing\n>> it merged. As far as me wearing my platform maintainer hat, I'm happy\n>> with the driver, and my other little notes are optional.\n>>\n>>> +static int unicam_probe(struct platform_device *pdev)\n>>> +{\n>>> + struct unicam_cfg *unicam_cfg;\n>>> + struct unicam_device *unicam;\n>>> + struct v4l2_ctrl_handler *hdl;\n>>> + struct resource *res;\n>>> + int ret;\n>>> +\n>>> + unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);\n>>> + if (!unicam)\n>>> + return -ENOMEM;\n>>> +\n>>> + unicam->pdev = pdev;\n>>> + unicam_cfg = &unicam->cfg;\n>>> +\n>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n>>> + unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);\n>>> + if (IS_ERR(unicam_cfg->base)) {\n>>> + unicam_err(unicam, \"Failed to get main io block\\n\");\n>>> + return PTR_ERR(unicam_cfg->base);\n>>> + }\n>>> +\n>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);\n>>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);\n>>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {\n>>> + unicam_err(unicam, \"Failed to get 2nd io block\\n\");\n>>> + return PTR_ERR(unicam_cfg->clk_gate_base);\n>>> + }\n>>> +\n>>> + unicam->clock = devm_clk_get(&pdev->dev, \"lp_clock\");\n>>> + if (IS_ERR(unicam->clock)) {\n>>> + unicam_err(unicam, \"Failed to get clock\\n\");\n>>> + return PTR_ERR(unicam->clock);\n>>> + }\n>>> +\n>>> + ret = platform_get_irq(pdev, 0);\n>>> + if (ret <= 0) {\n>>> + dev_err(&pdev->dev, \"No IRQ resource\\n\");\n>>> + return -ENODEV;\n>>> + }\n>>> + unicam_cfg->irq = ret;\n>>> +\n>>> + ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,\n>>> + \"unicam_capture0\", unicam);\n>>\n>> Looks like there's no need to keep \"irq\" in the device private struct.\n> \n> Agreed. I'll remove in v3.\n> \n>>> + if (ret) {\n>>> + dev_err(&pdev->dev, \"Unable to request interrupt\\n\");\n>>> + return -EINVAL;\n>>> + }\n>>> +\n>>> + ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);\n>>> + if (ret) {\n>>> + unicam_err(unicam,\n>>> + \"Unable to register v4l2 device.\\n\");\n>>> + return ret;\n>>> + }\n>>> +\n>>> + /* Reserve space for the controls */\n>>> + hdl = &unicam->ctrl_handler;\n>>> + ret = v4l2_ctrl_handler_init(hdl, 16);\n>>> + if (ret < 0)\n>>> + goto probe_out_v4l2_unregister;\n>>> + unicam->v4l2_dev.ctrl_handler = hdl;\n>>> +\n>>> + /* set the driver data in platform device */\n>>> + platform_set_drvdata(pdev, unicam);\n>>> +\n>>> + ret = of_unicam_connect_subdevs(unicam);\n>>> + if (ret) {\n>>> + dev_err(&pdev->dev, \"Failed to connect subdevs\\n\");\n>>> + goto free_hdl;\n>>> + }\n>>> +\n>>> + /* Enabling module functional clock */\n>>> + pm_runtime_enable(&pdev->dev);\n>>\n>> I think pm_runtime is only controlling the power domain for the PHY, not\n>> the clock (which you're handling manually).\n> \n> You're right. Copy and paste from the driver I'd based this on.\n> Will correct in v3.\n> \n> Dave\n> \n\nDave, I plan to review this Friday or Monday. It would help me if you could\npost a v3 before Friday so that I'm reviewing the latest code.\n\nIt would be great if you can also post your tc358743 patches. I have an RPi\nwith a tc358743 attached, so it would be very useful if I can review and test\nboth this driver and the tc358743 changes.\n\nI also have a selfish motive: I want to do a CEC demo next week during the\nKernel Recipes conference with my RPi/tc358743 and your driver. It's nice if\nI can use the latest version for that.\n\nRegards,\n\n\tHans\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxJmD6CXDz9s3T\n\tfor ;\n\tTue, 19 Sep 2017 20:20:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751457AbdISKU1 (ORCPT\n\t);\n\tTue, 19 Sep 2017 06:20:27 -0400","from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:49425 \"EHLO\n\tlb2-smtp-cloud8.xs4all.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1750948AbdISKUZ (ORCPT\n\t); Tue, 19 Sep 2017 06:20:25 -0400","from [IPv6:2001:420:44c1:2579:d841:6302:cc6a:1bf0]\n\t([IPv6:2001:420:44c1:2579:d841:6302:cc6a:1bf0])\n\tby smtp-cloud8.xs4all.net with ESMTPA\n\tid uFdZdYiRUb4gvuFdcdkvFo; Tue, 19 Sep 2017 12:20:24 +0200"],"Subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","To":"Dave Stevenson ,\n\tEric Anholt ","Cc":"Mauro Carvalho Chehab ,\n\tRob Herring , Hans Verkuil , \n\tStefan Wahren ,\n\tSakari Ailus , \n\tlinux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,\n\tdevicetree@vger.kernel.org","References":"\n\t<0d4dc119a2ba4917e0fecfe5084425830ec53ccb.1505314390.git.dave.stevenson@raspberrypi.org>\n\t<87k20vswdv.fsf@anholt.net>\n\t","From":"Hans Verkuil ","Message-ID":"","Date":"Tue, 19 Sep 2017 12:20:17 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfGpRdI2/3IT7P0nZRufPPj7ARK4rrUMW/E0jCm3Zgkh9LpVkjbY7CtxCEzIhoJoaG0WDtAVnToZwn6deOW8hM/7H1xUQB9UAtyRRvWptbLpP6nYhh65P\n\tfR9x4chmslbCIt2UIjAxqnnRhHzqEOq/dMioHLfGOYgtmPH+r+bCH67IixcUZjuP8qW/O32iK6ZnmLUXKZlAnhECBcbYSPv9b0lUe2GqLxb8e/IB2fTMceJ4\n\t1fvYeG4rQC/nsnOu2leukxFM3259Yc5EyCNKBFccvnpzZq8SerZ1IRCMXed+fS6CgiARJ+4nc05Qm6nYBphHGN9i6XOnOLd0Rd8aW1so5SQFJGNCbXumjog6\n\tW5bqIMBG77Z5IpguuaBFcijUvX24EQbSfgbHxp/PTnGkuChINQsZ3nsjDE53UTPP2/npkcOB8qt89YxWXEm0Q5krorX9O02DI+3sBPnDn7wA6R7T4CynqNOp\n\tJGYmVU5A30bblHD/kY/nXaFmGlXjPMxRS5M2b1T4xzqcB7tMWhAzsV4CnvRYcsDUTrZl18/UUGN6p5fU9PV8j2EFeXyskw5YkOpftTMW3Uksq+62qNSVqUuE\n\taKkP/F6MCyqjRSzw/otHmy+o09tVmB4NipR5q5pDFNIIFTKkJ4YFdVOpQWVvfMC6QjwWLTGEbMYwvvyXFjfjVp9furwyWa4zn6NNOna4g5KndQ==","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1770895,"web_url":"http://patchwork.ozlabs.org/comment/1770895/","msgid":"","list_archive_url":null,"date":"2017-09-19T11:42:05","subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","submitter":{"id":72357,"url":"http://patchwork.ozlabs.org/api/people/72357/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.org"},"content":"Hi Hans.\n\nThanks for the response.\n\nOn 19 September 2017 at 11:20, Hans Verkuil wrote:\n> On 09/19/17 11:50, Dave Stevenson wrote:\n>> Hi Eric.\n>>\n>> Thanks for the review.\n>>\n>> On 18 September 2017 at 19:18, Eric Anholt wrote:\n>>> Dave Stevenson writes:\n>>>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>>>> new file mode 100644\n>>>> index 0000000..5b1adc3\n>>>> --- /dev/null\n>>>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c\n>>>> @@ -0,0 +1,2192 @@\n>>>> +/*\n>>>> + * BCM2835 Unicam capture Driver\n>>>> + *\n>>>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.\n>>>> + *\n>>>> + * Dave Stevenson \n>>>> + *\n>>>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and\n>>>> + * TI CAL camera interface driver by Benoit Parrot.\n>>>> + *\n>>>\n>>> Possible future improvement: this description of the driver is really\n>>> nice and could be turned into kernel-doc.\n>>\n>> Documentation?! Surely not :-)\n>> For now I'll leave it as a task for another day.\n>>\n>>>> + * There are two camera drivers in the kernel for BCM283x - this one\n>>>> + * and bcm2835-camera (currently in staging).\n>>>> + *\n>>>> + * This driver is purely the kernel control the Unicam peripheral - there\n>>>\n>>> Maybe \"This driver directly controls...\"?\n>>\n>> Will do in v3.\n>>\n>>>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2\n>>>> + * or CCP2 data and writes it into SDRAM. The only processing options are\n>>>> + * to repack Bayer data into an alternate format, and applying windowing\n>>>> + * (currently not implemented).\n>>>> + * It should be possible to connect it to any sensor with a\n>>>> + * suitable output interface and V4L2 subdevice driver.\n>>>> + *\n>>>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,\n>>>\n>>> \"uses the\"\n>>\n>> Will do in v3.\n>>\n>>>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are\n>>>> + * delivered to the driver by the firmware. It only has sensor drivers\n>>>> + * for Omnivision OV5647, and Sony IMX219 sensors.\n>>>> + *\n>>>> + * The two drivers are mutually exclusive for the same Unicam instance.\n>>>> + * The VideoCore firmware checks the device tree configuration during boot.\n>>>> + * If it finds device tree nodes called csi0 or csi1 it will block the\n>>>> + * firmware from accessing the peripheral, and bcm2835-camera will\n>>>> + * not be able to stream data.\n>>>\n>>> Thanks for describing this here!\n>>>\n>>>> +/*\n>>>> + * The peripheral can unpack and repack between several of\n>>>> + * the Bayer raw formats, so any Bayer format can be advertised\n>>>> + * as the same Bayer order in each of the supported bit depths.\n>>>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8\n>>>> + * formats.\n>>>> + */\n>>>> +#define PIX_FMT_ALL_BGGR v4l2_fourcc('b', 'g', 'g', 'r')\n>>>> +#define PIX_FMT_ALL_RGGB v4l2_fourcc('r', 'g', 'g', 'b')\n>>>> +#define PIX_FMT_ALL_GBRG v4l2_fourcc('g', 'b', 'r', 'g')\n>>>> +#define PIX_FMT_ALL_GRBG v4l2_fourcc('g', 'r', 'b', 'g')\n>>>\n>>> Should thes fourccs be defined in a common v4l2 header, to reserve it\n>>> from clashing with others later?\n>>\n>> I'm only using them as flags and probably in a manner that nothing\n>> else is likely to copy, so it seems a little excessive to put them in\n>> a common header.\n>> Perhaps it's better to switch to 0xFFFFFFF0 to 0xFFFFFFF3 or other\n>> value that won't come up as a fourcc under any normal circumstance.\n>> Any thoughts from other people?\n>\n> I think that's better, yes.\n\nOK, happy to do that.\n\n>>\n>>> This is really the only question I have about this driver before seeing\n>>> it merged. As far as me wearing my platform maintainer hat, I'm happy\n>>> with the driver, and my other little notes are optional.\n>>>\n>>>> +static int unicam_probe(struct platform_device *pdev)\n>>>> +{\n>>>> + struct unicam_cfg *unicam_cfg;\n>>>> + struct unicam_device *unicam;\n>>>> + struct v4l2_ctrl_handler *hdl;\n>>>> + struct resource *res;\n>>>> + int ret;\n>>>> +\n>>>> + unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);\n>>>> + if (!unicam)\n>>>> + return -ENOMEM;\n>>>> +\n>>>> + unicam->pdev = pdev;\n>>>> + unicam_cfg = &unicam->cfg;\n>>>> +\n>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n>>>> + unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);\n>>>> + if (IS_ERR(unicam_cfg->base)) {\n>>>> + unicam_err(unicam, \"Failed to get main io block\\n\");\n>>>> + return PTR_ERR(unicam_cfg->base);\n>>>> + }\n>>>> +\n>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);\n>>>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);\n>>>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {\n>>>> + unicam_err(unicam, \"Failed to get 2nd io block\\n\");\n>>>> + return PTR_ERR(unicam_cfg->clk_gate_base);\n>>>> + }\n>>>> +\n>>>> + unicam->clock = devm_clk_get(&pdev->dev, \"lp_clock\");\n>>>> + if (IS_ERR(unicam->clock)) {\n>>>> + unicam_err(unicam, \"Failed to get clock\\n\");\n>>>> + return PTR_ERR(unicam->clock);\n>>>> + }\n>>>> +\n>>>> + ret = platform_get_irq(pdev, 0);\n>>>> + if (ret <= 0) {\n>>>> + dev_err(&pdev->dev, \"No IRQ resource\\n\");\n>>>> + return -ENODEV;\n>>>> + }\n>>>> + unicam_cfg->irq = ret;\n>>>> +\n>>>> + ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,\n>>>> + \"unicam_capture0\", unicam);\n>>>\n>>> Looks like there's no need to keep \"irq\" in the device private struct.\n>>\n>> Agreed. I'll remove in v3.\n>>\n>>>> + if (ret) {\n>>>> + dev_err(&pdev->dev, \"Unable to request interrupt\\n\");\n>>>> + return -EINVAL;\n>>>> + }\n>>>> +\n>>>> + ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);\n>>>> + if (ret) {\n>>>> + unicam_err(unicam,\n>>>> + \"Unable to register v4l2 device.\\n\");\n>>>> + return ret;\n>>>> + }\n>>>> +\n>>>> + /* Reserve space for the controls */\n>>>> + hdl = &unicam->ctrl_handler;\n>>>> + ret = v4l2_ctrl_handler_init(hdl, 16);\n>>>> + if (ret < 0)\n>>>> + goto probe_out_v4l2_unregister;\n>>>> + unicam->v4l2_dev.ctrl_handler = hdl;\n>>>> +\n>>>> + /* set the driver data in platform device */\n>>>> + platform_set_drvdata(pdev, unicam);\n>>>> +\n>>>> + ret = of_unicam_connect_subdevs(unicam);\n>>>> + if (ret) {\n>>>> + dev_err(&pdev->dev, \"Failed to connect subdevs\\n\");\n>>>> + goto free_hdl;\n>>>> + }\n>>>> +\n>>>> + /* Enabling module functional clock */\n>>>> + pm_runtime_enable(&pdev->dev);\n>>>\n>>> I think pm_runtime is only controlling the power domain for the PHY, not\n>>> the clock (which you're handling manually).\n>>\n>> You're right. Copy and paste from the driver I'd based this on.\n>> Will correct in v3.\n>>\n>> Dave\n>>\n>\n> Dave, I plan to review this Friday or Monday. It would help me if you could\n> post a v3 before Friday so that I'm reviewing the latest code.\n\nOK, will do.\nI'll hold off until tomorrow morning in the hope of some response on\nthe DT side.\n\n> It would be great if you can also post your tc358743 patches. I have an RPi\n> with a tc358743 attached, so it would be very useful if I can review and test\n> both this driver and the tc358743 changes.\n\nI'll sort those today. They're all pretty trivial, and are mainly\nallow more combinations of framerate and resolution to work. I've\nstill to do an exhaustive test though, and the Tosh chip can be a\nlittle fickle at times.\n\n> I also have a selfish motive: I want to do a CEC demo next week during the\n> Kernel Recipes conference with my RPi/tc358743 and your driver. It's nice if\n> I can use the latest version for that.\n\nFeel free to be selfish :-)\nI should say that my testing has been done against an in-house\ndesigned board, but the B101 seems to be nigh on identical. The fact\nthat it worked with the V1 patch set should hopefully mean that there\naren't any issues.\n\nThanks\n Dave\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tsecure) header.d=raspberrypi.org header.i=@raspberrypi.org\n\theader.b=\"yxgnRtMN\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi-org.20150623.gappssmtp.com\n\theader.i=@raspberrypi-org.20150623.gappssmtp.com header.b=\"RY6V6tOO\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxLZY44Fwz9s7m\n\tfor ;\n\tTue, 19 Sep 2017 21:42:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751210AbdISLmM (ORCPT\n\t);\n\tTue, 19 Sep 2017 07:42:12 -0400","from mx07-00252a01.pphosted.com ([62.209.51.214]:25863 \"EHLO\n\tmx07-00252a01.pphosted.com\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1750822AbdISLmK (ORCPT\n\t); Tue, 19 Sep 2017 07:42:10 -0400","from pps.filterd (m0102628.ppops.net [127.0.0.1])\n\tby mx07-00252a01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8JBd1sA017726\n\tfor ; Tue, 19 Sep 2017 12:42:09 +0100","from mail-pf0-f199.google.com (mail-pf0-f199.google.com\n\t[209.85.192.199])\n\tby mx07-00252a01.pphosted.com with ESMTP id 2d0sc01fy0-1\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128\n\tverify=OK)\n\tfor ; Tue, 19 Sep 2017 12:42:08 +0100","by mail-pf0-f199.google.com with SMTP id y29so5438242pff.6\n\tfor ;\n\tTue, 19 Sep 2017 04:42:08 -0700 (PDT)","by 10.100.185.135 with HTTP; Tue, 19 Sep 2017 04:42:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.org;\n\th=mime-version :\n\tin-reply-to : references : from : date : message-id : subject : to :\n\tcc :\n\tcontent-type; s=pp; bh=1AGEZnQtLor+t3TaKdyoHzZdP3hHTFL2OCN1M6DVAz4=; \n\tb=yxgnRtMNR74XK/o1loNqCngyrhhUKNH7le1Y/FFEY3QtnXPGm5aoJbELoYH0Q9+M5WZf\n\tGM6KmwuMCWuTp/lLSaA3qcKYTxs9MPwRdLOViLZnl/TZQtuQJUB3JbGSgjehEX91ncs+\n\tPpnxZy4dQ7IqpgPb2iCr55XgYp6Ifo+7A3jU0T8jwMsPEnRTx4xJRpzV1lfdECZ2s2i2\n\tV4teoEpajVpR+E0byGRo7bkW3mx/c2HPntK0qc/Bx+Jc78v1BgfwTTbVzRLCF7FGqadT\n\tRMf/bQ3TXPdQ7E4GDwrY9+DApITWehiLS72hPXjtCnMQ51j+lo+hDnlqLq5HZdguzpvB\n\tXw== ","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi-org.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=1AGEZnQtLor+t3TaKdyoHzZdP3hHTFL2OCN1M6DVAz4=;\n\tb=RY6V6tOOlwiRW78AKWqZ0Pf4x54xOuKKKsk/Dec5VoZtVcDE9zN4mLLHOH6TD4yqEf\n\ta3idtcgrx21NmBgPig+DtNZxSJfcyiXcHVi/cjZk8rLP0SRYNzbUH7stycdmQC5GIZNa\n\tWU9P7j1SUPlfPLqbeIJ5Qxl4fUbvhRRD9AkqWSrsGkA7Re2erbj/bUG3nxUYcACc+szn\n\theTGILhoHZcSfkJHEWYyy4zLWOaJxGm7eprNNRwON9n+SFJRz1woM2TsRjZDube8IHWJ\n\tf7B8Ppk6xHkeB04lZgtWGxhTzt7MzRXKUpJwXRop/VuMT7NlQN+Wa5MXpu4vW2g2u4kP\n\toQyg=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=1AGEZnQtLor+t3TaKdyoHzZdP3hHTFL2OCN1M6DVAz4=;\n\tb=T/S2QtIKKteVrgubhUIUn0et+wIDpvRtf3YL08COVxpe5LZpbArVHeTLuRAK5S2Fv1\n\tGws2/pGRGirEk4Tq4VUKBbKblud+E0GtpPORh+vHpyjmzz95dCgwH5rYUu0B5COiwKTF\n\t+x7mhlYG/Kg6cmeGDrphQKUCcJw5jNGyEE6zXaoHfwGBHztILSZ+n7Sr5RdufnhvgpbE\n\t34Y3ZYtVmHXLTIAPasLEYGGryN+B+h5it/eIuFaCm5AZ5WHdzmefqWpp2X1pWg2SbE6Y\n\ti8bZ+jmI6bUjkgL6TswEKvui5jzyRmMAl/Db7JB1lJx6aGY0Aso9XHTobYAWgcCLx85W\n\tGSng==","X-Gm-Message-State":"AHPjjUgCHbUw8VFCb/hax7UkcgejH/rIVoIcc5jC8LD3a6IKeQk/9oZ4\n\tsTLQ3yDVEZE6rYxZZq5XZQs44c4VB0+5ccoVXg6R2FCkoLRjFLznEROJjGgRhu0QO3HJRcK//QH\n\t7FFejOjaNb5hEmqE8+jVHdl+TG9EvfUD3Phco","X-Received":["by 10.98.158.201 with SMTP id f70mr1060860pfk.162.1505821326706; \n\tTue, 19 Sep 2017 04:42:06 -0700 (PDT)","by 10.98.158.201 with SMTP id f70mr1060839pfk.162.1505821326333; \n\tTue, 19 Sep 2017 04:42:06 -0700 (PDT)"],"X-Google-Smtp-Source":"AOwi7QC6Bofl9GGcEbFffI8Ruow/zVfXEaAz/aYbdT3cYWDcCeWguNtZ58obMlWNcNmgYd4uHCxrgcCtGJYuA/nheDM=","MIME-Version":"1.0","In-Reply-To":"","References":"\n\t<0d4dc119a2ba4917e0fecfe5084425830ec53ccb.1505314390.git.dave.stevenson@raspberrypi.org>\n\t<87k20vswdv.fsf@anholt.net>\n\t\n\t","From":"Dave Stevenson ","Date":"Tue, 19 Sep 2017 12:42:05 +0100","Message-ID":"","Subject":"Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2\n\tcamera interface","To":"Hans Verkuil ","Cc":"Eric Anholt , Mauro Carvalho Chehab ,\n\tRob Herring , Hans Verkuil , \n\tStefan Wahren ,\n\tSakari Ailus , \n\tlinux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,\n\tdevicetree@vger.kernel.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-19_05:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_spam_notspam policy=outbound_spam\n\tscore=0 priorityscore=1501\n\tmalwarescore=0 suspectscore=5 phishscore=0 bulkscore=0 spamscore=0\n\tclxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0\n\tclassifier=spam adjust=0 reason=mlx scancount=1\n\tengine=8.0.1-1707230000\n\tdefinitions=main-1709190166","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]