Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751651AbdFGMIP (ORCPT ); Wed, 7 Jun 2017 08:08:15 -0400 Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:46010 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751413AbdFGMIL (ORCPT ); Wed, 7 Jun 2017 08:08:11 -0400 Date: Wed, 7 Jun 2017 15:07:33 +0300 From: Sakari Ailus To: Rajmohan Mani Cc: linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-acpi@vger.kernel.org, Lee Jones , Linus Walleij , Alexandre Courbot , "Rafael J. Wysocki" , Len Brown Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Message-ID: <20170607120730.GZ1019@valkosipuli.retiisi.org.uk> References: <1496750118-5570-1-git-send-email-rajmohan.mani@intel.com> <1496750118-5570-4-git-send-email-rajmohan.mani@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1496750118-5570-4-git-send-email-rajmohan.mani@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15142 Lines: 554 Hi Rajmohan, Thanks for removing the redundant struct definition. A couple more comments below. Not really necessarily bugs but a few things to clean things up a bit. On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote: > The Kabylake platform coreboot (Chrome OS equivalent of > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC. > These operation regions are to enable/disable voltage > regulators, configure voltage regulators, enable/disable > clocks and to configure clocks. > > This config adds ACPI operation region support for TI TPS68470 PMIC. > TPS68470 device is an advanced power management unit that powers > a Compact Camera Module (CCM), generates clocks for image sensors, > drives a dual LED for flash and incorporates two LED drivers for > general purpose indicators. > This driver enables ACPI operation region support to control voltage > regulators and clocks for the TPS68470 PMIC. > > Signed-off-by: Rajmohan Mani > --- > drivers/acpi/Kconfig | 12 + > drivers/acpi/Makefile | 2 + > drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 468 insertions(+) > create mode 100644 drivers/acpi/pmic/pmic_tps68470.c > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 1ce52f8..218d22d 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -535,4 +535,16 @@ if ARM64 > source "drivers/acpi/arm64/Kconfig" > endif > > +config TPS68470_PMIC_OPREGION > + bool "ACPI operation region support for TPS68470 PMIC" > + help > + This config adds ACPI operation region support for TI TPS68470 PMIC. > + TPS68470 device is an advanced power management unit that powers > + a Compact Camera Module (CCM), generates clocks for image sensors, > + drives a dual LED for flash and incorporates two LED drivers for > + general purpose indicators. > + This driver enables ACPI operation region support control voltage > + regulators and clocks. > + Extra newline. > + > endif # ACPI > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index b1aacfc..7113d05 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o > > obj-$(CONFIG_ACPI_CONFIGFS) += acpi_configfs.o > > +obj-$(CONFIG_TPS68470_PMIC_OPREGION) += pmic/pmic_tps68470.o > + > video-objs += acpi_video.o video_detect.o > obj-y += dptf/ > > diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c > new file mode 100644 > index 0000000..b2d608b > --- /dev/null > +++ b/drivers/acpi/pmic/pmic_tps68470.c > @@ -0,0 +1,454 @@ > +/* > + * TI TPS68470 PMIC operation region driver > + * > + * Copyright (C) 2017 Intel Corporation. All rights reserved. > + * Author: Rajmohan Mani > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Based on drivers/acpi/pmic/intel_pmic* drivers > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +struct ti_pmic_table { > + u32 address; /* operation region address */ > + u32 reg; /* corresponding register */ > + u32 bitmask; /* bit mask for power, clock */ > +}; > + > +#define TI_PMIC_POWER_OPREGION_ID 0xB0 > +#define TI_PMIC_VR_VAL_OPREGION_ID 0xB1 > +#define TI_PMIC_CLOCK_OPREGION_ID 0xB2 > +#define TI_PMIC_CLKFREQ_OPREGION_ID 0xB3 > + > +struct ti_pmic_opregion { > + struct mutex lock; > + struct regmap *regmap; > +}; > + > +#define S_IO_I2C_EN (BIT(0) | BIT(1)) > + > +static const struct ti_pmic_table power_table[] = { > + { > + .address = 0x00, > + .reg = TPS68470_REG_S_I2C_CTL, > + .bitmask = S_IO_I2C_EN, > + /* S_I2C_CTL */ > + }, > + { > + .address = 0x04, > + .reg = TPS68470_REG_VCMCTL, > + .bitmask = BIT(0), > + /* VCMCTL */ > + }, > + { > + .address = 0x08, > + .reg = TPS68470_REG_VAUX1CTL, > + .bitmask = BIT(0), > + /* VAUX1_CTL */ > + }, > + { > + .address = 0x0C, > + .reg = TPS68470_REG_VAUX2CTL, > + .bitmask = BIT(0), > + /* VAUX2CTL */ > + }, > + { > + .address = 0x10, > + .reg = TPS68470_REG_VACTL, > + .bitmask = BIT(0), > + /* VACTL */ > + }, > + { > + .address = 0x14, > + .reg = TPS68470_REG_VDCTL, > + .bitmask = BIT(0), > + /* VDCTL */ > + }, > +}; > + > +/* Table to set voltage regulator value */ > +static const struct ti_pmic_table vr_val_table[] = { > + { > + .address = 0x00, > + .reg = TPS68470_REG_VSIOVAL, > + .bitmask = TPS68470_VSIOVAL_IOVOLT_MASK, > + /* TPS68470_REG_VSIOVAL */ > + }, > + { > + .address = 0x04, > + .reg = TPS68470_REG_VIOVAL, > + .bitmask = TPS68470_VIOVAL_IOVOLT_MASK, > + /* TPS68470_REG_VIOVAL */ > + }, > + { > + .address = 0x08, > + .reg = TPS68470_REG_VCMVAL, > + .bitmask = TPS68470_VCMVAL_VCVOLT_MASK, > + /* TPS68470_REG_VCMVAL */ > + }, > + { > + .address = 0x0C, > + .reg = TPS68470_REG_VAUX1VAL, > + .bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK, > + /* TPS68470_REG_VAUX1VAL */ > + }, > + { > + .address = 0x10, > + .reg = TPS68470_REG_VAUX2VAL, > + .bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK, > + /* TPS68470_REG_VAUX2VAL */ > + }, > + { > + .address = 0x14, > + .reg = TPS68470_REG_VAVAL, > + .bitmask = TPS68470_VAVAL_AVOLT_MASK, > + /* TPS68470_REG_VAVAL */ > + }, > + { > + .address = 0x18, > + .reg = TPS68470_REG_VDVAL, > + .bitmask = TPS68470_VDVAL_DVOLT_MASK, > + /* TPS68470_REG_VDVAL */ > + }, > +}; > + > +/* Table to configure clock frequency */ > +static const struct ti_pmic_table clk_freq_table[] = { > + { > + .address = 0x00, > + .reg = TPS68470_REG_POSTDIV2, > + .bitmask = BIT(0) | BIT(1), > + /* TPS68470_REG_POSTDIV2 */ > + }, > + { > + .address = 0x04, > + .reg = TPS68470_REG_BOOSTDIV, > + .bitmask = 0x1F, > + /* TPS68470_REG_BOOSTDIV */ > + }, > + { > + .address = 0x08, > + .reg = TPS68470_REG_BUCKDIV, > + .bitmask = 0x0F, > + /* TPS68470_REG_BUCKDIV */ > + }, > + { > + .address = 0x0C, > + .reg = TPS68470_REG_PLLSWR, > + .bitmask = 0x13, > + /* TPS68470_REG_PLLSWR */ > + }, > + { > + .address = 0x10, > + .reg = TPS68470_REG_XTALDIV, > + .bitmask = 0xFF, > + /* TPS68470_REG_XTALDIV */ > + }, > + { > + .address = 0x14, > + .reg = TPS68470_REG_PLLDIV, > + .bitmask = 0xFF, > + /* TPS68470_REG_PLLDIV */ > + }, > + { > + .address = 0x18, > + .reg = TPS68470_REG_POSTDIV, > + .bitmask = 0x83, > + /* TPS68470_REG_POSTDIV */ > + }, > +}; > + > +/* Table to configure and enable clocks */ > +static const struct ti_pmic_table clk_table[] = { > + { > + .address = 0x00, > + .reg = TPS68470_REG_PLLCTL, > + .bitmask = 0xF5, > + /* TPS68470_REG_PLLCTL */ > + }, > + { > + .address = 0x04, > + .reg = TPS68470_REG_PLLCTL2, > + .bitmask = BIT(0), > + /* TPS68470_REG_PLLCTL2 */ > + }, > + { > + .address = 0x08, > + .reg = TPS68470_REG_CLKCFG1, > + .bitmask = TPS68470_CLKCFG1_MODE_A_MASK | > + TPS68470_CLKCFG1_MODE_B_MASK, > + /* TPS68470_REG_CLKCFG1 */ > + }, > + { > + .address = 0x0C, > + .reg = TPS68470_REG_CLKCFG2, > + .bitmask = TPS68470_CLKCFG1_MODE_A_MASK | > + TPS68470_CLKCFG1_MODE_B_MASK, > + /* TPS68470_REG_CLKCFG2 */ > + }, > +}; > + > +static int pmic_get_reg_bit(u64 address, struct ti_pmic_table *table, > + int count, int *reg, int *bitmask) unsigned int count? > +{ > + u64 i; > + > + i = address / 4; > + > + if (i >= count) > + return -ENOENT; > + > + if (!reg || !bitmask) > + return -EINVAL; > + > + *reg = table[i].reg; > + *bitmask = table[i].bitmask; > + > + return 0; > +} > + > +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg, > + int bitmask, u64 *value) > +{ > + int data; Shouldn't you use unsigned int here? Same in the functions below. > + > + if (regmap_read(regmap, reg, &data)) > + return -EIO; > + > + *value = (data & bitmask) ? 1 : 0; > + return 0; > +} > + > +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg, > + int bitmask, u64 *value) > +{ > + int data; > + > + if (regmap_read(regmap, reg, &data)) > + return -EIO; > + > + *value = data & bitmask; > + return 0; > +} > + > +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg, > + int bitmask, u64 *value) > +{ > + int data; > + > + if (regmap_read(regmap, reg, &data)) > + return -EIO; > + > + *value = (data & bitmask) ? 1 : 0; > + return 0; > +} > + > +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg, > + int bitmask, u64 *value) > +{ > + int data; > + > + if (regmap_read(regmap, reg, &data)) > + return -EIO; > + > + *value = data & bitmask; > + return 0; > +} > + > +static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg, > + int bitmask, u64 value) > +{ > + return regmap_update_bits(regmap, reg, bitmask, value); > +} > + > +static acpi_status ti_pmic_common_handler(u32 function, > + acpi_physical_address address, > + u32 bits, u64 *value, > + void *handler_context, handler_context is unused. > + void *region_context, > + int (*get)(struct regmap *, > + int, int, u64 *), > + int (*update)(struct regmap *, > + int, int, u64), > + struct ti_pmic_table *table, > + int table_size) unsigned int. (Or size_t or whatever.) > +{ > + struct ti_pmic_opregion *opregion = region_context; > + struct regmap *regmap = opregion->regmap; > + int reg, ret, bitmask; > + > + if (bits != 32) > + return AE_BAD_PARAMETER; > + > + ret = pmic_get_reg_bit(address, table, > + table_size, ®, &bitmask); > + if (ret < 0) > + return AE_BAD_PARAMETER; > + > + if (function == ACPI_WRITE && (*value > bitmask)) Extra parentheses. > + return AE_BAD_PARAMETER; > + > + mutex_lock(&opregion->lock); > + > + ret = (function == ACPI_READ) ? > + get(regmap, reg, bitmask, value) : > + update(regmap, reg, bitmask, *value); > + > + mutex_unlock(&opregion->lock); > + > + return ret ? AE_ERROR : AE_OK; > +} > + > +static acpi_status ti_pmic_clk_freq_handler(u32 function, > + acpi_physical_address address, > + u32 bits, u64 *value, > + void *handler_context, > + void *region_context) > +{ > + return ti_pmic_common_handler(function, address, bits, value, > + handler_context, region_context, > + ti_tps68470_pmic_get_clk_freq, > + ti_tps68470_regmap_update_bits, > + (struct ti_pmic_table *) &clk_freq_table, You shouldn't use an explicit cast here. Instead make the function argument const as well and you're fine. > + ARRAY_SIZE(clk_freq_table)); > +} > + > +static acpi_status ti_pmic_clk_handler(u32 function, > + acpi_physical_address address, u32 bits, > + u64 *value, void *handler_context, > + void *region_context) > +{ > + return ti_pmic_common_handler(function, address, bits, value, > + handler_context, region_context, > + ti_tps68470_pmic_get_clk, > + ti_tps68470_regmap_update_bits, > + (struct ti_pmic_table *) &clk_table, > + ARRAY_SIZE(clk_table)); > +} > + > +static acpi_status ti_pmic_vr_val_handler(u32 function, > + acpi_physical_address address, > + u32 bits, u64 *value, > + void *handler_context, > + void *region_context) > +{ > + return ti_pmic_common_handler(function, address, bits, value, > + handler_context, region_context, > + ti_tps68470_pmic_get_vr_val, > + ti_tps68470_regmap_update_bits, > + (struct ti_pmic_table *) &vr_val_table, > + ARRAY_SIZE(vr_val_table)); > +} > + > +static acpi_status ti_pmic_power_handler(u32 function, > + acpi_physical_address address, > + u32 bits, u64 *value, > + void *handler_context, > + void *region_context) > +{ > + if (bits != 32) > + return AE_BAD_PARAMETER; > + > + /* set/clear for bit 0, bits 0 and 1 together */ > + if (function == ACPI_WRITE && > + !(*value == 0 || *value == 1 || *value == 3)) { > + return AE_BAD_PARAMETER; > + } > + > + return ti_pmic_common_handler(function, address, bits, value, > + handler_context, region_context, > + ti_tps68470_pmic_get_power, > + ti_tps68470_regmap_update_bits, > + (struct ti_pmic_table *) &power_table, > + ARRAY_SIZE(power_table)); > +} > + > +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev) > +{ > + struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent); > + acpi_handle handle = ACPI_HANDLE(pdev->dev.parent); > + struct device *dev = &pdev->dev; > + struct ti_pmic_opregion *opregion; > + acpi_status status; > + > + if (!dev || !pmic->regmap) { > + WARN(1, "dev or regmap is NULL\n"); > + return -EINVAL; > + } > + > + if (!handle) { > + WARN(1, "acpi handle is NULL\n"); > + return -ENODEV; > + } > + > + opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL); > + if (!opregion) > + return -ENOMEM; > + > + mutex_init(&opregion->lock); > + opregion->regmap = pmic->regmap; > + > + status = acpi_install_address_space_handler(handle, > + TI_PMIC_POWER_OPREGION_ID, > + ti_pmic_power_handler, > + NULL, opregion); > + if (ACPI_FAILURE(status)) mutex_destroy() after mutex_init() --- please add a label for this. > + return -ENODEV; > + > + status = acpi_install_address_space_handler(handle, > + TI_PMIC_VR_VAL_OPREGION_ID, > + ti_pmic_vr_val_handler, > + NULL, opregion); > + if (ACPI_FAILURE(status)) > + goto out_remove_power_handler; > + > + status = acpi_install_address_space_handler(handle, > + TI_PMIC_CLOCK_OPREGION_ID, > + ti_pmic_clk_handler, > + NULL, opregion); > + if (ACPI_FAILURE(status)) > + goto out_remove_vr_val_handler; > + > + status = acpi_install_address_space_handler(handle, > + TI_PMIC_CLKFREQ_OPREGION_ID, > + ti_pmic_clk_freq_handler, > + NULL, opregion); > + if (ACPI_FAILURE(status)) > + goto out_remove_clk_handler; > + > + return 0; > + > +out_remove_clk_handler: > + acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID, > + ti_pmic_clk_handler); > +out_remove_vr_val_handler: > + acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID, > + ti_pmic_vr_val_handler); > +out_remove_power_handler: > + acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID, > + ti_pmic_power_handler); > + return -ENODEV; > +} > + > +static struct platform_driver ti_tps68470_pmic_opregion_driver = { > + .probe = ti_tps68470_pmic_opregion_probe, > + .driver = { > + .name = "tps68470_pmic_opregion", > + }, > +}; > + > +builtin_platform_driver(ti_tps68470_pmic_opregion_driver) -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk