From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (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 CD19B1DA4C; Tue, 28 May 2024 18:09:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716919797; cv=none; b=OIk2yxse5rbYDCnpfXWbuLkg66yoG3p5UuUOzVs5wd3zUtQyYNmZvse/CLh7OeztsaP9ZehoUUzEsNUAxP7GD+78hrNu0bZvcV2egS8xN1CtTLkxWlkGCwK4Uo1Em5R72zdjP8Vg0YdDXYbmXm4sT3y4LD07EnojP+/pJricy6c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716919797; c=relaxed/simple; bh=FfkCmvn8rmMIG+GOvKiTWLsMfjkaV0rloiGxJwgk+iM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=NCRLYVOEpnoSlfwGKrvJ+f+Yea7lJzRAWDIqCVxBXqJ2u1nXMYWeUwE38MLThvd9XlgiMBzlSxvZ+5e2Rn4HT9gFbr02h1smt+C3/SVlQW6C5qFby8uFbyI1nQwzziDGRtFQQHfrIAk20v6Nm4luK2Q2SBRHWRZI3uUTJI2R5og= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=Ce2x0BiQ; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Ce2x0BiQ" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FE318D0; Tue, 28 May 2024 20:09:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1716919790; bh=FfkCmvn8rmMIG+GOvKiTWLsMfjkaV0rloiGxJwgk+iM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Ce2x0BiQ0uu8VvjRYGuTOWT1yZUXjj/JuL1BX1dyocWlaufKNbJAGdVtHJmRQ0dQB Z1uwKGsQ8O/gGNlU2+YwHGXa0AmNAuiGuyVKAHmmmQsDXhY1xh4YgP7ueS52xoTnNn 1b1xbXLkywf6o5mLqo9PAxZXXHXIvRrI16Mi4n8o= Date: Tue, 28 May 2024 21:09:41 +0300 From: Laurent Pinchart To: Linus Walleij Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, linux-pwm@vger.kernel.org, Alexandru Ardelean , Bartosz Golaszewski , Conor Dooley , Krzysztof Kozlowski , Lee Jones , Rob Herring , Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , Haibo Chen Subject: Re: [PATCH 4/5] gpio: adp5585: Add Analog Devices ADP5585 support Message-ID: <20240528180941.GA10903@pendragon.ideasonboard.com> References: <20240520195942.11582-1-laurent.pinchart@ideasonboard.com> <20240520195942.11582-5-laurent.pinchart@ideasonboard.com> <20240528122734.GA29970@pendragon.ideasonboard.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240528122734.GA29970@pendragon.ideasonboard.com> On Tue, May 28, 2024 at 03:27:34PM +0300, Laurent Pinchart wrote: > Hi Linus, > > On Tue, May 28, 2024 at 01:54:29PM +0200, Linus Walleij wrote: > > On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart wrote: > > > > > From: Haibo Chen > > > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > > matrix decoder, programmable logic, reset generator, and PWM generator. > > > This driver supports the GPIO function using the platform device > > > registered by the core MFD driver. > > > > > > The driver is derived from an initial implementation from NXP, available > > > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > > > adp5585-gpio support") in their BSP kernel tree. It has been extensively > > > rewritten. > > > > > > Signed-off-by: Haibo Chen > > > Signed-off-by: Laurent Pinchart > > > > (...) > > > > > +static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off) > > > +{ > > > + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); > > > + unsigned int bank = ADP5585_BANK(off); > > > + unsigned int bit = ADP5585_BIT(off); > > > + > > > + guard(mutex)(&adp5585_gpio->lock); > > > + > > > + return regmap_update_bits(adp5585_gpio->regmap, > > > + ADP5585_GPIO_DIRECTION_A + bank, > > > + bit, 0); > > > > First, I love the guarded mutex! > > Yes, it's neat :-) > > > But doesn't regmap already contain a mutex? Or is this one of those > > cases where regmap has been instantiated without a lock? > > regmap indeed includes a lock, but it will lock each register access > independently. In adp5585_gpio_get_value() we need to read two > registers atomically, so we need to cover them by a single lock. > > I could drop the lock from regmap, but I would then likely need to > introduce a lock in the parent mfd device that both the PWM and GPIO > children would use to protect bus access. That may make sense in the > future, but is a bit overkill for now I think. Actually, I think I can drop the lock. Concurrent access to the registers from different GPIOs are protected by the regmap lock, and concurrent access to groups of registers for the same GPIO isn't a valid use case as callers shouldn't do that. > > > + gc = &adp5585_gpio->gpio_chip; > > > + gc->parent = dev; > > > + gc->direction_input = adp5585_gpio_direction_input; > > > + gc->direction_output = adp5585_gpio_direction_output; > > > > And chance to implemen ->get_direction()? > > Sure, I'll add that to v2. > > > Other than this I think the driver is ready for merge, so with the > > comments fixed or addressed: > > Reviewed-by: Linus Walleij > > Thank you. -- Regards, Laurent Pinchart