Re: [PATCH v5 3/7] mfd: nct6694: Introduce transport abstraction with function pointers

[Lee Jones <lee@kernel.org>]

On Mon, 25 May 2026, a0282524688@gmail.com wrote:

> From: Ming Yu <a0282524688@gmail.com>
> 
> Add `read_msg` and `write_msg` function pointers to `struct nct6694`
> and provide static inline helpers for sub-devices. The USB-specific
> I/O functions are made static and assigned during probe.
> 
> This decouples sub-device drivers from the underlying USB implementation,
> paving the way for alternative transport interfaces (e.g., HIF) in the
> future without modifying client drivers.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
> Changes in v5:
> - Split from the monolithic v4 patch to follow the single logical change principle.
> 
>  drivers/mfd/nct6694.c       | 12 ++++++++----
>  include/linux/mfd/nct6694.h | 22 ++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 58c1cbcbe3f2..b88863a0b70f 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
> @@ -115,7 +115,9 @@ static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char
>   *
>   * Return: Negative value on error or 0 on success.
>   */
> -int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
> +static int nct6694_read_msg(struct nct6694 *nct6694,
> +			    const struct nct6694_cmd_header *cmd_hd,
> +			    void *buf)
>  {
>  	struct nct6694_usb_data *udata = nct6694->priv;
>  	union nct6694_usb_msg *msg = udata->usb_msg;
> @@ -153,7 +155,6 @@ int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *c
>  
>  	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
>  }
> -EXPORT_SYMBOL_GPL(nct6694_read_msg);
>  
>  /**
>   * nct6694_write_msg() - Write message to NCT6694 device
> @@ -166,7 +167,9 @@ EXPORT_SYMBOL_GPL(nct6694_read_msg);
>   *
>   * Return: Negative value on error or 0 on success.
>   */
> -int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
> +static int nct6694_write_msg(struct nct6694 *nct6694,
> +			     const struct nct6694_cmd_header *cmd_hd,
> +			     void *buf)
>  {
>  	struct nct6694_usb_data *udata = nct6694->priv;
>  	union nct6694_usb_msg *msg = udata->usb_msg;
> @@ -210,7 +213,6 @@ int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *
>  
>  	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
>  }
> -EXPORT_SYMBOL_GPL(nct6694_write_msg);
>  
>  static void usb_int_callback(struct urb *urb)
>  {
> @@ -327,6 +329,8 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	udata->udev = udev;
>  
>  	nct6694->priv = udata;
> +	nct6694->read_msg = nct6694_read_msg;
> +	nct6694->write_msg = nct6694_write_msg;
>  
>  	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
>  						   &nct6694_irq_domain_ops,
> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> index 3f5dd53f38de..3079c74110aa 100644
> --- a/include/linux/mfd/nct6694.h
> +++ b/include/linux/mfd/nct6694.h
> @@ -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?


-- 
Lee Jones