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

[Ming Yu]

Dear Lee,

Thanks for the review.

Lee Jones <lee@xxxxxxxxxx> 於 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.


Thanks,
Ming