From: Frieder Schrempf <frieder.schrempf@kontron.de>
There are regulators that use multiple registers for storing the
voltage. Add a get_reg_voltage_sel member to struct regulator_ops in
order to let drivers register a function that returns the currently
used register.
The pca9450 driver will be a user of this as the LDO5 regulator of
that chip uses two different control registers depending on the
state of an external signal.
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
drivers/regulator/helpers.c | 16 ++++++++++++++--
include/linux/regulator/driver.h | 5 +++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index ad2237a95572..e629b0bea3d0 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -223,6 +223,16 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
}
EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_pickable_regmap);
+unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
+{
+ const struct regulator_ops *ops = rdev->desc->ops;
+
+ if (ops->get_reg_voltage_sel)
+ return ops->get_reg_voltage_sel(rdev);
+
+ return rdev->desc->vsel_reg;
+}
+
/**
* regulator_get_voltage_sel_regmap - standard get_voltage_sel for regmap users
*
@@ -234,10 +244,11 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_pickable_regmap);
*/
int regulator_get_voltage_sel_regmap(struct regulator_dev *rdev)
{
+ unsigned int vsel_reg = regulator_get_hwreg_voltage_sel_regmap(rdev);
unsigned int val;
int ret;
- ret = regmap_read(rdev->regmap, rdev->desc->vsel_reg, &val);
+ ret = regmap_read(rdev->regmap, vsel_reg, &val);
if (ret != 0)
return ret;
@@ -260,11 +271,12 @@ EXPORT_SYMBOL_GPL(regulator_get_voltage_sel_regmap);
*/
int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel)
{
+ unsigned int vsel_reg = regulator_get_hwreg_voltage_sel_regmap(rdev);
int ret;
sel <<= ffs(rdev->desc->vsel_mask) - 1;
- ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+ ret = regmap_update_bits(rdev->regmap, vsel_reg,
rdev->desc->vsel_mask, sel);
if (ret)
return ret;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d3b4a3d4514a..c9953b2f63d5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -77,6 +77,10 @@ enum regulator_detection_severity {
* @get_voltage_sel: Return the currently configured voltage selector for the
* regulator; return -ENOTRECOVERABLE if regulator can't
* be read at bootup and hasn't been set yet.
+ * @get_reg_voltage_sel: Return the register used for getting/setting the
+ * voltage of the regulator. This is useful if the
+ * regulator uses multiple registers internally, switched
+ * by some condition like the state of an external signal.
* @list_voltage: Return one of the supported voltages, in microvolts; zero
* if the selector indicates a voltage that is unusable on this system;
* or negative errno. Selectors range from zero to one less than
@@ -168,6 +172,7 @@ struct regulator_ops {
int (*set_voltage_sel) (struct regulator_dev *, unsigned selector);
int (*get_voltage) (struct regulator_dev *);
int (*get_voltage_sel) (struct regulator_dev *);
+ unsigned int (*get_reg_voltage_sel) (struct regulator_dev *);
/* get/set regulator current */
int (*set_current_limit) (struct regulator_dev *,
--
2.39.1On Mon, Feb 13, 2023 at 04:58:22PM +0100, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > There are regulators that use multiple registers for storing the > voltage. Add a get_reg_voltage_sel member to struct regulator_ops in > order to let drivers register a function that returns the currently > used register. > > The pca9450 driver will be a user of this as the LDO5 regulator of > that chip uses two different control registers depending on the > state of an external signal. Aside from the build warnings the bots reported it's not clear to me that it's better to do this than it is to just have these drivers implement appropriate ops directly - there's probably going to be cases when it's a different bitfield in the same register, and by the time you've implemented the op so things aren't completely data driven I'm not sure how much you win by reusing the register read/write.
Hi Mark, On 14.02.23 22:06, Mark Brown wrote: > On Mon, Feb 13, 2023 at 04:58:22PM +0100, Frieder Schrempf wrote: >> From: Frieder Schrempf <frieder.schrempf@kontron.de> >> >> There are regulators that use multiple registers for storing the >> voltage. Add a get_reg_voltage_sel member to struct regulator_ops in >> order to let drivers register a function that returns the currently >> used register. >> >> The pca9450 driver will be a user of this as the LDO5 regulator of >> that chip uses two different control registers depending on the >> state of an external signal. > > Aside from the build warnings the bots reported it's not clear to > me that it's better to do this than it is to just have these > drivers implement appropriate ops directly - there's probably > going to be cases when it's a different bitfield in the same > register, and by the time you've implemented the op so things > aren't completely data driven I'm not sure how much you win by > reusing the register read/write. Thanks for the feedback. Makes sense to me, I can do that. Just to be sure: you are suggesting to to leave the core untouched, use the existing [get/set]_voltage() ops in the driver and reimplement the logic of regulator_[get/set]_voltage_sel_regmap() there, right? Thanks Frieder
On Thu, Feb 16, 2023 at 10:05:49AM +0100, Frieder Schrempf wrote: > Just to be sure: you are suggesting to to leave the core untouched, use > the existing [get/set]_voltage() ops in the driver and reimplement the > logic of regulator_[get/set]_voltage_sel_regmap() there, right? Yes, exactly. If we get a lot of drivers doing this we can always factor out later but it feels premature right now.
Hi Frieder,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on broonie-regulator/for-next]
[also build test WARNING on shawnguo/for-next arm/for-next arm/fixes arm64/for-next/core kvmarm/next rockchip/for-next soc/for-next xilinx-xlnx/master linus/master v6.2-rc8 next-20230214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link: https://lore.kernel.org/r/20230213155833.1644366-5-frieder%40fris.de
patch subject: [PATCH 4/6] regulator: Add operation to let drivers select vsel register
config: i386-randconfig-a003-20230213 (https://download.01.org/0day-ci/archive/20230214/202302141754.N3CvO9lF-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/b76ab45ee4b60334c27d870b6d744a937ff94636
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
git checkout b76ab45ee4b60334c27d870b6d744a937ff94636
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/regulator/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302141754.N3CvO9lF-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/regulator/helpers.c:226:14: warning: no previous prototype for function 'regulator_get_hwreg_voltage_sel_regmap' [-Wmissing-prototypes]
unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
^
drivers/regulator/helpers.c:226:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
^
static
1 warning generated.
vim +/regulator_get_hwreg_voltage_sel_regmap +226 drivers/regulator/helpers.c
225
> 226 unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
227 {
228 const struct regulator_ops *ops = rdev->desc->ops;
229
230 if (ops->get_reg_voltage_sel)
231 return ops->get_reg_voltage_sel(rdev);
232
233 return rdev->desc->vsel_reg;
234 }
235
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Frieder,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on broonie-regulator/for-next]
[also build test WARNING on shawnguo/for-next arm/for-next arm/fixes arm64/for-next/core kvmarm/next rockchip/for-next soc/for-next xilinx-xlnx/master linus/master v6.2-rc8 next-20230213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link: https://lore.kernel.org/r/20230213155833.1644366-5-frieder%40fris.de
patch subject: [PATCH 4/6] regulator: Add operation to let drivers select vsel register
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230214/202302140905.36f6bYXV-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/b76ab45ee4b60334c27d870b6d744a937ff94636
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
git checkout b76ab45ee4b60334c27d870b6d744a937ff94636
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/regulator/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302140905.36f6bYXV-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/regulator/helpers.c:226:14: warning: no previous prototype for 'regulator_get_hwreg_voltage_sel_regmap' [-Wmissing-prototypes]
226 | unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/regulator_get_hwreg_voltage_sel_regmap +226 drivers/regulator/helpers.c
225
> 226 unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
227 {
228 const struct regulator_ops *ops = rdev->desc->ops;
229
230 if (ops->get_reg_voltage_sel)
231 return ops->get_reg_voltage_sel(rdev);
232
233 return rdev->desc->vsel_reg;
234 }
235
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
© 2016 - 2026 Red Hat, Inc.