Re: [PATCH v9 1/2] regulator: Add driver for max77802 PMIC PMIC regulators

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

Hello Yuvaraj,

On 08/22/2014 08:01 AM, Yuvaraj Cd wrote:
>> +
>> +static int max77802_pmic_probe(struct platform_device *pdev)
>> +{
>> +       struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>> +       struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev);
>> +       struct max77802_regulator_prv *max77802;
>> +       int i, ret = 0, val;
>> +       struct regulator_config config = { };
>> +
>> +       /* This is allocated by the MFD driver */
>> +       if (!pdata) {
>> +               dev_err(&pdev->dev, "no platform data found for regulator\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       max77802 = devm_kzalloc(&pdev->dev,
>> +                               sizeof(struct max77802_regulator_prv),
>> +                               GFP_KERNEL);
>> +       if (!max77802)
>> +               return -ENOMEM;
>> +
>> +       if (iodev->dev->of_node) {
>> +               ret = max77802_pmic_dt_parse_pdata(pdev, pdata);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       config.dev = iodev->dev;
>> +       config.regmap = iodev->regmap;
>> +       config.driver_data = max77802;
>> +       platform_set_drvdata(pdev, max77802);
>> +
>> +       for (i = 0; i < MAX77802_REG_MAX; i++) {
>> +               struct regulator_dev *rdev;
>> +               int id = pdata->regulators[i].id;
>> +               int shift = max77802_get_opmode_shift(id);
>> +
>> +               config.init_data = pdata->regulators[i].initdata;
>> +               config.of_node = pdata->regulators[i].of_node;
>> +
>> +               ret = regmap_read(iodev->regmap, regulators[i].enable_reg, &val);
>> +               max77802->opmode[id] = val >> shift & MAX77802_OPMODE_MASK;
> 
> I have been using this patch series for adding UHS support for dw_mmc
> driver. During reboot testing I came across an issue where card
> detection fails due to vqmmc regulator not getting enabled. On
> debugging further, I found that PMIC driver is reading the operating
> mode during probe and reusing it in the enable function. With the UHS
> patches vqmmc regulator gets disabled during POWER_OFF and if we do
> warm reboot, an incorrect operating mode(OFF) is read. This leads to
> the vqmmc regulator staying disabled. I have referred to 77686 driver
> and observed that they are handling this a little differently. With
> the following change in the driver above issue is resolved:
> 
> -               ret = regmap_read(iodev->regmap,
> regulators[i].enable_reg, &val);
> -               max77802->opmode[id] = val >> shift & MAX77802_OPMODE_MASK;
> +               max77802->opmode[i] = regulators[i].enable_mask >> shift;
> 

I don't think this change is correct it its current form since
.enable_mask is initialized to MAX77802_OPMODE_MASK << shift so this is
actually setting the opmode to MAX77802_OPMODE_MASK.

Now, MAX77802_OPMODE_MASK has the same value than MAX77802_OPMODE_NORMAL
so what you are really doing here is initializing the opmode to
MAX77802_OPMODE_NORMAL.

> Please have a look and let me know, if there is any better way of handling this.
>

The first versions of this driver did in fact set the opmode to
MAX77802_OPMODE_NORMAL on probe but Mark asked me to read it from the
hardware instead [0]. I was indeed worried at the time that something like
this could happen on a warm reset [1].

Mark, any opinions on how this should be solved will be highly appreciated.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/6/16/576
[1]: https://lkml.org/lkml/2014/6/17/174