Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver

From: Lee Jones <lee@kernel.org>
To: Jeff LaBundy <jeff@labundy.com>
Cc: James Ogletree <James.Ogletree@cirrus.com>,
	James Ogletree <james.ogletree@opensource.cirrus.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Fred Treven <Fred.Treven@cirrus.com>,
	Ben Bright <Ben.Bright@cirrus.com>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
Date: Tue, 24 Oct 2023 16:47:14 +0100	[thread overview]
Message-ID: <20231024154714.GK8909@google.com> (raw)
In-Reply-To: <ZTcZIMbrFEhz+rm4@nixie71>

On Mon, 23 Oct 2023, Jeff LaBundy wrote:
> I understand that no customer would ever build the to-be-added codec
> driver _without_ the input driver, but the MFD must be generic enough
> to support this case. Would a codec-only implementation use f0 and ReDC
> estimation? If so, then these functions _do_ belong in the MFD, albeit
> with some comments to explain their nature.

I'm not going to be able to accept a single-function device into the
multi-function devices subsystem.  Please submit both once the codec is
ready.

> > > >> + struct device *dev = cs40l50->dev;
> > > >> + int error;
> > > >> +
> > > >> + mutex_init(&cs40l50->lock);
> > > > 
> > > > Don't you need to destroy this in the error path?
> > > 
> > > My understanding based on past feedback is that mutex_destroy()
> > > is an empty function unless mutex debugging is enabled, and there
> > > is no need cleanup the mutex explicitly. I will change this if you
> > > disagree with that feedback.
> > 
> > It just seems odd to create something and not tear it down.
> 
> mutex_init() is not creating anything; the mutex struct is allocated as
> part of the driver's private data, which is de-allocated as part of device
> managed resources being freed when the module is unloaded.
> 
> mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
> roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
> not to add mutex_destroy() because it adds unnecessary tear-down paths and
> remove() callbacks that wouldn't otherwise have to exist.

Fair enough.

-- 
Lee Jones [李琼斯]