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 510B8C83F33 for ; Tue, 5 Sep 2023 11:56: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:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Hg/pBs3IsyIutfFA++aNbYmAcFZkQ73mMAQZqQYTENQ=; b=qcKbJOKugPy7K8 VBXGljR6NTLIGTlc0jPSLqtidd54zRkpl/ccSNn3Leby1MHKFie60F9nJSfQtqro1n4P+5uwiBFkU 7giSQdO3+0qIEbiPFoo/KxzDTIF52F+oPnZExBeNEFj9K0muk0eP6Tfu+3UalWuy5DTeJpAmVzi9k 0sna/oLVZbyFwgDj/IXHKv7so55067p2nIPMQc1pxixtmzIGS2BCAX2T6AzCuBKJ5UWvRGkdq5IxU w56AdMPOVpev2xacPVAi1viqHg553jlOIH9ybSRVQlQZgB1BsBuJaF6S03uBBWeEJEKwKkI5ifaPg aL31PaD9Cumt+5GYhAxw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qdUeh-005xK4-2s; Tue, 05 Sep 2023 11:55:39 +0000 Received: from mgamail.intel.com ([134.134.136.31]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qdUee-005xIS-1g for linux-arm-kernel@lists.infradead.org; Tue, 05 Sep 2023 11:55:38 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693914936; x=1725450936; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=/cTDMMGwDXqGrvj5wts+NYLjOYzxjei9B4vYLZWvOR0=; b=KrPBYtt6J05oZptHafCOJFqIpPjjXlnSl4aDnrLG7C7TLajRBBdqb4mo iUBl+BQOna5fIu47NEHQt6bEoi0w6DCKv5RsaGjDHZjbMhc6Uksv+w7kp /fGPivDx1Y643KpHzSGvG9SW9AOCj7tSvLsdgArFLaWAnvv7wOQrbJtlx JDs+aGPXNHP7Q5eLFOpjNjxqRCFujWee8h0mHPUTGvyMxWfxild/npHFP NoqvZv2YF0nZrspDY9S0lqX3G9got4QKND+ANIInE5akpWkrsOEY6pyJA wFy3Zwpu0xCDsH16Iu0GTjersf0tphjDWYnnQAmavvx055j3e5vfDWqKY Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="440751298" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="440751298" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="884290537" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="884290537" Received: from unknown (HELO smile.fi.intel.com) ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:17 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1qdUeN-006fwq-2t; Tue, 05 Sep 2023 14:55:19 +0300 Date: Tue, 5 Sep 2023 14:55:19 +0300 From: Andy Shevchenko To: Ryan Chen Cc: "jk@codeconstruct.com.au" , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Rob Herring , Krzysztof Kozlowski , Andrew Jeffery , Philipp Zabel , Wolfram Sang , "linux-i2c@vger.kernel.org" , Florian Fainelli , Jean Delvare , William Zhang , Tyrone Ting , Tharun Kumar P , Conor Dooley , Phil Edworthy , "openbmc@lists.ozlabs.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-aspeed@lists.ozlabs.org" , "=linux-kernel@vger.kernel.org" <=linux-kernel@vger.kernel.org>, Andi Shyti Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230714074522.23827-1-ryan_chen@aspeedtech.com> <20230714074522.23827-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230905_045536_622917_B0CD4FAE X-CRM114-Status: GOOD ( 31.76 ) 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 Tue, Sep 05, 2023 at 06:52:37AM +0000, Ryan Chen wrote: > > From: Andy Shevchenko > > Sent: Friday, July 14, 2023 4:55 PM > > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote: ... > > > +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & > > GENMASK(7, 0)) > > > > > +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) & > > GENMASK(5, 0)) > > > > > +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) & > > GENMASK(4, 0)) + 1) > > > > With right shifts it's better to have GENMASK to be applied first, it will show > > exact MSB of the bitfield. > > > > (Ditto for other cases similar to these) > It will update next version. > #define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) > #define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) Note, these were just an example, check _all_ of the similar cases. In general any reviewer's comment should be considered for the entire code where it makes sense. ... > divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz); > for_each_set_bit(divisor, &divisor, 32) { This is wrong. > if ((divisor + 1) <= 32) > break; > divisor >>= 1; And this. > baseclk_idx++; > } for_each_set_bit() should use two different variables. > } else { > baseclk_idx = i + 1; > divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > } > } ... > divisor = min_t(unsigned long, divisor, 32); Can't you use min()? min_t is a beast with some subtle corner cases. ... > Sorry I don't catch this split slave out to separate change? > Do you mean go for another file name example ast2600_i2c_slave.c ? No, I mean patch 1: Introduce the driver with only master support patch 2: Add slave capability (all what is under ifdeffery for I2C_SLAVE) ... > static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > { > struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > int ret; This is not needed, you may return directly. > /* send start */ > dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > msg->len, msg->len > 1 ? "s" : "", > msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > i2c_bus->master_xfer_cnt = 0; > i2c_bus->buf_index = 0; > if (msg->flags & I2C_M_RD) { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_rx(i2c_bus); return ...; if ... > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_rx(i2c_bus); > else > ret = ast2600_i2c_setup_byte_rx(i2c_bus); > } else { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > else > ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); Same way. > } > > return ret; > } ... > > Wrong memory accessors. You should use something from asm/byteorder.h > > which includes linux/byteorder/generic.h. > > Sorry, about these parts. I quit no idea. > This is chip register limited, it only support dword write, not support byte write. > So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. > Or I may need your help point me a good way. > for (i = 0; i < xfer_len; i++) { > wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > if (i % 4 == 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - 3); > } > } > > if (--i % 4 != 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > } Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind. With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle). -- 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