[U-Boot] [PATCH v3 06/17] dm: regulator: add implementation of driver model regulator uclass

[Simon Glass]

Hi Przemyslaw,

On 3 April 2015 at 10:09, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> Hello Simon,
>
>
> On 03/29/2015 03:07 PM, Simon Glass wrote:
>>
>> Hi Przemyslaw,
>>
>> On 24 March 2015 at 14:30, Przemyslaw Marczak <p.marczak at samsung.com>
>> wrote:

[snip]

>
>>> +
>>> +       info->min_uV = fdtdec_get_int(gd->fdt_blob, offset,
>>> +                                     "regulator-min-microvolt", -1);
>>> +       if (info->min_uV < 0)
>>> +               return -ENXIO;
>>> +
>>> +       info->max_uV = fdtdec_get_int(gd->fdt_blob, offset,
>>> +                                     "regulator-max-microvolt", -1);
>>> +       if (info->max_uV < 0)
>>> +               return -ENXIO;
>>> +
>>> +       /* Optional constraints */
>>> +       info->min_uA = fdtdec_get_int(gd->fdt_blob, offset,
>>> +                                     "regulator-min-microamp", -1);
>>> +       info->max_uA = fdtdec_get_int(gd->fdt_blob, offset,
>>> +                                     "regulator-max-microamp", -1);
>>> +       info->always_on = fdtdec_get_bool(gd->fdt_blob, offset,
>>> +                                        "regulator-always-on");
>>> +       info->boot_on = fdtdec_get_bool(gd->fdt_blob, offset,
>>> +                                        "regulator-boot-on");
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int regulator_get(char *name, struct udevice **devp)
>>> +{
>>> +       struct dm_regulator_info *info;
>>> +       struct udevice *dev;
>>> +       int ret;
>>> +
>>> +       *devp = NULL;
>>> +
>>> +       for (ret = uclass_first_device(UCLASS_REGULATOR, &dev);
>>> +            dev;
>>> +            ret = uclass_next_device(&dev)) {
>>
>>
>> This will probe all devices. See my suggestion about creating
>> uclass_find_first_device()/uclass_find_next_device() in the next
>> patch.
>>
>> As before, I think this could use a function like
>> uclass_get_device_by_name().
>>
>
> Yes, this is the same. But in this case, there is one more  issue, which is
> the regulator device name.
> Usually after bind -> the dev->name is the same as node name. This is good,
> since it's natural that regulator IC, provides e.g "ldo1", or some other
> device-output name.
> But we would like check the "regulator-name" property. For this patch-set,
> the name is fixed at device probe stage, when dev->ofdata_to_platdata() is
> called, and the regulator constraints, the same as it's name goes to struct
> dm_regulator_info.
>
> I put the dm_regulator_info into uclass priv, because it it uclass-specific,
> the same as struct dm_i2c_bus is specific for i2c buses.
>
> But, the ucalss priv is allocated at device probe stage.
>
> I can't use the .per_child_platdata_auto_alloc_size, since the parent is a
> pmic, and its uclass type is different.
>
> Actually I could, move the dm_regulator_info only to device platdata, but
> then, the drivers should take care of this uclass-specific structure
> allocation.
> Is it acceptable?
>
> But then, also ambiguous seem to be filling platdata (struct
> dm_regulator_info) in uclass post_bind() method.
>
> So then, maybe reasonable is:
> - move dm_regulator_info from dev->uclass_priv to dev->platdata - is
> allocated after device bind
>
> - add .post_bind() method to regulator uclass, and get the "regulator-name"
> in it - only
>
> - fill all platdata constraints on call to dev->ofdata_to_platdata()
>
> Then, I could avoid probing every device, when checking the regulator name.
> But, still I can't use the uclass.c functions, since I'm checking the
> regulator info at dev->platdata.
>
> So I update the function as below:
>
> + uclass_foreach_dev(dev, uc) {
> +       if (!dev->platdata)
> +               continue;
> +
> +       info = dev->platdata;
> +       if (!strcmp(name, info->name)) {
> +               ret = device_probe(dev);
> +               if (ret)
> +                              ....
> +                       *regulator = dev;
> +                       return ret;
> +               }
> +       }
>
>

The problem here is similar to I2C which uses per-child platdata
(specified by the uclass) for the bus address. This is different from
device platdata. I think you are suggesting that we should support
uclass platdata. In this case we would have for each device:

- device platform data
- parent platform data
- uclass platform data

The last one is not supported. I have through several times about
adding it. The alternative is to require each device to provide a
platform data struct at the start of its own platform data, which the
uclass can find and use. This is not really in the spirit of driver
model. But for now this is what I have done with USB (see
dm/usb-working). struct usb_platdata appears at the start of struct
exynos_ehci_platdata but is owned by the uclass.

If PMIC has use for uclass platform data, then perhaps that would be
enough users to warrant it. You could add a patch to do this (don't
forget to update tests) or you could do what I have done with USB and
we can tidy it up later.

Re your naming problem, apart from case the device name and
regulator-compatible name are the same. So perhaps just use
case-insensitive search for regulators? But if not then I take back my
comment about using a common function for searching for regulator
names. You are really searching the platform data for the
regulator-compatible string, not the device name, and so the common
function cannot be used.

[snip]

Regards,
Simon