From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 07 Apr 2015 17:31:52 +0200 Subject: [U-Boot] [PATCH v3 06/17] dm: regulator: add implementation of driver model regulator uclass In-Reply-To: References: <1425399883-14053-1-git-send-email-p.marczak@samsung.com> <1427229051-20170-1-git-send-email-p.marczak@samsung.com> <1427229051-20170-7-git-send-email-p.marczak@samsung.com> <551EBB36.7060409@samsung.com> Message-ID: <5523F868.6060605@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 04/05/2015 08:30 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 3 April 2015 at 10:09, Przemyslaw Marczak wrote: >> Hello Simon, >> >> >> On 03/29/2015 03:07 PM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 24 March 2015 at 14:30, Przemyslaw Marczak >>> 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