From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9AB04CD128A for ; Sat, 6 Apr 2024 05:27:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=QJMiNfXFRDQs2qM5SC6gbyLqQml0rRpXpmzL19RHbuU=; b=PGddkbIzE4VI4t q1ihq3tIlMk+UKGt3y3lDvpFFoipT4BGp0nqUqw8Ru2sJoCT5XzQFTbYjGExHi/nUml82ouA5m6+t pXXMcJu0ShnyVvaAs2gQ8yuFPeqGKwTUQ88HjLdCqPhYhu8FAos0r78/TK+A/LpqSBIgV2FbL/HmH rK4LnSlZpHJEq8So0YZquuaauk+OjL2m4VlDATqMxBiFetZUmeVc5G1Lmx2CIVN2Kr0NAlOQMWZ6J AGoCqd56HutwGl6s1CUUstHQvn4VChuPI7o2nWG5+jwdzWq2NFLT9IJsVzyx+KpsVp7OTJo/K6t1Y C8VL3G9veiFKdoXRowdg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsyZe-00000009iKs-3Gu3; Sat, 06 Apr 2024 05:26:42 +0000 Received: from mail-108-mta247.mxroute.com ([136.175.108.247]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsyZb-00000009iKT-0fgQ for linux-arm-kernel@lists.infradead.org; Sat, 06 Apr 2024 05:26:40 +0000 Received: from filter006.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta247.mxroute.com (ZoneMTA) with ESMTPSA id 18eb1daa55d0003bea.00a for (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384); Sat, 06 Apr 2024 05:21:20 +0000 X-Zone-Loop: e11e9b68d0ebe8b6f745fa29d35dbf698605e2e9592b X-Originating-IP: [136.175.111.2] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=luigi311.com; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: From:References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=BNON1J0zR9mGYVO4Em2CLHDLLvir7/ucqCdtp1IZ9CM=; b=lYYo38LYiW6zyh7FZufIl1WrzC eisaOu/2/JNj0Asq7SEj5D8O1BNbyKV8Xxzme3LXfzwSoQlbvuUhsIFqujBChYeqh09PebGSaquFJ EKgVdk9UoTU0WmhHy0ck2qrk9wP/jyLcbw5Ac2p8SJ1SJfq9Th9xsMfrilUwnOtW8Im91SlXAOPSI 56nA9jIB22PoEB4yAN2wZ1yhzx0RmXBG4NJV8UbUI2sEv2JNmcLAzzvsGq4lkt0owpfehT9W9D5Oh 0A9TfIFZAuba817gNb/gBw2rZffqNoLcm36yN6yDAkcnEX2hAQSJkU/xWsYPVb8aaGixuoHkIx3KD Wr7P1BjQ==; Message-ID: <3050aed0-df58-440f-87b9-d91d30382c41@luigi311.com> Date: Fri, 5 Apr 2024 23:21:15 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 21/25] drivers: media: i2c: imx258: Use macros To: Tommaso Merciai Cc: Sakari Ailus , linux-media@vger.kernel.org, dave.stevenson@raspberrypi.com, jacopo.mondi@ideasonboard.com, mchehab@kernel.org, robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, devicetree@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, pavel@ucw.cz, phone-devel@vger.kernel.org, Ondrej Jirman References: <20240403150355.189229-1-git@luigi311.com> <20240403150355.189229-22-git@luigi311.com> <082190a8-7ac5-4240-9a16-6b9168c67d57@luigi311.com> Content-Language: en-US From: Luis Garcia In-Reply-To: X-Authenticated-Id: git@luigi311.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240405_222639_297285_FF799ACB X-CRM114-Status: GOOD ( 29.95 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 4/5/24 08:11, Tommaso Merciai wrote: > Hi Luis, > > On Fri, Apr 05, 2024 at 04:33:38AM -0600, Luis Garcia wrote: >> On 4/4/24 00:46, Sakari Ailus wrote: >>> On Wed, Apr 03, 2024 at 01:17:26PM -0600, Luigi311 wrote: >>>> On 4/3/24 10:23, Sakari Ailus wrote: >>>>> Hi Luis, >>>>> >>>>> On Wed, Apr 03, 2024 at 09:03:50AM -0600, git@luigi311.com wrote: >>>>>> From: Luis Garcia >>>>>> >>>>>> Use understandable macros instead of raw values. >>>>>> >>>>>> Signed-off-by: Ondrej Jirman >>>>>> Signed-off-by: Luis Garcia >>>>>> --- >>>>>> drivers/media/i2c/imx258.c | 434 ++++++++++++++++++------------------- >>>>>> 1 file changed, 207 insertions(+), 227 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >>>>>> index e2ecf6109516..30352c33f63c 100644 >>>>>> --- a/drivers/media/i2c/imx258.c >>>>>> +++ b/drivers/media/i2c/imx258.c >>>>>> @@ -33,8 +33,6 @@ >>>>>> #define IMX258_VTS_30FPS_VGA 0x034c >>>>>> #define IMX258_VTS_MAX 65525 >>>>>> >>>>>> -#define IMX258_REG_VTS 0x0340 >>>>>> - >>>>>> /* HBLANK control - read only */ >>>>>> #define IMX258_PPL_DEFAULT 5352 >>>>>> >>>>>> @@ -90,6 +88,53 @@ >>>>>> #define IMX258_PIXEL_ARRAY_WIDTH 4208U >>>>>> #define IMX258_PIXEL_ARRAY_HEIGHT 3120U >>>>>> >>>>>> +/* regs */ >>>>>> +#define IMX258_REG_PLL_MULT_DRIV 0x0310 >>>>>> +#define IMX258_REG_IVTPXCK_DIV 0x0301 >>>>>> +#define IMX258_REG_IVTSYCK_DIV 0x0303 >>>>>> +#define IMX258_REG_PREPLLCK_VT_DIV 0x0305 >>>>>> +#define IMX258_REG_IOPPXCK_DIV 0x0309 >>>>>> +#define IMX258_REG_IOPSYCK_DIV 0x030b >>>>>> +#define IMX258_REG_PREPLLCK_OP_DIV 0x030d >>>>>> +#define IMX258_REG_PHASE_PIX_OUTEN 0x3030 >>>>>> +#define IMX258_REG_PDPIX_DATA_RATE 0x3032 >>>>>> +#define IMX258_REG_SCALE_MODE 0x0401 >>>>>> +#define IMX258_REG_SCALE_MODE_EXT 0x3038 >>>>>> +#define IMX258_REG_AF_WINDOW_MODE 0x7bcd >>>>>> +#define IMX258_REG_FRM_LENGTH_CTL 0x0350 >>>>>> +#define IMX258_REG_CSI_LANE_MODE 0x0114 >>>>>> +#define IMX258_REG_X_EVN_INC 0x0381 >>>>>> +#define IMX258_REG_X_ODD_INC 0x0383 >>>>>> +#define IMX258_REG_Y_EVN_INC 0x0385 >>>>>> +#define IMX258_REG_Y_ODD_INC 0x0387 >>>>>> +#define IMX258_REG_BINNING_MODE 0x0900 >>>>>> +#define IMX258_REG_BINNING_TYPE_V 0x0901 >>>>>> +#define IMX258_REG_FORCE_FD_SUM 0x300d >>>>>> +#define IMX258_REG_DIG_CROP_X_OFFSET 0x0408 >>>>>> +#define IMX258_REG_DIG_CROP_Y_OFFSET 0x040a >>>>>> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH 0x040c >>>>>> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT 0x040e >>>>>> +#define IMX258_REG_SCALE_M 0x0404 >>>>>> +#define IMX258_REG_X_OUT_SIZE 0x034c >>>>>> +#define IMX258_REG_Y_OUT_SIZE 0x034e >>>>>> +#define IMX258_REG_X_ADD_STA 0x0344 >>>>>> +#define IMX258_REG_Y_ADD_STA 0x0346 >>>>>> +#define IMX258_REG_X_ADD_END 0x0348 >>>>>> +#define IMX258_REG_Y_ADD_END 0x034a >>>>>> +#define IMX258_REG_EXCK_FREQ 0x0136 >>>>>> +#define IMX258_REG_CSI_DT_FMT 0x0112 >>>>>> +#define IMX258_REG_LINE_LENGTH_PCK 0x0342 >>>>>> +#define IMX258_REG_SCALE_M_EXT 0x303a >>>>>> +#define IMX258_REG_FRM_LENGTH_LINES 0x0340 >>>>>> +#define IMX258_REG_FINE_INTEG_TIME 0x0200 >>>>>> +#define IMX258_REG_PLL_IVT_MPY 0x0306 >>>>>> +#define IMX258_REG_PLL_IOP_MPY 0x030e >>>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H 0x0820 >>>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L 0x0822 >>>>>> + >>>>>> +#define REG8(a, v) { a, v } >>>>>> +#define REG16(a, v) { a, ((v) >> 8) & 0xff }, { (a) + 1, (v) & 0xff } >>>>> >>>>> The patch is nice but these macros are better replaced by the V4L2 CCI >>>>> helper that also offers register access functions. Could you add a patch to >>>>> convert the driver to use it (maybe after this one)? >>>>> >>>> >>>> Ohh perfect, using something else would be great. Ill go ahead and see >>>> if I can get that working. >>> >>> Thanks. It may be easier to just do it in this one actually. Up to you. >>> >> >> I've made the swap but looks like its not playing nice with my ppp, >> its causing a crash and showing a call trace as soon as it does its >> first read to check the identity. I went in and dropped the cci_read >> and left it with the original implementation and I'm getting a very >> similar crash with cci_write too so it looks like its not liking >> how I'm implementing it. Looking at the few other drivers that were >> swapped over to use that, I don't seem to be missing anything. It's >> a big change so its not really something I can describe what I've >> changed but I do have the change on my github here >> https://github.com/luigi311/linux/commit/840593acb20eee87ce361e6929edf51eefbbe737 > > I checked your commit to switch to cci helper. > I think you are missing the right cci regmap initialization. > > Please take care to use: devm_cci_regmap_init_i2c > > /** > * devm_cci_regmap_init_i2c() - Create regmap to use with cci_*() register > * access functions > * > * @client: i2c_client to create the regmap for > * @reg_addr_bits: register address width to use (8 or 16) > * > * Note the memory for the created regmap is devm() managed, tied to the client. > * > * Return: %0 on success or a negative error code on failure. > */ > > Check drivers/media/i2c/imx290.c:1530 > Hope this help :) > > Note: > Somewhere into the github commit you are reading 16bit reg and storing > that into 64bit val. Take care! :) > > Thanks & Regards, > Tommaso > > Ohh my god your right, looks like i missed that when i was replicating the cci patch for that same driver lol. Looks like that fixed it and the driver is now loading so i can continue to debug it and make sure everything is set correct since i had a few that were messed up since i couldn't test it. >> if you can provide some guidance, if not I can skip this change >> all together and we can do a separate attempt at swapping over to it. >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel