[RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver

[RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver

[Andy Shevchenko]

ACPI core in conjunction with platform driver core provides
an infrastructure to enumerate ACPI devices. Use it in order
to remove a lot of boilerplate code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Not sure what buys us to run _INI on PM calls. It's against the spec AFAICT.
In any case ACPICA runs _INI as per specification when devices are
instantiated.

 drivers/platform/x86/hp_accel.c | 74 +++++++--------------------------
 1 file changed, 14 insertions(+), 60 deletions(-)

diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 8c0867bda828..69f86b761c7f 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -29,7 +29,6 @@
 #include "../../misc/lis3lv02d/lis3lv02d.h"
 
 #define DRIVER_NAME     "hp_accel"
-#define ACPI_MDPS_CLASS "accelerometer"
 
 /* Delayed LEDs infrastructure ------------------------------------ */
 
@@ -78,7 +77,6 @@ static const struct acpi_device_id lis3lv02d_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
 
-
 /**
  * lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
  * @lis3: pointer to the device struct
@@ -87,14 +85,6 @@ MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
  */
 static int lis3lv02d_acpi_init(struct lis3lv02d *lis3)
 {
-	struct acpi_device *dev = lis3->bus_priv;
-	if (!lis3->init_required)
-		return 0;
-
-	if (acpi_evaluate_object(dev->handle, METHOD_NAME__INI,
-				 NULL, NULL) != AE_OK)
-		return -EINVAL;
-
 	return 0;
 }
 
@@ -278,30 +268,6 @@ static struct delayed_led_classdev hpled_led = {
 	.set_brightness = hpled_set,
 };
 
-static acpi_status
-lis3lv02d_get_resource(struct acpi_resource *resource, void *context)
-{
-	if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
-		struct acpi_resource_extended_irq *irq;
-		u32 *device_irq = context;
-
-		irq = &resource->data.extended_irq;
-		*device_irq = irq->interrupts[0];
-	}
-
-	return AE_OK;
-}
-
-static void lis3lv02d_enum_resources(struct acpi_device *device)
-{
-	acpi_status status;
-
-	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-					lis3lv02d_get_resource, &lis3_dev.irq);
-	if (ACPI_FAILURE(status))
-		printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
-}
-
 static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
 				  struct serio *port)
 {
@@ -331,23 +297,19 @@ static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
 	return false;
 }
 
-static int lis3lv02d_add(struct acpi_device *device)
+static int lis3lv02d_probe(struct platform_device *device)
 {
 	int ret;
 
-	if (!device)
-		return -EINVAL;
-
-	lis3_dev.bus_priv = device;
+	lis3_dev.bus_priv = ACPI_COMPANION(&device->dev);
 	lis3_dev.init = lis3lv02d_acpi_init;
 	lis3_dev.read = lis3lv02d_acpi_read;
 	lis3_dev.write = lis3lv02d_acpi_write;
-	strcpy(acpi_device_name(device), DRIVER_NAME);
-	strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
-	device->driver_data = &lis3_dev;
 
 	/* obtain IRQ number of our device from ACPI */
-	lis3lv02d_enum_resources(device);
+	ret = platform_get_irq_optional(device, 0);
+	if (ret > 0)
+		lis3_dev.irq = ret;
 
 	/* If possible use a "standard" axes order */
 	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
@@ -359,7 +321,6 @@ static int lis3lv02d_add(struct acpi_device *device)
 	}
 
 	/* call the core layer do its init */
-	lis3_dev.init_required = true;
 	ret = lis3lv02d_init_device(&lis3_dev);
 	if (ret)
 		return ret;
@@ -381,11 +342,8 @@ static int lis3lv02d_add(struct acpi_device *device)
 	return ret;
 }
 
-static int lis3lv02d_remove(struct acpi_device *device)
+static int lis3lv02d_remove(struct platform_device *device)
 {
-	if (!device)
-		return -EINVAL;
-
 	i8042_remove_filter(hp_accel_i8042_filter);
 	lis3lv02d_joystick_disable(&lis3_dev);
 	lis3lv02d_poweroff(&lis3_dev);
@@ -396,7 +354,6 @@ static int lis3lv02d_remove(struct acpi_device *device)
 	return lis3lv02d_remove_fs(&lis3_dev);
 }
 
-
 #ifdef CONFIG_PM_SLEEP
 static int lis3lv02d_suspend(struct device *dev)
 {
@@ -407,14 +364,12 @@ static int lis3lv02d_suspend(struct device *dev)
 
 static int lis3lv02d_resume(struct device *dev)
 {
-	lis3_dev.init_required = false;
 	lis3lv02d_poweron(&lis3_dev);
 	return 0;
 }
 
 static int lis3lv02d_restore(struct device *dev)
 {
-	lis3_dev.init_required = true;
 	lis3lv02d_poweron(&lis3_dev);
 	return 0;
 }
@@ -434,17 +389,16 @@ static const struct dev_pm_ops hp_accel_pm = {
 #endif
 
 /* For the HP MDPS aka 3D Driveguard */
-static struct acpi_driver lis3lv02d_driver = {
-	.name  = DRIVER_NAME,
-	.class = ACPI_MDPS_CLASS,
-	.ids   = lis3lv02d_device_ids,
-	.ops = {
-		.add     = lis3lv02d_add,
-		.remove  = lis3lv02d_remove,
+static struct platform_driver lis3lv02d_driver = {
+	.probe	= lis3lv02d_probe,
+	.remove	= lis3lv02d_remove,
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.pm	= HP_ACCEL_PM,
+		.acpi_match_table = lis3lv02d_device_ids,
 	},
-	.drv.pm = HP_ACCEL_PM,
 };
-module_acpi_driver(lis3lv02d_driver);
+module_platform_driver(lis3lv02d_driver);
 
 MODULE_DESCRIPTION("Glue between LIS3LV02Dx and HP ACPI BIOS and support for disk protection LED.");
 MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
-- 
2.30.2

Re: [RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver

[Hans de Goede]

Hi,

On 8/3/21 10:08 PM, Andy Shevchenko wrote:
> ACPI core in conjunction with platform driver core provides
> an infrastructure to enumerate ACPI devices. Use it in order
> to remove a lot of boilerplate code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> Not sure what buys us to run _INI on PM calls. It's against the spec AFAICT.
> In any case ACPICA runs _INI as per specification when devices are
> instantiated.

_INI used to also be ran on resume for some reason, but that was recently
changed.

You're right that calling it is no longer necessary now that we no longer
do that.

But the changes related to this are really separate from the platform
driver conversion, please split this into 2 patches.

Also for the next version please Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
and ask him to test, I think he has access to hardware to test this.

Regards,

Hans


> 
>  drivers/platform/x86/hp_accel.c | 74 +++++++--------------------------
>  1 file changed, 14 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
> index 8c0867bda828..69f86b761c7f 100644
> --- a/drivers/platform/x86/hp_accel.c
> +++ b/drivers/platform/x86/hp_accel.c
> @@ -29,7 +29,6 @@
>  #include "../../misc/lis3lv02d/lis3lv02d.h"
>  
>  #define DRIVER_NAME     "hp_accel"
> -#define ACPI_MDPS_CLASS "accelerometer"
>  
>  /* Delayed LEDs infrastructure ------------------------------------ */
>  
> @@ -78,7 +77,6 @@ static const struct acpi_device_id lis3lv02d_device_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
>  
> -
>  /**
>   * lis3lv02d_acpi_init - ACPI _INI method: initialize the device.
>   * @lis3: pointer to the device struct
> @@ -87,14 +85,6 @@ MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
>   */
>  static int lis3lv02d_acpi_init(struct lis3lv02d *lis3)
>  {
> -	struct acpi_device *dev = lis3->bus_priv;
> -	if (!lis3->init_required)
> -		return 0;
> -
> -	if (acpi_evaluate_object(dev->handle, METHOD_NAME__INI,
> -				 NULL, NULL) != AE_OK)
> -		return -EINVAL;
> -
>  	return 0;
>  }
>  
> @@ -278,30 +268,6 @@ static struct delayed_led_classdev hpled_led = {
>  	.set_brightness = hpled_set,
>  };
>  
> -static acpi_status
> -lis3lv02d_get_resource(struct acpi_resource *resource, void *context)
> -{
> -	if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
> -		struct acpi_resource_extended_irq *irq;
> -		u32 *device_irq = context;
> -
> -		irq = &resource->data.extended_irq;
> -		*device_irq = irq->interrupts[0];
> -	}
> -
> -	return AE_OK;
> -}
> -
> -static void lis3lv02d_enum_resources(struct acpi_device *device)
> -{
> -	acpi_status status;
> -
> -	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> -					lis3lv02d_get_resource, &lis3_dev.irq);
> -	if (ACPI_FAILURE(status))
> -		printk(KERN_DEBUG DRIVER_NAME ": Error getting resources\n");
> -}
> -
>  static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
>  				  struct serio *port)
>  {
> @@ -331,23 +297,19 @@ static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
>  	return false;
>  }
>  
> -static int lis3lv02d_add(struct acpi_device *device)
> +static int lis3lv02d_probe(struct platform_device *device)
>  {
>  	int ret;
>  
> -	if (!device)
> -		return -EINVAL;
> -
> -	lis3_dev.bus_priv = device;
> +	lis3_dev.bus_priv = ACPI_COMPANION(&device->dev);
>  	lis3_dev.init = lis3lv02d_acpi_init;
>  	lis3_dev.read = lis3lv02d_acpi_read;
>  	lis3_dev.write = lis3lv02d_acpi_write;
> -	strcpy(acpi_device_name(device), DRIVER_NAME);
> -	strcpy(acpi_device_class(device), ACPI_MDPS_CLASS);
> -	device->driver_data = &lis3_dev;
>  
>  	/* obtain IRQ number of our device from ACPI */
> -	lis3lv02d_enum_resources(device);
> +	ret = platform_get_irq_optional(device, 0);
> +	if (ret > 0)
> +		lis3_dev.irq = ret;
>  
>  	/* If possible use a "standard" axes order */
>  	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
> @@ -359,7 +321,6 @@ static int lis3lv02d_add(struct acpi_device *device)
>  	}
>  
>  	/* call the core layer do its init */
> -	lis3_dev.init_required = true;
>  	ret = lis3lv02d_init_device(&lis3_dev);
>  	if (ret)
>  		return ret;
> @@ -381,11 +342,8 @@ static int lis3lv02d_add(struct acpi_device *device)
>  	return ret;
>  }
>  
> -static int lis3lv02d_remove(struct acpi_device *device)
> +static int lis3lv02d_remove(struct platform_device *device)
>  {
> -	if (!device)
> -		return -EINVAL;
> -
>  	i8042_remove_filter(hp_accel_i8042_filter);
>  	lis3lv02d_joystick_disable(&lis3_dev);
>  	lis3lv02d_poweroff(&lis3_dev);
> @@ -396,7 +354,6 @@ static int lis3lv02d_remove(struct acpi_device *device)
>  	return lis3lv02d_remove_fs(&lis3_dev);
>  }
>  
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int lis3lv02d_suspend(struct device *dev)
>  {
> @@ -407,14 +364,12 @@ static int lis3lv02d_suspend(struct device *dev)
>  
>  static int lis3lv02d_resume(struct device *dev)
>  {
> -	lis3_dev.init_required = false;
>  	lis3lv02d_poweron(&lis3_dev);
>  	return 0;
>  }
>  
>  static int lis3lv02d_restore(struct device *dev)
>  {
> -	lis3_dev.init_required = true;
>  	lis3lv02d_poweron(&lis3_dev);
>  	return 0;
>  }
> @@ -434,17 +389,16 @@ static const struct dev_pm_ops hp_accel_pm = {
>  #endif
>  
>  /* For the HP MDPS aka 3D Driveguard */
> -static struct acpi_driver lis3lv02d_driver = {
> -	.name  = DRIVER_NAME,
> -	.class = ACPI_MDPS_CLASS,
> -	.ids   = lis3lv02d_device_ids,
> -	.ops = {
> -		.add     = lis3lv02d_add,
> -		.remove  = lis3lv02d_remove,
> +static struct platform_driver lis3lv02d_driver = {
> +	.probe	= lis3lv02d_probe,
> +	.remove	= lis3lv02d_remove,
> +	.driver	= {
> +		.name	= DRIVER_NAME,
> +		.pm	= HP_ACCEL_PM,
> +		.acpi_match_table = lis3lv02d_device_ids,
>  	},
> -	.drv.pm = HP_ACCEL_PM,
>  };
> -module_acpi_driver(lis3lv02d_driver);
> +module_platform_driver(lis3lv02d_driver);
>  
>  MODULE_DESCRIPTION("Glue between LIS3LV02Dx and HP ACPI BIOS and support for disk protection LED.");
>  MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
> 

Re: [RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver

[Andy Shevchenko]

On Fri, Aug 06, 2021 at 03:42:40PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/3/21 10:08 PM, Andy Shevchenko wrote:
> > ACPI core in conjunction with platform driver core provides
> > an infrastructure to enumerate ACPI devices. Use it in order
> > to remove a lot of boilerplate code.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > 
> > Not sure what buys us to run _INI on PM calls. It's against the spec AFAICT.
> > In any case ACPICA runs _INI as per specification when devices are
> > instantiated.
> 
> _INI used to also be ran on resume for some reason, but that was recently
> changed.
> 
> You're right that calling it is no longer necessary now that we no longer
> do that.
> 
> But the changes related to this are really separate from the platform
> driver conversion, please split this into 2 patches.
> 
> Also for the next version please Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> and ask him to test, I think he has access to hardware to test this.

v2 has been sent.

-- 
With Best Regards,
Andy Shevchenko