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 95E4BC7EE22 for ; Mon, 8 May 2023 12:38:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234233AbjEHMiR (ORCPT ); Mon, 8 May 2023 08:38:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39088 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233885AbjEHMiQ (ORCPT ); Mon, 8 May 2023 08:38:16 -0400 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 620F938F2A; Mon, 8 May 2023 05:38:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1683549490; x=1715085490; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=TGcBs2WMT86YIl2FW8BGGb2rDomhZlvsCn9sqxbnMAI=; b=ZBoJT4slsjhYJpnFDUZvd41ehz1Md5wkogKECTZ2xKyotvfyjWz008a3 1lk04J1bpMzAjvqpbTj1LOV2R54o7SePQlI8H46ULcPyvueRgoSeCdbPk NVhMETH3U/E64K97EhpcNLrDREgcGDV+JcOIskl4jVzG5ey56uAG9BBzc 4WftOKdYORR+e8A/r7zjZrVYXEGcEN8cenKnxabIjm417JjkOeD9hNrYe 8jUUFW9EOk4T1/dVia0fjdWHaDr8xrUfjAqeBjCiVUpbPiltkFvwW5E8q kk7By/NsR5tQwWvmzSZ9VlnFoFpRYZJL/MTnn/M7ssbQlpH+rNYYodkZe g==; X-IronPort-AV: E=McAfee;i="6600,9927,10703"; a="377724348" X-IronPort-AV: E=Sophos;i="5.99,259,1677571200"; d="scan'208";a="377724348" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 May 2023 05:38:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10703"; a="822653340" X-IronPort-AV: E=Sophos;i="5.99,259,1677571200"; d="scan'208";a="822653340" Received: from smile.fi.intel.com ([10.237.72.54]) by orsmga004.jf.intel.com with ESMTP; 08 May 2023 05:38:04 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1pw07t-00ArL0-0V; Mon, 08 May 2023 15:38:01 +0300 Date: Mon, 8 May 2023 15:38:00 +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 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: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org On Sun, May 07, 2023 at 02:21:10AM +0000, Ryan Chen wrote: > > On Sun, Apr 30, 2023 at 12:17:12PM +0800, Ryan Chen wrote: ... > > > +#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. > > Update by following. > > #define AST2600_GLOBAL_INIT \ > (AST2600_I2CG_CTRL_NEW_REG | \ > AST2600_I2CG_CTRL_NEW_CLK_DIV) As I mentioned the last two can occupy a single line. ... > > > + /* 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() ? > Sorry do you mean there have a function call str_read_write? > Can you point me where it is for refer? string_helpers.h. > > > + msg->len, msg->len > 1 ? "s" : "", > > > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); ... > > > + 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? > Sorry, that is because the register only support for 4 byte align write. > That the reason I need put for byte write to 4 byte align write. Yes, that's fine. The problem is in _how_ the driver does it. We have a lot of helpers in the kernel to access unaligned data. ... > > > + return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE; > > > > IRQ_RETVAL() ? > Sorry, most return is handled or not handled. > Do you mean replace it just " return IRQ_RETVAL(ret);" Have you had a chance to look in the implementation of IRQ_RETVAL() ? I believe if you do, you will find the answer to your question. ... > > > + 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? > > > Sorry, I can't catch your point, can you guide me more about it? if (BUFF_MODE) { void __iomem buf_base; buf_base = devm_platform_ioremap_and_get_resource(pdev, 1, &res); if (IS_ERR(buf_base)) mode = BYTE_MODE; else { ->buf_base = buf_base; ->buf_size = ... } } ... > > > + 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. > > > Will update > ret = of_property_read_u32(dev->of_node, "clock-frequency", &bus_freq); > if (ret < 0) { > dev_warn(dev, "Could not read clock-frequency property\n"); > i2c_bus->bus_frequency = I2C_SPEED_STANDARD; > } else { > i2c_bus->bus_frequency = bus_freq; > } No, just use the I2C core API to fill this property in the specific i2c_timings structure. -- With Best Regards, Andy Shevchenko