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
