From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Wed, 08 Apr 2015 09:37:30 +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> <5523F868.6060605@samsung.com> Message-ID: <5524DABA.70709@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/08/2015 03:47 AM, Simon Glass wrote: > Hi Przemyslaw, > > On 7 April 2015 at 09:31, Przemyslaw Marczak wrote: >> 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 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