From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 BBEB5449EA4 for ; Wed, 17 Jun 2026 14:39:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781707189; cv=none; b=sEhTbZUzWrjD3+9TARxXqPnFactefPnix0HRzFqCZa/vdv9LIlQuRflrYNFD4X1+T5HYEnSKld0mdYpZyoYzCPV7unDJKyEkLfHO34oiXLjp0ywbhQTvMqEvJ0FzIztJdK5yUJlz5Qq3BbHfdeuUtJ9J3xeAv45QteyxTk6P6lY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781707189; c=relaxed/simple; bh=eDP3yoc0EQreRQ61CL/QQ7bLBonEfo5flrX3GDtYugo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=fCxlDIcK9REuQWqkBK9LqbPG8Z0zhTNlsvfMvPX4SYCVl3Huo5rTfSGUzabYyzdFU7GCCuB0C0+0CeyRdts5tcS7G8QLEfb4xQDgYd9Wgzw2lQyPszaBhI9CL5ZzlxdLLcLKdGYjmqhIStBHClGkddIACJNyXmKO4C8vT1/zJNQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=P9cceI80; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="P9cceI80" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9CCE01F000E9; Wed, 17 Jun 2026 14:39:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781707188; bh=l+ytFxU964CBlwtGggNVTs2GG5wt5By5/dhgdQM2J1Q=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=P9cceI80/l0bIFWm0/docwRNPqRke5pvpUg2mb3yt5GyubcdxmvoRYUV3fZACQayT dJvAjl+zLsUl8hGcAFLYIt9OEOZ8QF3MkMGxAHUnPFV6EdcHaGFgWFG4T+swJQCckJ +30lkFMIwYu3msbpHeNynJQB05+YHQJKsSmvIjYbSy+2qpjE+US/L9Sp285TiXxtBb wHN1v38/R2cR6cO35dTa234j5xN7OmhPNZOz/2nBQPvlED7zDGRlx2/IDgYg7Iqzaz Wmjm2qiwjGUFEp2wq4G4yWcAP8B1BhoIrip1psjgHrBI37Zt3J08oknVld9ErxS7O5 QLjkXq6gCVmjQ== Date: Wed, 17 Jun 2026 15:39:43 +0100 From: Lee Jones To: Ming Yu Cc: Ming Yu , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 3/7] mfd: nct6694: Introduce transport abstraction with function pointers Message-ID: <20260617143943.GI10056@google.com> References: <20260525081736.2904310-1-a0282524688@gmail.com> <20260525081736.2904310-4-a0282524688@gmail.com> <20260604162519.GE4151951@google.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: On Fri, 05 Jun 2026, Ming Yu wrote: > Dear Lee, > > Thanks for the review. > > Lee Jones 於 2026年6月5日週五 上午12:25寫道： > > > > > @@ -92,9 +92,27 @@ struct nct6694 { > > > spinlock_t irq_lock; > > > unsigned int irq_enable; > > > void *priv; > > > + > > > + int (*read_msg)(struct nct6694 *nct6694, > > > + const struct nct6694_cmd_header *cmd_hd, > > > + void *buf); > > > + int (*write_msg)(struct nct6694 *nct6694, > > > + const struct nct6694_cmd_header *cmd_hd, > > > + void *buf); > > > }; > > > > > > -int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf); > > > -int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf); > > > +static inline int nct6694_read_msg(struct nct6694 *nct6694, > > > + const struct nct6694_cmd_header *cmd_hd, > > > + void *buf) > > > +{ > > > + return nct6694->read_msg(nct6694, cmd_hd, buf); > > > +} > > > + > > > +static inline int nct6694_write_msg(struct nct6694 *nct6694, > > > + const struct nct6694_cmd_header *cmd_hd, > > > + void *buf) > > > +{ > > > + return nct6694->write_msg(nct6694, cmd_hd, buf); > > > +} > > > > I very much dislike pointers to functions and abstraction for the sake > > of abstraction, at least at the driver-level. > > > > Can you find another way to solve this problem please? > > > > The reason for the abstraction is that patch 7/7 introduces a second > transport (HIF/eSPI), and the sub-device drivers need a way to perform > I/O without being coupled to a specific transport. > > I'd like to rework this -- could you let me know which approach you'd prefer? > > Keep nct6694_read_msg()/nct6694_write_msg() as exported functions in > nct6694-core.c, with the core dispatching to the appropriate > transport-specific implementation based on an internal transport type > indicator. > > Use a const struct nct6694_transport_ops *ops in struct nct6694, > following the pattern used by subsystems like regmap_bus and > spi_controller_mem_ops. > > Happy to go with whichever direction you think is cleaner. Can you let Regmap handle it? Most of our multi-bus drivers either create a Regmap in their bus-specific leaf driver then pass it up when initialising via the core, or use the Ops offered by Regmap Bus. Calling from core (higher level) _back_ into the leaf bus-specific drivers (lower level) is generally decried as are driver level Ops. I'm aware we have 2 drivers already doing this, but this has not been the way for the past 16 and 13 years! -- Lee Jones