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 3B2B8FA3728 for ; Fri, 13 Sep 2024 08:01:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:Subject:References:In-Reply-To:Message-Id:Cc:To:From:Date: MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=4CzBYefCbC8etZ+AyFpSOC775tw2tVhYGtceXnzwH1U=; b=l6MsfmVkAH0SOp/z5OmVct5Amj H+KyhE0AmIBZHS+YSE64SBvvswmeLJzRUb7iUhMbiLovYm4+0qW4xsJS1Aad//4H7yrwKnkNNo8Fc VSLt4cxz3+PbjzM3/z7AxYRj4bIGnv2mA4Hf3qNi4oxMA7o33kEVyCu3j8lK+TacLHw+MjtakMif/ FX3OIAIWaPHyBNjoqTrJKIuWDvBet65cN1M8a/OlMX65JB/4VkqeJV2logF8vxqGaUk7u0HbLvq/k 7LPxdeO+IeLkKEdzadZhApqT0KUTEkhNX8OhHGSm6r6Iz+Jc0mOiJezTvoqFZYhRpevYYwYcOazS+ bvIGmj7A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sp1Ew-0000000FGbO-2dyL; Fri, 13 Sep 2024 08:01:14 +0000 Received: from fout7-smtp.messagingengine.com ([103.168.172.150]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sp1Dh-0000000FG8L-12J6 for linux-arm-kernel@lists.infradead.org; Fri, 13 Sep 2024 07:59:58 +0000 Received: from phl-compute-10.internal (phl-compute-10.phl.internal [10.202.2.50]) by mailfout.phl.internal (Postfix) with ESMTP id 31A40138048D; Fri, 13 Sep 2024 03:59:56 -0400 (EDT) Received: from phl-imap-11 ([10.202.2.101]) by phl-compute-10.internal (MEProxy); Fri, 13 Sep 2024 03:59:56 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=arndb.de; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm1; t=1726214396; x=1726300796; bh=4CzBYefCbC8etZ+AyFpSOC775tw2tVhYGtceXnzwH1U=; b= FMtRYG/NxCbpPWzUbeUcs2kSWrmne3N2oB9HDtAZjdnxZV08DTuPG8XRVB1msUvz AQeDI18TYIq7dCx4eVRf8X6lkvhpqBUnlV+I1rCjFv9ksVoFh3jqU+c5d0lxpG+n aSBpj/NFtraf1gvGeakqK0rP7MiVcLry5Qwv01vQ+wSw7XKlLsBmguda+Mgpjm/t cAENj5nzkA4DIYMhedjdVHqnb799lYFL43cvgFxuTW9oz9ZH4eMDcPFk5HMzQ1Cl kekNu+GokziTe2U44vqbnCQSJ1VpET4YW1Pxf3Y0XhHv/vhFrDVo+9R+DVNK1l4+ F79VUWZkl8zvlGloD2WwCA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1726214396; x= 1726300796; bh=4CzBYefCbC8etZ+AyFpSOC775tw2tVhYGtceXnzwH1U=; b=M Y+rZsBRmkwECMX2ef/FHDEDwdJ0FSRMwahs2wE2rxPXJFINXJJlQ4W1L8vGPI8/5 8WD+7VCrVlig4QR+xLjvbVBK7b4sKJlbfwOI4G0M+aT18qI+b77Nl053UwFB9SdZ QhBKvXUvjIbuPNyDAKymd/fDn+dvDCN/CRHQT4FXZ7uXawtpUswQIF+BxXwtVJ+V fjN+9m974ZhzBJe+RA+JTVB//NWwTxzR6UPLmHAyAKwAmZrCDQIfRXhrmZk9bzaK SjzxCAUbdjsNVkS0727EnF0nFmIL3/Ied61XkHmaPyTTCtvXCllIhFPMhb+BDIfE 9+be7x29ms3+D3KqErtfA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrudejiedggeeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepofggfffhvfevkfgjfhfutgfgsehtjeertdertddt necuhfhrohhmpedftehrnhguuceuvghrghhmrghnnhdfuceorghrnhgusegrrhhnuggsrd guvgeqnecuggftrfgrthhtvghrnhephfdthfdvtdefhedukeetgefggffhjeeggeetfefg gfevudegudevledvkefhvdeinecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpe hmrghilhhfrhhomheprghrnhgusegrrhhnuggsrdguvgdpnhgspghrtghpthhtohepvdek pdhmohguvgepshhmthhpohhuthdprhgtphhtthhopehuthhsrghvrdgrghgrrhifrghlse grnhgrlhhoghdrtghomhdprhgtphhtthhopegrughsphdqlhhinhhugiesrghnrghlohhg rdgtohhmpdhrtghpthhtoheprghrthhurhhsrdgrrhhtrghmohhnohhvshesrghnrghloh hgrdgtohhmpdhrtghpthhtoheptggrthgrlhhinhdrmhgrrhhinhgrshesrghrmhdrtgho mhdprhgtphhtthhopehmthhurhhquhgvthhtvgessggrhihlihgsrhgvrdgtohhmpdhrtg hpthhtohepsghrghhlsegsghguvghvrdhplhdprhgtphhtthhopegrnhguihdrshhhhiht iheskhgvrhhnvghlrdhorhhgpdhrtghpthhtoheptghonhhorhdoughtsehkvghrnhgvlh drohhrghdprhgtphhtthhopehjihhrihhslhgrsgihsehkvghrnhgvlhdrohhrgh X-ME-Proxy: Feedback-ID: i56a14606:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id 6C8EC222006F; Fri, 13 Sep 2024 03:59:55 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface MIME-Version: 1.0 Date: Fri, 13 Sep 2024 07:59:34 +0000 From: "Arnd Bergmann" To: arturs.artamonovs@analog.com, "Catalin Marinas" , "Will Deacon" , "Greg Malysa" , "Philipp Zabel" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Utsav Agarwal" , "Michael Turquette" , "Stephen Boyd" , "Linus Walleij" , "Bartosz Golaszewski" , "Thomas Gleixner" , "Andi Shyti" , "Greg Kroah-Hartman" , "Jiri Slaby" , "Olof Johansson" , soc@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, "open list:GPIO SUBSYSTEM" , linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, adsp-linux@analog.com, "Nathan Barrett-Morrison" Message-Id: In-Reply-To: <20240912-test-v1-15-458fa57c8ccf@analog.com> References: <20240912-test-v1-0-458fa57c8ccf@analog.com> <20240912-test-v1-15-458fa57c8ccf@analog.com> Subject: Re: [PATCH 15/21] i2c: Add driver for ADI ADSP-SC5xx platforms Content-Type: text/plain Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240913_005957_582491_72F84164 X-CRM114-Status: GOOD ( 23.63 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 12, 2024, at 18:25, Arturs Artamonovs via B4 Relay wrote: > + > +config I2C_ADI_TWI_CLK_KHZ > + int "ADI TWI I2C clock (kHz)" > + depends on I2C_ADI_TWI > + range 21 400 > + default 50 > + help > + The unit of the TWI clock is kHz. This does not look like something that should be a compile-time option, the kernel needs to be able to run on different configurations. > + > +static void adi_twi_handle_interrupt(struct adi_twi_iface *iface, > + unsigned short twi_int_status, > + bool polling) > +{ > + u16 writeValue; > + unsigned short mast_stat = ioread16(&iface->regs_base->master_stat); It's a bit unusual to use ioread16()/iowrite16() instead of the normal readw()/writew(). > + } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT && > + iface->cur_msg + 1 < iface->msg_num) { > + > + if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD) { > + writeValue = ioread16(&iface->regs_base->master_ctl) > + | MDIR; > + iowrite16(writeValue, &iface->regs_base->master_ctl); > + } else { > + writeValue = ioread16(&iface->regs_base->master_ctl) > + & ~MDIR; > + iowrite16(writeValue, &iface->regs_base->master_ctl); The use of a structure instead of register offset macros makes these lines rather long, especially at five levels of indentation. Maybe this can be restructured for readability. > + if (ioread16(&iface->regs_base->master_stat) & SDASEN) { > + int cnt = 9; > + > + do { > + iowrite16(SCLOVR, &iface->regs_base->master_ctl); > + udelay(6); > + iowrite16(0, &iface->regs_base->master_ctl); > + udelay(6); > + } while ((ioread16(&iface->regs_base->master_stat) & SDASEN) Since writes on device mappings are posted, the delay between the two iowrite16() is not really meaningful, unless you add another ioread16() or readw() before the delay. Mapping these with ioremap_np() should also work. > + iowrite16(SDAOVR | SCLOVR, &iface->regs_base->master_ctl); > + udelay(6); > + iowrite16(SDAOVR, &iface->regs_base->master_ctl); > + udelay(6); > + iowrite16(0, &iface->regs_base->master_ctl); > + } Same here. > +/* Interrupt handler */ > +static irqreturn_t adi_twi_handle_all_interrupts(struct adi_twi_iface > *iface, > + bool polling) > +{ > + irqreturn_t handled = IRQ_NONE; > + unsigned short twi_int_status; > + > + while (1) { > + twi_int_status = ioread16(&iface->regs_base->int_stat); > + if (!twi_int_status) > + return handled; > + /* Clear interrupt status */ > + iowrite16(twi_int_status, &iface->regs_base->int_stat); > + adi_twi_handle_interrupt(iface, twi_int_status, polling); > + handled = IRQ_HANDLED; > + } > +} > + > +static irqreturn_t adi_twi_interrupt_entry(int irq, void *dev_id) > +{ > + struct adi_twi_iface *iface = dev_id; > + unsigned long flags; > + irqreturn_t handled; > + > + spin_lock_irqsave(&iface->lock, flags); > + handled = adi_twi_handle_all_interrupts(iface, false); > + spin_unlock_irqrestore(&iface->lock, flags); > + return handled; > +} Interrupt handlers are called with IRQs disabled, so no need to turn them off again. > +static SIMPLE_DEV_PM_OPS(i2c_adi_twi_pm, > + i2c_adi_twi_suspend, i2c_adi_twi_resume); > +#define I2C_ADI_TWI_PM_OPS (&i2c_adi_twi_pm) > +#else > +#define I2C_ADI_TWI_PM_OPS NULL > +#endif Please convert to DEFINE_SIMPLE_DEV_PM_OPS() and remove the #ifdef. > +#ifdef CONFIG_OF > +static const struct of_device_id adi_twi_of_match[] = { > + { > + .compatible = "adi,twi", > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, adi_twi_of_match); > +#endif No need to optimize for non-CONFIG_OF builds, we don't support traditional board files on arm64. > + match = of_match_device(of_match_ptr(adi_twi_of_match), &pdev->dev); This of_match_ptr() and the second one later should also get removed then. > \ No newline at end of file > Whitespace damage. Arnd