Re: [PATCH v3 3/4] mfd: tps65917: Add driver for the TPS65917 PMIC

[Keerthy]

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@xxxxxx>
> > > > ---
> > > > 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, &reg);
> > > > +		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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/