Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1429557 > unrolled thread

[PATCH v3 3/3] acpi/pmic: Add support for PMIC regs operation region

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 3/3] acpi/pmic: Add support for PMIC regs operation region Bin Gao <bin.gao@linux.intel.com> - 2016-06-23 10:10 +0200
    Re: [PATCH v3 3/3] acpi/pmic: Add support for PMIC regs operation  region Aaron Lu <aaron.lu@intel.com> - 2016-06-23 11:10 +0200

#1429557 — [PATCH v3 3/3] acpi/pmic: Add support for PMIC regs operation region

Broxton platform firmware has defined new customized operation
regions called regs for PMIC chip which is used to handle the
PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.

The intel_pmic_regs structure is created also for this purpose
of handling the PMIC register read and write.

Signed-off-by: Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Bin Gao <bin.gao@intel.com>
---
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 +++++++++++++++++++++++++++++++++++++-----
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index 410e96f..53b7052 100644
--- a/drivers/acpi/pmic/intel_pmic.c
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -21,12 +21,14 @@
 
 #define PMIC_POWER_OPREGION_ID		0x8d
 #define PMIC_THERMAL_OPREGION_ID	0x8c
+#define PMIC_REGS_OPREGION_ID		0x8f
 
 struct intel_pmic_opregion {
 	struct mutex lock;
 	struct acpi_lpat_conversion_table *lpat_table;
 	struct regmap *regmap;
 	struct intel_pmic_opregion_data *data;
+	struct pmic_regs_handler	ctx;
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,
@@ -204,6 +206,52 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
 	return AE_OK;
 }
 
+static acpi_status intel_pmic_regs_handler(u32 function,
+	acpi_physical_address address, u32 bits, u64 *value64,
+		void *handler_context, void *region_context)
+{
+	struct intel_pmic_opregion *opregion = region_context;
+	struct intel_pmic_opregion_data *d = opregion->data;
+	int result = 0;
+
+	switch (address) {
+	case 0:
+		return AE_OK;
+	case 1:
+		opregion->ctx.address |= (*value64 & 0xff) << 8;
+		return AE_OK;
+	case 2:
+		opregion->ctx.address |= *value64 & 0xff;
+		return AE_OK;
+	case 3:
+		opregion->ctx.value = *value64 & 0xff;
+		return AE_OK;
+	case 4:
+		if (*value64) {
+			result = d->regs_write(opregion->regmap,
+					opregion->ctx.address,
+					opregion->ctx.value);
+		} else {
+			result = d->regs_read(opregion->regmap,
+					opregion->ctx.address,
+					&opregion->ctx.value);
+			if (result == 0)
+				*value64 = opregion->ctx.value;
+		}
+		memset(&opregion->ctx, 0x00, sizeof(opregion->ctx));
+	}
+
+	if (result < 0) {
+		if (result == -EINVAL)
+			return AE_BAD_PARAMETER;
+		else
+			return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+
 int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 					struct regmap *regmap,
 					struct intel_pmic_opregion_data *d)
@@ -227,21 +275,31 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 	opregion->lpat_table = acpi_lpat_get_conversion_table(handle);
 
 	status = acpi_install_address_space_handler(handle,
-						    PMIC_POWER_OPREGION_ID,
-						    intel_pmic_power_handler,
-						    NULL, opregion);
+				PMIC_POWER_OPREGION_ID,
+				intel_pmic_power_handler,
+					NULL, opregion);
+	if (ACPI_FAILURE(status)) {
+		ret = -ENODEV;
+		goto out_error;
+	}
+
+	status = acpi_install_address_space_handler(handle,
+				PMIC_THERMAL_OPREGION_ID,
+				intel_pmic_thermal_handler,
+					NULL, opregion);
 	if (ACPI_FAILURE(status)) {
+		acpi_remove_address_space_handler(handle,
+				PMIC_POWER_OPREGION_ID,
+				intel_pmic_power_handler);
 		ret = -ENODEV;
 		goto out_error;
 	}
 
 	status = acpi_install_address_space_handler(handle,
-						    PMIC_THERMAL_OPREGION_ID,
-						    intel_pmic_thermal_handler,
-						    NULL, opregion);
+					PMIC_REGS_OPREGION_ID,
+				intel_pmic_regs_handler, NULL,
+			opregion);
 	if (ACPI_FAILURE(status)) {
-		acpi_remove_address_space_handler(handle, PMIC_POWER_OPREGION_ID,
-						  intel_pmic_power_handler);
 		ret = -ENODEV;
 		goto out_error;
 	}
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
index 2f39ee0..bdb7dcc 100644
--- a/drivers/acpi/pmic/intel_pmic.h
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -7,6 +7,11 @@ struct pmic_table {
 	int bit;	/* control bit for power */
 };
 
+struct pmic_regs_handler {
+	u16 address;		/* pmic regs address */
+	unsigned int value;	/* value to write @regs address */
+};
+
 struct intel_pmic_opregion_data {
 	int (*get_power)(struct regmap *r, int reg, int bit, u64 *value);
 	int (*update_power)(struct regmap *r, int reg, int bit, bool on);
-- 
1.9.1

[toc] | [next] | [standalone]

#1429614 — Re: [PATCH v3 3/3] acpi/pmic: Add support for PMIC regs operation region

On Thu, Jun 23, 2016 at 01:06:30AM -0700, Bin Gao wrote:
> Broxton platform firmware has defined new customized operation
> regions called regs for PMIC chip which is used to handle the
> PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
> 
> The intel_pmic_regs structure is created also for this purpose
> of handling the PMIC register read and write.
> 
> Signed-off-by: Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Bin Gao <bin.gao@intel.com>
> ---
> Changes in v3: none
> Changes in v2: none
>  drivers/acpi/pmic/intel_pmic.c | 74 +++++++++++++++++++++++++++++++++++++-----
>  drivers/acpi/pmic/intel_pmic.h |  5 +++
>  2 files changed, 71 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index 410e96f..53b7052 100644
> --- a/drivers/acpi/pmic/intel_pmic.c
> +++ b/drivers/acpi/pmic/intel_pmic.c
> @@ -21,12 +21,14 @@
>  
>  #define PMIC_POWER_OPREGION_ID		0x8d
>  #define PMIC_THERMAL_OPREGION_ID	0x8c
> +#define PMIC_REGS_OPREGION_ID		0x8f
>  
>  struct intel_pmic_opregion {
>  	struct mutex lock;
>  	struct acpi_lpat_conversion_table *lpat_table;
>  	struct regmap *regmap;
>  	struct intel_pmic_opregion_data *data;
> +	struct pmic_regs_handler	ctx;

What about call it pmic_regs_ctx? pmic_regs_handler sounds like a
function to me.

>  };
>  
>  static int pmic_get_reg_bit(int address, struct pmic_table *table,
> @@ -204,6 +206,52 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
>  	return AE_OK;
>  }
>  
> +static acpi_status intel_pmic_regs_handler(u32 function,

It looks like a IOCTL interface, so maybe call it
intel_pmic_ioctl_handler, but this is just bikeshedding.
If you decide to change it, then the "struct pmic_regs_handler" should
probably be renamed as "struct pmic_ioctl_ctx".

> +	acpi_physical_address address, u32 bits, u64 *value64,
> +		void *handler_context, void *region_context)
> +{
> +	struct intel_pmic_opregion *opregion = region_context;
> +	struct intel_pmic_opregion_data *d = opregion->data;
> +	int result = 0;
> +
> +	switch (address) {
> +	case 0:
> +		return AE_OK;
> +	case 1:
> +		opregion->ctx.address |= (*value64 & 0xff) << 8;
> +		return AE_OK;
> +	case 2:
> +		opregion->ctx.address |= *value64 & 0xff;
> +		return AE_OK;
> +	case 3:
> +		opregion->ctx.value = *value64 & 0xff;
> +		return AE_OK;
> +	case 4:
> +		if (*value64) {
> +			result = d->regs_write(opregion->regmap,
> +					opregion->ctx.address,
> +					opregion->ctx.value);
> +		} else {
> +			result = d->regs_read(opregion->regmap,
> +					opregion->ctx.address,
> +					&opregion->ctx.value);
> +			if (result == 0)
> +				*value64 = opregion->ctx.value;
> +		}
> +		memset(&opregion->ctx, 0x00, sizeof(opregion->ctx));

Perhaps add a default tag and print some warning in case later BIOS
updates but our driver doesn't keep up.


> +	}
> +
> +	if (result < 0) {
> +		if (result == -EINVAL)
> +			return AE_BAD_PARAMETER;
> +		else
> +			return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +
>  int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
>  					struct regmap *regmap,
>  					struct intel_pmic_opregion_data *d)
> @@ -227,21 +275,31 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
>  	opregion->lpat_table = acpi_lpat_get_conversion_table(handle);
>  
>  	status = acpi_install_address_space_handler(handle,
> -						    PMIC_POWER_OPREGION_ID,
> -						    intel_pmic_power_handler,
> -						    NULL, opregion);
> +				PMIC_POWER_OPREGION_ID,
> +				intel_pmic_power_handler,
> +					NULL, opregion);

Better not touch these lines if there is no code change. If you want to
change the coding style, do it in another patch. But please proper align
them.

> +	if (ACPI_FAILURE(status)) {
> +		ret = -ENODEV;
> +		goto out_error;
> +	}
> +
> +	status = acpi_install_address_space_handler(handle,
> +				PMIC_THERMAL_OPREGION_ID,
> +				intel_pmic_thermal_handler,
> +					NULL, opregion);
>  	if (ACPI_FAILURE(status)) {
> +		acpi_remove_address_space_handler(handle,
> +				PMIC_POWER_OPREGION_ID,
> +				intel_pmic_power_handler);
>  		ret = -ENODEV;
>  		goto out_error;
>  	}
>  
>  	status = acpi_install_address_space_handler(handle,
> -						    PMIC_THERMAL_OPREGION_ID,
> -						    intel_pmic_thermal_handler,
> -						    NULL, opregion);
> +					PMIC_REGS_OPREGION_ID,
> +				intel_pmic_regs_handler, NULL,
> +			opregion);
>  	if (ACPI_FAILURE(status)) {
> -		acpi_remove_address_space_handler(handle, PMIC_POWER_OPREGION_ID,
> -						  intel_pmic_power_handler);

Shouldn't the power&thermal operation region handlers be removed here?

Thanks,
Aaron

>  		ret = -ENODEV;
>  		goto out_error;
>  	}
> diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
> index 2f39ee0..bdb7dcc 100644
> --- a/drivers/acpi/pmic/intel_pmic.h
> +++ b/drivers/acpi/pmic/intel_pmic.h
> @@ -7,6 +7,11 @@ struct pmic_table {
>  	int bit;	/* control bit for power */
>  };
>  
> +struct pmic_regs_handler {
> +	u16 address;		/* pmic regs address */
> +	unsigned int value;	/* value to write @regs address */
> +};
> +
>  struct intel_pmic_opregion_data {
>  	int (*get_power)(struct regmap *r, int reg, int bit, u64 *value);
>  	int (*update_power)(struct regmap *r, int reg, int bit, bool on);
> -- 
> 1.9.1
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web