| Message ID | 1496750118-5570-2-git-send-email-rajmohan.mani@intel.com |
|---|---|
| State | New |
| Headers | show |
Hi Rajmohan, On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote: > +/* > + * tps68470_reg_read: Read a single tps68470 register. > + * > + * @tps: Device to read from. > + * @reg: Register to read. > + * @val: Contains the value > + */ > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg, > + unsigned int *val) > +{ > + int ret; > + > + mutex_lock(&tps->lock); > + ret = regmap_read(tps->regmap, reg, val); > + mutex_unlock(&tps->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(tps68470_reg_read); > + > +/* > + * tps68470_reg_write: Write a single tps68470 register. > + * > + * @tps68470: Device to write to. > + * @reg: Register to write to. > + * @val: Value to write. > + */ > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg, > + unsigned int val) > +{ > + int ret; > + > + mutex_lock(&tps->lock); > + ret = regmap_write(tps->regmap, reg, val); > + mutex_unlock(&tps->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(tps68470_reg_write); > + > +/* > + * tps68470_update_bits: Modify bits w.r.t mask and val. > + * > + * @tps68470: Device to write to. > + * @reg: Register to read-write to. > + * @mask: Mask. > + * @val: Value to write. > + */ > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg, > + unsigned int mask, unsigned int val) > +{ > + int ret; > + > + mutex_lock(&tps->lock); > + ret = regmap_update_bits(tps->regmap, reg, mask, val); > + mutex_unlock(&tps->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(tps68470_update_bits); I'm not sure you need those above wrappers at all, regmap is handling locking in any case. > +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 tps68470 *tps) > +{ > + unsigned int version; > + int ret; > + > + ret = tps68470_reg_read(tps, TPS68470_REG_REVID, &version); > + if (ret < 0) { > + dev_err(tps->dev, > + "Failed to read revision register: %d\n", ret); > + return ret; > + } > + > + dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version); > + > + ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff); > + if (ret < 0) > + return ret; > + > + /* FIXME: configure these dynamically */ > + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */ > + ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2); > + if (ret < 0) > + return ret; > + > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2); > + if (ret < 0) > + return ret; > + > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2); > + if (ret < 0) > + return ret; > + > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2); > + if (ret < 0) > + return ret; > + > + /* > + * When SDA and SCL are routed to GPIO1 and GPIO2, the mode > + * for these GPIOs must be configured using their respective > + * GPCTLxA registers as inputs with no pull-ups. > + */ > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0); > + if (ret < 0) > + return ret; > + > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0); > + if (ret < 0) > + return ret; > + > + /* Enable daisy chain */ > + ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1); > + if (ret < 0) > + return ret; > + > + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US, > + TPS68470_DAISY_CHAIN_DELAY_US + 10); > + return 0; > +} > + > +static int tps68470_probe(struct i2c_client *client) > +{ > + struct tps68470 *tps; > + int ret; > + > + tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL); > + if (!tps) > + return -ENOMEM; > + > + mutex_init(&tps->lock); > + i2c_set_clientdata(client, tps); > + tps->dev = &client->dev; > + > + tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > + if (IS_ERR(tps->regmap)) { > + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret); > + return PTR_ERR(tps->regmap); > + } > + > + ret = mfd_add_devices(tps->dev, -1, tps68470s, > + ARRAY_SIZE(tps68470s), NULL, 0, NULL); > + if (ret < 0) { > + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret); > + return ret; > + } devm_mfd_add_devices()? > + ret = tps68470_chip_init(tps); > + if (ret < 0) { > + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret); > + goto fail; > + } > + > + return 0; > +fail: > + mutex_lock(&tps->lock); Why do you need to lock here? > + mfd_remove_devices(tps->dev); > + mutex_unlock(&tps->lock); > + > + return ret; > +} > + > +static int tps68470_remove(struct i2c_client *client) > +{ > + struct tps68470 *tps = i2c_get_clientdata(client); > + > + mutex_lock(&tps->lock); > + mfd_remove_devices(tps->dev); > + mutex_unlock(&tps->lock); > + > + return 0; > +} > + > +static const struct acpi_device_id tps68470_acpi_ids[] = { > + {"INT3472"}, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids); > + > +static struct i2c_driver tps68470_driver = { > + .driver = { > + .name = "tps68470", > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids), > + }, > + .probe_new = tps68470_probe, > + .remove = tps68470_remove, > +}; <snip> > +/** > + * struct tps68470 - tps68470 sub-driver chip access routines > + * > + * Device data may be used to access the TPS68470 chip > + */ > + > +struct tps68470 { > + struct device *dev; > + struct regmap *regmap; > + /* > + * Used to synchronize access to tps68470_ operations > + * and addition and removal of mfd devices > + */ > + struct mutex lock; Is this lock really necessary at all? Actually, you probable don't even need this structure at all if you just rely on regmap functions in the drivers. Thanks,
On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> 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? > + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */ > +} > +static int tps68470_probe(struct i2c_client *client) > +{ > + struct tps68470 *tps; > + int ret; > + > + tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL); > + if (!tps) > + return -ENOMEM; > + > + mutex_init(&tps->lock); > + i2c_set_clientdata(client, tps); > + tps->dev = &client->dev; > + > + tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > + if (IS_ERR(tps->regmap)) { > + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret); > + return PTR_ERR(tps->regmap); > + } > + > + ret = mfd_add_devices(tps->dev, -1, tps68470s, > + ARRAY_SIZE(tps68470s), NULL, 0, NULL); devm_? > + if (ret < 0) { > + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret); > + return ret; > + } > + > + ret = tps68470_chip_init(tps); > + if (ret < 0) { > + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret); > + goto fail; > + } > + > + return 0; > +fail: > + mutex_lock(&tps->lock); I'm not sure you need this mutex to be held here. Otherwise your code has a bug with locking. > + mfd_remove_devices(tps->dev); > + mutex_unlock(&tps->lock); > + > + return ret; Taking above into consideration I suggest to clarify your locking scheme. > +} > + > +static int tps68470_remove(struct i2c_client *client) > +{ > + struct tps68470 *tps = i2c_get_clientdata(client); > + > + mutex_lock(&tps->lock); > + mfd_remove_devices(tps->dev); > + mutex_unlock(&tps->lock); Ditto. > + return 0; > +} > +/** > + * struct tps68470 - tps68470 sub-driver chip access routines > + * kbuild bot will be unhappy. You need to file a description per field. > + * Device data may be used to access the TPS68470 chip > + */ > + > +struct tps68470 { > + struct device *dev; > + struct regmap *regmap; > + /* > + * Used to synchronize access to tps68470_ operations > + * and addition and removal of mfd devices > + */ Move it to kernel-doc above. > + struct mutex lock; > +};
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
Hi Rajmohan,
[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.12-rc4 next-20170607]
[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: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
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=alpha
All error/warnings (new ones prefixed by >>):
>> drivers/mfd/tps68470.c:217:1: warning: data definition has no type or storage class
MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/tps68470.c:217:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/mfd/tps68470.c:217:1: warning: parameter names (without types) in function declaration
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);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +217 drivers/mfd/tps68470.c
211
212 static const struct acpi_device_id tps68470_acpi_ids[] = {
213 {"INT3472"},
214 {},
215 };
216
> 217 MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
218
219 static struct i2c_driver tps68470_driver = {
220 .driver = {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <rajmohan.mani@intel.com> 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.
Hi Heikki, Thanks for the reviews and patience. > -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] > Sent: Tuesday, June 06, 2017 5:49 AM > To: Mani, Rajmohan <rajmohan.mani@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij > <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J. > Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 > > Hi Rajmohan, > > On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote: > > +/* > > + * tps68470_reg_read: Read a single tps68470 register. > > + * > > + * @tps: Device to read from. > > + * @reg: Register to read. > > + * @val: Contains the value > > + */ > > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg, > > + unsigned int *val) > > +{ > > + int ret; > > + > > + mutex_lock(&tps->lock); > > + ret = regmap_read(tps->regmap, reg, val); > > + mutex_unlock(&tps->lock); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(tps68470_reg_read); > > + > > +/* > > + * tps68470_reg_write: Write a single tps68470 register. > > + * > > + * @tps68470: Device to write to. > > + * @reg: Register to write to. > > + * @val: Value to write. > > + */ > > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg, > > + unsigned int val) > > +{ > > + int ret; > > + > > + mutex_lock(&tps->lock); > > + ret = regmap_write(tps->regmap, reg, val); > > + mutex_unlock(&tps->lock); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(tps68470_reg_write); > > + > > +/* > > + * tps68470_update_bits: Modify bits w.r.t mask and val. > > + * > > + * @tps68470: Device to write to. > > + * @reg: Register to read-write to. > > + * @mask: Mask. > > + * @val: Value to write. > > + */ > > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg, > > + unsigned int mask, unsigned int val) { > > + int ret; > > + > > + mutex_lock(&tps->lock); > > + ret = regmap_update_bits(tps->regmap, reg, mask, val); > > + mutex_unlock(&tps->lock); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(tps68470_update_bits); > > I'm not sure you need those above wrappers at all, regmap is handling locking in > any case. > I had this following question from Alan Cox on the original code without these wrappers. "What is the model for insuring that no interrupt or thread of a driver is not in parallel issuing a tps68470_ operation when the device goes away (eg if I down the i2c controller) ?" To address the above concerns, I got extra cautious and implemented locks around the regmap_* calls. Now, I have been asked from more than one reviewer about the necessity of the same. With the use of devm_* calls, tps68470_remove() goes away and leaves the driver just with regmap_* calls. Unless I hear from Alan or other reviewers otherwise, I will drop these wrappers around regmap_* calls. > > +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 tps68470 *tps) { > > + unsigned int version; > > + int ret; > > + > > + ret = tps68470_reg_read(tps, TPS68470_REG_REVID, &version); > > + if (ret < 0) { > > + dev_err(tps->dev, > > + "Failed to read revision register: %d\n", ret); > > + return ret; > > + } > > + > > + dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version); > > + > > + ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff); > > + if (ret < 0) > > + return ret; > > + > > + /* FIXME: configure these dynamically */ > > + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */ > > + ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2); > > + if (ret < 0) > > + return ret; > > + > > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2); > > + if (ret < 0) > > + return ret; > > + > > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2); > > + if (ret < 0) > > + return ret; > > + > > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * When SDA and SCL are routed to GPIO1 and GPIO2, the mode > > + * for these GPIOs must be configured using their respective > > + * GPCTLxA registers as inputs with no pull-ups. > > + */ > > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0); > > + if (ret < 0) > > + return ret; > > + > > + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0); > > + if (ret < 0) > > + return ret; > > + > > + /* Enable daisy chain */ > > + ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1); > > + if (ret < 0) > > + return ret; > > + > > + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US, > > + TPS68470_DAISY_CHAIN_DELAY_US + 10); > > + return 0; > > +} > > + > > +static int tps68470_probe(struct i2c_client *client) { > > + struct tps68470 *tps; > > + int ret; > > + > > + tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL); > > + if (!tps) > > + return -ENOMEM; > > + > > + mutex_init(&tps->lock); > > + i2c_set_clientdata(client, tps); > > + tps->dev = &client->dev; > > + > > + tps->regmap = devm_regmap_init_i2c(client, > &tps68470_regmap_config); > > + if (IS_ERR(tps->regmap)) { > > + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret); > > + return PTR_ERR(tps->regmap); > > + } > > + > > + ret = mfd_add_devices(tps->dev, -1, tps68470s, > > + ARRAY_SIZE(tps68470s), NULL, 0, NULL); > > + if (ret < 0) { > > + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret); > > + return ret; > > + } > > devm_mfd_add_devices()? > Ack > > + ret = tps68470_chip_init(tps); > > + if (ret < 0) { > > + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret); > > + goto fail; > > + } > > + > > + return 0; > > +fail: > > + mutex_lock(&tps->lock); > > Why do you need to lock here? > Same as explained above (to address Alan's comments) > > + mfd_remove_devices(tps->dev); > > + mutex_unlock(&tps->lock); > > + > > + return ret; > > +} > > + > > +static int tps68470_remove(struct i2c_client *client) { > > + struct tps68470 *tps = i2c_get_clientdata(client); > > + > > + mutex_lock(&tps->lock); > > + mfd_remove_devices(tps->dev); > > + mutex_unlock(&tps->lock); > > + > > + return 0; > > +} > > + > > +static const struct acpi_device_id tps68470_acpi_ids[] = { > > + {"INT3472"}, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids); > > + > > +static struct i2c_driver tps68470_driver = { > > + .driver = { > > + .name = "tps68470", > > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids), > > + }, > > + .probe_new = tps68470_probe, > > + .remove = tps68470_remove, > > +}; > > <snip> > > > +/** > > + * struct tps68470 - tps68470 sub-driver chip access routines > > + * > > + * Device data may be used to access the TPS68470 chip */ > > + > > +struct tps68470 { > > + struct device *dev; > > + struct regmap *regmap; > > + /* > > + * Used to synchronize access to tps68470_ operations > > + * and addition and removal of mfd devices > > + */ > > + struct mutex lock; > > Is this lock really necessary at all? Actually, you probable don't even need this > structure at all if you just rely on regmap functions in the drivers. > Ack I am looking into this and will get back with v2. > > Thanks, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGkgQW5keSwNCg0KVGhhbmtzIGZvciB0aGUgcmV2aWV3cyBhbmQgcGF0aWVuY2UuDQoNCj4gLS0t LS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQW5keSBTaGV2Y2hlbmtvIFttYWlsdG86 YW5keS5zaGV2Y2hlbmtvQGdtYWlsLmNvbV0NCj4gU2VudDogVHVlc2RheSwgSnVuZSAwNiwgMjAx NyA2OjAwIEFNDQo+IFRvOiBNYW5pLCBSYWptb2hhbiA8cmFqbW9oYW4ubWFuaUBpbnRlbC5jb20+ DQo+IENjOiBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eC1ncGlvQHZnZXIua2Vy bmVsLm9yZzsgbGludXgtDQo+IGFjcGlAdmdlci5rZXJuZWwub3JnOyBMZWUgSm9uZXMgPGxlZS5q b25lc0BsaW5hcm8ub3JnPjsgTGludXMgV2FsbGVpag0KPiA8bGludXMud2FsbGVpakBsaW5hcm8u b3JnPjsgQWxleGFuZHJlIENvdXJib3QgPGdudXJvdUBnbWFpbC5jb20+OyBSYWZhZWwgSi4NCj4g V3lzb2NraSA8cmp3QHJqd3lzb2NraS5uZXQ+OyBMZW4gQnJvd24gPGxlbmJAa2VybmVsLm9yZz4N Cj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MSAxLzNdIG1mZDogQWRkIG5ldyBtZmQgZGV2aWNlIFRQ UzY4NDcwDQo+IA0KPiBPbiBUdWUsIEp1biA2LCAyMDE3IGF0IDI6NTUgUE0sIFJham1vaGFuIE1h bmkgPHJham1vaGFuLm1hbmlAaW50ZWwuY29tPg0KPiB3cm90ZToNCj4gPiBUaGUgVFBTNjg0NzAg ZGV2aWNlIGlzIGFuIGFkdmFuY2VkIHBvd2VyIG1hbmFnZW1lbnQgdW5pdCB0aGF0IHBvd2VycyBh DQo+ID4gQ29tcGFjdCBDYW1lcmEgTW9kdWxlIChDQ00pLCBnZW5lcmF0ZXMgY2xvY2tzIGZvciBp bWFnZSBzZW5zb3JzLA0KPiA+IGRyaXZlcyBhIGR1YWwgTEVEIGZvciBGbGFzaCBhbmQgaW5jb3Jw b3JhdGVzIHR3byBMRUQgZHJpdmVycyBmb3INCj4gPiBnZW5lcmFsIHB1cnBvc2UgaW5kaWNhdG9y cy4NCj4gPg0KPiA+IFRoaXMgcGF0Y2ggYWRkcyBzdXBwb3J0IGZvciBUUFM2ODQ3MCBtZmQgZGV2 aWNlLg0KPiANCj4gSSBkdW5ubyB3aHkgeW91IGRlY2lkZSB0byBzZW5kIHRoaXMgb3V0IG5vdywg c2VlIG15IGNvbW1lbnRzIGJlbG93Lg0KPiANCg0KV2UgZGVjaWRlZCB0byBnbyB3aXRoIHRoZSBz dWJtaXNzaW9uIG9mIHRoZXNlIGRyaXZlcnMgZm9yIHVwc3RyZWFtIHJldmlldyBzb29uZXIgcmF0 aGVyIHRoYW4gbGF0ZXIuDQoNCj4gPiArc3RhdGljIGludCB0cHM2ODQ3MF9jaGlwX2luaXQoc3Ry dWN0IHRwczY4NDcwICp0cHMpIHsNCj4gPiArICAgICAgIHVuc2lnbmVkIGludCB2ZXJzaW9uOw0K PiA+ICsgICAgICAgaW50IHJldDsNCj4gDQo+ID4gKyAgICAgICAvKiBGSVhNRTogY29uZmlndXJl IHRoZXNlIGR5bmFtaWNhbGx5ICovDQo+IA0KPiBTbywgd2hhdCBwcmV2ZW50cyB5b3UgdG8gZml4 IHRoaXM/DQo+IA0KDQpJIHdpbGwgcmVzcG9uZCBvbiB0b3Agb2YgU2FrYXJpJ3MgcmVzcG9uc2Uu DQoNCj4gPiArICAgICAgIC8qIEVuYWJsZSBEYWlzeSBDaGFpbiBMRE8gYW5kIGNvbmZpZ3VyZSBy ZWxldmFudCBHUElPcyBhcw0KPiA+ICsgb3V0cHV0ICovDQo+IA0KPiA+ICt9DQo+IA0KPiA+ICtz dGF0aWMgaW50IHRwczY4NDcwX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpIHsNCj4g PiArICAgICAgIHN0cnVjdCB0cHM2ODQ3MCAqdHBzOw0KPiA+ICsgICAgICAgaW50IHJldDsNCj4g PiArDQo+ID4gKyAgICAgICB0cHMgPSBkZXZtX2t6YWxsb2MoJmNsaWVudC0+ZGV2LCBzaXplb2Yo KnRwcyksIEdGUF9LRVJORUwpOw0KPiA+ICsgICAgICAgaWYgKCF0cHMpDQo+ID4gKyAgICAgICAg ICAgICAgIHJldHVybiAtRU5PTUVNOw0KPiA+ICsNCj4gPiArICAgICAgIG11dGV4X2luaXQoJnRw cy0+bG9jayk7DQo+ID4gKyAgICAgICBpMmNfc2V0X2NsaWVudGRhdGEoY2xpZW50LCB0cHMpOw0K PiA+ICsgICAgICAgdHBzLT5kZXYgPSAmY2xpZW50LT5kZXY7DQo+ID4gKw0KPiA+ICsgICAgICAg dHBzLT5yZWdtYXAgPSBkZXZtX3JlZ21hcF9pbml0X2kyYyhjbGllbnQsDQo+ICZ0cHM2ODQ3MF9y ZWdtYXBfY29uZmlnKTsNCj4gPiArICAgICAgIGlmIChJU19FUlIodHBzLT5yZWdtYXApKSB7DQo+ ID4gKyAgICAgICAgICAgICAgIGRldl9lcnIodHBzLT5kZXYsICJkZXZtX3JlZ21hcF9pbml0X2ky YyBFcnJvciAlZFxuIiwgcmV0KTsNCj4gPiArICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIo dHBzLT5yZWdtYXApOw0KPiA+ICsgICAgICAgfQ0KPiA+ICsNCj4gDQo+ID4gKyAgICAgICByZXQg PSBtZmRfYWRkX2RldmljZXModHBzLT5kZXYsIC0xLCB0cHM2ODQ3MHMsDQo+ID4gKyAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgQVJSQVlfU0laRSh0cHM2ODQ3MHMpLCBOVUxMLCAwLCBOVUxM KTsNCj4gDQo+IGRldm1fPw0KPiANCg0KQWNrDQoNCj4gPiArICAgICAgIGlmIChyZXQgPCAwKSB7 DQo+ID4gKyAgICAgICAgICAgICAgIGRldl9lcnIodHBzLT5kZXYsICJtZmRfYWRkX2RldmljZXMg ZmFpbGVkOiAlZFxuIiwgcmV0KTsNCj4gPiArICAgICAgICAgICAgICAgcmV0dXJuIHJldDsNCj4g PiArICAgICAgIH0NCj4gPiArDQo+ID4gKyAgICAgICByZXQgPSB0cHM2ODQ3MF9jaGlwX2luaXQo dHBzKTsNCj4gPiArICAgICAgIGlmIChyZXQgPCAwKSB7DQo+ID4gKyAgICAgICAgICAgICAgIGRl dl9lcnIodHBzLT5kZXYsICJUUFM2ODQ3MCBJbml0IEVycm9yICVkXG4iLCByZXQpOw0KPiA+ICsg ICAgICAgICAgICAgICBnb3RvIGZhaWw7DQo+ID4gKyAgICAgICB9DQo+ID4gKw0KPiA+ICsgICAg ICAgcmV0dXJuIDA7DQo+IA0KPiA+ICtmYWlsOg0KPiA+ICsgICAgICAgbXV0ZXhfbG9jaygmdHBz LT5sb2NrKTsNCj4gDQo+IEknbSBub3Qgc3VyZSB5b3UgbmVlZCB0aGlzIG11dGV4IHRvIGJlIGhl bGQgaGVyZS4NCj4gT3RoZXJ3aXNlIHlvdXIgY29kZSBoYXMgYSBidWcgd2l0aCBsb2NraW5nLg0K PiANCg0KUmVwZWF0aW5nIHRoZSByZXNwb25zZSB0byBIZWlra2kgaGVyZQ0KDQpJIGhhZCB0aGlz IGZvbGxvd2luZyBxdWVzdGlvbiBmcm9tIEFsYW4gQ294IG9uIHRoZSBvcmlnaW5hbCBjb2RlIHdp dGhvdXQgdGhlc2Ugd3JhcHBlcnMuDQoNCiJXaGF0IGlzIHRoZSBtb2RlbCBmb3IgaW5zdXJpbmcg dGhhdCBubyBpbnRlcnJ1cHQgb3IgdGhyZWFkIG9mIGEgZHJpdmVyIGlzIG5vdCBpbiBwYXJhbGxl bCBpc3N1aW5nIGEgdHBzNjg0NzBfIG9wZXJhdGlvbiB3aGVuIHRoZSBkZXZpY2UgZ29lcyBhd2F5 IChlZyBpZiBJIGRvd24gdGhlIGkyYyBjb250cm9sbGVyKSA/Ig0KDQpUbyBhZGRyZXNzIHRoZSBh Ym92ZSBjb25jZXJucywgSSBnb3QgZXh0cmEgY2F1dGlvdXMgYW5kIGltcGxlbWVudGVkIGxvY2tz IGFyb3VuZCB0aGUgcmVnbWFwXyogY2FsbHMuDQpOb3csIEkgaGF2ZSBiZWVuIGFza2VkIGZyb20g bW9yZSB0aGFuIG9uZSByZXZpZXdlciBhYm91dCB0aGUgbmVjZXNzaXR5IG9mIHRoZSBzYW1lLg0K V2l0aCB0aGUgdXNlIG9mIGRldm1fKiBjYWxscywgdHBzNjg0NzBfcmVtb3ZlKCkgZ29lcyBhd2F5 IGFuZCBsZWF2ZXMgdGhlIGRyaXZlciBqdXN0IHdpdGggcmVnbWFwXyogY2FsbHMuDQpVbmxlc3Mg SSBoZWFyIGZyb20gQWxhbiBvciBvdGhlciByZXZpZXdlcnMgb3RoZXJ3aXNlLCBJIHdpbGwgZHJv cCB0aGVzZSB3cmFwcGVycyBhcm91bmQgcmVnbWFwXyogY2FsbHMuDQoNCj4gPiArICAgICAgIG1m ZF9yZW1vdmVfZGV2aWNlcyh0cHMtPmRldik7DQo+ID4gKyAgICAgICBtdXRleF91bmxvY2soJnRw cy0+bG9jayk7DQo+ID4gKw0KPiA+ICsgICAgICAgcmV0dXJuIHJldDsNCj4gDQo+IFRha2luZyBh Ym92ZSBpbnRvIGNvbnNpZGVyYXRpb24gSSBzdWdnZXN0IHRvIGNsYXJpZnkgeW91ciBsb2NraW5n IHNjaGVtZS4NCj4gDQoNClNhbWUgYXMgYWJvdmUuDQoNCj4gPiArfQ0KPiA+ICsNCj4gPiArc3Rh dGljIGludCB0cHM2ODQ3MF9yZW1vdmUoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCkgew0KPiA+ ICsgICAgICAgc3RydWN0IHRwczY4NDcwICp0cHMgPSBpMmNfZ2V0X2NsaWVudGRhdGEoY2xpZW50 KTsNCj4gPiArDQo+IA0KPiA+ICsgICAgICAgbXV0ZXhfbG9jaygmdHBzLT5sb2NrKTsNCj4gPiAr ICAgICAgIG1mZF9yZW1vdmVfZGV2aWNlcyh0cHMtPmRldik7DQo+ID4gKyAgICAgICBtdXRleF91 bmxvY2soJnRwcy0+bG9jayk7DQo+IA0KPiBEaXR0by4NCj4gDQoNClNhbWUgYXMgYWJvdmUNCg0K PiA+ICsgICAgICAgcmV0dXJuIDA7DQo+ID4gK30NCj4gDQo+ID4gKy8qKg0KPiA+ICsgKiBzdHJ1 Y3QgdHBzNjg0NzAgLSB0cHM2ODQ3MCBzdWItZHJpdmVyIGNoaXAgYWNjZXNzIHJvdXRpbmVzDQo+ ID4gKyAqDQo+IA0KPiBrYnVpbGQgYm90IHdpbGwgYmUgdW5oYXBweS4gWW91IG5lZWQgdG8gZmls ZSBhIGRlc2NyaXB0aW9uIHBlciBmaWVsZC4NCj4gDQoNCkFjaw0KSXQgbG9va3MgbGlrZSB0aGlz IHN0cnVjdHVyZSB3aWxsIGdvIGF3YXksIG9uY2UgSSBpbXBsZW1lbnQgdGhlIGZlZWRiYWNrIGZy b20gSGVpa2tpLg0KDQo+ID4gKyAqIERldmljZSBkYXRhIG1heSBiZSB1c2VkIHRvIGFjY2VzcyB0 aGUgVFBTNjg0NzAgY2hpcCAqLw0KPiA+ICsNCj4gPiArc3RydWN0IHRwczY4NDcwIHsNCj4gPiAr ICAgICAgIHN0cnVjdCBkZXZpY2UgKmRldjsNCj4gPiArICAgICAgIHN0cnVjdCByZWdtYXAgKnJl Z21hcDsNCj4gDQo+ID4gKyAgICAgICAvKg0KPiA+ICsgICAgICAgICogVXNlZCB0byBzeW5jaHJv bml6ZSBhY2Nlc3MgdG8gdHBzNjg0NzBfIG9wZXJhdGlvbnMNCj4gPiArICAgICAgICAqIGFuZCBh ZGRpdGlvbiBhbmQgcmVtb3ZhbCBvZiBtZmQgZGV2aWNlcw0KPiA+ICsgICAgICAgICovDQo+IA0K PiBNb3ZlIGl0IHRvIGtlcm5lbC1kb2MgYWJvdmUuDQo+IA0KDQpTYW1lIGFzIGFib3ZlDQoNCj4g PiArICAgICAgIHN0cnVjdCBtdXRleCBsb2NrOw0KPiA+ICt9Ow0KPiANCj4gLS0NCj4gV2l0aCBC ZXN0IFJlZ2FyZHMsDQo+IEFuZHkgU2hldmNoZW5rbw0K -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > <rajmohan.mani@intel.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > <rajmohan.mani@intel.com> 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.
Hi Lee, > Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470 > > 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 > > > <rajmohan.mani@intel.com> 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 Raj
SGkgTGVlLA0KDQo+ID4gT24gRnJpLCAwOSBKdW4gMjAxNywgTWFuaSwgUmFqbW9oYW4gd3JvdGU6 DQo+ID4NCj4gPiA+IEhpIEFuZHksDQo+ID4gPg0KPiA+ID4gPiBPbiBUdWUsIEp1biAwNiwgMjAx NyBhdCAwMzo1OTo0OVBNICswMzAwLCBBbmR5IFNoZXZjaGVua28gd3JvdGU6DQo+ID4gPiA+ID4g T24gVHVlLCBKdW4gNiwgMjAxNyBhdCAyOjU1IFBNLCBSYWptb2hhbiBNYW5pDQo+ID4gPiA+IDxy YWptb2hhbi5tYW5pQGludGVsLmNvbT4gd3JvdGU6DQo+ID4gPiA+ID4gPiBUaGUgVFBTNjg0NzAg ZGV2aWNlIGlzIGFuIGFkdmFuY2VkIHBvd2VyIG1hbmFnZW1lbnQgdW5pdCB0aGF0DQo+ID4gPiA+ ID4gPiBwb3dlcnMgYSBDb21wYWN0IENhbWVyYSBNb2R1bGUgKENDTSksIGdlbmVyYXRlcyBjbG9j a3MgZm9yDQo+ID4gPiA+ID4gPiBpbWFnZSBzZW5zb3JzLCBkcml2ZXMgYSBkdWFsIExFRCBmb3Ig Rmxhc2ggYW5kIGluY29ycG9yYXRlcw0KPiA+ID4gPiA+ID4gdHdvIExFRCBkcml2ZXJzIGZvciBn ZW5lcmFsIHB1cnBvc2UgaW5kaWNhdG9ycy4NCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiBUaGlz IHBhdGNoIGFkZHMgc3VwcG9ydCBmb3IgVFBTNjg0NzAgbWZkIGRldmljZS4NCj4gPiA+ID4gPg0K PiA+ID4gPiA+IEkgZHVubm8gd2h5IHlvdSBkZWNpZGUgdG8gc2VuZCB0aGlzIG91dCBub3csIHNl ZSBteSBjb21tZW50cw0KPiBiZWxvdy4NCj4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gK3N0YXRpYyBp bnQgdHBzNjg0NzBfY2hpcF9pbml0KHN0cnVjdCB0cHM2ODQ3MCAqdHBzKSB7DQo+ID4gPiA+ID4g PiArICAgICAgIHVuc2lnbmVkIGludCB2ZXJzaW9uOw0KPiA+ID4gPiA+ID4gKyAgICAgICBpbnQg cmV0Ow0KPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiArICAgICAgIC8qIEZJWE1FOiBjb25maWd1cmUg dGhlc2UgZHluYW1pY2FsbHkgKi8NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IFNvLCB3aGF0IHByZXZl bnRzIHlvdSB0byBmaXggdGhpcz8NCj4gPiA+ID4NCj4gPiA+ID4gTm90aGluZyBJIHN1cHBvc2Uu IFRoZXkncmUgaG93ZXZlciBub3QgbmVlZGVkIHJpZ2h0IG5vdyBhbmQgY2FuIGJlDQo+ID4gPiA+ IGltcGxlbWVudGVkIGxhdGVyIG9uIGlmIHRoZXkncmUgZXZlciBuZWVkZWQuDQo+ID4gPiA+DQo+ ID4gPg0KPiA+ID4gQWNrDQo+ID4NCj4gPiBXaGF0IGRvZXMgdGhpcyBtZWFuPyAgSXMgdGhlIHBs YW4gdG8gZml4IGl0IG9yIG5vdD8gIEkgZG9uJ3Qgd2FudA0KPiA+IEZJWE1FcyBpbiB0aGUgY29k ZSB0aGF0IGEpIGNhbiBiZSBmaXhlZCByaWdodCBhd2F5IG9yIGIpIG1pZ2h0IG5ldmVyIGJlDQo+ IGZpeGVkLg0KPiA+DQo+IA0KPiBJIG1lYW50IHRoYXQgdGhpcyBjYW4gYmUgaW1wbGVtZW50ZWQg bGF0ZXIgb24sIGlmIHRoZXJlJ3MgYSBuZWVkLg0KPiBJIHdpbGwgbG9vayBpbnRvIHRoaXMgYW5k IHNlZSBob3cgdGhpcyBjYW4gYmUgZml4ZWQuDQo+IA0KDQpUaGFua3MgZm9yIHlvdXIgcGF0aWVu Y2UuDQpUaGlzIGlzIGZpeGVkIHdpdGggdjQgb2YgdGhpcyBzZXJpZXMuDQoNClJhag0K -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > > > <rajmohan.mani@intel.com> 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.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 3eb5c93..c5e51bc 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1311,6 +1311,18 @@ 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 I2C + select MFD_CORE + select REGMAP_I2C + 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 + that are often used in portable devices. + 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..ca174fb --- /dev/null +++ b/drivers/mfd/tps68470.c @@ -0,0 +1,227 @@ +/* + * TPS68470 chip family multi-function driver + * + * Copyright (C) 2017 Intel Corporation + * Authors: + * Rajmohan Mani <rajmohan.mani@intel.com> + * Tianshu Qiu <tian.shu.qiu@intel.com> + * Jian Xu Zheng <jian.xu.zheng@intel.com> + * Yuning Pu <yuning.pu@intel.com> + * + * 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. + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/mfd/core.h> +#include <linux/mfd/tps68470.h> +#include <linux/init.h> +#include <linux/regmap.h> + +static const struct mfd_cell tps68470s[] = { + { + .name = "tps68470-gpio", + }, + { + .name = "tps68470_pmic_opregion", + }, +}; + +/* + * tps68470_reg_read: Read a single tps68470 register. + * + * @tps: Device to read from. + * @reg: Register to read. + * @val: Contains the value + */ +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg, + unsigned int *val) +{ + int ret; + + mutex_lock(&tps->lock); + ret = regmap_read(tps->regmap, reg, val); + mutex_unlock(&tps->lock); + return ret; +} +EXPORT_SYMBOL_GPL(tps68470_reg_read); + +/* + * tps68470_reg_write: Write a single tps68470 register. + * + * @tps68470: Device to write to. + * @reg: Register to write to. + * @val: Value to write. + */ +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg, + unsigned int val) +{ + int ret; + + mutex_lock(&tps->lock); + ret = regmap_write(tps->regmap, reg, val); + mutex_unlock(&tps->lock); + return ret; +} +EXPORT_SYMBOL_GPL(tps68470_reg_write); + +/* + * tps68470_update_bits: Modify bits w.r.t mask and val. + * + * @tps68470: Device to write to. + * @reg: Register to read-write to. + * @mask: Mask. + * @val: Value to write. + */ +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg, + unsigned int mask, unsigned int val) +{ + int ret; + + mutex_lock(&tps->lock); + ret = regmap_update_bits(tps->regmap, reg, mask, val); + mutex_unlock(&tps->lock); + return ret; +} +EXPORT_SYMBOL_GPL(tps68470_update_bits); + +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 tps68470 *tps) +{ + unsigned int version; + int ret; + + ret = tps68470_reg_read(tps, TPS68470_REG_REVID, &version); + if (ret < 0) { + dev_err(tps->dev, + "Failed to read revision register: %d\n", ret); + return ret; + } + + dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version); + + ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff); + if (ret < 0) + return ret; + + /* FIXME: configure these dynamically */ + /* Enable Daisy Chain LDO and configure relevant GPIOs as output */ + ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2); + if (ret < 0) + return ret; + + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2); + if (ret < 0) + return ret; + + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2); + if (ret < 0) + return ret; + + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2); + if (ret < 0) + return ret; + + /* + * When SDA and SCL are routed to GPIO1 and GPIO2, the mode + * for these GPIOs must be configured using their respective + * GPCTLxA registers as inputs with no pull-ups. + */ + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0); + if (ret < 0) + return ret; + + ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0); + if (ret < 0) + return ret; + + /* Enable daisy chain */ + ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1); + if (ret < 0) + return ret; + + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US, + TPS68470_DAISY_CHAIN_DELAY_US + 10); + return 0; +} + +static int tps68470_probe(struct i2c_client *client) +{ + struct tps68470 *tps; + int ret; + + tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL); + if (!tps) + return -ENOMEM; + + mutex_init(&tps->lock); + i2c_set_clientdata(client, tps); + tps->dev = &client->dev; + + tps->regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); + if (IS_ERR(tps->regmap)) { + dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret); + return PTR_ERR(tps->regmap); + } + + ret = mfd_add_devices(tps->dev, -1, tps68470s, + ARRAY_SIZE(tps68470s), NULL, 0, NULL); + if (ret < 0) { + dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret); + return ret; + } + + ret = tps68470_chip_init(tps); + if (ret < 0) { + dev_err(tps->dev, "TPS68470 Init Error %d\n", ret); + goto fail; + } + + return 0; +fail: + mutex_lock(&tps->lock); + mfd_remove_devices(tps->dev); + mutex_unlock(&tps->lock); + + return ret; +} + +static int tps68470_remove(struct i2c_client *client) +{ + struct tps68470 *tps = i2c_get_clientdata(client); + + mutex_lock(&tps->lock); + mfd_remove_devices(tps->dev); + mutex_unlock(&tps->lock); + + return 0; +} + +static const struct acpi_device_id tps68470_acpi_ids[] = { + {"INT3472"}, + {}, +}; + +MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids); + +static struct i2c_driver tps68470_driver = { + .driver = { + .name = "tps68470", + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids), + }, + .probe_new = tps68470_probe, + .remove = tps68470_remove, +}; +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..440c802 --- /dev/null +++ b/include/linux/mfd/tps68470.h @@ -0,0 +1,167 @@ +/* + * 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. + */ + +#ifndef __LINUX_MFD_TPS68470_H +#define __LINUX_MFD_TPS68470_H + +#include <linux/i2c.h> + +/* All register addresses */ +#define TPS68470_REG_GSTAT 0x01 +#define TPS68470_REG_VRSTA 0x02 +#define TPS68470_REG_VRSHORT 0x03 +#define TPS68470_REG_INTMASK 0x04 +#define TPS68470_REG_VCOSPEED 0x05 +#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_PITCTL 0x23 +#define TPS68470_REG_WAKECFG 0x24 +#define TPS68470_REG_IOWAKESTAT 0x25 +#define TPS68470_REG_GPDI 0x26 +#define TPS68470_REG_GPDO 0x27 +#define TPS68470_REG_ILEDCTL 0x28 +#define TPS68470_REG_WLEDSTAT 0x29 +#define TPS68470_REG_VWLEDILIM 0x2A +#define TPS68470_REG_VWLEDVAL 0x2B +#define TPS68470_REG_WLEDMAXRER 0x2C +#define TPS68470_REG_WLEDMAXT 0x2D +#define TPS68470_REG_WLEDMAXAF 0x2E +#define TPS68470_REG_WLEDMAXF 0x2F +#define TPS68470_REG_WLEDTO 0x30 +#define TPS68470_REG_VWLEDCTL 0x31 +#define TPS68470_REG_WLEDTIMER_MSB 0x32 +#define TPS68470_REG_WLEDTIMER_LSB 0x33 +#define TPS68470_REG_WLEDC1 0x34 +#define TPS68470_REG_WLEDC2 0x35 +#define TPS68470_REG_WLEDCTL 0x36 +#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_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_OSC_EXT_CAP_SHIFT 4 +#define TPS68470_OSC_EXT_CAP_DEFAULT 0x05 /* 10pf */ + +#define TPS68470_CLK_SRC_SHIFT 7 +#define TPS68470_CLK_SRC_GPIO3 0 +#define TPS68470_CLK_SRC_XTAL 1 + +#define TPS68470_DRV_STR_1MA 0 +#define TPS68470_DRV_STR_2MA 1 +#define TPS68470_DRV_STR_4MA 2 +#define TPS68470_DRV_STR_8MA 3 +#define TPS68470_DRV_STR_A_SHIFT 0 +#define TPS68470_DRV_STR_B_SHIFT 2 + +#define TPS68470_OUTPUT_XTAL_BUFFERED 1 +#define TPS68470_PLL_OUTPUT_ENABLE 2 +#define TPS68470_PLL_OUTPUT_SS_ENABLE 3 +#define TPS68470_OUTPUT_A_SHIFT 0 +#define TPS68470_OUTPUT_B_SHIFT 2 + +#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 + +#define TPS68470_PLL_STARTUP_DELAY_US 1000 +#define TPS68470_DAISY_CHAIN_DELAY_US 3000 + +/** + * struct tps68470 - tps68470 sub-driver chip access routines + * + * Device data may be used to access the TPS68470 chip + */ + +struct tps68470 { + struct device *dev; + struct regmap *regmap; + /* + * Used to synchronize access to tps68470_ operations + * and addition and removal of mfd devices + */ + struct mutex lock; +}; + +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg, + unsigned int *val); +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg, + unsigned int val); +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg, + unsigned int mask, unsigned int val); + +#endif /* __LINUX_MFD_TPS68470_H */
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@intel.com> --- drivers/mfd/Kconfig | 12 +++ drivers/mfd/Makefile | 1 + drivers/mfd/tps68470.c | 227 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/tps68470.h | 167 +++++++++++++++++++++++++++++++ 4 files changed, 407 insertions(+) create mode 100644 drivers/mfd/tps68470.c create mode 100644 include/linux/mfd/tps68470.h