[U-Boot] [PATCH v3 06/17] dm: regulator: add implementation of driver model regulator uclass
Przemyslaw Marczak
p.marczak at samsung.com
Wed Apr 8 09:37:30 CEST 2015
Hello Simon,
On 04/08/2015 03:47 AM, Simon Glass wrote:
> 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.
>
Sorry, I wrote nonsense. I meant something different.
We could use per-child-platdata field for pmic uclass driver with the
assumption, that each pmic's child's uclass is the same type (e.g.
regulator). But this assumption will be broken by fixed-regulator. And
also, pmic's childs are not only regulators - e.g. one can be RTC, other
charger, etc...
>>
>>>
>>> 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.
>
Ok, I will rebase it.
>>
>>>
>>> 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
>
Thanks,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
More information about the U-Boot
mailing list