Hi,
On 2-Dec-24 5:34 PM, Ilpo Järvinen wrote:
> On Sat, 16 Nov 2024, Hans de Goede wrote:
>
>> The x86-android-tablets code needs to be able to get a serdev_controller
>> device from a PCI parent, rather then by the ACPI HID+UID of the parent,
>> because on some tablets the UARTs are enumerated as PCI devices instead
>> of ACPI devices.
>>
>> Split the code to walk the device hierarchy to find the serdev_controller
>> from its parents out into a get_serdev_controller_from_parent() helper
>> so that the x86-android-tablets code can re-use it.
>>
>> Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> drivers/platform/x86/serdev_helpers.h | 60 +++++++++++++++------------
>> 1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
>> index bcf3a0c356ea..b592b9ff6d93 100644
>> --- a/drivers/platform/x86/serdev_helpers.h
>> +++ b/drivers/platform/x86/serdev_helpers.h
>> @@ -22,32 +22,14 @@
>> #include <linux/string.h>
>>
>> static inline struct device *
>> -get_serdev_controller(const char *serial_ctrl_hid,
>> - const char *serial_ctrl_uid,
>> - int serial_ctrl_port,
>> - const char *serdev_ctrl_name)
>> +get_serdev_controller_from_parent(struct device *ctrl_dev,
>> + int serial_ctrl_port,
>> + const char *serdev_ctrl_name)
>> {
>> - struct device *ctrl_dev, *child;
>> - struct acpi_device *ctrl_adev;
>> + struct device *child;
>> char name[32];
>> int i;
>>
>> - ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> - if (!ctrl_adev) {
>> - pr_err("error could not get %s/%s serial-ctrl adev\n",
>> - serial_ctrl_hid, serial_ctrl_uid);
>> - return ERR_PTR(-ENODEV);
>> - }
>> -
>> - /* get_first_physical_node() returns a weak ref */
>> - ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev));
>> - if (!ctrl_dev) {
>> - pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> - serial_ctrl_hid, serial_ctrl_uid);
>> - ctrl_dev = ERR_PTR(-ENODEV);
>> - goto put_ctrl_adev;
>> - }
>> -
>> /* Walk host -> uart-ctrl -> port -> serdev-ctrl */
>> for (i = 0; i < 3; i++) {
>> switch (i) {
>> @@ -67,14 +49,40 @@ get_serdev_controller(const char *serial_ctrl_hid,
>> put_device(ctrl_dev);
>> if (!child) {
>> pr_err("error could not find '%s' device\n", name);
>> - ctrl_dev = ERR_PTR(-ENODEV);
>> - goto put_ctrl_adev;
>> + return ERR_PTR(-ENODEV);
>> }
>>
>> ctrl_dev = child;
>> }
>>
>> -put_ctrl_adev:
>> - acpi_dev_put(ctrl_adev);
>> return ctrl_dev;
>> }
>> +
>> +static inline struct device *
>> +get_serdev_controller(const char *serial_ctrl_hid,
>> + const char *serial_ctrl_uid,
>> + int serial_ctrl_port,
>> + const char *serdev_ctrl_name)
>> +{
>> + struct acpi_device *adev;
>> + struct device *parent;
>> +
>> + adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> + if (!adev) {
>> + pr_err("error could not get %s/%s serial-ctrl adev\n",
>> + serial_ctrl_hid, serial_ctrl_uid);
>
> Hi,
>
> With the current code (and I suppose this moved too), W=1 build detects
> that dell_uart_bl_pdev_probe() passed NULL which is then being formatted
> here with %s. While it "works", it would be useful to solve the warning
> and perhaps "/(null)" appearing in the print is also confusing to user
> so maybe do another patch to change serial_ctrl_uid to e.g.:
>
> serial_ctrl_uid ?: "*"
>
> (There's another print below with the same problem).
Ack, for v3 I'll insert a new patch in the series before this patch
fixing this in the original code, including a Fixes: tag of the commit
introducing this.
Regards,
Hans
>
> --
> i.
>
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + /* get_first_physical_node() returns a weak ref */
>> + parent = get_device(acpi_get_first_physical_node(adev));
>> + acpi_dev_put(adev);
>> + if (!parent) {
>> + pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> + serial_ctrl_hid, serial_ctrl_uid);
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + /* This puts our reference on parent and returns a ref on the ctrl */
>> + return get_serdev_controller_from_parent(parent, serial_ctrl_port, serdev_ctrl_name);
>> +}
>>
>
