by Hans de Goede

[permalink] [raw]

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

Hi,

On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
>
> Hans, if you have few minutes it would be appreciated to glance on the
> below for some issues if any since you did pass quite a good quest
> with other PMIC drivers.

I've gone over this driver, nothing stands out in a bad way to me,
IOW this seems like a normal PMIC OpRegion handler to me.

Regards,

Hans

>
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <[email protected]> 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 <[email protected]>
>> ---
>> drivers/acpi/Kconfig | 12 +
>> drivers/acpi/Makefile | 2 +
>
>> drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
>
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
>
>> 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 line, remove.
>
>> 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 <[email protected]>
>> + *
>> + * 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)
>> +{
>> + u64 i;
>> +
>
>> + i = address / 4;
>
>
>> +
>
> Remove this line.
>
>> + 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;
>> +
>> + 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,
>> + 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)
>> +{
>> + 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))
>> + 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,
>> + 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;
>> + }
>
> I dunno if WARNs make user experience any better.
> Besides that I would double check you may have such cases.
>
>> +
>> + 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))
>> + 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)
>> --
>> 1.9.1
>>
>

2017-06-07 02:00:46

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

Hi Rajmohan,

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.12-rc4 next-20170606]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Rajmohan-Mani/mfd-Add-new-mfd-device-TPS68470/20170607-080306
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

drivers/mfd/tps68470.c: In function 'tps68470_probe':
>> drivers/mfd/tps68470.c:175:3: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/ret +175 drivers/mfd/tps68470.c

159
160 static int tps68470_probe(struct i2c_client *client)
161 {
162 struct tps68470 *tps;
163 int ret;
164
165 tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
166 if (!tps)
167 return -ENOMEM;
168
169 mutex_init(&tps->lock);
170 i2c_set_clientdata(client, tps);
171 tps->dev = &client->dev;
172
173 tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
174 if (IS_ERR(tps->regmap)) {
> 175 dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
176 return PTR_ERR(tps->regmap);
177 }
178
179 ret = mfd_add_devices(tps->dev, -1, tps68470s,
180 ARRAY_SIZE(tps68470s), NULL, 0, NULL);
181 if (ret < 0) {
182 dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
183 return ret;

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.18 kB)
.config.gz (48.13 kB)
Download all attachments

2017-06-07 10:07:56

Hi Lee,

> > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> >
> > > Hi Andy,
> > >
> > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > > <[email protected]> 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.
> > > > >
> > > > > I dunno why you decide to send this out now, see my comments
> below.
> > > > >
> > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > + unsigned int version;
> > > > > > + int ret;
> > > > >
> > > > > > + /* FIXME: configure these dynamically */
> > > > >
> > > > > So, what prevents you to fix this?
> > > >
> > > > Nothing I suppose. They're however not needed right now and can be
> > > > implemented later on if they're ever needed.
> > > >
> > >
> > > Ack
> >
> > What does this mean? Is the plan to fix it or not? I don't want
> > FIXMEs in the code that a) can be fixed right away or b) might never be
> fixed.
> >
>
> I meant that this can be implemented later on, if there's a need.
> I will look into this and see how this can be fixed.
>

Thanks for your patience.
This is fixed with v4 of this series.

Raj

2017-07-20 08:15:42

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

On Wed, 19 Jul 2017, Mani, Rajmohan wrote:

> Hi Lee,
>
> > > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> > >
> > > > Hi Andy,
> > > >
> > > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > > > <[email protected]> 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.
> > > > > >
> > > > > > I dunno why you decide to send this out now, see my comments
> > below.
> > > > > >
> > > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > > + unsigned int version;
> > > > > > > + int ret;
> > > > > >
> > > > > > > + /* FIXME: configure these dynamically */
> > > > > >
> > > > > > So, what prevents you to fix this?
> > > > >
> > > > > Nothing I suppose. They're however not needed right now and can be
> > > > > implemented later on if they're ever needed.
> > > > >
> > > >
> > > > Ack
> > >
> > > What does this mean? Is the plan to fix it or not? I don't want
> > > FIXMEs in the code that a) can be fixed right away or b) might never be
> > fixed.
> > >
> >
> > I meant that this can be implemented later on, if there's a need.
> > I will look into this and see how this can be fixed.
> >
>
> Thanks for your patience.
> This is fixed with v4 of this series.

Great, thanks.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog