Messages in this thread | | | Date | Fri, 23 May 2014 08:52:12 +0530 | From | Keerthy <> | Subject | Re: [PATCH v3 3/4] mfd: tps65917: Add driver for the TPS65917 PMIC |
| |
On Thursday 22 May 2014 05:18 PM, Lee Jones wrote: >>>> The TPS65917 chip is a power management IC for Portable Navigation Systems >>>> and Tablet Computing devices. It contains the following components: >>>> >>>> - Regulators. >>>> - Over Temperature warning and Shut down. >>>> >>>> This patch adds support for tps65917 mfd device. At this time only >>>> the regulator functionality is made available. >>>> >>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>> --- >>>> v3 Changes: >>>> >>>> * Header file formating >>>> * Changed the cache_type to REGCACHE_RBTREE >>>> * removed unnecessary code >>>> * Corrected documentation style >>>> * Added pm_power_off function >>>> >>>> v2 Changes: >>>> >>>> * Added volatile register check as some of the registers >>>> in the set are volatile. >>>> drivers/mfd/Kconfig | 12 + >>>> drivers/mfd/Makefile | 1 + >>>> drivers/mfd/tps65917.c | 594 +++++++++++++++++ >>>> include/linux/mfd/tps65917.h | 1485 ++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 2092 insertions(+) >>>> create mode 100644 drivers/mfd/tps65917.c >>>> create mode 100644 include/linux/mfd/tps65917.h > [...] > >>>> + for (i = 0; i < TPS65917_NUM_CLIENTS; i++) { >>>> + if (i == 0) { >>>> + tps65917->i2c_clients[i] = i2c; >>> This is messy. Move this line out of the loop and change the loop >>> parameters to start from 1. Then we can reduce all of this >>> unnecessary indentation. >> There is a common thing we do after if and else. Removing i == 0 >> part out of the >> loop would mean repeating the common part. This way seems >> better. > Ah yes, good point. > >>>> + } else { >>>> + tps65917->i2c_clients[i] = >>>> + i2c_new_dummy(i2c->adapter, >>>> + i2c->addr + i); >>>> + if (!tps65917->i2c_clients[i]) { >>>> + dev_err(tps65917->dev, >>>> + "can't attach client %d\n", i); >>>> + ret = -ENOMEM; >>>> + goto err_i2c; >>>> + } >>>> + >>>> + tps65917->i2c_clients[i]->dev.of_node = of_node_get(node); >>> Don't forget to decrement the reference when you've finished with it. >> I did not get this. > Do you know what of_node_get() does? > > [...] > >>> What happens if !node? Then no children get registered and this has >>> all been a waste of time? >> Only DT way is possible. This check is redundant. I will add a check >> at the beginning for !node. > If that's the case you should add 'depends on OF' in the Kconfig. > >>>> +struct tps65917_reg_init { >>>> + /* >>>> + * 0: reload default values from OTP on warm reset >>>> + * 1: maintain voltage from VSEL on warm reset >>>> + */ >>>> + bool warm_reset; >>> Where is this used? >>> >>>> + /* >>>> + * 0: i2c selection of voltage >>>> + * 1: pin selection of voltage. >>>> + */ >>>> + bool roof_floor; >>> And this? >>> >>>> + /* >>>> + * For SMPS >>>> + * >>>> + * 0: Off >>>> + * 1: AUTO >>>> + * 2: ECO >>>> + * 3: Forced PWM >>>> + * >>>> + * For LDO >>>> + * >>>> + * 0: Off >>>> + * 1: On >>>> + */ >>>> + int mode_sleep; >>> And this? >>> >>>> + u8 vsel; >>> And this? >> All of the above can be used by regulator driver. > Doesn't the regulator driver have its own header file? Why are these > in a shared file if they're not used anywhere else? > > [...] > >>>> + if (pdata->mux_from_pdata) { >>>> + reg = pdata->pad1; >>>> + ret = regmap_write(tps65917->regmap[slave], addr, reg); >>>> + if (ret) >>>> + goto err_irq; >>>> + } else { >>>> + ret = regmap_read(tps65917->regmap[slave], addr, ®); >>>> + if (ret) >>>> + goto err_irq; >>>> + }i >>> What does the read do? You're not doing anything with the value. >> This pad1 and pad2 stuff is not needed. I will remove this. > Then why is it in here? > > Did you copy this code from somewhere, if so, where? > > Okay, I just answered my own question. There is so much common code > in between this and palmas, there is no way I'm going to accept this > driver. Please merge it in with the palmas driver! > The chip is more like a subset of palmas with lot of register offset changes and register bit field changes. Adding this would make it clumsy. There could be lot of checks. That is why i chose to write a new driver.
Palmas driver already supports palmas variants and tps659038. Merging this would mean more and more checks :-/.
Regards, Keerthy
| |