LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: Eric Piel <eric.piel@tremplin-utc.net>,
	Mark Gross <mgross@linux.intel.com>
Subject: Re: [RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver
Date: Fri, 6 Aug 2021 15:42:40 +0200	[thread overview]
Message-ID: <48332796-1b9a-c897-c695-e66b116386be@redhat.com> (raw)
In-Reply-To: <20210803200820.3259-1-andriy.shevchenko@linux.intel.com>

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");
> 


  reply	other threads:[~2021-08-06 13:42 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 20:08 Andy Shevchenko
2021-08-06 13:42 ` Hans de Goede [this message]
2021-08-23  9:33   ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48332796-1b9a-c897-c695-e66b116386be@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=eric.piel@tremplin-utc.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgross@linux.intel.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --subject='Re: [RFT, PATCH v1 1/1] platform/x86: hp_accel: Convert to be a platform driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).