On 9.08.2024 8:09 PM, Maximilian Luz wrote: > Hi, > > Thanks for taking this up! A couple of comments below: [...] > Doc needs updating, this is just the one copied from > ssam_controller_caps_load_acpi(). Oops. [...] >> }; >> -/* -- ACPI based device setup. ---------------------------------------------- */ >> +/* -- Serial device setup. ---------------------------------------------- */ > > Nitpick, but could we maybe keep that at 80 columns please? :) Sure > >> static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc, >> void *ctx) >> @@ -352,13 +355,28 @@ static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc, >> return AE_CTRL_TERMINATE; >> } >> -static acpi_status ssam_serdev_setup_via_acpi(acpi_handle handle, >> - struct serdev_device *serdev) >> +static int ssam_serdev_setup_via_acpi(struct serdev_device *serdev, acpi_handle handle) >> { >> - return acpi_walk_resources(handle, METHOD_NAME__CRS, >> - ssam_serdev_setup_via_acpi_crs, serdev); >> + acpi_status status; >> + >> + status = acpi_walk_resources(handle, METHOD_NAME__CRS, >> + ssam_serdev_setup_via_acpi_crs, serdev); >> + >> + return status ? -ENXIO : 0; >> } >> +static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev) >> +{ >> + if (ssh) >> + return ssam_serdev_setup_via_acpi(serdev, ssh->handle); >> + >> + /* TODO: these values may differ per board/implementation */ >> + serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ); > > Isn't this defined in the DT spec that you're adding as "current-speed" > in patch 2? Why not load it from there? Yeah and it's not under `required:`.. i added it for future proofing > >> + serdev_device_set_flow_control(serdev, true); >> + serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); >> + >> + return 0; >> +} >> /* -- Power management. ----------------------------------------------------- */ >> @@ -624,13 +642,15 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev) >> acpi_status astatus; > > This can be removed, see below. > >> int status; >> - status = gpiod_count(dev, NULL); >> - if (status < 0) >> - return dev_err_probe(dev, status, "no GPIO found\n"); >> + if (ssh) { >> + status = gpiod_count(dev, NULL); >> + if (status < 0) >> + return dev_err_probe(dev, status, "no GPIO found\n"); >> - status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios); >> - if (status) >> - return status; >> + status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios); >> + if (status) >> + return status; >> + } >> /* Allocate controller. */ >> ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); >> @@ -655,7 +675,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev) >> goto err_devopen; >> } >> - astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev); >> + astatus = ssam_serdev_setup(ssh, serdev);> if (ACPI_FAILURE(astatus)) { > > ssam_serdev_setup() returns an int, so this should now just use > "status". Right > >> status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n"); >> goto err_devinit; >> @@ -717,10 +737,31 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev) >> * For now let's thus default power/wakeup to false. >> */ >> device_set_wakeup_capable(dev, true); >> + >> + /* >> + * When using DT, we have to register the platform hub driver manually, >> + * as it can't be matched based on top-level board compatible (like it >> + * does the ACPI case). >> + */ >> + if (!ssh) { >> + struct platform_device *ph_pdev = >> + platform_device_register_simple("surface_aggregator_platform_hub", >> + 0, NULL, 0); >> + if (IS_ERR(ph_pdev)) >> + return dev_err_probe(dev, PTR_ERR(ph_pdev), >> + "Failed to register the platform hub driver\n"); >> + } >> + >> + status = ssam_register_clients(&serdev->dev, ctrl); >> + if (status) >> + goto err_clients; > > Is the ssam_register_clients() call required or is it a remnant from a > previous version? We're now not adding any children to the controller > itself but model ACPI and do all of that with the platform hub. So > unless I'm missing something, I think this should not be necessary. Yeah, the platform hub driver calls it anyway [...] >> +static const struct software_node *ssam_node_group_sl7[] = { >> + &ssam_node_root, >> + &ssam_node_bat_ac, >> + &ssam_node_bat_main, >> + &ssam_node_tmp_perf_profile_with_fan, >> + &ssam_node_fan_speed, >> + &ssam_node_hid_sam_keyboard, > > Did you check if there are any other HID devices connected? In the past, > keyboard and touchpad have been split into two separate devices, so is > it a combo keyboard + touchpad device this time? Some models also had > HID-based sensor and other devices. No, touchpad is wired directly to the SoC via QSPI, same for the touch panel > > Would just be good to know if this can be assumed to be complete or if > we're maybe missing something here. > >> + /* TODO: evaluate thermal sensors devices when we get a driver for that */ > > FYI I've posted the driver at [1]. It needs a small Kbuild dependency > fix but apart from that I think it should be final, if you want to give > that a try. > > [1]: https://lore.kernel.org/lkml/20240804230832.247852-1-luzmaximilian@xxxxxxxxx/T/ I'll give it a shot, thanks > > The rest looks fine. I'll try to find some time to update my SPX branch > this weekend and give it a spin. About time that thing lands upstream ;) Konrad