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

Gil Fine
2022-05-16T13:12:15Z

Hi Mika,

On Mon, May 16, 2022 at 12:34:06PM +0300, Mika Westerberg wrote:
> 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.

OK, got you and fixed that.
Please let me know when you think that I can send out the v4 series.

--
Thanks,
Gil