From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Thu, 25 Mar 2021 10:20:56 -0400 Subject: [PATCH v2 1/2] power: regulator: add driver for ANATOP regulator In-Reply-To: <058eec11-4d4f-e6b9-a1a4-05106979a59b@linaro.org> References: <20210323134836.99512-1-grandpaul@gmail.com> <20210323134836.99512-2-grandpaul@gmail.com> <872fc093-626d-8324-fa2c-38d12714329f@seco.com> <058eec11-4d4f-e6b9-a1a4-05106979a59b@linaro.org> Message-ID: <3bcf73f3-cb19-1980-f716-048171b75583@seco.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 3/23/21 10:01 PM, Ying-Chun Liu (PaulLiu) wrote: > Hi Sean, > > Thanks for the review. I fix almost of the issues. Will upload the v3 soon. > > Still have some questions. > > > Sean Anderson ? 2021/3/23 ??11:06 ??: >> >> >> >> ????if (anatop_reg->supply) { >> ??????? ret = regulator_set_value(anatop_reg->supply, uV + 150000); >> ??????? if (ret) >> ??????????? return ret; >> ????} >> > > What is 150000? Is it the min_dropout_uV? Yes. > > Should I set it to 125000 instead? Yes. > > > >> >> >> >> ????ret = regulator_set_enable(sreg->supply, true); >> ????if (ret) >> ??????? return ret; >> > > Since vin-supply is optional, I change it to > > ??????? ret = device_get_supply_regulator(dev, "vin-supply", > ????????????????????????????????????????? &anatop_reg->supply); > ??????? if (!ret) { > ??????????????? ret = regulator_set_enable(anatop_reg->supply, true); > ??????????????? if (ret) > ??????????????????????? return ret; > ??????? } > > Is this ok? Yes. --Sean > >>> + >>> +??? ret = ofnode_read_u32(dev_ofnode(dev), >> >>> +U_BOOT_DRIVER(anatop_regulator) = { >>> +??? .name = "anatop_regulator", >>> +??? .id = UCLASS_REGULATOR, >>> +??? .ops = &anatop_regulator_ops, >>> +??? .of_match = of_anatop_regulator_match_tbl, >>> +??? .plat_auto = sizeof(struct anatop_regulator), >>> +??? .probe = anatop_regulator_probe, >>> +}; >>> > > > Yours, > Paul >