| Message ID | 1404505467-26526-9-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
I'd really like Linus Walleij to look over this if possible. On Fri, 04 Jul 2014, Javier Martinez Canillas wrote: > Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling > (DVS) support that allows output voltage to change dynamically. > > For MAX77686, these regulators are Buck regulators 2, 3 and 4. > > Each Buck output voltage is selected using a set of external > inputs: DVS1-3 and SELB2-4. > > DVS registers can be used to configure the output voltages for each > Buck regulator and which one is active is controled by DVSx lines. > > SELBx lines are used to control if individual Buck lines are ON or OFF. > > This patch adds support to configure the DVSx and SELBx lines > from DT and to setup and read the GPIO lines connected to them. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > > Changes since v6: > - Add a comment that max77686_read_gpios() function can sleep. > Sugggested by Krzysztof Kozlowski > --- > drivers/mfd/max77686.c | 119 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77686.h | 18 ++++--- > 2 files changed, 129 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c > index 8650832..d193873 100644 > --- a/drivers/mfd/max77686.c > +++ b/drivers/mfd/max77686.c > @@ -32,8 +32,10 @@ > #include <linux/mfd/core.h> > #include <linux/mfd/max77686.h> > #include <linux/mfd/max77686-private.h> > +#include <linux/gpio/consumer.h> > #include <linux/err.h> > #include <linux/of.h> > +#include <linux/export.h> > > #define I2C_ADDR_RTC (0x0C >> 1) > > @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { > {}, > }; > > +static void max77686_dt_parse_dvs_gpio(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int i; > + > + /* > + * NOTE: we don't consider GPIO errors fatal; board may have some lines > + * directly pulled high or low and thus doesn't specify them. > + */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) > + pd->buck_gpio_dvs[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); > + > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) > + pd->buck_gpio_selb[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); > +} Not sure why you've pulled this out for parse_dt_pdata()? > +/** > + * max77686_setup_gpios() - init DVS-related GPIOs > + * @dev: device whose platform data contains the dvs GPIOs information > + * > + * This function claims / initalizations GPIOs related to DVS if they are s/initialzations/initialises > + * defined. This may have the effect of switching voltages if the s/if the/if > + * pdata->buck_default_idx does not match the boot time state of pins. > + */ > +int max77686_setup_gpios(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int buck_default_idx = pd->buck_default_idx; You're saving 4 characters once here. Just use the variable in pd. > + int ret; > + int i; Don't you want some locking around this? > + /* Set all SELB high to avoid glitching while DVS is changing */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; > + > + /* OK if some GPIOs aren't defined */ > + if (IS_ERR(gpio)) > + continue; > + > + ret = gpiod_direction_output_raw(gpio, 1); > + if (ret) { > + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); > + return ret; > + } > + } > + > + /* Set our initial setting */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; > + > + /* OK if some GPIOs aren't defined */ > + if (IS_ERR(gpio)) > + continue; > + > + /* If a GPIO is valid, set it */ > + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); > + if (ret) { > + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); > + return ret; > + } > + } > + > + /* Now set SELB low to take effect */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; > + > + if (!IS_ERR(gpio)) > + gpiod_set_value(gpio, 0); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(max77686_setup_gpios); > + > +/** > + * max77686_read_gpios() - read the current state of the dvs GPIOs > + * @pdata: platform data that contains the dvs GPIOs information > + * > + * We call this function at bootup to detect what slot the firmware was > + * using for the DVS GPIOs. That way we can properly preserve the firmware's > + * voltage settings > + * > + * This function can sleep so must not be called from atomic context. > + */ > +int max77686_read_gpios(struct max77686_platform_data *pdata) > +{ > + int buck_default_idx = pdata->buck_default_idx; Remove this, it adds very little. > + int result = 0; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { > + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; > + > + /* OK if some GPIOs aren't defined; we'll use default */ > + if (IS_ERR(gpio)) { > + result |= buck_default_idx & (1 << i); s/(1 << i)/BIT(i)/ > + continue; > + } > + > + if (gpiod_get_value_cansleep(gpio)) > + result |= 1 << i; Same here. > + } > + > + return result; > +} > +EXPORT_SYMBOL_GPL(max77686_read_gpios); > + > static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device > *dev) > { > + struct device_node *np = dev->of_node; > struct max77686_platform_data *pd; > > pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); > @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device > return NULL; > > dev->platform_data = pd; > + > + /* Read default index and ignore errors, since default is 0 */ > + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", If it doesn't already have one, this needs a DT Ack and associated documentation. > + &pd->buck_default_idx); > + > + max77686_dt_parse_dvs_gpio(dev); Bring code back into here and remove the call. > return pd; > } > > diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h > index 4cbcc13..46a736b 100644 > --- a/include/linux/mfd/max77686.h > +++ b/include/linux/mfd/max77686.h > @@ -99,15 +99,17 @@ struct max77686_platform_data { > struct max77686_opmode_data *opmode_data; > > /* > - * GPIO-DVS feature is not enabled with the current version of > - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default > - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. > + * GPIO-DVS feature is not fully enabled with the current version of > + * MAX77686 driver, but the driver does support using a DVS index other > + * than the default of 0. > */ > - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ > - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ > - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ > - unsigned int buck3_voltage[8]; > - unsigned int buck4_voltage[8]; > + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ > + int buck_default_idx; /* Default value of DVS1, 2, 3 */ > + > + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ > }; > > +extern int max77686_setup_gpios(struct device *dev); > +extern int max77686_read_gpios(struct max77686_platform_data *pdata); > + > #endif /* __LINUX_MFD_MAX77686_H */
Hello Lee, Thanks a lot for your feedback. On 07/09/2014 05:13 PM, Lee Jones wrote: > I'd really like Linus Walleij to look over this if possible. > Ok, I'll cc Linus when posting the next version of the series. > On Fri, 04 Jul 2014, Javier Martinez Canillas wrote: >> Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling >> (DVS) support that allows output voltage to change dynamically. >> >> For MAX77686, these regulators are Buck regulators 2, 3 and 4. >> >> Each Buck output voltage is selected using a set of external >> inputs: DVS1-3 and SELB2-4. >> >> DVS registers can be used to configure the output voltages for each >> Buck regulator and which one is active is controled by DVSx lines. >> >> SELBx lines are used to control if individual Buck lines are ON or OFF. >> >> This patch adds support to configure the DVSx and SELBx lines >> from DT and to setup and read the GPIO lines connected to them. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> --- >> >> Changes since v6: >> - Add a comment that max77686_read_gpios() function can sleep. >> Sugggested by Krzysztof Kozlowski >> --- >> drivers/mfd/max77686.c | 119 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/mfd/max77686.h | 18 ++++--- >> 2 files changed, 129 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c >> index 8650832..d193873 100644 >> --- a/drivers/mfd/max77686.c >> +++ b/drivers/mfd/max77686.c >> @@ -32,8 +32,10 @@ >> #include <linux/mfd/core.h> >> #include <linux/mfd/max77686.h> >> #include <linux/mfd/max77686-private.h> >> +#include <linux/gpio/consumer.h> >> #include <linux/err.h> >> #include <linux/of.h> >> +#include <linux/export.h> >> >> #define I2C_ADDR_RTC (0x0C >> 1) >> >> @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { >> {}, >> }; >> >> +static void max77686_dt_parse_dvs_gpio(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int i; >> + >> + /* >> + * NOTE: we don't consider GPIO errors fatal; board may have some lines >> + * directly pulled high or low and thus doesn't specify them. >> + */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) >> + pd->buck_gpio_dvs[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); >> + >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) >> + pd->buck_gpio_selb[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); >> +} > > Not sure why you've pulled this out for parse_dt_pdata()? > Ok, I'll remove that function and put the code in max77686_i2c_parse_dt_pdata(). >> +/** >> + * max77686_setup_gpios() - init DVS-related GPIOs >> + * @dev: device whose platform data contains the dvs GPIOs information >> + * >> + * This function claims / initalizations GPIOs related to DVS if they are > > s/initialzations/initialises > Right, will fix the typo. >> + * defined. This may have the effect of switching voltages if the > > s/if the/if > Ditto. >> + * pdata->buck_default_idx does not match the boot time state of pins. >> + */ >> +int max77686_setup_gpios(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int buck_default_idx = pd->buck_default_idx; > > You're saving 4 characters once here. Just use the variable in pd. > Ok >> + int ret; >> + int i; > > Don't you want some locking around this? > This function is only called in the max77686 and max77802 regulator drivers probe functions and we expect to have either one of those in a system but never both so this function can't really be executed concurrently. And even in the case that a system has both max77686 and max77802 PMICs, all the data accessed is local to the driver's struct device so I think there is no need to add any locking here. >> + /* Set all SELB high to avoid glitching while DVS is changing */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >> + >> + /* OK if some GPIOs aren't defined */ >> + if (IS_ERR(gpio)) >> + continue; >> + >> + ret = gpiod_direction_output_raw(gpio, 1); >> + if (ret) { >> + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); >> + return ret; >> + } >> + } >> + >> + /* Set our initial setting */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; >> + >> + /* OK if some GPIOs aren't defined */ >> + if (IS_ERR(gpio)) >> + continue; >> + >> + /* If a GPIO is valid, set it */ >> + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); >> + if (ret) { >> + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); >> + return ret; >> + } >> + } >> + >> + /* Now set SELB low to take effect */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >> + >> + if (!IS_ERR(gpio)) >> + gpiod_set_value(gpio, 0); >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(max77686_setup_gpios); >> + >> +/** >> + * max77686_read_gpios() - read the current state of the dvs GPIOs >> + * @pdata: platform data that contains the dvs GPIOs information >> + * >> + * We call this function at bootup to detect what slot the firmware was >> + * using for the DVS GPIOs. That way we can properly preserve the firmware's >> + * voltage settings >> + * >> + * This function can sleep so must not be called from atomic context. >> + */ >> +int max77686_read_gpios(struct max77686_platform_data *pdata) >> +{ >> + int buck_default_idx = pdata->buck_default_idx; > > Remove this, it adds very little. > Ok. >> + int result = 0; >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { >> + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; >> + >> + /* OK if some GPIOs aren't defined; we'll use default */ >> + if (IS_ERR(gpio)) { >> + result |= buck_default_idx & (1 << i); > > s/(1 << i)/BIT(i)/ > Ok. >> + continue; >> + } >> + >> + if (gpiod_get_value_cansleep(gpio)) >> + result |= 1 << i; > > Same here. > Ok. >> + } >> + >> + return result; >> +} >> +EXPORT_SYMBOL_GPL(max77686_read_gpios); >> + >> static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >> *dev) >> { >> + struct device_node *np = dev->of_node; >> struct max77686_platform_data *pd; >> >> pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); >> @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >> return NULL; >> >> dev->platform_data = pd; >> + >> + /* Read default index and ignore errors, since default is 0 */ >> + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", > > If it doesn't already have one, this needs a DT Ack and associated > documentation. > Yes, these DT binding properties are documented in Patch 17/24 "mfd: max77686: Add documentation for DVS bindings" and I've cc'ed devicetree@vger.kernel.org so hopefully I can get some feedback/ack from DT maintainers. >> + &pd->buck_default_idx); >> + >> + max77686_dt_parse_dvs_gpio(dev); > > Bring code back into here and remove the call. > Sure, will do. >> return pd; >> } >> >> diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h >> index 4cbcc13..46a736b 100644 >> --- a/include/linux/mfd/max77686.h >> +++ b/include/linux/mfd/max77686.h >> @@ -99,15 +99,17 @@ struct max77686_platform_data { >> struct max77686_opmode_data *opmode_data; >> >> /* >> - * GPIO-DVS feature is not enabled with the current version of >> - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default >> - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. >> + * GPIO-DVS feature is not fully enabled with the current version of >> + * MAX77686 driver, but the driver does support using a DVS index other >> + * than the default of 0. >> */ >> - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >> - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ >> - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ >> - unsigned int buck3_voltage[8]; >> - unsigned int buck4_voltage[8]; >> + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >> + int buck_default_idx; /* Default value of DVS1, 2, 3 */ >> + >> + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ >> }; >> >> +extern int max77686_setup_gpios(struct device *dev); >> +extern int max77686_read_gpios(struct max77686_platform_data *pdata); >> + >> #endif /* __LINUX_MFD_MAX77686_H */ > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 4, 2014 at 10:24 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling > (DVS) support that allows output voltage to change dynamically. > > For MAX77686, these regulators are Buck regulators 2, 3 and 4. > > Each Buck output voltage is selected using a set of external > inputs: DVS1-3 and SELB2-4. > > DVS registers can be used to configure the output voltages for each > Buck regulator and which one is active is controled by DVSx lines. > > SELBx lines are used to control if individual Buck lines are ON or OFF. > > This patch adds support to configure the DVSx and SELBx lines > from DT and to setup and read the GPIO lines connected to them. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> (...) > +#include <linux/gpio/consumer.h> THANKS for using modern interfaces! > +static void max77686_dt_parse_dvs_gpio(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int i; > + > + /* > + * NOTE: we don't consider GPIO errors fatal; board may have some lines > + * directly pulled high or low and thus doesn't specify them. > + */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) > + pd->buck_gpio_dvs[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); > + > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) > + pd->buck_gpio_selb[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); > +} Rob Jones has a patch cooking that adds gpio_get_array() so this thing merits also adding devm_gpiod_get_array() I think? > +/** > + * max77686_setup_gpios() - init DVS-related GPIOs > + * @dev: device whose platform data contains the dvs GPIOs information > + * > + * This function claims / initalizations GPIOs related to DVS if they are > + * defined. This may have the effect of switching voltages if the > + * pdata->buck_default_idx does not match the boot time state of pins. > + */ > +int max77686_setup_gpios(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int buck_default_idx = pd->buck_default_idx; > + int ret; > + int i; > + > + /* Set all SELB high to avoid glitching while DVS is changing */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; > + > + /* OK if some GPIOs aren't defined */ > + if (IS_ERR(gpio)) > + continue; > + > + ret = gpiod_direction_output_raw(gpio, 1); Why does this have to be raw? Usually that is not to be used. Apart from this it looks OK. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 5, 2014 at 1:54 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling > (DVS) support that allows output voltage to change dynamically. > > For MAX77686, these regulators are Buck regulators 2, 3 and 4. > > Each Buck output voltage is selected using a set of external > inputs: DVS1-3 and SELB2-4. > > DVS registers can be used to configure the output voltages for each > Buck regulator and which one is active is controled by DVSx lines. > > SELBx lines are used to control if individual Buck lines are ON or OFF. > > This patch adds support to configure the DVSx and SELBx lines > from DT and to setup and read the GPIO lines connected to them. The entire series looks nice. Few minor comments from my side. I guess still one more version in needed as per other ppls comment. You may add, Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > > Changes since v6: > - Add a comment that max77686_read_gpios() function can sleep. > Sugggested by Krzysztof Kozlowski > --- > drivers/mfd/max77686.c | 119 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77686.h | 18 ++++--- > 2 files changed, 129 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c > index 8650832..d193873 100644 > --- a/drivers/mfd/max77686.c > +++ b/drivers/mfd/max77686.c > @@ -32,8 +32,10 @@ > #include <linux/mfd/core.h> > #include <linux/mfd/max77686.h> > #include <linux/mfd/max77686-private.h> > +#include <linux/gpio/consumer.h> > #include <linux/err.h> > #include <linux/of.h> > +#include <linux/export.h> > > #define I2C_ADDR_RTC (0x0C >> 1) > > @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { > {}, > }; > > +static void max77686_dt_parse_dvs_gpio(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int i; > + > + /* > + * NOTE: we don't consider GPIO errors fatal; board may have some lines > + * directly pulled high or low and thus doesn't specify them. > + */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) > + pd->buck_gpio_dvs[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); > + > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) > + pd->buck_gpio_selb[i] = > + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); > +} > + > +/** > + * max77686_setup_gpios() - init DVS-related GPIOs > + * @dev: device whose platform data contains the dvs GPIOs information > + * > + * This function claims / initalizations GPIOs related to DVS if they are > + * defined. This may have the effect of switching voltages if the > + * pdata->buck_default_idx does not match the boot time state of pins. > + */ > +int max77686_setup_gpios(struct device *dev) > +{ > + struct max77686_platform_data *pd = dev_get_platdata(dev); > + int buck_default_idx = pd->buck_default_idx; > + int ret; > + int i; > + > + /* Set all SELB high to avoid glitching while DVS is changing */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; > + > + /* OK if some GPIOs aren't defined */ > + if (IS_ERR(gpio)) > + continue; > + > + ret = gpiod_direction_output_raw(gpio, 1); > + if (ret) { > + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); > + return ret; > + } > + } > + > + /* Set our initial setting */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; > + > + /* OK if some GPIOs aren't defined */ > + if (IS_ERR(gpio)) > + continue; > + > + /* If a GPIO is valid, set it */ > + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); > + if (ret) { > + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); > + return ret; > + } > + } > + > + /* Now set SELB low to take effect */ > + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { > + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; > + > + if (!IS_ERR(gpio)) > + gpiod_set_value(gpio, 0); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(max77686_setup_gpios); > + > +/** > + * max77686_read_gpios() - read the current state of the dvs GPIOs > + * @pdata: platform data that contains the dvs GPIOs information > + * > + * We call this function at bootup to detect what slot the firmware was > + * using for the DVS GPIOs. That way we can properly preserve the firmware's > + * voltage settings > + * > + * This function can sleep so must not be called from atomic context. > + */ > +int max77686_read_gpios(struct max77686_platform_data *pdata) > +{ > + int buck_default_idx = pdata->buck_default_idx; > + int result = 0; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { > + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; > + > + /* OK if some GPIOs aren't defined; we'll use default */ > + if (IS_ERR(gpio)) { > + result |= buck_default_idx & (1 << i); > + continue; > + } > + > + if (gpiod_get_value_cansleep(gpio)) > + result |= 1 << i; > + } > + > + return result; > +} > +EXPORT_SYMBOL_GPL(max77686_read_gpios); > + > static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device > *dev) > { > + struct device_node *np = dev->of_node; > struct max77686_platform_data *pd; > > pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); > @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device > return NULL; > > dev->platform_data = pd; > + > + /* Read default index and ignore errors, since default is 0 */ > + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", > + &pd->buck_default_idx); Any error checking code here. Say if pmic-buck-default-dvs-idx exceed 8? > + > + max77686_dt_parse_dvs_gpio(dev); > + > return pd; > } > > diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h > index 4cbcc13..46a736b 100644 > --- a/include/linux/mfd/max77686.h > +++ b/include/linux/mfd/max77686.h > @@ -99,15 +99,17 @@ struct max77686_platform_data { > struct max77686_opmode_data *opmode_data; > > /* > - * GPIO-DVS feature is not enabled with the current version of > - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default > - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. > + * GPIO-DVS feature is not fully enabled with the current version of > + * MAX77686 driver, but the driver does support using a DVS index other > + * than the default of 0. > */ > - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ > - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ > - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ > - unsigned int buck3_voltage[8]; > - unsigned int buck4_voltage[8]; I think when DVS gpio is used all the 8 voltage levels are fetched from DT during booting and the registers are programmed accordingly. Any further set/get_voltage just changes the GPIO lines. Any reason why this method is not used? > + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ > + int buck_default_idx; /* Default value of DVS1, 2, 3 */ > + > + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ > }; > > +extern int max77686_setup_gpios(struct device *dev); > +extern int max77686_read_gpios(struct max77686_platform_data *pdata); > + > #endif /* __LINUX_MFD_MAX77686_H */ > -- > 2.0.0.rc2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Linus, On 07/10/2014 11:46 AM, Linus Walleij wrote: > On Fri, Jul 4, 2014 at 10:24 PM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: > >> Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling >> (DVS) support that allows output voltage to change dynamically. >> >> For MAX77686, these regulators are Buck regulators 2, 3 and 4. >> >> Each Buck output voltage is selected using a set of external >> inputs: DVS1-3 and SELB2-4. >> >> DVS registers can be used to configure the output voltages for each >> Buck regulator and which one is active is controled by DVSx lines. >> >> SELBx lines are used to control if individual Buck lines are ON or OFF. >> >> This patch adds support to configure the DVSx and SELBx lines >> from DT and to setup and read the GPIO lines connected to them. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > (...) >> +#include <linux/gpio/consumer.h> > > THANKS for using modern interfaces! > Thanks to you and Alexandre for keep improving the GPIO subsystem! >> +static void max77686_dt_parse_dvs_gpio(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int i; >> + >> + /* >> + * NOTE: we don't consider GPIO errors fatal; board may have some lines >> + * directly pulled high or low and thus doesn't specify them. >> + */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) >> + pd->buck_gpio_dvs[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); >> + >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) >> + pd->buck_gpio_selb[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); >> +} > > Rob Jones has a patch cooking that adds gpio_get_array() so this thing > merits also adding devm_gpiod_get_array() I think? > Yes, I just asked [0] Rob on the other thread if he is already implementing the descriptor-based version of his devm_request_gpio_array() or if I should go and implement it. Now, I wonder if that can be done in a follow-up patch (e.g: use the new devm_gpiod_get_array once it lands in Torvalds tree) since this series already touches several subsystems (mfd, regulators, clk and rtc) so if possible I would prefer to not add another cross-subsystem dependency :) >> +/** >> + * max77686_setup_gpios() - init DVS-related GPIOs >> + * @dev: device whose platform data contains the dvs GPIOs information >> + * >> + * This function claims / initalizations GPIOs related to DVS if they are >> + * defined. This may have the effect of switching voltages if the >> + * pdata->buck_default_idx does not match the boot time state of pins. >> + */ >> +int max77686_setup_gpios(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int buck_default_idx = pd->buck_default_idx; >> + int ret; >> + int i; >> + >> + /* Set all SELB high to avoid glitching while DVS is changing */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >> + >> + /* OK if some GPIOs aren't defined */ >> + if (IS_ERR(gpio)) >> + continue; >> + >> + ret = gpiod_direction_output_raw(gpio, 1); > > Why does this have to be raw? Usually that is not to be used. > Right, I can't think of a good reason why this has to be raw and not just use gpiod_direction_output() which will check the active-low flag and set the value accordingly. I'll change it on the next revision. > Apart from this it looks OK. > Great, thanks a lot for your feedback. > Yours, > Linus Walleij > Best regards, Javier [0]: https://lkml.org/lkml/2014/7/10/722 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Amit, On 07/10/2014 11:59 AM, amit daniel kachhap wrote: > On Sat, Jul 5, 2014 at 1:54 AM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: >> Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling >> (DVS) support that allows output voltage to change dynamically. >> >> For MAX77686, these regulators are Buck regulators 2, 3 and 4. >> >> Each Buck output voltage is selected using a set of external >> inputs: DVS1-3 and SELB2-4. >> >> DVS registers can be used to configure the output voltages for each >> Buck regulator and which one is active is controled by DVSx lines. >> >> SELBx lines are used to control if individual Buck lines are ON or OFF. >> >> This patch adds support to configure the DVSx and SELBx lines >> from DT and to setup and read the GPIO lines connected to them. > > The entire series looks nice. Few minor comments from my side. I guess > still one more version in needed as per other ppls comment. > You may add, > Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > Thanks. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> --- >> >> Changes since v6: >> - Add a comment that max77686_read_gpios() function can sleep. >> Sugggested by Krzysztof Kozlowski >> --- >> drivers/mfd/max77686.c | 119 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/mfd/max77686.h | 18 ++++--- >> 2 files changed, 129 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c >> index 8650832..d193873 100644 >> --- a/drivers/mfd/max77686.c >> +++ b/drivers/mfd/max77686.c >> @@ -32,8 +32,10 @@ >> #include <linux/mfd/core.h> >> #include <linux/mfd/max77686.h> >> #include <linux/mfd/max77686-private.h> >> +#include <linux/gpio/consumer.h> >> #include <linux/err.h> >> #include <linux/of.h> >> +#include <linux/export.h> >> >> #define I2C_ADDR_RTC (0x0C >> 1) >> >> @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { >> {}, >> }; >> >> +static void max77686_dt_parse_dvs_gpio(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int i; >> + >> + /* >> + * NOTE: we don't consider GPIO errors fatal; board may have some lines >> + * directly pulled high or low and thus doesn't specify them. >> + */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) >> + pd->buck_gpio_dvs[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); >> + >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) >> + pd->buck_gpio_selb[i] = >> + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); >> +} >> + >> +/** >> + * max77686_setup_gpios() - init DVS-related GPIOs >> + * @dev: device whose platform data contains the dvs GPIOs information >> + * >> + * This function claims / initalizations GPIOs related to DVS if they are >> + * defined. This may have the effect of switching voltages if the >> + * pdata->buck_default_idx does not match the boot time state of pins. >> + */ >> +int max77686_setup_gpios(struct device *dev) >> +{ >> + struct max77686_platform_data *pd = dev_get_platdata(dev); >> + int buck_default_idx = pd->buck_default_idx; >> + int ret; >> + int i; >> + >> + /* Set all SELB high to avoid glitching while DVS is changing */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >> + >> + /* OK if some GPIOs aren't defined */ >> + if (IS_ERR(gpio)) >> + continue; >> + >> + ret = gpiod_direction_output_raw(gpio, 1); >> + if (ret) { >> + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); >> + return ret; >> + } >> + } >> + >> + /* Set our initial setting */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; >> + >> + /* OK if some GPIOs aren't defined */ >> + if (IS_ERR(gpio)) >> + continue; >> + >> + /* If a GPIO is valid, set it */ >> + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); >> + if (ret) { >> + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); >> + return ret; >> + } >> + } >> + >> + /* Now set SELB low to take effect */ >> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >> + >> + if (!IS_ERR(gpio)) >> + gpiod_set_value(gpio, 0); >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(max77686_setup_gpios); >> + >> +/** >> + * max77686_read_gpios() - read the current state of the dvs GPIOs >> + * @pdata: platform data that contains the dvs GPIOs information >> + * >> + * We call this function at bootup to detect what slot the firmware was >> + * using for the DVS GPIOs. That way we can properly preserve the firmware's >> + * voltage settings >> + * >> + * This function can sleep so must not be called from atomic context. >> + */ >> +int max77686_read_gpios(struct max77686_platform_data *pdata) >> +{ >> + int buck_default_idx = pdata->buck_default_idx; >> + int result = 0; >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { >> + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; >> + >> + /* OK if some GPIOs aren't defined; we'll use default */ >> + if (IS_ERR(gpio)) { >> + result |= buck_default_idx & (1 << i); >> + continue; >> + } >> + >> + if (gpiod_get_value_cansleep(gpio)) >> + result |= 1 << i; >> + } >> + >> + return result; >> +} >> +EXPORT_SYMBOL_GPL(max77686_read_gpios); >> + >> static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >> *dev) >> { >> + struct device_node *np = dev->of_node; >> struct max77686_platform_data *pd; >> >> pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); >> @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >> return NULL; >> >> dev->platform_data = pd; >> + >> + /* Read default index and ignore errors, since default is 0 */ >> + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", >> + &pd->buck_default_idx); > Any error checking code here. Say if pmic-buck-default-dvs-idx exceed 8? I'm not a DT expert but AFAIK the kernel should expect the data in a FDT to be correct and should not validate it on runtime. There is work-in-progress to add a proper schema checking for DTS to the dtc so on build time it can be validated that a DTS is valid. AFAIU the only thing that the kernel should check is if a required property does not exist. >> + >> + max77686_dt_parse_dvs_gpio(dev); >> + >> return pd; >> } >> >> diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h >> index 4cbcc13..46a736b 100644 >> --- a/include/linux/mfd/max77686.h >> +++ b/include/linux/mfd/max77686.h >> @@ -99,15 +99,17 @@ struct max77686_platform_data { >> struct max77686_opmode_data *opmode_data; >> >> /* >> - * GPIO-DVS feature is not enabled with the current version of >> - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default >> - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. >> + * GPIO-DVS feature is not fully enabled with the current version of >> + * MAX77686 driver, but the driver does support using a DVS index other >> + * than the default of 0. >> */ >> - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >> - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ >> - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ >> - unsigned int buck3_voltage[8]; >> - unsigned int buck4_voltage[8]; > I think when DVS gpio is used all the 8 voltage levels are fetched > from DT during booting and the registers are programmed accordingly. > Any further set/get_voltage just changes the GPIO lines. > Any reason why this method is not used? What are you describing is how the DT binding works for other Maxim PMICs that also have DVS support right? (e.g: max8997). The DVS binding in max77686/802 is actually a subset of the one in max8997 so it let's you only choose which single DVS index is going to be used. The GPIOs in "max77686,pmic-buck-dvs-gpios" should match "pmic-buck-default-dvs-idx". To be honest I just took the DT binding that was used in the max77xxx driver from the Chrome OS 3.8 kernel and didn't compare it with other Maxim PMICs DT bingings. I wonder if I should just take the DVS patches out from this initial version to avoid blocking the max77802 support and then we can discuss this in more detail. >> + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >> + int buck_default_idx; /* Default value of DVS1, 2, 3 */ >> + >> + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ >> }; >> >> +extern int max77686_setup_gpios(struct device *dev); >> +extern int max77686_read_gpios(struct max77686_platform_data *pdata); >> + >> #endif /* __LINUX_MFD_MAX77686_H */ >> -- >> 2.0.0.rc2 >> Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Jul 10, 2014 at 6:45 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> I think when DVS gpio is used all the 8 voltage levels are fetched >> from DT during booting and the registers are programmed accordingly. >> Any further set/get_voltage just changes the GPIO lines. >> Any reason why this method is not used? > > What are you describing is how the DT binding works for other Maxim PMICs that > also have DVS support right? (e.g: max8997). > > The DVS binding in max77686/802 is actually a subset of the one in max8997 so it > let's you only choose which single DVS index is going to be used. The GPIOs in > "max77686,pmic-buck-dvs-gpios" should match "pmic-buck-default-dvs-idx". > > To be honest I just took the DT binding that was used in the max77xxx driver > from the Chrome OS 3.8 kernel and didn't compare it with other Maxim PMICs DT > bingings. > > I wonder if I should just take the DVS patches out from this initial version to > avoid blocking the max77802 support and then we can discuss this in more detail. To give background: * On exynos5250 / exynos5420 / exynos5800 the CPUFreq driver exposes more than 8 different operating points. You could argue about whether that's useful but that's the way it is right now. That means that using the GPIOs is not trivial (you'd have to use a mix of GPIOs and i2c and use heuristics). * On the Samsung Chromebook 2 we use the DVFS GPIOs to get back to a sane state after a warm reset. When the CPU resets itself all GPIOs will default back to their reset state. That will effectively transfer us to DVFS slot 0. We make sure that the kernel always modifies a different DVFS slot. * On all known boards all DVFS GPIOs were hooked up. I originally wrote code assuming that someone could design a board with fewer lines hooked up but I don't know of that being done. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 11, 2014 at 7:15 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Amit, > > On 07/10/2014 11:59 AM, amit daniel kachhap wrote: >> On Sat, Jul 5, 2014 at 1:54 AM, Javier Martinez Canillas >> <javier.martinez@collabora.co.uk> wrote: >>> Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling >>> (DVS) support that allows output voltage to change dynamically. >>> >>> For MAX77686, these regulators are Buck regulators 2, 3 and 4. >>> >>> Each Buck output voltage is selected using a set of external >>> inputs: DVS1-3 and SELB2-4. >>> >>> DVS registers can be used to configure the output voltages for each >>> Buck regulator and which one is active is controled by DVSx lines. >>> >>> SELBx lines are used to control if individual Buck lines are ON or OFF. >>> >>> This patch adds support to configure the DVSx and SELBx lines >>> from DT and to setup and read the GPIO lines connected to them. >> >> The entire series looks nice. Few minor comments from my side. I guess >> still one more version in needed as per other ppls comment. >> You may add, >> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >> > > Thanks. > >>> >>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >>> --- >>> >>> Changes since v6: >>> - Add a comment that max77686_read_gpios() function can sleep. >>> Sugggested by Krzysztof Kozlowski >>> --- >>> drivers/mfd/max77686.c | 119 +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/mfd/max77686.h | 18 ++++--- >>> 2 files changed, 129 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c >>> index 8650832..d193873 100644 >>> --- a/drivers/mfd/max77686.c >>> +++ b/drivers/mfd/max77686.c >>> @@ -32,8 +32,10 @@ >>> #include <linux/mfd/core.h> >>> #include <linux/mfd/max77686.h> >>> #include <linux/mfd/max77686-private.h> >>> +#include <linux/gpio/consumer.h> >>> #include <linux/err.h> >>> #include <linux/of.h> >>> +#include <linux/export.h> >>> >>> #define I2C_ADDR_RTC (0x0C >> 1) >>> >>> @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { >>> {}, >>> }; >>> >>> +static void max77686_dt_parse_dvs_gpio(struct device *dev) >>> +{ >>> + struct max77686_platform_data *pd = dev_get_platdata(dev); >>> + int i; >>> + >>> + /* >>> + * NOTE: we don't consider GPIO errors fatal; board may have some lines >>> + * directly pulled high or low and thus doesn't specify them. >>> + */ >>> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) >>> + pd->buck_gpio_dvs[i] = >>> + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); >>> + >>> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) >>> + pd->buck_gpio_selb[i] = >>> + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); >>> +} >>> + >>> +/** >>> + * max77686_setup_gpios() - init DVS-related GPIOs >>> + * @dev: device whose platform data contains the dvs GPIOs information >>> + * >>> + * This function claims / initalizations GPIOs related to DVS if they are >>> + * defined. This may have the effect of switching voltages if the >>> + * pdata->buck_default_idx does not match the boot time state of pins. >>> + */ >>> +int max77686_setup_gpios(struct device *dev) >>> +{ >>> + struct max77686_platform_data *pd = dev_get_platdata(dev); >>> + int buck_default_idx = pd->buck_default_idx; >>> + int ret; >>> + int i; >>> + >>> + /* Set all SELB high to avoid glitching while DVS is changing */ >>> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >>> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >>> + >>> + /* OK if some GPIOs aren't defined */ >>> + if (IS_ERR(gpio)) >>> + continue; >>> + >>> + ret = gpiod_direction_output_raw(gpio, 1); >>> + if (ret) { >>> + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + /* Set our initial setting */ >>> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { >>> + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; >>> + >>> + /* OK if some GPIOs aren't defined */ >>> + if (IS_ERR(gpio)) >>> + continue; >>> + >>> + /* If a GPIO is valid, set it */ >>> + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); >>> + if (ret) { >>> + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + /* Now set SELB low to take effect */ >>> + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { >>> + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; >>> + >>> + if (!IS_ERR(gpio)) >>> + gpiod_set_value(gpio, 0); >>> + } >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(max77686_setup_gpios); >>> + >>> +/** >>> + * max77686_read_gpios() - read the current state of the dvs GPIOs >>> + * @pdata: platform data that contains the dvs GPIOs information >>> + * >>> + * We call this function at bootup to detect what slot the firmware was >>> + * using for the DVS GPIOs. That way we can properly preserve the firmware's >>> + * voltage settings >>> + * >>> + * This function can sleep so must not be called from atomic context. >>> + */ >>> +int max77686_read_gpios(struct max77686_platform_data *pdata) >>> +{ >>> + int buck_default_idx = pdata->buck_default_idx; >>> + int result = 0; >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { >>> + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; >>> + >>> + /* OK if some GPIOs aren't defined; we'll use default */ >>> + if (IS_ERR(gpio)) { >>> + result |= buck_default_idx & (1 << i); >>> + continue; >>> + } >>> + >>> + if (gpiod_get_value_cansleep(gpio)) >>> + result |= 1 << i; >>> + } >>> + >>> + return result; >>> +} >>> +EXPORT_SYMBOL_GPL(max77686_read_gpios); >>> + >>> static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >>> *dev) >>> { >>> + struct device_node *np = dev->of_node; >>> struct max77686_platform_data *pd; >>> >>> pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); >>> @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >>> return NULL; >>> >>> dev->platform_data = pd; >>> + >>> + /* Read default index and ignore errors, since default is 0 */ >>> + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", >>> + &pd->buck_default_idx); >> Any error checking code here. Say if pmic-buck-default-dvs-idx exceed 8? > > I'm not a DT expert but AFAIK the kernel should expect the data in a FDT to be > correct and should not validate it on runtime. There is work-in-progress to add > a proper schema checking for DTS to the dtc so on build time it can be validated > that a DTS is valid. Hmm its fine then. > > AFAIU the only thing that the kernel should check is if a required property does > not exist. > >>> + >>> + max77686_dt_parse_dvs_gpio(dev); >>> + >>> return pd; >>> } >>> >>> diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h >>> index 4cbcc13..46a736b 100644 >>> --- a/include/linux/mfd/max77686.h >>> +++ b/include/linux/mfd/max77686.h >>> @@ -99,15 +99,17 @@ struct max77686_platform_data { >>> struct max77686_opmode_data *opmode_data; >>> >>> /* >>> - * GPIO-DVS feature is not enabled with the current version of >>> - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default >>> - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. >>> + * GPIO-DVS feature is not fully enabled with the current version of >>> + * MAX77686 driver, but the driver does support using a DVS index other >>> + * than the default of 0. >>> */ >>> - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >>> - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ >>> - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ >>> - unsigned int buck3_voltage[8]; >>> - unsigned int buck4_voltage[8]; >> I think when DVS gpio is used all the 8 voltage levels are fetched >> from DT during booting and the registers are programmed accordingly. >> Any further set/get_voltage just changes the GPIO lines. >> Any reason why this method is not used? > > What are you describing is how the DT binding works for other Maxim PMICs that > also have DVS support right? (e.g: max8997). > > The DVS binding in max77686/802 is actually a subset of the one in max8997 so it > let's you only choose which single DVS index is going to be used. The GPIOs in > "max77686,pmic-buck-dvs-gpios" should match "pmic-buck-default-dvs-idx". > > To be honest I just took the DT binding that was used in the max77xxx driver > from the Chrome OS 3.8 kernel and didn't compare it with other Maxim PMICs DT > bingings. > > I wonder if I should just take the DVS patches out from this initial version to > avoid blocking the max77802 support and then we can discuss this in more detail. Agreed DVS functionality can be added later. > >>> + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ >>> + int buck_default_idx; /* Default value of DVS1, 2, 3 */ >>> + >>> + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ >>> }; >>> >>> +extern int max77686_setup_gpios(struct device *dev); >>> +extern int max77686_read_gpios(struct max77686_platform_data *pdata); >>> + >>> #endif /* __LINUX_MFD_MAX77686_H */ >>> -- >>> 2.0.0.rc2 >>> > > Best regards, > Javier > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Javier, On 11.07.2014 03:45, Javier Martinez Canillas wrote: > On 07/10/2014 11:59 AM, amit daniel kachhap wrote: >> On Sat, Jul 5, 2014 at 1:54 AM, Javier Martinez Canillas >> <javier.martinez@collabora.co.uk> wrote: [snip] >>> @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >>> return NULL; >>> >>> dev->platform_data = pd; >>> + >>> + /* Read default index and ignore errors, since default is 0 */ >>> + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", >>> + &pd->buck_default_idx); >> Any error checking code here. Say if pmic-buck-default-dvs-idx exceed 8? > > I'm not a DT expert but AFAIK the kernel should expect the data in a FDT to be > correct and should not validate it on runtime. There is work-in-progress to add > a proper schema checking for DTS to the dtc so on build time it can be validated > that a DTS is valid. > > AFAIU the only thing that the kernel should check is if a required property does > not exist. I'd disagree on this. IMHO schema (if it progresses further, as unfortunately I can't find time to dedicate to it and looks like it's similar for other people that used to be involved) should be focused on structural checks, i.e. proper layout of nodes and properties, basic data types and so, to figure out common errors earlier than at boot-up time. On kernel side this should be treated in the same way as platform data. I agree that some existing drivers do little to validate incoming data, but I believe it is a good practice to validate things that the driver has no control over, especially when it's about a PMIC, when invalid data can have quite serious effects and detecting even some of them (e.g. value to big, which would overflow in target bit field) might prevent a failure. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Tomasz, On 07/11/2014 11:43 AM, Tomasz Figa wrote: > Hi Javier, > > On 11.07.2014 03:45, Javier Martinez Canillas wrote: >> On 07/10/2014 11:59 AM, amit daniel kachhap wrote: >>> On Sat, Jul 5, 2014 at 1:54 AM, Javier Martinez Canillas >>> <javier.martinez@collabora.co.uk> wrote: > > [snip] > >>>> @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device >>>> return NULL; >>>> >>>> dev->platform_data = pd; >>>> + >>>> + /* Read default index and ignore errors, since default is 0 */ >>>> + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", >>>> + &pd->buck_default_idx); >>> Any error checking code here. Say if pmic-buck-default-dvs-idx exceed 8? >> >> I'm not a DT expert but AFAIK the kernel should expect the data in a FDT to be >> correct and should not validate it on runtime. There is work-in-progress to add >> a proper schema checking for DTS to the dtc so on build time it can be validated >> that a DTS is valid. >> >> AFAIU the only thing that the kernel should check is if a required property does >> not exist. > > I'd disagree on this. > > IMHO schema (if it progresses further, as unfortunately I can't find > time to dedicate to it and looks like it's similar for other people that > used to be involved) should be focused on structural checks, i.e. proper > layout of nodes and properties, basic data types and so, to figure out > common errors earlier than at boot-up time. > > On kernel side this should be treated in the same way as platform data. > I agree that some existing drivers do little to validate incoming data, > but I believe it is a good practice to validate things that the driver > has no control over, especially when it's about a PMIC, when invalid > data can have quite serious effects and detecting even some of them > (e.g. value to big, which would overflow in target bit field) might > prevent a failure. > Thanks a lot for the clarification and I completely agree with your explanation. I'll add proper validation for the data obtained by DT then. It would be nice if this was documented somewhere (or maybe I missed it). > Best regards, > Tomasz > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c index 8650832..d193873 100644 --- a/drivers/mfd/max77686.c +++ b/drivers/mfd/max77686.c @@ -32,8 +32,10 @@ #include <linux/mfd/core.h> #include <linux/mfd/max77686.h> #include <linux/mfd/max77686-private.h> +#include <linux/gpio/consumer.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/export.h> #define I2C_ADDR_RTC (0x0C >> 1) @@ -101,9 +103,119 @@ static const struct of_device_id max77686_pmic_dt_match[] = { {}, }; +static void max77686_dt_parse_dvs_gpio(struct device *dev) +{ + struct max77686_platform_data *pd = dev_get_platdata(dev); + int i; + + /* + * NOTE: we don't consider GPIO errors fatal; board may have some lines + * directly pulled high or low and thus doesn't specify them. + */ + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) + pd->buck_gpio_dvs[i] = + devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i); + + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) + pd->buck_gpio_selb[i] = + devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i); +} + +/** + * max77686_setup_gpios() - init DVS-related GPIOs + * @dev: device whose platform data contains the dvs GPIOs information + * + * This function claims / initalizations GPIOs related to DVS if they are + * defined. This may have the effect of switching voltages if the + * pdata->buck_default_idx does not match the boot time state of pins. + */ +int max77686_setup_gpios(struct device *dev) +{ + struct max77686_platform_data *pd = dev_get_platdata(dev); + int buck_default_idx = pd->buck_default_idx; + int ret; + int i; + + /* Set all SELB high to avoid glitching while DVS is changing */ + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; + + /* OK if some GPIOs aren't defined */ + if (IS_ERR(gpio)) + continue; + + ret = gpiod_direction_output_raw(gpio, 1); + if (ret) { + dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret); + return ret; + } + } + + /* Set our initial setting */ + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) { + struct gpio_desc *gpio = pd->buck_gpio_dvs[i]; + + /* OK if some GPIOs aren't defined */ + if (IS_ERR(gpio)) + continue; + + /* If a GPIO is valid, set it */ + gpiod_direction_output(gpio, (buck_default_idx >> i) & 1); + if (ret) { + dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret); + return ret; + } + } + + /* Now set SELB low to take effect */ + for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) { + struct gpio_desc *gpio = pd->buck_gpio_selb[i]; + + if (!IS_ERR(gpio)) + gpiod_set_value(gpio, 0); + } + + return 0; +} +EXPORT_SYMBOL_GPL(max77686_setup_gpios); + +/** + * max77686_read_gpios() - read the current state of the dvs GPIOs + * @pdata: platform data that contains the dvs GPIOs information + * + * We call this function at bootup to detect what slot the firmware was + * using for the DVS GPIOs. That way we can properly preserve the firmware's + * voltage settings + * + * This function can sleep so must not be called from atomic context. + */ +int max77686_read_gpios(struct max77686_platform_data *pdata) +{ + int buck_default_idx = pdata->buck_default_idx; + int result = 0; + int i; + + for (i = 0; i < ARRAY_SIZE(pdata->buck_gpio_dvs); i++) { + struct gpio_desc *gpio = pdata->buck_gpio_dvs[i]; + + /* OK if some GPIOs aren't defined; we'll use default */ + if (IS_ERR(gpio)) { + result |= buck_default_idx & (1 << i); + continue; + } + + if (gpiod_get_value_cansleep(gpio)) + result |= 1 << i; + } + + return result; +} +EXPORT_SYMBOL_GPL(max77686_read_gpios); + static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device *dev) { + struct device_node *np = dev->of_node; struct max77686_platform_data *pd; pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); @@ -111,6 +223,13 @@ static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device return NULL; dev->platform_data = pd; + + /* Read default index and ignore errors, since default is 0 */ + of_property_read_u32(np, "max77686,pmic-buck-default-dvs-idx", + &pd->buck_default_idx); + + max77686_dt_parse_dvs_gpio(dev); + return pd; } diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h index 4cbcc13..46a736b 100644 --- a/include/linux/mfd/max77686.h +++ b/include/linux/mfd/max77686.h @@ -99,15 +99,17 @@ struct max77686_platform_data { struct max77686_opmode_data *opmode_data; /* - * GPIO-DVS feature is not enabled with the current version of - * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default - * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW. + * GPIO-DVS feature is not fully enabled with the current version of + * MAX77686 driver, but the driver does support using a DVS index other + * than the default of 0. */ - int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ - int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */ - unsigned int buck2_voltage[8]; /* buckx_voltage in uV */ - unsigned int buck3_voltage[8]; - unsigned int buck4_voltage[8]; + struct gpio_desc *buck_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */ + int buck_default_idx; /* Default value of DVS1, 2, 3 */ + + struct gpio_desc *buck_gpio_selb[3]; /* Buck regulators 2, 3, 4 */ }; +extern int max77686_setup_gpios(struct device *dev); +extern int max77686_read_gpios(struct max77686_platform_data *pdata); + #endif /* __LINUX_MFD_MAX77686_H */