Re: [PATCH 1/2] Exynos4 NURI: configure regulators and PMIC

[MyungJoo Ham]

On Sun, Jun 19, 2011 at 12:12 AM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jun 16, 2011 at 06:09:31PM +0900, MyungJoo Ham wrote:
>
>> Âstatic struct regulator_init_data emmc_fixed_voltage_init_data = {
>>    .constraints      Â= {
>> + Â Â Â Â Â Â .min_uV Â Â Â Â = 2800000,
>> + Â Â Â Â Â Â .max_uV Â Â Â Â = 2800000,
>>        .name      = "VMEM_VDD_2.8V",
>> Â Â Â Â Â Â Â .valid_ops_mask = REGULATOR_CHANGE_STATUS,
>
> Since the regualtor can't change voltage specifying the voltage here
> isn't going to achieve anything - to get the voltage reported through
> get_voltage() you need to put the voltage in the platform data for the
> fixed regulator.

Ah.. they are useless. I'll remove them.

>
>> +static struct regulator_consumer_supply nuri_max8997_ldo1_consumer[] = {
>> + Â Â REGULATOR_SUPPLY("vadc", NULL), /* Used by CPU's ADC drv */
>> +};
>
> In the ADC regulator patch you called the supply vdd (though the chip
> normally calls it vadc so that's the better name)...

Um.. this happened as I have seperated NURI-board platform patch for
ADC and PMIC.
After ADC patch, that name became ("vdd", "s5p-adc").
I'll let them either be merged or be disjoint completely.

Anyway, do you think "vadc" is better than "vdd" for ADC drivers? The
circuit schematics says the pin on the consumer side is "VDD33_ADC"
(VDD 3.3V for ADC).


>
> Extra ' too.
>
>> +static struct regulator_consumer_supply nuri_max8997_ldo8_consumer[] = {
>> + Â Â REGULATOR_SUPPLY("vusb_d", NULL), /* Used by CPU */
>> + Â Â REGULATOR_SUPPLY("vdac", NULL), /* Used by CPU */
>> +};
>
> Another VADC? ÂFor a different supply?

That's DAC (opposite to ADC).

>
>> +       .state_mem   Â= {
>> +           .enabled    Â= 0,
>
> No need to initialize to zero.

Ok, I'll remove every zero-initialization from the patch.

>
>> +static struct regulator_init_data nuri_max8997_ldo10_data = {
>> +   .constraints  Â= {
>
> You should be able to use __initdata for a lot of this by the way.

Ah.. way to reduce some weight. Fine.

>
>> +#define NURI_PMIC_GPIO Â Â Â Â Â Â Â EXYNOS4_GPX0(7)
>> +static void __init nuri_pmic_init(void)
>> +{
>> + Â Â int gpio;
>> +
>> + Â Â nuri_max8997_pdata.irq_base = irq_get_next_irq(IRQ_GPIO_END);
>> + Â Â gpio = NURI_PMIC_GPIO;
>> + Â Â gpio_request(gpio, "AP_PMIC_IRQ");
>> + Â Â s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(0xf));
>> + Â Â s3c_gpio_setpull(gpio, S3C_GPIO_PULL_NONE);
>> +}
>
> I'm not sure both the #define and the variable make sense here...
>

I've defined them because two statements are using the gpio address of
NURI_PMIC.



Thanks you very much!

- MyungJoo
-- 
MyungJoo Ham (íëì), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
--
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/