MAX8952 PMIC Driver Initial Release
Subsystems (2)
PATCH 1/1 MAX8952 PMIC Driver Initial Release
MAX8952 PMIC is used to provide VDD_ARM for S5PC210/UNIVERSAL board. In this initial release, users can set voltages for four DVS modes, RAMP delay values, and SYNC frequency. Controlling FPWM/SYNC_MODE/Pull-Down/ Ramp Modes and reading CHIP_ID is not supported in this release.
If MAX8952's EN is not going to be changed by this driver, the user should set gpio_en_writable as false. In S5PC210/UNIVERSL board, controlling EN pin at MAX8952 driver can be dangerous because this gpio pin is shared with another PMIC.
We assume that V_OUT is capable to provide every voltage from 770mV to 1.40V in 10mV steps although the data sheet has some ambiguity on it.
--- drivers/regulator/Kconfig | 8 + drivers/regulator/Makefile | 1 + drivers/regulator/max8952.c | 374 +++++++++++++++++++++++++++++++++++++ include/linux/regulator/max8952.h | 141 ++++++++++++++ 4 files changed, 524 insertions(+), 0 deletions(-) create mode 100644 drivers/regulator/max8952.c create mode 100644 include/linux/regulator/max8952.h
drivers/regulator/Kconfig +8 -0
@@ -100,6 +100,14 @@ config REGULATOR_MAX8925 help Say y here to support the voltage regulaltor of Maxim MAX8925 PMIC. +config REGULATOR_MAX8952 + tristate "Maxim MAX8952 Power Management IC" + depends on I2C + help + This driver controls a Maxim 8952 voltage output regulator + via I2C bus. Maxim 8952 has one voltage output and supports 4 DVS + modes ranging from 0.77V to 1.40V by 0.01V steps. + config REGULATOR_MAX8998 tristate "Maxim 8998 voltage regulator" depends on MFD_MAX8998
drivers/regulator/Makefile +1 -0
@@ -17,6 +17,7 @@ obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o +obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o
drivers/regulator/max8952.c new file +374 -0
374 lines not on this page · raw message
include/linux/regulator/max8952.h new file +141 -0
141 lines not on this page · raw message
Signature · 1 line
1.6.3.3
PATCH 1/1 MAX8952 PMIC Driver Initial Release
Quoted 4 lines · MyungJoo Ham
If MAX8952's EN is not going to be changed by this driver, the user should set
gpio_en_writableas false. In S5PC210/UNIVERSL board, controlling EN pin at MAX8952 driver can be dangerous because this gpio pin is shared with another PMIC.
If you're including this sort of board specific comment in the driver for a generic chip like a regulator it should be a bit of a warning sign -
drivers/regulator/max8952.c line 61
Quoted 7 lines
drivers/regulator/max8952.c new file +7
-0+static int max8952_i2c_read (struct i2c_client *client,u8 reg,u8 *dest) +{ +int ret; + + ret =i2c_smbus_read_byte_data (client, reg); +if (ret <0 ) +return -EIO ;
If you've got an error code back you should return it directly rather than replacing it with -EIO.
drivers/regulator/max8952.c line 85
Quoted 10 lines
drivers/regulator/max8952.c new file +10
-0+static int max8952_read_reg (struct max8952_data *max8952,u8 reg) +{ +u8 value =0 ; +int ret; + +mutex_lock (&max8952->mutex); + ret =max8952_i2c_read (max8952->client, reg, &value); +mutex_unlock (&max8952->mutex); +if (!ret) + ret = value &0xff ;
I don't see any need for the lock here - you're doing simple SMBus transactions which should already be atomic. Locks are needed when you're doing multiple transactions in your driver but that's not the case at this level.
drivers/regulator/max8952.c line 123
Quoted 6 lines
drivers/regulator/max8952.c new file +6
-0+static int max8952_is_enabled (struct regulator_dev *rdev) +{ +struct max8952_data *max8952 =rdev_get_drvdata (rdev); + +if (gpio_is_valid (max8952->pdata->gpio_en)) +return gpio_get_value (max8952->pdata->gpio_en);
You're not allowed to read the status of output GPIOs by the gpiolib API, you need to remember what you set it to.
drivers/regulator/max8952.c line 126
Quoted 3 lines
drivers/regulator/max8952.c new file +3
-0+ +return -ENXIO ; +}
If there's no GPIO I'd expect the driver to report that the regulator is always enabled since the most likely configuration is that the enable is latched high. Otherwise users will get confused.
drivers/regulator/max8952.c line 168
Quoted 11 lines
drivers/regulator/max8952.c new file +11
-0+static int max8952_get_voltage (struct regulator_dev *rdev) +{ +struct max8952_data *max8952 =rdev_get_drvdata (rdev); +u8 vid =0 ; + +if (gpio_is_valid (max8952->pdata->gpio_vid0) && +gpio_is_valid (max8952->pdata->gpio_vid1)) { +if (gpio_get_value (max8952->pdata->gpio_vid0)) + vid +=1 ; +if (gpio_get_value (max8952->pdata->gpio_vid1)) + vid +=2 ;
Again, you can't read GPIOs that you're using for output.
Quoted 4 lines
drivers/regulator/max8952.c new file +4
-0+static int max8952_set_voltage (struct regulator_dev *rdev, +int min_uV,int max_uV) +{ +struct max8952_data *max8952 =rdev_get_drvdata (rdev);
drivers/regulator/max8952.c line 188
Quoted 10 lines
drivers/regulator/max8952.c new file +10
-0+u8 vid =-1 , i; + +for (i =0 ; i <MAX8952_NUM_DVS_MODE ; i++) { +int volt =max8952_voltage (max8952, i); + +/* Set the voltage as low as possible within the range */ +if ((volt <= max_uV) && (volt >= min_uV)) +if ((vid ==-1 ) || + (max8952_voltage (max8952, vid) > volt)) + vid = i;
The indentation here is badly confused - the second branch of the or is massively indented.
drivers/regulator/max8952.c line 203
Quoted 4 lines
drivers/regulator/max8952.c new file +4
-0+static int max8952_do_nothing (struct regulator_dev *rdev) +{ +return 0 ; +}
Why have you done this?
drivers/regulator/max8952.c line 213
Quoted 2 lines
drivers/regulator/max8952.c new file +2
-0+ .set_suspend_enable = max8952_do_nothing, + .set_suspend_disable = max8952_disable,
If the chip has no explicit suspend mode control then remove these.
drivers/regulator/max8952.c line 234
Quoted 1 line
drivers/regulator/max8952.c new file +1
-0+pr_info ("MAX8952 Probing...\n" );
Remove this.
drivers/regulator/max8952.c line 252
Quoted 5 lines
drivers/regulator/max8952.c new file +5
-0+if (pdata->gpio_en_writable == false) { + max8952_ops.enable =NULL ; + max8952_ops.disable =NULL ; + max8952_ops.set_suspend_disable = max8952_do_nothing; + }
This is going to break if you have more than one of these regulators in the system.
drivers/regulator/max8952.c line 264
Quoted 3 lines
drivers/regulator/max8952.c new file +3
-0+ ret =IS_ERR (max8952->rdev); +if (ret) +dev_err (max8952->dev,"regulator init failed\n" );
Print the error code too.
drivers/regulator/max8952.c line 303
Quoted 3 lines
drivers/regulator/max8952.c new file +3
-0+ }else { +pr_err ("MAX8952 Initilization Failed: Wrong GPIO setting.\n" ); +return -EINVAL ;
dev_err().
drivers/regulator/max8952.c line 311
Quoted 6 lines
drivers/regulator/max8952.c new file +6
-0+max8952_write_reg (max8952,MAX8952_REG_SYNC , + (max8952_read_reg (max8952,MAX8952_REG_SYNC ) &0x3F ) | + ((pdata->sync_freq &0x3 ) <<6 )); +max8952_write_reg (max8952,MAX8952_REG_RAMP , + (max8952_read_reg (max8952,MAX8952_REG_RAMP ) &0x1F ) | + ((pdata->ramp_speed &0x7 ) <<5 ));
This looks like it should be platform data.
drivers/regulator/max8952.c line 335
Quoted 1 line
drivers/regulator/max8952.c new file +1
-0+i2c_set_clientdata (client,NULL );
Not needed.
drivers/regulator/max8952.c line 364
Quoted 7 lines
drivers/regulator/max8952.c new file +7
-0+#ifdef CONFIG_REGULATOR_RESUME +/* becasue alway_power_on, add this feature, +* To do this at bootloader is better */ +beforeresume_initcall (max8952_pmic_init); +#else +subsys_initcall (max8952_pmic_init); +#endif
There's no CONFIG_REGULATOR_RESUME in mainline.
include/linux/regulator/max8952.h line 129
Quoted 10 lines
include/linux/regulator/max8952.h new file +10
-0+struct max8952_platform_data { +int gpio_vid0; +int gpio_vid1; +int gpio_en; + +/* +* In C210 Universal Board, EN is connected to PWR_EN, which can also +* turn off the whole system. In such a case, disable en_writable. +*/ +bool gpio_en_writable;
This should not be in the driver - any regulator could be used in a fashion that's critical to system operation and the regulator API always_on constraint will do this for you.
PATCH 1/1 MAX8952 PMIC Driver Initial Release
Thank you for the comments. They will be applied to the next patch revision releasing soon.
Quoted 2 lines · Mark Brown
On Thu, Aug 19, 2010 at 04:05:15PM +0900, MyungJoo Ham wrote:
(snip)
Quoted 9 lines
Quoted 7 lines
+ ? ? max8952_write_reg(max8952,
MAX8952_REG_SYNC,
+ ? ? ? ? ? ? ? ? ? ? (max8952_read_reg(max8952, MAX8952_REG_SYNC)&0x3F) |
+ ? ? ? ? ? ? ? ? ? ? ((pdata->sync_freq&0x3) << 6));
+ ? ? max8952_write_reg(max8952,MAX8952_REG_RAMP,
+ ? ? ? ? ? ? ? ? ? ? (max8952_read_reg(max8952, MAX8952_REG_RAMP)&0x1F) |
+ ? ? ? ? ? ? ? ? ? ? ((pdata->ramp_speed&0x7) << 5));This looks like it should be platform data.
However, could you please clarify more about this? They are already from platform data (pdata->ramp_speed and pdata->sync_freq).
Signature · 5 lines
MyungJoo Ham (???), Ph.D. Mobile Software Platform Lab, Digital Media and Communications (DMC) Business Samsung Electronics cell: 82-10-6714-2858
PATCH 1/1 MAX8952 PMIC Driver Initial Release
Quoted 2 lines · MyungJoo Ham
However, could you please clarify more about this? They are already from platform data (
pdata->ramp_speedandpdata->sync_freq).
Sorry, I misread - the code is fine.