Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757392AbdLUCja (ORCPT ); Wed, 20 Dec 2017 21:39:30 -0500 Received: from out20-38.mail.aliyun.com ([115.124.20.38]:53653 "EHLO out20-38.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755928AbdLUCj1 (ORCPT ); Wed, 20 Dec 2017 21:39:27 -0500 X-Alimail-AntiSpam: AC=CONTINUE;BC=0.07496803|-1;CH=green;FP=0|0|0|0|0|-1|-1|-1;HT=e02c03300;MF=yong.deng@magewell.com;NM=1;PH=DS;RN=25;RT=25;SR=0;TI=SMTPD_---.9oSzJrE_1513823938; Date: Thu, 21 Dec 2017 10:38:58 +0800 From: Yong To: Chen-Yu Tsai Cc: Maxime Ripard , Mauro Carvalho Chehab , Rob Herring , Mark Rutland , "David S. Miller" , Greg Kroah-Hartman , Hans Verkuil , Randy Dunlap , Benoit Parrot , Stanimir Varbanov , Arnd Bergmann , Hugues Fruchet , Philipp Zabel , Benjamin Gaignard , Ramesh Shanmugasundaram , Yannick Fertre , Sakari Ailus , Rick Chang , Linux Media Mailing List , devicetree , linux-arm-kernel , linux-kernel , linux-sunxi , =?UTF-8?B?T25kxZllag==?= Jirman Subject: Re: [linux-sunxi] [PATCH v3 1/3] media: V3s: Add support for Allwinner CSI. Message-Id: <20171221103858.94ead278ccc591ff218e72f6@magewell.com> In-Reply-To: References: <1510558216-43800-1-git-send-email-yong.deng@magewell.com> X-Mailer: Sylpheed 3.6.0 (GTK+ 2.24.30; i686-pc-mingw32) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6523 Lines: 165 Hi, On Tue, 19 Dec 2017 18:35:49 +0800 Chen-Yu Tsai wrote: > On Mon, Nov 13, 2017 at 3:30 PM, Yong Deng wrote: > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface > > and CSI1 is used for parallel interface. This is not documented in > > datasheet but by testing and guess. > > > > This patch implement a v4l2 framework driver for it. > > > > Currently, the driver only support the parallel interface. MIPI-CSI2, > > ISP's support are not included in this patch. > > > > Signed-off-by: Yong Deng > > --- > > drivers/media/platform/Kconfig | 1 + > > drivers/media/platform/Makefile | 2 + > > drivers/media/platform/sunxi/sun6i-csi/Kconfig | 9 + > > drivers/media/platform/sunxi/sun6i-csi/Makefile | 3 + > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 918 +++++++++++++++++++++ > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 146 ++++ > > .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 203 +++++ > > .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 722 ++++++++++++++++ > > .../media/platform/sunxi/sun6i-csi/sun6i_video.h | 61 ++ > > 9 files changed, 2065 insertions(+) > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c > > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h > > > > [...] > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c > > new file mode 100644 > > index 0000000..0cebcbd > > --- /dev/null > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c > > [...] > > > +/* ----------------------------------------------------------------------------- > > + * Media Operations > > + */ > > +static int sun6i_video_formats_init(struct sun6i_video *video) > > +{ > > + struct v4l2_subdev_mbus_code_enum mbus_code = { 0 }; > > + struct sun6i_csi *csi = video->csi; > > + struct v4l2_format format; > > + struct v4l2_subdev *subdev; > > + u32 pad; > > + const u32 *pixformats; > > + int pixformat_count = 0; > > + u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */ > > + int codes_count = 0; > > + int num_fmts = 0; > > + int i, j; > > + > > + subdev = sun6i_video_remote_subdev(video, &pad); > > + if (subdev == NULL) > > + return -ENXIO; > > + > > + /* Get supported pixformats of CSI */ > > + pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats); > > + if (pixformat_count <= 0) > > + return -ENXIO; > > + > > + /* Get subdev formats codes */ > > + mbus_code.pad = pad; > > + mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, > > + &mbus_code)) { > > + if (codes_count >= ARRAY_SIZE(subdev_codes)) { > > + dev_warn(video->csi->dev, > > + "subdev_codes array is full!\n"); > > + break; > > + } > > + subdev_codes[codes_count] = mbus_code.code; > > + codes_count++; > > + mbus_code.index++; > > + } > > + > > + if (!codes_count) > > + return -ENXIO; > > + > > + /* Get supported formats count */ > > + for (i = 0; i < codes_count; i++) { > > + for (j = 0; j < pixformat_count; j++) { > > + if (!sun6i_csi_is_format_support(csi, pixformats[j], > > + mbus_code.code)) { > > Bug here. You are testing against mbus_code.code, which would be whatever > was leftover from the previous section. Instead you should be testing > against subdev_codes[i], the list you just built. > > Without it, it won't get past this part of the code if the last enumerated > media bus format isn't supported. > > > + continue; > > + } > > + num_fmts++; > > + } > > + } > > + > > + if (!num_fmts) > > + return -ENXIO; > > + > > + video->num_formats = num_fmts; > > + video->formats = devm_kcalloc(video->csi->dev, num_fmts, > > + sizeof(struct sun6i_csi_format), GFP_KERNEL); > > + if (!video->formats) > > + return -ENOMEM; > > + > > + /* Get supported formats */ > > + num_fmts = 0; > > + for (i = 0; i < codes_count; i++) { > > + for (j = 0; j < pixformat_count; j++) { > > + if (!sun6i_csi_is_format_support(csi, pixformats[j], > > + mbus_code.code)) { > > Same here. > > This gets me past the enumeration part of things... > > > + continue; > > + } > > + > > + video->formats[num_fmts].fourcc = pixformats[j]; > > + video->formats[num_fmts].mbus_code = > > + mbus_code.code; > > + video->formats[num_fmts].bpp = > > + v4l2_pixformat_get_bpp(pixformats[j]); > > + num_fmts++; > > + } > > + } > > + > > + /* setup default format */ > > + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > + format.fmt.pix.width = 1280; > > + format.fmt.pix.height = 720; > > + format.fmt.pix.pixelformat = video->formats[0].fourcc; > > + sun6i_video_set_fmt(video, &format); > > But my system crashes here within the OV5640 driver. > So no tests about the actual functionality. This was on the Bananapi M3, > which has an A83T SoC. > > > In general I think you should make your driver much more noisy than it > currently is. I spent the whole afternoon adding error messages and > debug traces to narrow down the issue. Sorry for making such a simple mistake. > > ChenYu Thanks, Yong