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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98D97C77B78 for ; Tue, 2 May 2023 20:33:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229605AbjEBUdX (ORCPT ); Tue, 2 May 2023 16:33:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229506AbjEBUdW (ORCPT ); Tue, 2 May 2023 16:33:22 -0400 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DCB1819A2; Tue, 2 May 2023 13:33:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1683059600; x=1714595600; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=1FXPdk1irRsOX44r9OYmKGk0aEfOim/Li+bhY4Tx/p0=; b=TEvsodSaLaM+LN14ckhidXGKIFYJkAAzodVf3tcX7Y4Q8AgieDV1/dx/ uDu0wCLpBQ5KUy8ShVgxOQ2nqQRdRZ8FgA6FFs7dnXiuP1BgpFHvsmew4 KLtWzBzJJWtC2RBq0WEHMoEv3gT2jNS97b8vEzZkybp33wrnaQ46R9Q5B UO66a0My2qvtn4oS6gtcrTte4dnftoR+RQFSgC1tS+XVGvi7QeE0K759a FFKJyEWywPWwN0qcoA/tA5WitjN+Iglqn3lRSdQpiwkc9bCJdNKJy3JsR qdzB8wQaN3CzbzgrnbVePMwNqZtL3x5GGF+jo2EAmN0os0ENLuoo+nRHL A==; X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="328866785" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="328866785" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 May 2023 13:32:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="1026223663" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="1026223663" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga005.fm.intel.com with ESMTP; 02 May 2023 13:32:07 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1ptwfM-008Dpy-13; Tue, 02 May 2023 23:32:04 +0300 Date: Tue, 2 May 2023 23:32:04 +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, Andi Shyti Subject: Re: [PATCH v11 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230430041712.3247998-1-ryan_chen@aspeedtech.com> <20230430041712.3247998-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230430041712.3247998-3-ryan_chen@aspeedtech.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Sun, Apr 30, 2023 at 12:17:12PM +0800, Ryan Chen wrote: > Add i2c new register mode driver to support AST2600 i2c > new register mode. AST2600 i2c controller have legacy and > new register mode. The new register mode have global register > support 4 base clock for scl clock selection, and new clock > divider mode. The i2c new register mode have separate register > set to control i2c master and slave. ... > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Ordered? ... > +#define AST2600_GLOBAL_INIT \ > + (AST2600_I2CG_CTRL_NEW_REG | \ > + AST2600_I2CG_CTRL_NEW_CLK_DIV) Make just a one TAB and put the last two lines on the single one. ... > +#define I2CCG_DIV_CTRL 0xC6411208 Is it decimal? Is it combination of bitfields? Can you add a comment what is this magic? ... > +struct ast2600_i2c_timing_table { > + u32 divisor; > + u32 timing; > +}; Is it even used? ... > +enum xfer_mode { > + BYTE_MODE = 0, Why explicit assignment? > + BUFF_MODE, > + DMA_MODE, > +}; ... > + base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2); > + base_clk2 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2); > + base_clk3 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2); > + base_clk4 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2); The same equation is used per each byte of clk_div_reg? Can it be rewritten to avoid this and using loop, so you will have an array of base_clk to be filled? Don't forget to use GENMASK(). ... > + if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 0; > + divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency); > + } else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 1; > + divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency); > + } else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 2; > + divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency); > + } else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 3; > + divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency); Will be optimized with above suggestion, I believe. > + } else { > + baseclk_idx = 4; > + divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency); > + inc = 0; > + while ((divisor + inc) > 32) { > + inc |= divisor & 0x1; > + divisor >>= 1; > + baseclk_idx++; > + } > + divisor += inc; I think the above loop can be rewritten to have better representation. > + } ... > + baseclk_idx &= 0xf; GENMASK()? ... > + scl_low = ((divisor * 9) / 16) - 1; > + scl_low = min_t(u32, scl_low, 0xf); This can be done in one line. Also, why not 15? ... > + scl_high = (divisor - scl_low - 2) & 0xf; GENMASK()? ... > + data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx); Too many parentheses. ... > + /* due to master slave is common buffer, so need force the master stop not issue */ > + if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) { GENMASK() ? > + writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + i2c_bus->cmd_err = -EBUSY; > + writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > + complete(&i2c_bus->cmd_complete); > + } ... > + /* send start */ > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write", str_read_write() ? > + msg->len, msg->len > 1 ? "s" : "", > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); Err.. There can be alignment issues. > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); The above code is ugly. Can you think about it and write in a better way? ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); Ditto. ... Do you have similar code pieces? Why not doing it in a separate function with parameters? ... > + return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE; IRQ_RETVAL() ? ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR); GENMASK() ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR); Ditto. > + if (i2c_bus->mode == BYTE_MODE) { > + writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER); Ditto. > + } else { > + /* Set interrupt generation of I2C slave controller */ > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER); > + } ... > + WARN_ON(!i2c_bus->slave); Why? ... > +static const struct of_device_id ast2600_i2c_bus_of_table[] = { > + { > + .compatible = "aspeed,ast2600-i2cv2", > + }, > + {} > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); ... > + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL); > + if (!i2c_bus) > + return dev_err_probe(dev, -ENOMEM, "no memory allocated\n"); No. We do not print error message for ENOMEM. You homework to find why. ... > + if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma")) device_property_read_bool() ? > + i2c_bus->mode = DMA_MODE; ... > + if (i2c_bus->mode == BUFF_MODE) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res && resource_size(res) >= 2) { > + i2c_bus->buf_base = devm_ioremap_resource(dev, res); > + > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > + i2c_bus->buf_size = resource_size(res) / 2; > + } else { > + i2c_bus->mode = BYTE_MODE; > + } > + } Can be done without additional checks and with a simple call to devm_platform_ioremap_resource(). No? ... > + /* i2c timeout counter: use base clk4 1Mhz, > + * per unit: 1/(1000/4096) = 4096us > + */ Wrong multi-line style of the comment. ... > + ret = of_property_read_u32(dev->of_node, > + "i2c-scl-clk-low-timeout-us", > + &i2c_bus->timeout); > + if (!ret) > + i2c_bus->timeout /= 4096; What is this and why I2C core timings (standard) can't be utilized here? ... > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency); > + if (ret < 0) { > + dev_warn(dev, "Could not read clock-frequency property\n"); > + i2c_bus->bus_frequency = 100000; > + } There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code. ... > + i2c_bus->adap.dev.of_node = dev->of_node; device_set_node() ... > + if (of_property_read_bool(dev->of_node, "smbus-alert")) { Doesn't core have already support for this? > + i2c_bus->alert_enable = true; > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data); > + if (!i2c_bus->ara) > + dev_warn(dev, "Failed to register ARA client\n"); > + > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } else { > + i2c_bus->alert_enable = false; > + /* Set interrupt generation of I2C master controller */ > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } ... > + dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n", > + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000, > + i2c_bus->mode); Useless noise. -- With Best Regards, Andy Shevchenko 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 lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (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 3CF29C77B78 for ; Tue, 2 May 2023 20:34:28 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4Q9sJ22p6bz3cdy for ; Wed, 3 May 2023 06:34:26 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=RcN7d81n; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=linux.intel.com (client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=andriy.shevchenko@linux.intel.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=RcN7d81n; dkim-atps=neutral Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Q9sGt0kPrz3bxY; Wed, 3 May 2023 06:33:24 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1683059606; x=1714595606; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=1FXPdk1irRsOX44r9OYmKGk0aEfOim/Li+bhY4Tx/p0=; b=RcN7d81nJtG85r1NRHSo8c3Ucr6UI4zjXsdBmI5+KUJKFZ/urT+H77A2 2HjPWBIxuNbm6F7Rnys2V/dSVWNqrybVGQVS5kwutGvsEnJRqkR3/Q4yE IFScKNSAPtc+pGc0F5B16jEomJdrxS2mzCSWwyfuT2ACtL7ngu+Q8nNEG 4k1CMHXOqohOiZUOmA/ChKRQTnSCdRosSR9rSmmHehFWMg32S+KLQVRh4 lcAr9ukzs0OcbD18Kz3LiV+63oEom7x8hgxuhhkc2SZ8YPJIys1bdq5LJ aM73SIAo3kld9C9rSy4kCU2XK2GEizSSBJr3tre98MgErQIPm/yj2F6jS g==; X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="328866794" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="328866794" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 May 2023 13:32:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="1026223663" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="1026223663" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga005.fm.intel.com with ESMTP; 02 May 2023 13:32:07 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1ptwfM-008Dpy-13; Tue, 02 May 2023 23:32:04 +0300 Date: Tue, 2 May 2023 23:32:04 +0300 From: Andy Shevchenko To: Ryan Chen Subject: Re: [PATCH v11 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230430041712.3247998-1-ryan_chen@aspeedtech.com> <20230430041712.3247998-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230430041712.3247998-3-ryan_chen@aspeedtech.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-aspeed@lists.ozlabs.org, Brendan Higgins , Conor Dooley , linux-i2c@vger.kernel.org, Krzysztof Kozlowski , jk@codeconstruct.com.au, Jean Delvare , Andi Shyti , Phil Edworthy , Florian Fainelli , =linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org, Joel Stanley , devicetree@vger.kernel.org, William Zhang , Rob Herring , linux-arm-kernel@lists.infradead.org, Tharun Kumar P , Andrew Jeffery , Wolfram Sang , Tyrone Ting , Philipp Zabel Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On Sun, Apr 30, 2023 at 12:17:12PM +0800, Ryan Chen wrote: > Add i2c new register mode driver to support AST2600 i2c > new register mode. AST2600 i2c controller have legacy and > new register mode. The new register mode have global register > support 4 base clock for scl clock selection, and new clock > divider mode. The i2c new register mode have separate register > set to control i2c master and slave. ... > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Ordered? ... > +#define AST2600_GLOBAL_INIT \ > + (AST2600_I2CG_CTRL_NEW_REG | \ > + AST2600_I2CG_CTRL_NEW_CLK_DIV) Make just a one TAB and put the last two lines on the single one. ... > +#define I2CCG_DIV_CTRL 0xC6411208 Is it decimal? Is it combination of bitfields? Can you add a comment what is this magic? ... > +struct ast2600_i2c_timing_table { > + u32 divisor; > + u32 timing; > +}; Is it even used? ... > +enum xfer_mode { > + BYTE_MODE = 0, Why explicit assignment? > + BUFF_MODE, > + DMA_MODE, > +}; ... > + base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2); > + base_clk2 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2); > + base_clk3 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2); > + base_clk4 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2); The same equation is used per each byte of clk_div_reg? Can it be rewritten to avoid this and using loop, so you will have an array of base_clk to be filled? Don't forget to use GENMASK(). ... > + if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 0; > + divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency); > + } else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 1; > + divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency); > + } else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 2; > + divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency); > + } else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 3; > + divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency); Will be optimized with above suggestion, I believe. > + } else { > + baseclk_idx = 4; > + divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency); > + inc = 0; > + while ((divisor + inc) > 32) { > + inc |= divisor & 0x1; > + divisor >>= 1; > + baseclk_idx++; > + } > + divisor += inc; I think the above loop can be rewritten to have better representation. > + } ... > + baseclk_idx &= 0xf; GENMASK()? ... > + scl_low = ((divisor * 9) / 16) - 1; > + scl_low = min_t(u32, scl_low, 0xf); This can be done in one line. Also, why not 15? ... > + scl_high = (divisor - scl_low - 2) & 0xf; GENMASK()? ... > + data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx); Too many parentheses. ... > + /* due to master slave is common buffer, so need force the master stop not issue */ > + if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) { GENMASK() ? > + writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + i2c_bus->cmd_err = -EBUSY; > + writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > + complete(&i2c_bus->cmd_complete); > + } ... > + /* send start */ > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write", str_read_write() ? > + msg->len, msg->len > 1 ? "s" : "", > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); Err.. There can be alignment issues. > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); The above code is ugly. Can you think about it and write in a better way? ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); Ditto. ... Do you have similar code pieces? Why not doing it in a separate function with parameters? ... > + return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE; IRQ_RETVAL() ? ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR); GENMASK() ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR); Ditto. > + if (i2c_bus->mode == BYTE_MODE) { > + writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER); Ditto. > + } else { > + /* Set interrupt generation of I2C slave controller */ > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER); > + } ... > + WARN_ON(!i2c_bus->slave); Why? ... > +static const struct of_device_id ast2600_i2c_bus_of_table[] = { > + { > + .compatible = "aspeed,ast2600-i2cv2", > + }, > + {} > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); ... > + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL); > + if (!i2c_bus) > + return dev_err_probe(dev, -ENOMEM, "no memory allocated\n"); No. We do not print error message for ENOMEM. You homework to find why. ... > + if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma")) device_property_read_bool() ? > + i2c_bus->mode = DMA_MODE; ... > + if (i2c_bus->mode == BUFF_MODE) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res && resource_size(res) >= 2) { > + i2c_bus->buf_base = devm_ioremap_resource(dev, res); > + > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > + i2c_bus->buf_size = resource_size(res) / 2; > + } else { > + i2c_bus->mode = BYTE_MODE; > + } > + } Can be done without additional checks and with a simple call to devm_platform_ioremap_resource(). No? ... > + /* i2c timeout counter: use base clk4 1Mhz, > + * per unit: 1/(1000/4096) = 4096us > + */ Wrong multi-line style of the comment. ... > + ret = of_property_read_u32(dev->of_node, > + "i2c-scl-clk-low-timeout-us", > + &i2c_bus->timeout); > + if (!ret) > + i2c_bus->timeout /= 4096; What is this and why I2C core timings (standard) can't be utilized here? ... > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency); > + if (ret < 0) { > + dev_warn(dev, "Could not read clock-frequency property\n"); > + i2c_bus->bus_frequency = 100000; > + } There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code. ... > + i2c_bus->adap.dev.of_node = dev->of_node; device_set_node() ... > + if (of_property_read_bool(dev->of_node, "smbus-alert")) { Doesn't core have already support for this? > + i2c_bus->alert_enable = true; > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data); > + if (!i2c_bus->ara) > + dev_warn(dev, "Failed to register ARA client\n"); > + > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } else { > + i2c_bus->alert_enable = false; > + /* Set interrupt generation of I2C master controller */ > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } ... > + dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n", > + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000, > + i2c_bus->mode); Useless noise. -- With Best Regards, Andy Shevchenko 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 2FAE7C77B78 for ; Tue, 2 May 2023 20:34:20 +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=kmAXiyyMCqG191JzcM70oylyiq7YSYAzkhHHEC1bx6E=; b=vQ8+3SdxFbU22v jduDJxvtWwWtOzN8xLHxQrGIg5osBeJB9Revg0A+8OjjEwBRTxMGRbEG1XvXMayoKFmHu+Oro6ehN x4ypjPY94eoL/FapHxBZ71YG7hYfvUXVEme8bFA0Csiz6/Py99qEfpVPni6d2Xz/rQTskWHZ2gt45 7bZkGFGAeGC+QX7vU4DVvIyENI31NDXXBma6/mlDDoLE/m3AYP2W1cPitiwoBXClTX/ghKDzoUOrI GYFCIyk4Rvb9adsc4ey1nElU2VLpyLzUBvzrOPtA1ZvNkhLSpPLfjD3hQNwXZiRMnsTRq9gqDUXFB tWZvfMGMK3pmLr3IwPew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1ptwgi-002aoo-1M; Tue, 02 May 2023 20:33:28 +0000 Received: from mga17.intel.com ([192.55.52.151]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1ptwgf-002ao5-13 for linux-arm-kernel@lists.infradead.org; Tue, 02 May 2023 20:33:27 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1683059605; x=1714595605; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=1FXPdk1irRsOX44r9OYmKGk0aEfOim/Li+bhY4Tx/p0=; b=DK/jLQ6bPx+22ynlKuHpQA2IzR3zPouCnh14ff8hKbpDY6U56NpvUXBW XV0g6NYrpnOjcDgs2IurbxVrPTqRWBxdxfXY2UVJbOtqlQYmzAc3A0b+Q UtnDnU6TqAtq4XR/WWGaHljYZTdnwkHh0/hdIwxT3XNHHqO6tJPJ7qNww na02WWTJDjvKPVR2aW3KWnMhIb2Ab5NVms56yE1JpaK7g0mpY2Wlo34NL zYOs4NenpPm4Rvghl9/+BYmWOJTWNaalUYynjgPiGD5L9N/JjzZMdBXQL r8XFkTiKijOG0iNMXaMXmW7GREG1qtBWrRHOdftey1wOBdsvpdA6iyCM1 g==; X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="328866791" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="328866791" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 May 2023 13:32:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10698"; a="1026223663" X-IronPort-AV: E=Sophos;i="5.99,245,1677571200"; d="scan'208";a="1026223663" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga005.fm.intel.com with ESMTP; 02 May 2023 13:32:07 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1ptwfM-008Dpy-13; Tue, 02 May 2023 23:32:04 +0300 Date: Tue, 2 May 2023 23:32:04 +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, Andi Shyti Subject: Re: [PATCH v11 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230430041712.3247998-1-ryan_chen@aspeedtech.com> <20230430041712.3247998-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230430041712.3247998-3-ryan_chen@aspeedtech.com> 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-20230502_133325_413762_E9B9F237 X-CRM114-Status: GOOD ( 30.26 ) 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 Sun, Apr 30, 2023 at 12:17:12PM +0800, Ryan Chen wrote: > Add i2c new register mode driver to support AST2600 i2c > new register mode. AST2600 i2c controller have legacy and > new register mode. The new register mode have global register > support 4 base clock for scl clock selection, and new clock > divider mode. The i2c new register mode have separate register > set to control i2c master and slave. ... > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Ordered? ... > +#define AST2600_GLOBAL_INIT \ > + (AST2600_I2CG_CTRL_NEW_REG | \ > + AST2600_I2CG_CTRL_NEW_CLK_DIV) Make just a one TAB and put the last two lines on the single one. ... > +#define I2CCG_DIV_CTRL 0xC6411208 Is it decimal? Is it combination of bitfields? Can you add a comment what is this magic? ... > +struct ast2600_i2c_timing_table { > + u32 divisor; > + u32 timing; > +}; Is it even used? ... > +enum xfer_mode { > + BYTE_MODE = 0, Why explicit assignment? > + BUFF_MODE, > + DMA_MODE, > +}; ... > + base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2); > + base_clk2 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2); > + base_clk3 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2); > + base_clk4 = (i2c_bus->apb_clk * 10) / > + (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2); The same equation is used per each byte of clk_div_reg? Can it be rewritten to avoid this and using loop, so you will have an array of base_clk to be filled? Don't forget to use GENMASK(). ... > + if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 0; > + divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency); > + } else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 1; > + divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency); > + } else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 2; > + divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency); > + } else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) { > + baseclk_idx = 3; > + divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency); Will be optimized with above suggestion, I believe. > + } else { > + baseclk_idx = 4; > + divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency); > + inc = 0; > + while ((divisor + inc) > 32) { > + inc |= divisor & 0x1; > + divisor >>= 1; > + baseclk_idx++; > + } > + divisor += inc; I think the above loop can be rewritten to have better representation. > + } ... > + baseclk_idx &= 0xf; GENMASK()? ... > + scl_low = ((divisor * 9) / 16) - 1; > + scl_low = min_t(u32, scl_low, 0xf); This can be done in one line. Also, why not 15? ... > + scl_high = (divisor - scl_low - 2) & 0xf; GENMASK()? ... > + data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx); Too many parentheses. ... > + /* due to master slave is common buffer, so need force the master stop not issue */ > + if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) { GENMASK() ? > + writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + i2c_bus->cmd_err = -EBUSY; > + writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > + complete(&i2c_bus->cmd_complete); > + } ... > + /* send start */ > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write", str_read_write() ? > + msg->len, msg->len > 1 ? "s" : "", > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); Err.. There can be alignment issues. > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); The above code is ugly. Can you think about it and write in a better way? ... > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > + if (i % 4 == 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3); > + } > + if (--i % 4 != 3) > + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4)); Ditto. ... Do you have similar code pieces? Why not doing it in a separate function with parameters? ... > + return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE; IRQ_RETVAL() ? ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR); GENMASK() ... > + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR); Ditto. > + if (i2c_bus->mode == BYTE_MODE) { > + writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER); Ditto. > + } else { > + /* Set interrupt generation of I2C slave controller */ > + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER); > + } ... > + WARN_ON(!i2c_bus->slave); Why? ... > +static const struct of_device_id ast2600_i2c_bus_of_table[] = { > + { > + .compatible = "aspeed,ast2600-i2cv2", > + }, > + {} > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); ... > + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL); > + if (!i2c_bus) > + return dev_err_probe(dev, -ENOMEM, "no memory allocated\n"); No. We do not print error message for ENOMEM. You homework to find why. ... > + if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma")) device_property_read_bool() ? > + i2c_bus->mode = DMA_MODE; ... > + if (i2c_bus->mode == BUFF_MODE) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res && resource_size(res) >= 2) { > + i2c_bus->buf_base = devm_ioremap_resource(dev, res); > + > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > + i2c_bus->buf_size = resource_size(res) / 2; > + } else { > + i2c_bus->mode = BYTE_MODE; > + } > + } Can be done without additional checks and with a simple call to devm_platform_ioremap_resource(). No? ... > + /* i2c timeout counter: use base clk4 1Mhz, > + * per unit: 1/(1000/4096) = 4096us > + */ Wrong multi-line style of the comment. ... > + ret = of_property_read_u32(dev->of_node, > + "i2c-scl-clk-low-timeout-us", > + &i2c_bus->timeout); > + if (!ret) > + i2c_bus->timeout /= 4096; What is this and why I2C core timings (standard) can't be utilized here? ... > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency); > + if (ret < 0) { > + dev_warn(dev, "Could not read clock-frequency property\n"); > + i2c_bus->bus_frequency = 100000; > + } There are macro for standard speeds. Moreover, there is a function to parse properties, no need to open code. ... > + i2c_bus->adap.dev.of_node = dev->of_node; device_set_node() ... > + if (of_property_read_bool(dev->of_node, "smbus-alert")) { Doesn't core have already support for this? > + i2c_bus->alert_enable = true; > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data); > + if (!i2c_bus->ara) > + dev_warn(dev, "Failed to register ARA client\n"); > + > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } else { > + i2c_bus->alert_enable = false; > + /* Set interrupt generation of I2C master controller */ > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } ... > + dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n", > + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000, > + i2c_bus->mode); Useless noise. -- 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