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

[Simon Glass]

Hi Przemyslaw,

On 7 April 2015 at 09:31, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> 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 I2C bus uses UCLASS_I2C. The chips use a different UCLASS. If
there is no specific driver for the chip then we will use
UCLASS_I2C_GENERIC, but in general it could be anything. So perhaps
there is no difference here?

Per-child platdata works for I2C buses because its children are all
I2C chips, whatever their uclass.

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

OK thanks. As a reminded, please continue to rebase on dm/next.

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

OK.

Regards,
Simon