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

[Lee Jones <lee@kernel.org>]

On Fri, 05 Jun 2026, Ming Yu wrote:

> Dear Lee,
> 
> Thanks for the review.
> 
> Lee Jones <lee@kernel.org> 於 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