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

Andy Shevchenko
2024-08-26T20:54:40Z

Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
> From: Konrad Dybcio <quic_kdybcio@quicinc.com>
>
> 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?

--
With Best Regards,
Andy Shevchenko