Re: [PATCH 4/5] platform/x86: x86-android-tablets: Add support for getting serdev-controller by PCI parent

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sun, Nov 10, 2024 at 11:27:31PM +0100, Hans de Goede wrote:
> On 10-Nov-24 12:51 PM, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 12:05 AM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +       if (dev_info->use_pci)
> >> +               ctrl_dev = get_serdev_controller_by_pci_parent(info);
> >> +       else
> >> +               ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0,
> >> +                                                info->ctrl_devname);
> > 
> > I would expect that they both take info as an argument...
> 
> The "old" get_serdev_controller() helper for getting the controller
> by ACPI HID + UID is also used outside of the x86-android-tablets code
> and struct x86_serdev_info is x86-android-tablets specific.

I see, thanks for elaborating this.

> >>         if (IS_ERR(ctrl_dev))
> >>                 return PTR_ERR(ctrl_dev);

...

> >>  struct x86_serdev_info {
> >> +       /* For ACPI enumerated controllers */
> >>         const char *ctrl_hid;
> >>         const char *ctrl_uid;
> >> +       /* For PCI enumerated controllers */
> >> +       unsigned int ctrl_devfn;
> >> +       /* Typically "serial0" */
> >>         const char *ctrl_devname;
> > 
> > Why not union as we have a type selector, i.e. use_pci ?
> 
> A union would be possible. I simply did not think of that.
> 
> Not sure if it is worth the trouble though, since it saves
> only 8 bytes on a struct which is currently used only 3 
> times in the driver.

Saving bytes is not the main point, the main point is to make the code robust
against writing both fields and make things confusing. Along with that, union
gives a reader the clue that those fields are never used at the same time. So,
can you consider using union?

-- 
With Best Regards,
Andy Shevchenko