From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C621B15CBC for ; Tue, 6 Dec 2022 15:24:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670340261; x=1701876261; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=sp1kX2dPljTrHQsc7vo9PXw4LW7YS91wuLLg6PS0UVo=; b=DR64pkIaox9n1ZpV8qntXPo1QFOnVLkXn//e72h/oN4lcew2vHjJb4vX UEXPgr979SRhsjigaKMbjAgD/fIepM8pyOoukeF0uz+ja0p0/u2e/rk4+ CTw6xm/RWfFLCXZqbjgBrhepnL5l8+x3yMexevjlp9yAaxGaFqHYHPGRe b5zSQz26KqgPmKsjQjQxjHeXGRXtWPUwSBmyPklhtjzbZQ8COw7dckwbF gcRErSyktBWYROFeQI4dwbtn69LHjvOmrTo5iEJAcinTrBf300SivcJ2o RizirsEIEJRcxILBUbTzesiIJVqMiFoFffjc8P7pVYWdy+LRh07R1N/ux Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="402924085" X-IronPort-AV: E=Sophos;i="5.96,222,1665471600"; d="scan'208";a="402924085" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Dec 2022 07:24:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="646248416" X-IronPort-AV: E=Sophos;i="5.96,222,1665471600"; d="scan'208";a="646248416" Received: from smile.fi.intel.com ([10.237.72.54]) by orsmga002.jf.intel.com with ESMTP; 06 Dec 2022 07:23:58 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1p2ZnX-005MPs-2q; Tue, 06 Dec 2022 17:23:55 +0200 Date: Tue, 6 Dec 2022 17:23:55 +0200 From: Andy Shevchenko To: Binbin Zhou Cc: Wolfram Sang , Wolfram Sang , Mika Westerberg , linux-i2c@vger.kernel.org, loongarch@lists.linux.dev, devicetree@vger.kernel.org, Huacai Chen , WANG Xuerui , Arnd Bergmann , Rob Herring , Krzysztof Kozlowski , Jianmin Lv Subject: Re: [PATCH V5 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Message-ID: References: Precedence: bulk X-Mailing-List: loongarch@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: 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 On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote: > This I2C module is integrated into the Loongson-2K SoCs and Loongson > LS7A bridge chip. Much better, thanks! ... > +/* > + * The I2C controller has a fixed I2C bus frequency by default, but to > + * be compatible with more client devices, we can obtain the set I2C > + * bus frequency from ACPI or FDT. > + */ > +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) > +{ > + u16 val = 0x12c; /* Default value of I2C divider latch register */ Besides comment better to be placed on top of the commented line, the value is better to have its own definition where you place the comment and elaborate what it means in practice (The clock frequency is changed? Bus speed is different?) > + struct i2c_timings *t = &priv->i2c_t; > + u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev); > + > + i2c_parse_fw_timings(priv->dev, t, false); > + > + if (acpi_speed || t->bus_freq_hz) > + val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1; > + > + /* Set LS2X I2C frequency */ > + writel(val, priv->base + I2C_LS2X_PRER_LO); > +} ... > + writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST, > + priv->base + I2C_LS2X_CTR); Wrong indentation. ... > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, > + IRQF_SHARED, "ls2x-i2c", priv); > + if (r < 0) > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq); You requested IRQ without filling all data structures. Is it fine? Have you checked that with CONFIG_DEBUG_SHIRQ being enabled? ... > + r = devm_i2c_add_adapter(dev, adap); > + if (r) > + return dev_err_probe(dev, r, "Failure adding adapter\n"); > + > + return 0; > +} Looking at the above... > +static int ls2x_i2c_remove(struct platform_device *pdev) > +{ > + struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&priv->adapter); ...are you sure this is correct? > + return 0; > +} ... > +static int ls2x_i2c_suspend(struct device *dev) > +{ > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > + priv->suspended = 1; No protection needed? > + return 0; > +} > + > +static int ls2x_i2c_resume(struct device *dev) > +{ > + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); > + priv->suspended = 0; Ditto. > + ls2x_i2c_reginit(priv); > + return 0; > +} ... > +MODULE_ALIAS("platform:ls2x-i2c"); Why is this required? -- With Best Regards, Andy Shevchenko