by Yadwinder Singh Brar

[permalink] [raw]

Subject: Re: [PATCH 2/2] regulator: Add support for MAX77686.

Hi Sylwester,

On Thu, May 10, 2012 at 1:24 AM, Sylwester Nawrocki <[email protected]> wrote:
> Hi,
>
> just a few nitpicks...
>
>
> On 05/09/2012 06:24 PM, Yadwinder Singh wrote:
>>
>> From: Yadwinder Singh Brar<[email protected]>
>>
>> Add support for PMIC/regulator portion of MAX77686 multifunction device.
>> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of
>> driver
>> which supports setting and getting the voltage of a regulator with I2C
>> interface.
>>
>> Signed-off-by: Yadwinder Singh Brar<[email protected]>
>
> ...
>
>> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
>> new file mode 100644
>> index 0000000..4aa9722
>> --- /dev/null
>> +++ b/drivers/regulator/max77686.c
>> @@ -0,0 +1,660 @@
>> +/*
>> + * max77686.c - Regulator driver for the Maxim 77686
>> + *
>> + * Copyright (C) 2012 Samsung Electronics
>
>
> I believe this should read:
>
> ?+ * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>
> In patch 1/2 the copyright notice is also not exactly correct.
>
>
>> + * Chiwoong Byun<[email protected]>
>
>
> s/smasung/samsung
>
> ...
>

Thanks for pointing. I will rectify above mistakes.

>> +static int max77686_get_enable_register(struct regulator_dev *rdev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int *reg, int *mask, int *pattern)
>> +{
>> + ? ? ? int rid = rdev_get_id(rdev);
>> +
>> + ? ? ? switch (rid) {
>> + ? ? ? case MAX77686_LDO1...MAX77686_LDO26:
>> + ? ? ? ? ? ? ? *reg = MAX77686_REG_LDO1CTRL1 + (rid - MAX77686_LDO1);
>> + ? ? ? ? ? ? ? *mask = 0xC0;
>> + ? ? ? ? ? ? ? *pattern = 0xC0;
>
>
> What about using lower case for all hex numbers ?
>
> ...
>

Yes, i will take care of this.

>> +static int max77686_get_voltage_register(struct regulator_dev *rdev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int *_reg, int *_shift, int
>> *_mask)
>> +{
>
> ...
>
>> + ? ? ? case MAX77686_BUCK2:
>> + ? ? ? ? ? ? ? reg = MAX77686_REG_BUCK2DVS1;
>> + ? ? ? ? ? ? ? mask = 0xff;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case MAX77686_BUCK3:
>> + ? ? ? ? ? ? ? reg = MAX77686_REG_BUCK3DVS1;
>> + ? ? ? ? ? ? ? mask = 0xff;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case MAX77686_BUCK4:
>> + ? ? ? ? ? ? ? reg = MAX77686_REG_BUCK4DVS1;
>> + ? ? ? ? ? ? ? mask = 0xff;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case MAX77686_BUCK5...MAX77686_BUCK9:
>> + ? ? ? ? ? ? ? reg = MAX77686_REG_BUCK5OUT + (rid - MAX77686_BUCK5) * 2;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? }
>> +
>> + ? ? ? *_reg = reg;
>> + ? ? ? *_shift = shift;
>> + ? ? ? *_mask = mask;
>> +
>> + ? ? ? return 0;
>> +}
>
>
> Thanks,
> Sylwester
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Thanks,
Yadwinder.

2012-05-10 09:34:47

On Tue, May 15, 2012 at 07:17:11PM +0530, Yadwinder Singh Brar wrote:
> On Thu, May 10, 2012 at 12:17 AM, Mark Brown
> > On Wed, May 09, 2012 at 09:54:55PM +0530, Yadwinder Singh wrote:

> >> + ? ? if (iodev->dev->of_node) {
> >> + ? ? ? ? ? ? ret = max77686_pmic_dt_parse_pdata(iodev, pdata);
> >> + ? ? ? ? ? ? if (ret)
> >> + ? ? ? ? ? ? ? ? ? ? return ret;

> > This ought to use of_regulator_match().

> While using it I am seeing that though it reduces few lines of code in
> our driver , it adds a huge array(of_regulator_match[])in which we
> have to populate the strings(name) which we already have in
> regulator_desc[ ], Isn't it overhead ?

Well, I think half the problem here is that you're only instantiating
things you find in the device tree. If you were unconditionally
instantiating all the regulators then suddenly this becomes a lot
neater.

> >> + ? ? ? ? ? ? rdev[i] = regulator_register(&regulators[id], max77686->dev,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pdata->regulators[i].initdata,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?max77686, NULL);

> > No, you should unconditionally register all regulators the device
> > physically has. ?This is useful for debug and simplifies the code.

> If we have to use only 2 or 3 regulators on our board out off 36 or
> lets take a case if our chip supports 50/100 regulators, I think i
> will a overhead to register all unused regulators as well as
> populating all the regulators in DT or platform data.

This isn't really a terribly realistic situation - if there are were
that many unused regulators the part selection is clearly not
appropriate to the system and most hardware engineers just wouldn't do
it.

Having information about all the regulators also allows us to do things
like power off any unused regulators which were left on after boot (eg,
due to bootloader or the PMIC defaults).

Attachments:

(No filename) (1.84 kB)
signature.asc (836.00 B)
Digital signature Download all attachments