Hi,
On 11-Nov-24 10:37 AM, Andy Shevchenko wrote:
> 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@xxxxxxxxxx> 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?
Ack, I will prepare a v2 using an union when I have some time to work on this.
Regards,
Hans
