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

[Przemyslaw Marczak]

Hello Simon,

On 04/05/2015 08:30 PM, Simon Glass wrote:
> 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

Yes, but note, that the uclass type is the same for I2C bus and i2c 
chip. This is a different than for the PMIC, for which childs uclass 
type are usually different than for the parent.
In this case I can't use the field per-child-platdata.

>
> 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.

The example of usb is good enough. I could move the "dm_regulator_info" 
into the dm_regulator_platdata, and add "void *dev_platdata" at the end 
of it.
 From the other side, the regulator constraints and modes, are all what 
we need for this uclass.
We have also the fixed-regulators which some platform data are the same 
as for the generic regulators - then the dev->uclass_platdata is 
reasonable for this purpose.

I will add the uclass platdata to struct udevice and also some test to 
test/dm drivers. I will send it as a separate patch.

>
> 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.

Then we could provide two functions:
- regulator_by_devname() - search for device -> name
- regulator_by_platname() - search for platdata -> name

>
> [snip]
>
> Regards,
> Simon
>

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com