RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
From: Mani, Rajmohan
Date: Thu Jul 20 2017 - 21:00:33 EST
Hi Lee,
Thanks for the reviews.
> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@xxxxxxxxxx]
> Sent: Thursday, July 20, 2017 2:03 AM
> To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; Linus Walleij <linus.walleij@xxxxxxxxxx>; Alexandre
> Courbot <gnurou@xxxxxxxxx>; Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>; Len
> Brown <lenb@xxxxxxxxxx>; sakari.ailus@xxxxxxxxxxxxxxx; Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx>
> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
>
> On Wed, 19 Jul 2017, Rajmohan Mani wrote:
>
> > The 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 patch adds support for TPS68470 mfd device.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> > ---
> > drivers/mfd/Kconfig | 18 +++++++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/tps68470.c | 110
> +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/tps68470.h | 97
> > ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 226 insertions(+)
> > create mode 100644 drivers/mfd/tps68470.c create mode 100644
> > include/linux/mfd/tps68470.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3eb5c93..960be43 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> > This driver can also be built as a module. If so, the module
> > will be called tps65217.
> >
> > +config MFD_TPS68470
> > + bool "TI TPS68470 Power Management / LED chips"
> > + depends on ACPI && I2C=y
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + select I2C_DESIGNWARE_PLATFORM
> > + help
> > + If you say yes here you get support for the TPS68470 series of
> > + Power Management / LED chips.
> > +
> > + These include voltage regulators, led and other features
>
> LED(s)
>
Ack
> > + that are often used in portable devices.
> > +
> > + This option is a bool as it provides an ACPI operation
> > + region, which must be available before any of the devices
> > + using this, are probed. This option also configures the
>
> Remove the ','.
>
Ack
> > + designware-i2c driver to be builtin, for the same reason.
>
> built-in
>
Ack
> > +
> > config MFD_TI_LP873X
> > tristate "TI LP873X Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c16bf1e..6dd2b94 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
> > obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> > obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> > obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
> > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> > obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> > obj-$(CONFIG_MENELAUS) += menelaus.o
> >
> > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file
> > mode 100644 index 0000000..a692af7
> > --- /dev/null
> > +++ b/drivers/mfd/tps68470.c
> > @@ -0,0 +1,110 @@
> > +/*
> > + * TPS68470 chip family multi-function driver
>
> Does it really cover a family? If so 'TPS68470' seems very specific.
>
> "Multi-Function Driver" or even better "Core" or "Parent" driver.
>
No. This is just for TPS68470.
I picked "Parent" driver
> > + * Copyright (C) 2017 Intel Corporation
>
> '\n' here.
>
Ack
> > + * Authors:
> > + * Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> > + * Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
> > + * Jian Xu Zheng <jian.xu.zheng@xxxxxxxxx>
> > + * Yuning Pu <yuning.pu@xxxxxxxxx>
>
> Tab these out:
>
> * Authors:
> * Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> * Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
> * Jian Xu Zheng <jian.xu.zheng@xxxxxxxxx>
> * Yuning Pu <yuning.pu@xxxxxxxxx>
>
Ack
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied
> > + warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> Can you use the short version?
>
Ack
I will update the other patches of this series too.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/regmap.h>
> > +
> > +static const struct mfd_cell tps68470s[] = {
> > + {
> > + .name = "tps68470-gpio",
> > + },
> > + {
> > + .name = "tps68470_pmic_opregion",
> > + },
> > +};
>
> Make these one line each:
>
> { .name = "tps68470-gpio" }
>
> ... and drop the ','
>
Ack
> > +static const struct regmap_config tps68470_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = TPS68470_REG_MAX,
> > +};
> > +
> > +static int tps68470_chip_init(struct device *dev, struct regmap
> > +*regmap) {
> > + unsigned int version;
> > + int ret;
> > +
> > + /* Force software reset */
> > + ret = regmap_write(regmap, TPS68470_REG_RESET,
> TPS68470_REG_RESET_MASK);
> > + if (ret < 0)
>
> Will 'if (!ret)' do?
>
We intend to check error conditions and bail out. So, if (ret < 0) works for this.
> > + return ret;
> > +
> > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> > + if (ret < 0) {
>
> Same
>
We intend to check error conditions and bail out. So, if (ret < 0) works for this.
> > + dev_err(dev, "Failed to read revision register: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > + return 0;
> > +}
> > +
> > +static int tps68470_probe(struct i2c_client *client) {
> > + struct device *dev = &client->dev;
> > + struct regmap *regmap;
> > + int ret;
> > +
> > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + i2c_set_clientdata(client, regmap);
> > +
> > + ret = tps68470_chip_init(dev, regmap);
> > + if (ret < 0) {
>
> Same
>
We intend to check error conditions and bail out. So, if (ret < 0) works for this.
> > + dev_err(dev, "TPS68470 Init Error %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> > + if (ret < 0) {
> > + dev_err(dev, "devm_mfd_add_devices failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct acpi_device_id tps68470_acpi_ids[] = {
> > + {"INT3472"},
> > + {},
> > +};
> > +
>
> Remove this line.
>
Ack
> > +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
> > +
> > +static struct i2c_driver tps68470_driver = {
> > + .driver = {
> > + .name = "tps68470",
> > + .acpi_match_table = tps68470_acpi_ids,
> > + },
> > + .probe_new = tps68470_probe,
> > +};
> > +builtin_i2c_driver(tps68470_driver);
> > diff --git a/include/linux/mfd/tps68470.h
> > b/include/linux/mfd/tps68470.h new file mode 100644 index
> > 0000000..44f9d9f
> > --- /dev/null
> > +++ b/include/linux/mfd/tps68470.h
> > @@ -0,0 +1,97 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation
> > + *
> > + * Functions to access TPS68470 power management chip.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied
> > +warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> Short version?
>
Ack
> > + */
> > +
> > +#ifndef __LINUX_MFD_TPS68470_H
> > +#define __LINUX_MFD_TPS68470_H
> > +
> > +/* Register addresses */
> > +#define TPS68470_REG_POSTDIV2 0x06
> > +#define TPS68470_REG_BOOSTDIV 0x07
> > +#define TPS68470_REG_BUCKDIV 0x08
> > +#define TPS68470_REG_PLLSWR 0x09
> > +#define TPS68470_REG_XTALDIV 0x0A
> > +#define TPS68470_REG_PLLDIV 0x0B
> > +#define TPS68470_REG_POSTDIV 0x0C
> > +#define TPS68470_REG_PLLCTL 0x0D
> > +#define TPS68470_REG_PLLCTL2 0x0E
> > +#define TPS68470_REG_CLKCFG1 0x0F
> > +#define TPS68470_REG_CLKCFG2 0x10
> > +#define TPS68470_REG_GPCTL0A 0x14
> > +#define TPS68470_REG_GPCTL0B 0x15
> > +#define TPS68470_REG_GPCTL1A 0x16
> > +#define TPS68470_REG_GPCTL1B 0x17
> > +#define TPS68470_REG_GPCTL2A 0x18
> > +#define TPS68470_REG_GPCTL2B 0x19
> > +#define TPS68470_REG_GPCTL3A 0x1A
> > +#define TPS68470_REG_GPCTL3B 0x1B
> > +#define TPS68470_REG_GPCTL4A 0x1C
> > +#define TPS68470_REG_GPCTL4B 0x1D
> > +#define TPS68470_REG_GPCTL5A 0x1E
> > +#define TPS68470_REG_GPCTL5B 0x1F
> > +#define TPS68470_REG_GPCTL6A 0x20
> > +#define TPS68470_REG_GPCTL6B 0x21
> > +#define TPS68470_REG_SGPO 0x22
> > +#define TPS68470_REG_GPDI 0x26
> > +#define TPS68470_REG_GPDO 0x27
> > +#define TPS68470_REG_VCMVAL 0x3C
> > +#define TPS68470_REG_VAUX1VAL 0x3D
> > +#define TPS68470_REG_VAUX2VAL 0x3E
> > +#define TPS68470_REG_VIOVAL 0x3F
> > +#define TPS68470_REG_VSIOVAL 0x40
> > +#define TPS68470_REG_VAVAL 0x41
> > +#define TPS68470_REG_VDVAL 0x42
> > +#define TPS68470_REG_S_I2C_CTL 0x43
> > +#define TPS68470_REG_VCMCTL 0x44
> > +#define TPS68470_REG_VAUX1CTL 0x45
> > +#define TPS68470_REG_VAUX2CTL 0x46
> > +#define TPS68470_REG_VACTL 0x47
> > +#define TPS68470_REG_VDCTL 0x48
> > +#define TPS68470_REG_RESET 0x50
> > +#define TPS68470_REG_REVID 0xFF
> > +
> > +#define TPS68470_REG_MAX TPS68470_REG_REVID
> > +
> > +/* Register field definitions */
> > +
> > +#define TPS68470_REG_RESET_MASK GENMASK(7, 0)
> > +#define TPS68470_VAVAL_AVOLT_MASK GENMASK(6, 0)
> > +
> > +#define TPS68470_VDVAL_DVOLT_MASK GENMASK(5, 0)
> > +#define TPS68470_VCMVAL_VCVOLT_MASK GENMASK(6, 0)
> > +#define TPS68470_VIOVAL_IOVOLT_MASK GENMASK(6, 0)
> > +#define TPS68470_VSIOVAL_IOVOLT_MASK GENMASK(6, 0)
> > +#define TPS68470_VAUX1VAL_AUX1VOLT_MASK GENMASK(6, 0)
> > +#define TPS68470_VAUX2VAL_AUX2VOLT_MASK GENMASK(6, 0)
> > +
> > +#define TPS68470_VACTL_EN_MASK GENMASK(0, 0)
> > +#define TPS68470_VDCTL_EN_MASK GENMASK(0, 0)
> > +#define TPS68470_VCMCTL_EN_MASK GENMASK(0, 0)
> > +#define TPS68470_S_I2C_CTL_EN_MASK GENMASK(1, 0)
> > +#define TPS68470_VAUX1CTL_EN_MASK GENMASK(0, 0)
> > +#define TPS68470_VAUX2CTL_EN_MASK GENMASK(0, 0)
> > +#define TPS68470_PLL_EN_MASK GENMASK(0, 0)
> > +
> > +#define TPS68470_CLKCFG1_MODE_A_MASK GENMASK(1, 0)
> > +#define TPS68470_CLKCFG1_MODE_B_MASK GENMASK(3, 2)
> > +
> > +#define TPS68470_GPIO_CTL_REG_A(x) (TPS68470_REG_GPCTL0A +
> (x) * 2)
> > +#define TPS68470_GPIO_CTL_REG_B(x) (TPS68470_REG_GPCTL0B +
> (x) * 2)
> > +#define TPS68470_GPIO_MODE_MASK GENMASK(1, 0)
> > +#define TPS68470_GPIO_MODE_IN 0
> > +#define TPS68470_GPIO_MODE_IN_PULLUP 1
> > +#define TPS68470_GPIO_MODE_OUT_CMOS 2
> > +#define TPS68470_GPIO_MODE_OUT_ODRAIN 3
> > +
> > +#endif /* __LINUX_MFD_TPS68470_H */
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org â Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog