RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

["Mani, Rajmohan" <rajmohan.mani@xxxxxxxxx>]

Hi Sakari,

Thanks for the reviews.

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@xxxxxx]
> Sent: Wednesday, June 07, 2017 5:08 AM
> To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; Lee Jones <lee.jones@xxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; Alexandre Courbot <gnurou@xxxxxxxxx>; Rafael J.
> Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> 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.
> 

Ack

> 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 <rajmohan.mani@xxxxxxxxx>
> > ---
> >  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.
> 

Ack

> > +
> >  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 <rajmohan.mani@xxxxxxxxx>
> > + *
> > + * 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 <linux/acpi.h>
> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +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?
> 

Ack
I will also rename the variable to be table_size, so it's more meaningful.

> > +{
> > +	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.
> 

Ack

> > +
> > +	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.
> 

Ack

> > +					  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.)
> 

Ack

> > +{
> > +	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, &reg, &bitmask);
> > +	if (ret < 0)
> > +		return AE_BAD_PARAMETER;
> > +
> > +	if (function == ACPI_WRITE && (*value > bitmask))
> 
> Extra parentheses.
> 

Ack

> > +		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.
> 

Ack

> > +				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@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html