Hi Andy,
Thank you for the review.
Note this has already been merged though.
Still there are some good suggestions here for a follow-up
cleanup patch.
Regards,
Hans
On 8/26/24 10:54 PM, Andy Shevchenko wrote:
> Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
>> From: Konrad Dybcio <quic_kdybcio@xxxxxxxxxxx>
>>
>> Add basic support for registering the aggregator module on Device Tree-
>> based platforms. These include at least three generations of Qualcomm
>> Snapdragon-based Surface devices:
>>
>> - SC8180X / SQ1 / SQ2: Pro X,
>> - SC8280XP / SQ3: Devkit 2023, Pro 9
>> - X Elite: Laptop 7 / Pro11
>>
>> Thankfully, the aggregators on these seem to be configured in an
>> identical way, which allows for using these settings as defaults and
>> no DT properties need to be introduced (until that changes, anyway).
>>
>> Based on the work done by Maximilian Luz, largely rewritten.
>
> ...
>
>> sdev->dev.fwnode = fwnode_handle_get(node);
>> + sdev->dev.of_node = to_of_node(node);
>
> Please, use device_set_node() instead of those two.
>
> ...
>
>> +static int ssam_controller_caps_load(struct device *dev, struct ssam_controller_caps *caps)
>> +{
>
>> + acpi_handle handle = ACPI_HANDLE(dev);
>
> It's a bit non-standard way to check if we run on DT or ACPI. The others (most
> of them?) do something like this:
>
> struct fwnode_handle *fwnode = dev_fwnode(...);
>
> if (is_of_node(fwnode))
> return X;
> if (is_acpi_node(fwnode)) // also more precise _device or _data variant
> return Y;
>
> return ERROR;
>
>> + /* Set defaults. */
>> + caps->ssh_power_profile = U32_MAX;
>> + caps->screen_on_sleep_idle_timeout = U32_MAX;
>> + caps->screen_off_sleep_idle_timeout = U32_MAX;
>> + caps->d3_closes_handle = false;
>> + caps->ssh_buffer_size = U32_MAX;
>> +
>> + if (handle)
>> + return ssam_controller_caps_load_from_acpi(handle, caps);
>
> Yeah, I see that you use handle here, that's why it's up to you how to proceed
> with that.
>
>> + else
>> + return ssam_controller_caps_load_from_of(dev, caps);
>
> But just note that we have 4 options for fwnode type here and this covers 3 and
> I don't know if you ever have an ACPI data node or software node and what you
> want to do with that.
>
>> +}
>
> ...
>
>> gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>> - if (IS_ERR(gpiod))
>> - return PTR_ERR(gpiod);
>> -
>> - irq = gpiod_to_irq(gpiod);
>> - gpiod_put(gpiod);
>> + if (IS_ERR(gpiod)) {
>> + irq = fwnode_irq_get(dev_fwnode(dev), 0);
>> + } else {
>> + irq = gpiod_to_irq(gpiod);
>> + gpiod_put(gpiod);
>> + }
>
> Can't you try fwnode_irq_get_byname() followed by fwnode_irq_get()? And why do
> you need unnamed variant to begin with? As far as I understand it's pure DT and
> names are there, no?
>
> ...
>
>> #include <linux/kernel.h>
>> #include <linux/kref.h>
>> #include <linux/module.h>
>> +#include <linux/of.h>
>
> I do not see how you use this. You probably missed mod_devicetable.h.
>
>> +#include <linux/platform_device.h>
>> #include <linux/pm.h>
>> #include <linux/serdev.h>
>> #include <linux/sysfs.h>
>
> ...
>
>> + /*
>> + * 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");
>
>> + }
>> +
>> + if (ssh)
>
> Simply 'else' ? And making condition positive?
>
> ...
>
>> -static const struct acpi_device_id ssam_serial_hub_match[] = {
>> +static const struct acpi_device_id ssam_serial_hub_acpi_match[] = {
>> { "MSHW0084", 0 },
>> { },
>
> At some point, please drop that 0 part above and the comma in the terminator
> entry.
>
>> };
>> -MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_match);
>> +MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_acpi_match);
>
> Do you really need this renaming?
>
> ...
>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id ssam_serial_hub_of_match[] = {
>> + { .compatible = "microsoft,surface-sam", },
>
> No inner comma.
>
>> + { },
>
> No comma for the terminator.
>
>> +};
>> +MODULE_DEVICE_TABLE(of, ssam_serial_hub_of_match);
>> +#endif
>>
>> static struct serdev_device_driver ssam_serial_hub = {
>> .probe = ssam_serial_hub_probe,
>> .remove = ssam_serial_hub_remove,
>> .driver = {
>> .name = "surface_serial_hub",
>> - .acpi_match_table = ssam_serial_hub_match,
>> + .acpi_match_table = ACPI_PTR(ssam_serial_hub_acpi_match),
>
> No, please do not use ACPI_PTR(), it's more harmful than helpful.
>
>> + .of_match_table = of_match_ptr(ssam_serial_hub_of_match),
>
> There is ongoing task to drop of_match_ptr(), so for ACPI_PTR().
>
>> .pm = &ssam_serial_hub_pm_ops,
>> .shutdown = ssam_serial_hub_shutdown,
>> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>
> ...
>
>> --- a/drivers/platform/surface/surface3_power.c
>> +++ b/drivers/platform/surface/surface3_power.c
>> @@ -479,6 +479,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
>> }
>>
>> acpi_dev_clear_dependencies(adev);
>> +
>> return 0;
>> }
>
> Stray change.
>
> ...
>
>> +/* Devices for Surface Laptop 7. */
>> +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,
>> + /* TODO: evaluate thermal sensors devices when we get a driver for that */
>> + NULL,
>
> At some point please drop commas at the terminator entries. This will make code
> more robust against quite unlikely but potential rebase-like mistakes (when a
> new entry is added behind the terminator).
>
>> +};
>
> ...
>
>> -static const struct acpi_device_id ssam_platform_hub_match[] = {
>> +static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
>> /* Surface Pro 4, 5, and 6 (OMBR < 0x10) */
>> { "MSHW0081", (unsigned long)ssam_node_group_gen5 },
>>
>> @@ -400,18 +413,41 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
>>
>> { },
>> };
>> -MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
>> +MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
>
> If renaming is needed, it can be done in a separate patch.
>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id ssam_platform_hub_of_match[] = {
>> + /* Surface Laptop 7 */
>> + { .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 },
>> + { .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 },
>> + { },
>> +};
>> +#endif
>
> As per above.
>
> ...
>
>> static int ssam_platform_hub_probe(struct platform_device *pdev)
>> {
>> const struct software_node **nodes;
>> + const struct of_device_id *match;
>> + struct device_node *fdt_root;
>> struct ssam_controller *ctrl;
>> struct fwnode_handle *root;
>> int status;
>>
>> nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
>
> Hmm... Why this doesn't use simple device_get_match_data()?
>
>> - if (!nodes)
>> - return -ENODEV;
>> + if (!nodes) {
>> + fdt_root = of_find_node_by_path("/");
>> + if (!fdt_root)
>> + return -ENODEV;
>> +
>> + match = of_match_node(ssam_platform_hub_of_match, fdt_root);
>> + of_node_put(fdt_root);
>> + if (!match)
>> + return -ENODEV;
>> +
>> + nodes = (const struct software_node **)match->data;
>
> This is quite strange! Where are they being defined?
>
>> + if (!nodes)
>> + return -ENODEV;
>> + }
>
> ...
>
>> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
>
> Can it be platfrom device ID table instead? But do you really need it?
>
