Re: [PATCH 3/3] platform/surface: Add OF support

[Konrad Dybcio <konradybcio@gmail.com>]

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@gmail.com/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