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

Przemyslaw Marczak
2015-04-07T15:31:52Z

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