Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Mon, May 16, 2022 at 11:59:03AM +0300, Gil Fine wrote:
> Hi Mika,
> 
> On Mon, May 16, 2022 at 11:34:15AM +0300, Mika Westerberg wrote:
> > Hi Gil,
> > 
> > On Sun, May 15, 2022 at 11:27:46PM +0300, Gil Fine wrote:
> > > > > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> > > > 
> > > > Also can we please make it take some real type and not something
> > > > arbitrary?
> > > You mean the names, right?
> > > Something like:
> > > int tb_switch_tmu_config_enable(struct device *parent, void *rate)
> > > If so, yes, I will
> > 
> > I mean use a real type, not void *.
> > 
> > > > 
> > > > Can it be const too?
> > > IIUC, it shall be a function pointer with specified signature otherwise it will fail
> > > at compilation
> > 
> > Okay then I suggest to make a reasonable "API" function that handles
> > all this internally that does not take arbitrary pointers. Remember to
> > document it in kernel-doc too.
> 
> This is a function pointer that shall be passed to device_for_each_child()
> And it has to be defined as:
> 
> int (*fn)(struct device *dev, void *data)
> 
> Similar as here e.g.:
> 
> static int remove_retimer(struct device *dev, void *data)
> {
> »·······struct tb_retimer *rt = tb_to_retimer(dev);
> »·······struct tb_port *port = data;
> 
> »·······if (rt && rt->port == port)
> »·······»·······tb_retimer_remove(rt);
> »·······return 0;
> }
> 
> void tb_retimer_remove_all(struct tb_port *port)
> {
> »·······struct usb4_port *usb4;
> 
> »·······usb4 = port->usb4;
> »·······if (usb4)
> »·······»·······device_for_each_child_reverse(&usb4->dev, port,
> »·······»·······»·······»·······»·······      remove_retimer);
> }
> 
> So not sure I get you...

The difference is that above it is static function not exposed outside
of that file and used directly below its implementation.

In your case you make it non-static "API" function exported from tmu.c
and called from tb.c.

So instead I suggest to put the device_for_each_child() in tmu.c and
then the tb_switch_tmu_config_enable() static right above it. Please
also name the resulting API function consistently.