| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 23 Jun 2016 17:07:56 +0800
From: Aaron Lu <aaron.lu@...el.com>
To: Bin Gao <bin.gao@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Paul Gortmaker <paul.gortmaker@...driver.com>,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
ysiyer <yegnesh.s.iyer@...el.com>,
Ajay Thomas <ajay.thomas.david.rajamanickam@...el.com>,
Bin Gao <bin.gao@...el.com>
Subject: 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@...el.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@...ux.intel.com>
> Signed-off-by: Bin Gao <bin.gao@...el.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
>
Powered by blists - more mailing lists