Re: [PATCH RESEND] tps65023: Allow platforms to specify DCDC_2 and DCDC_3 value.

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sat, Sep 04, 2010 at 01:12:54PM -0400, Michael Williamson wrote:
> Hello Dmitry,
> 
> On 09/04/2010 01:01 PM, Dmitry Torokhov wrote:
> > Hi Michael,
> > 
> > On Sat, Sep 04, 2010 at 12:04:31PM -0400, Michael Williamson wrote:
> >> The TPS65023 regulator includes 5 regulators (3 DCDC and 2 LDO's).
> >> 2 of the DCDC regulators are of fixed voltage and can only be
> >> turned on or off via the I2C interface.  The current driver defaults
> >> the values of the fixed DCDC regulators.  However, the actual
> >> voltage of these regulators may be set differently for a board (via
> >> voltage divider circuit, etc.).
> >>
> >> This patch allows a platform to pass in the actual voltage used
> >> for these DCDC supplies such that they are reported correctly in
> >> sysfs.
> >>
> >> Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
> >> ---
> >> Previous submission said 1/3.  Should only be 1/1.
> >>
> >>  drivers/regulator/tps65023-regulator.c |   17 +++++++++++++----
> >>  1 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/regulator/tps65023-regulator.c b/drivers/regulator/tps65023-regulator.c
> >> index cd6d4fc..68cd7d3 100644
> >> --- a/drivers/regulator/tps65023-regulator.c
> >> +++ b/drivers/regulator/tps65023-regulator.c
> >> @@ -124,7 +124,7 @@ struct tps_pmic {
> >>  	struct regulator_desc desc[TPS65023_NUM_REGULATOR];
> >>  	struct i2c_client *client;
> >>  	struct regulator_dev *rdev[TPS65023_NUM_REGULATOR];
> >> -	const struct tps_info *info[TPS65023_NUM_REGULATOR];
> >> +	struct tps_info *info[TPS65023_NUM_REGULATOR];
> >>  	struct mutex io_lock;
> >>  };
> >>  
> >> @@ -462,9 +462,10 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
> >>  				     const struct i2c_device_id *id)
> >>  {
> >>  	static int desc_id;
> >> -	const struct tps_info *info = (void *)id->driver_data;
> >> +	struct tps_info *info = (void *)id->driver_data;
> > 
> > id is shared between multiple instances of the device and is constant.
> > It is a bad idea to cast away constness and modify the data; you should
> > move data that may be adjusted into tps_pmic structure.
> > 
> > Even if we expect to only a single instance of the device in question we
> > should follow best practices.
> > 
> Yes of course.  Hadn't thought of multiple instances.  I will correct and resubmit.  
> Thank you.
> 
> >>  	struct regulator_init_data *init_data;
> > 
> > This should probably be marked const too as it comes from platform code.
> > 
> >>  	struct regulator_dev *rdev;
> >> +	struct regulation_constraints *c;
> > 
> > const as well?
> > 
> Right.
> 
> >>  	struct tps_pmic *tps;
> >>  	int i;
> >>  	int error;
> >> @@ -501,6 +502,14 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
> >>  		tps->desc[i].type = REGULATOR_VOLTAGE;
> >>  		tps->desc[i].owner = THIS_MODULE;
> >>  
> >> +		/* Override default DCDC2/3 values if provided */
> >> +		c = &init_data->constraints;
> >> +		if ((i == TPS65023_DCDC_2) || (i == TPS65023_DCDC_3)
> >> +			&& c->min_uV && c->min_uV == c->max_uV) {
> > 
> > Min and max are the same? You sure you got the condition right? 
> > 
> These DCDC regulators are fixed in hardware, they cannot be controlled and should
> only have one value (minus hardware tolerance, I suppose).  I thought that such a 
> sanity check would be required.  What should the reported value be if a range is 
> provided? 
> 

Ah, I see. If there was a comment it would be clearer though (I see it
described in the commit log but I think this is one of cases when
comment in the code might be useful too).

BTW, what if board code simply wants to reduce the range, for one reason
or another? Maybe c->min_uV <= c->max_uV would be a better sanity check?

-- 
Dmitry