[v3,2/2] platform/surface: aggregator: Log critical errors during SAM probing

Emits messages upon errors during probing of SAM.  Hopefully this could
provide useful context to user for the purpose of diagnosis when
something miserable happen.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
 drivers/platform/surface/aggregator/core.c | 42 ++++++++++++++--------
 1 file changed, 27 insertions(+), 15 deletions(-)

Hi,

On 5/5/24 3:07 PM, Weifeng Liu wrote:
> Emits messages upon errors during probing of SAM.  Hopefully this could
> provide useful context to user for the purpose of diagnosis when
> something miserable happen.
> 
> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/surface/aggregator/core.c | 42 ++++++++++++++--------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index 7b1871eb7a6f..046fa63446bf 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -618,15 +618,17 @@ static const struct acpi_gpio_mapping ssam_acpi_gpios[] = {
>  
>  static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  {
> -	struct acpi_device *ssh = ACPI_COMPANION(&serdev->dev);
> +	struct device *dev = &serdev->dev;
> +	struct acpi_device *ssh = ACPI_COMPANION(dev);
>  	struct ssam_controller *ctrl;
>  	acpi_status astatus;
>  	int status;
>  
> -	if (gpiod_count(&serdev->dev, NULL) < 0)
> -		return -ENODEV;
> +	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(&serdev->dev, ssam_acpi_gpios);
> +	status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
>  	if (status)
>  		return status;
>  
> @@ -637,8 +639,11 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  
>  	/* Initialize controller. */
>  	status = ssam_controller_init(ctrl, serdev);
> -	if (status)
> +	if (status) {
> +		dev_err_probe(dev, status,
> +			      "failed to initialize ssam controller\n");
>  		goto err_ctrl_init;
> +	}
>  
>  	ssam_controller_lock(ctrl);
>  
> @@ -657,14 +662,13 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  	if (status == -ENXIO)
>  		status = -EPROBE_DEFER;
>  	if (status) {
> -		dev_err_probe(&serdev->dev, status,
> -			      "failed to open serdev device\n");
> +		dev_err_probe(dev, status, "failed to open serdev device\n");
>  		goto err_devopen;
>  	}
>  
>  	astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
>  	if (ACPI_FAILURE(astatus)) {
> -		status = -ENXIO;
> +		status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
>  		goto err_devinit;
>  	}
>  
> @@ -680,25 +684,33 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  	 * states.
>  	 */
>  	status = ssam_log_firmware_version(ctrl);
> -	if (status)
> +	if (status) {
> +		dev_err_probe(dev, status, "failed to get firmware version\n");
>  		goto err_initrq;
> +	}
>  
>  	status = ssam_ctrl_notif_d0_entry(ctrl);
> -	if (status)
> +	if (status) {
> +		dev_err_probe(dev, status, "D0-entry notification failed\n");
>  		goto err_initrq;
> +	}
>  
>  	status = ssam_ctrl_notif_display_on(ctrl);
> -	if (status)
> +	if (status) {
> +		dev_err_probe(dev, status, "display-on notification failed\n");
>  		goto err_initrq;
> +	}
>  
> -	status = sysfs_create_group(&serdev->dev.kobj, &ssam_sam_group);
> +	status = sysfs_create_group(&dev->kobj, &ssam_sam_group);
>  	if (status)
>  		goto err_initrq;
>  
>  	/* Set up IRQ. */
>  	status = ssam_irq_setup(ctrl);
> -	if (status)
> +	if (status) {
> +		dev_err_probe(dev, status, "failed to setup IRQ\n");
>  		goto err_irq;
> +	}
>  
>  	/* Finally, set main controller reference. */
>  	status = ssam_try_set_controller(ctrl);
> @@ -715,7 +727,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  	 *       resumed. In short, this causes some spurious unwanted wake-ups.
>  	 *       For now let's thus default power/wakeup to false.
>  	 */
> -	device_set_wakeup_capable(&serdev->dev, true);
> +	device_set_wakeup_capable(dev, true);
>  	acpi_dev_clear_dependencies(ssh);
>  
>  	return 0;
> @@ -723,7 +735,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>  err_mainref:
>  	ssam_irq_free(ctrl);
>  err_irq:
> -	sysfs_remove_group(&serdev->dev.kobj, &ssam_sam_group);
> +	sysfs_remove_group(&dev->kobj, &ssam_sam_group);
>  err_initrq:
>  	ssam_controller_lock(ctrl);
>  	ssam_controller_shutdown(ctrl);