Message-ID: <4dab4f36-309d-4b95-8b01-84963ca08d16@redhat.com>
Date: Tue, 27 Aug 2024 11:07:19 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
 Konrad Dybcio <konradybcio@...nel.org>
Cc: Rob Herring <robh@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Maximilian Luz <luzmaximilian@...il.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Marijn Suijten <marijn.suijten@...ainline.org>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-acpi@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <quic_kdybcio@...cinc.com>
Subject: Re: [PATCH v3 3/3] platform/surface: Add OF support

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@...cinc.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?
> 

Powered by blists - more mailing lists