Re: [PATCH] regulator: max8973: add regulator driver support

[Laxman Dewangan <ldewangan@nvidia.com>]

On Monday 19 November 2012 04:22 PM, Guennadi Liakhovetski wrote:
>
> Hi Laxman
>
> drivers in the tree. Well, I came to two conclusions so far: (1) The
> current regulator API is not very well suitable for such regulators. I
> would imagine, one would need two methods: for setting the "normal" and
> the DVS voltage. Instead of this drivers are trying to be smart at
> guessing, which voltage the user is trying to set now... (2) Drivers do
> this in different ways and at least out of the 2 drivers I looked at both
> have bugs and different ones at that. I'll send a separate email,
> describing what I found suspicious in them.
>
> Of course, all the above was just my DVS-newbie impression, which can very
> well be absolutely wrong.
>

If there is multipel VOUT register for single vout then these registers 
are generally selected by the input pin of device.
In a given system, you can connect the gpios pin to this input pins to 
select the proper VOUT register.
The register update through i2c consume more time and changing the gpio 
state is comparatively less.
So if you have let say 4 voltages 1.10, 1.11, 1.12, 1.13 and having 4 
VOUT register. Then program these vout val to vout reg like 1.10 to 
vout_reg0, 1.11 to vout_reg1 etc.
Now for changing voltage between these will just require to change the 
gpio pin state, not the register update and so it will be faster. You 
can achive the voltage change by gpio pin state change.

Now if your DVS have more volatge scaling then you can use the LRU 
mechanism to use the vout register for this new value.



>> +     max->dev =&client->dev;
>> +     max->desc.name = id->name;
>> +     max->desc.id = 0;
> Don't you have to be able to process multiple such devices?

Not really require as device have only one output. The different devices 
will have different registrations and so does not matter here.




>> +     max->enable_external_control = pdata->enable_ext_control;
>> +     max->dvs_gpio = pdata->dvs_gpio;
>> +     max->curr_gpio_val = pdata->dvs_def_state;
>> +     max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
>> +     max->lru_index[0] = max->curr_vout_reg;
> Here you actually need an offset within your register address space, so,
> should be
>
> +       max->lru_index[0] = pdata->dvs_def_state;


Yaah, seems some issue if vout_base is not zero. But really dont require 
here as MAX8973_VOUT is 0 in this case.