| Message ID | 20240504194632.2456-1-balejk@matfyz.cz |
|---|---|
| Headers | show |
| Series | initial support for Marvell 88PM886 PMIC | expand |
On Sat, May 04, 2024 at 09:37:06PM +0200, Karel Balej wrote: > +static const struct regulator_ops pm886_ldo_ops = { > + .list_voltage = regulator_list_voltage_table, > + .map_voltage = regulator_map_voltage_iterate, > + .set_voltage_sel = regulator_set_voltage_sel_regmap, > + .get_voltage_sel = regulator_get_voltage_sel_regmap, > + .enable = regulator_enable_regmap, > + .disable = regulator_disable_regmap, > + .is_enabled = regulator_is_enabled_regmap, > + .get_current_limit = pm886_regulator_get_ilim, Do these regulators actually enforce this limit or is this just a spec limit beyond which regulation may fail? If it's just a spec limit I'd not expect this operation to be provided, it's more for a hard limit where the regulator will detect and act on issues. I don't see an error interrupt or anything and this would be an unusual feature for a LDO.
Mark Brown, 2024-05-06T00:15:01+09:00: > On Sat, May 04, 2024 at 09:37:06PM +0200, Karel Balej wrote: > > > +static const struct regulator_ops pm886_ldo_ops = { > > + .list_voltage = regulator_list_voltage_table, > > + .map_voltage = regulator_map_voltage_iterate, > > + .set_voltage_sel = regulator_set_voltage_sel_regmap, > > + .get_voltage_sel = regulator_get_voltage_sel_regmap, > > + .enable = regulator_enable_regmap, > > + .disable = regulator_disable_regmap, > > + .is_enabled = regulator_is_enabled_regmap, > > + .get_current_limit = pm886_regulator_get_ilim, > > Do these regulators actually enforce this limit or is this just a spec > limit beyond which regulation may fail? If it's just a spec limit I'd > not expect this operation to be provided, it's more for a hard limit > where the regulator will detect and act on issues. I don't see an error > interrupt or anything and this would be an unusual feature for a LDO. I'm afraid I don't have the answer -- my only reference is the vendor version of the driver and I don't see anything there based on which I would be able to tell. But based on what you write, my guess would be that it's just a spec limit. Should I then drop this op and the max_uA values from all the regulators? Thank you, K. B.
On Sun, May 05, 2024 at 08:52:06PM +0200, Karel Balej wrote: > Should I then drop this op and the max_uA values from all the > regulators? Probably, yes.
On Sat, 04 May 2024, Karel Balej wrote: > Marvell 88PM886 is a PMIC which provides various functions such as > onkey, battery, charger and regulators. It is found for instance in the > samsung,coreprimevelte smartphone with which this was tested. Implement > basic support to allow for the use of regulators and onkey. > > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > > Notes: > v6: > - Address Lee's comments: > - Don't break long line in the power off handler. > - Set PLATFORM_DEVID_NONE. This should be safe with respect to > collisions since there are no known devices with more than one of > these PMICs, plus given their general purpose nature it is unlikely > that there would ever be. Also include the corresponding header. > - Move all defines to the header. > - Give the base page's maximum register its real name. > - Set irq_base to 0. > v5: > - Address Mark's feedback: > - Move regmap config back out of the header and rename it. Also lower > its maximum register based on what's actually used in the downstream > code. > RFC v4: > - Use MFD_CELL_* macros. > - Address Lee's feedback: > - Do not define regmap_config.val_bits and .reg_bits. > - Drop everything regulator related except mfd_cell (regmap > initialization, IDs enum etc.). Drop pm886_initialize_subregmaps. > - Do not store regmap pointers as an array as there is now only one > regmap. Also drop the corresponding enum. > - Move regmap_config to the header as it is needed in the regulators > driver. > - pm886_chip.whoami -> chip_id > - Reword chip ID mismatch error message and print the ID as > hexadecimal. > - Fix includes in include/linux/88pm886.h. > - Drop the pm886_irq_number enum and define the (for the moment) only > IRQ explicitly. > - Have only one MFD cell for all regulators as they are now registered > all at once in the regulators driver. > - Reword commit message. > - Make device table static and remove comma after the sentinel to signal > that nothing should come after it. > RFC v3: > - Drop onkey cell .of_compatible. > - Rename LDO page offset and regmap to REGULATORS. > RFC v2: > - Remove some abstraction. > - Sort includes alphabetically and add linux/of.h. > - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE. > - Use more temporaries and break long lines. > - Do not initialize ret in probe. > - Use the wakeup-source DT property. > - Rename ret to err. > - Address Lee's comments: > - Drop patched in presets for base regmap and related defines. > - Use full sentences in comments. > - Remove IRQ comment. > - Define regmap_config member values. > - Rename data to sys_off_data. > - Add _PMIC suffix to Kconfig. > - Use dev_err_probe. > - Do not store irq_data. > - s/WHOAMI/CHIP_ID > - Drop LINUX part of include guard name. > - Merge in the regulator series modifications in order to have more > devices and modify the commit message accordingly. Changes with > respect to the original regulator series patches: > - ret -> err > - Add temporary for dev in pm88x_initialize_subregmaps. > - Drop of_compatible for the regulators. > - Do not duplicate LDO regmap for bucks. > - Rewrite commit message. > > drivers/mfd/88pm886.c | 148 ++++++++++++++++++++++++++++++++++++ > drivers/mfd/Kconfig | 12 +++ > drivers/mfd/Makefile | 1 + > include/linux/mfd/88pm886.h | 69 +++++++++++++++++ > 4 files changed, 230 insertions(+) > create mode 100644 drivers/mfd/88pm886.c > create mode 100644 include/linux/mfd/88pm886.h I don't see any more issues. Are you planning on seeing to Mark's review comments?
Lee Jones, 2024-05-31T11:24:52+01:00:
> Are you planning on seeing to Mark's review comments?
Indeed, I'm hoping that I will be able to send it over the weekend.
K. B.