Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[Andy Shevchenko]

On Thu, Nov 5, 2015 at 3:34 PM, Chen Feng  wrote:
> Add driver support for HiSilicon Hi655x voltage regulators.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> +   unsigned int value = 0;
> +
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   regmap_read(rdev->regmap, ctrl_regs->status_reg, );
> +   return (value & BIT(regulator->ctrl_mask));
> +}
> +
> +static int hi655x_enable(struct regulator_dev *rdev)
> +{
> +   int ret = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   ret = regmap_update_bits(rdev->regmap, ctrl_regs->enable_reg,
> +regulator->ctrl_mask, regulator->ctrl_mask);
> +   return ret;
> +}
> +
> +static int hi655x_disable(struct regulator_dev *rdev)
> +{
> +   int ret = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   ret = regmap_update_bits(rdev->regmap, ctrl_regs->disable_reg,
> +regulator->ctrl_mask, regulator->ctrl_mask);
> +   return ret;
> +}
> +
> +static int hi655x_get_voltage(struct regulator_dev *rdev)
> +{
> +   unsigned int value = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +   struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
> +
> +   regmap_read(rdev->regmap, vset_regs->vset_reg, );
> +
> +   return regulator->vset_table[value];
> +}
> +
> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> + int min_uV, int max_uV, unsigned *selector)
> +{
> +   int i = 0;
> +   int ret = 0;
> +   int vol = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +
> +   struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
> +
> +   /**
> +* search the matched vol and get its index
> +*/
> +   for (i = 0; i < regulator->vol_numb; i++) {
> +   vol = regulator->vset_table[i];
> +   if ((vol >= min_uV) && (vol <= max_uV))
> +   break;
> +   }
> +
> +   if (i == regulator->vol_numb)
> +   return -1;
> +
> +   regmap_update_bits(rdev->regmap, vset_regs->vset_reg,
> +  regulator->vset_mask, i);
> +   *selector = i;
> +
> +   return ret;
> +}
> +
> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> +   /* hi655x pmic on hi6220 SoC only support normal mode */
> +   if (mode == REGULATOR_MODE_NORMAL)
> +   return REGULATOR_MODE_NORMAL;
> +   else
> +   return -EINVAL;
> +}
> +
> +static int hi655x_set_mode(struct regulator_dev *rdev,
> +  unsigned int mode)
> +
> +{
> +   if (mode == REGULATOR_MODE_NORMAL)
> +   return 0;
> +   else
> +   return -EINVAL;
> +}
> +
> +static struct regulator_ops hi655x_regulator_ops = {
> +   .is_enabled = hi655x_is_enabled,
> +   .enable = hi655x_enable,
> +   .disable = hi655x_disable,
> +   .list_voltage = regulator_list_voltage_table,
> +   .get_voltage = hi655x_get_voltage,
> +   .set_voltage = hi655x_set_voltage,
> +   .set_mode = hi655x_set_mode,
> +};
> +
> +static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
> +   {
> +   .compatible = "hisilicon,hi655x-regulator-pmic",
> +   },
> +};
> +MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
> +
> +/**
> + * get the hi655x specific data from dt node.
> + */
> +static void of_get_hi655x_ctr(struct hi655x_regulator *regulator,
> + struct device *dev, struct device_node *np)
> +{
> +   unsigned int *vset_table = NULL;
> +
> +   of_property_read_u32_array(np, "regulator-ctrl-regs",

device property API?

> +  (u32 *)>ctrl_regs, 0x3);
> +   of_property_read_u32(np, "regulator-ctrl-mask", 
> >ctrl_mask);
> +   of_property_read_u32(np, "regulator-vset-regs",
> +(u32 *)>vset_regs);
> +   of_property_read_u32(np, "regulator-vset-mask", 

Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[kbuild test robot]

Hi Chen,

[auto build test ERROR on regulator/for-next]
[also build test ERROR on v4.3 next-20151106]

url:
https://github.com/0day-ci/linux/commits/Chen-Feng/Add-Support-for-Hi6220-PMIC-Hi6553-MFD-Core/20151105-215603
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 
for-next
config: arm-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   drivers/regulator/hi6220-mtcmos.c: In function 'hi6220_mtcmos_enable':
>> drivers/regulator/hi6220-mtcmos.c:105:2: error: expected ';' before 'return'
 return ret;
 ^
>> drivers/regulator/hi6220-mtcmos.c:106:1: warning: no return statement in 
>> function returning non-void [-Wreturn-type]
}
^
--
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_disable':
>> drivers/regulator/hi655x-regulator.c:65:2: warning: ISO C90 forbids mixed 
>> declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_get_voltage':
   drivers/regulator/hi655x-regulator.c:81:2: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_set_voltage':
   drivers/regulator/hi655x-regulator.c:101:2: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_regulator_probe':
   drivers/regulator/hi655x-regulator.c:189:6: warning: unused variable 'ret' 
[-Wunused-variable]
 int ret = 0;
 ^

vim +65 drivers/regulator/hi655x-regulator.c

49  struct hi655x_regulator_ctrl_regs *ctrl_regs = 
>ctrl_regs;
50  
51  ret = regmap_update_bits(rdev->regmap, ctrl_regs->enable_reg,
52   regulator->ctrl_mask, 
regulator->ctrl_mask);
53  return ret;
54  }
55  
56  static int hi655x_disable(struct regulator_dev *rdev)
57  {
58  int ret = 0;
59  struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
60  
61  if (!regulator) {
62  pr_err("get driver data error!\n");
63  return -ENODEV;
64  }
  > 65  struct hi655x_regulator_ctrl_regs *ctrl_regs = 
>ctrl_regs;
66  
67  ret = regmap_update_bits(rdev->regmap, ctrl_regs->disable_reg,
68   regulator->ctrl_mask, 
regulator->ctrl_mask);
69  return ret;
70  }
71  
72  static int hi655x_get_voltage(struct regulator_dev *rdev)
73  {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[kbuild test robot]

Hi Chen,

[auto build test ERROR on regulator/for-next]
[also build test ERROR on v4.3 next-20151106]

url:
https://github.com/0day-ci/linux/commits/Chen-Feng/Add-Support-for-Hi6220-PMIC-Hi6553-MFD-Core/20151105-215603
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 
for-next
config: arm-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   drivers/regulator/hi6220-mtcmos.c: In function 'hi6220_mtcmos_enable':
>> drivers/regulator/hi6220-mtcmos.c:105:2: error: expected ';' before 'return'
 return ret;
 ^
>> drivers/regulator/hi6220-mtcmos.c:106:1: warning: no return statement in 
>> function returning non-void [-Wreturn-type]
}
^
--
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_disable':
>> drivers/regulator/hi655x-regulator.c:65:2: warning: ISO C90 forbids mixed 
>> declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_get_voltage':
   drivers/regulator/hi655x-regulator.c:81:2: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_set_voltage':
   drivers/regulator/hi655x-regulator.c:101:2: warning: ISO C90 forbids mixed 
declarations and code [-Wdeclaration-after-statement]
 struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
 ^
   drivers/regulator/hi655x-regulator.c: In function 'hi655x_regulator_probe':
   drivers/regulator/hi655x-regulator.c:189:6: warning: unused variable 'ret' 
[-Wunused-variable]
 int ret = 0;
 ^

vim +65 drivers/regulator/hi655x-regulator.c

49  struct hi655x_regulator_ctrl_regs *ctrl_regs = 
>ctrl_regs;
50  
51  ret = regmap_update_bits(rdev->regmap, ctrl_regs->enable_reg,
52   regulator->ctrl_mask, 
regulator->ctrl_mask);
53  return ret;
54  }
55  
56  static int hi655x_disable(struct regulator_dev *rdev)
57  {
58  int ret = 0;
59  struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
60  
61  if (!regulator) {
62  pr_err("get driver data error!\n");
63  return -ENODEV;
64  }
  > 65  struct hi655x_regulator_ctrl_regs *ctrl_regs = 
>ctrl_regs;
66  
67  ret = regmap_update_bits(rdev->regmap, ctrl_regs->disable_reg,
68   regulator->ctrl_mask, 
regulator->ctrl_mask);
69  return ret;
70  }
71  
72  static int hi655x_get_voltage(struct regulator_dev *rdev)
73  {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[Andy Shevchenko]

On Thu, Nov 5, 2015 at 3:34 PM, Chen Feng  wrote:
> Add driver support for HiSilicon Hi655x voltage regulators.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> +   unsigned int value = 0;
> +
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   regmap_read(rdev->regmap, ctrl_regs->status_reg, );
> +   return (value & BIT(regulator->ctrl_mask));
> +}
> +
> +static int hi655x_enable(struct regulator_dev *rdev)
> +{
> +   int ret = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   ret = regmap_update_bits(rdev->regmap, ctrl_regs->enable_reg,
> +regulator->ctrl_mask, regulator->ctrl_mask);
> +   return ret;
> +}
> +
> +static int hi655x_disable(struct regulator_dev *rdev)
> +{
> +   int ret = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +   struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> +   ret = regmap_update_bits(rdev->regmap, ctrl_regs->disable_reg,
> +regulator->ctrl_mask, regulator->ctrl_mask);
> +   return ret;
> +}
> +
> +static int hi655x_get_voltage(struct regulator_dev *rdev)
> +{
> +   unsigned int value = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +   struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
> +
> +   regmap_read(rdev->regmap, vset_regs->vset_reg, );
> +
> +   return regulator->vset_table[value];
> +}
> +
> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> + int min_uV, int max_uV, unsigned *selector)
> +{
> +   int i = 0;
> +   int ret = 0;
> +   int vol = 0;
> +   struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +   if (!regulator) {
> +   pr_err("get driver data error!\n");
> +   return -ENODEV;
> +   }
> +
> +   struct hi655x_regulator_vset_regs *vset_regs = >vset_regs;
> +
> +   /**
> +* search the matched vol and get its index
> +*/
> +   for (i = 0; i < regulator->vol_numb; i++) {
> +   vol = regulator->vset_table[i];
> +   if ((vol >= min_uV) && (vol <= max_uV))
> +   break;
> +   }
> +
> +   if (i == regulator->vol_numb)
> +   return -1;
> +
> +   regmap_update_bits(rdev->regmap, vset_regs->vset_reg,
> +  regulator->vset_mask, i);
> +   *selector = i;
> +
> +   return ret;
> +}
> +
> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> +   /* hi655x pmic on hi6220 SoC only support normal mode */
> +   if (mode == REGULATOR_MODE_NORMAL)
> +   return REGULATOR_MODE_NORMAL;
> +   else
> +   return -EINVAL;
> +}
> +
> +static int hi655x_set_mode(struct regulator_dev *rdev,
> +  unsigned int mode)
> +
> +{
> +   if (mode == REGULATOR_MODE_NORMAL)
> +   return 0;
> +   else
> +   return -EINVAL;
> +}
> +
> +static struct regulator_ops hi655x_regulator_ops = {
> +   .is_enabled = hi655x_is_enabled,
> +   .enable = hi655x_enable,
> +   .disable = hi655x_disable,
> +   .list_voltage = regulator_list_voltage_table,
> +   .get_voltage = hi655x_get_voltage,
> +   .set_voltage = hi655x_set_voltage,
> +   .set_mode = hi655x_set_mode,
> +};
> +
> +static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
> +   {
> +   .compatible = "hisilicon,hi655x-regulator-pmic",
> +   },
> +};
> +MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
> +
> +/**
> + * get the hi655x specific data from dt node.
> + */
> +static void of_get_hi655x_ctr(struct hi655x_regulator *regulator,
> + struct device *dev, struct device_node *np)
> +{
> +   unsigned int *vset_table = NULL;
> +
> +   of_property_read_u32_array(np, "regulator-ctrl-regs",

device property API?

> +  (u32 *)>ctrl_regs, 0x3);
> +   of_property_read_u32(np, "regulator-ctrl-mask", 
> >ctrl_mask);
> +   of_property_read_u32(np, "regulator-vset-regs",
> +(u32 *)>vset_regs);
> +   

Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[Mark Brown]

On Thu, Nov 05, 2015 at 09:34:47PM +0800, Chen Feng wrote:

> +config REGULATOR_HI6220_MTCMOS
> +bool "Hisilicon Hi6220 mtcmos support"
> +depends on ARCH_HISI
> +help
> +  This driver provides support for the mtcmos regulators of Hi6220 
> Soc.
> +

The Kconfig and Makefile updates for MCTMOS should have been in the
patch adding the driver for that.

> +config REGULATOR_HI655X
> +bool "HiSilicon Hi655x PMIC voltage regulator support"
> +depends on ARCH_HISI

For both of these we should have an || COMPILE_TEST and there's no need
for either to be bool I can see, they should be tristate.

> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> + unsigned int value = 0;
> +
> + struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> + struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> + regmap_read(rdev->regmap, ctrl_regs->status_reg, );
> + return (value & BIT(regulator->ctrl_mask));
> +}

Use the standard regmap helpers, don't open code them.

> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> +   int min_uV, int max_uV, unsigned *selector)

Use the standard helpers, including one of the map_voltage()s and
set_voltage_sel_regmap(), don't open code them.

> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> + /* hi655x pmic on hi6220 SoC only support normal mode */
> + if (mode == REGULATOR_MODE_NORMAL)
> + return REGULATOR_MODE_NORMAL;
> + else
> + return -EINVAL;
> +}

If the device only has one mode it should not have any mode operations,
they're only meaningful if there are multiple modes to set and they are
optional.


signature.asc
Description: PGP signature

Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[Mark Brown]

On Thu, Nov 05, 2015 at 09:34:47PM +0800, Chen Feng wrote:

> +config REGULATOR_HI6220_MTCMOS
> +bool "Hisilicon Hi6220 mtcmos support"
> +depends on ARCH_HISI
> +help
> +  This driver provides support for the mtcmos regulators of Hi6220 
> Soc.
> +

The Kconfig and Makefile updates for MCTMOS should have been in the
patch adding the driver for that.

> +config REGULATOR_HI655X
> +bool "HiSilicon Hi655x PMIC voltage regulator support"
> +depends on ARCH_HISI

For both of these we should have an || COMPILE_TEST and there's no need
for either to be bool I can see, they should be tristate.

> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> + unsigned int value = 0;
> +
> + struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> + struct hi655x_regulator_ctrl_regs *ctrl_regs = >ctrl_regs;
> +
> + regmap_read(rdev->regmap, ctrl_regs->status_reg, );
> + return (value & BIT(regulator->ctrl_mask));
> +}

Use the standard regmap helpers, don't open code them.

> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> +   int min_uV, int max_uV, unsigned *selector)

Use the standard helpers, including one of the map_voltage()s and
set_voltage_sel_regmap(), don't open code them.

> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> + /* hi655x pmic on hi6220 SoC only support normal mode */
> + if (mode == REGULATOR_MODE_NORMAL)
> + return REGULATOR_MODE_NORMAL;
> + else
> + return -EINVAL;
> +}

If the device only has one mode it should not have any mode operations,
they're only meaningful if there are multiple modes to set and they are
optional.


signature.asc
Description: PGP signature