Message ID | 1469414580-14121-5-git-send-email-preid@electromag.com.au (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Hi, [auto build test ERROR on battery/master] [also build test ERROR on v4.7 next-20160729] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Phil-Reid/power-sbs-battery-fixup-gpio-detect-irq-handling/20160725-160137 base: git://git.infradead.org/battery-2.6.git master config: x86_64-randconfig-s1-07310624 (attached as .config) compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/power/sbs-battery.c: In function 'sbs_get_battery_presence_and_health': >> drivers/power/sbs-battery.c:305: error: implicit declaration of function 'gpiod_get_value_cansleep' drivers/power/sbs-battery.c: In function 'sbs_populate_pdata': drivers/power/sbs-battery.c:743: error: implicit declaration of function 'gpio_to_desc' drivers/power/sbs-battery.c:743: warning: assignment makes pointer from integer without a cast drivers/power/sbs-battery.c: In function 'sbs_probe': >> drivers/power/sbs-battery.c:870: error: implicit declaration of function 'gpiod_to_irq' vim +/gpiod_get_value_cansleep +305 drivers/power/sbs-battery.c 299 union power_supply_propval *val) 300 { 301 s32 ret; 302 struct sbs_info *chip = i2c_get_clientdata(client); 303 304 if (psp == POWER_SUPPLY_PROP_PRESENT && chip->gpio_detect) { > 305 ret = gpiod_get_value_cansleep(chip->gpio_detect); 306 if (ret < 0) 307 return ret; 308 val->intval = ret; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, [auto build test ERROR on battery/master] [also build test ERROR on v4.7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Phil-Reid/power-sbs-battery-fixup-gpio-detect-irq-handling/20160725-160137 base: git://git.infradead.org/battery-2.6.git master config: x86_64-randconfig-s0-07310624 (attached as .config) compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/power/sbs-battery.c: In function 'sbs_get_battery_presence_and_health': drivers/power/sbs-battery.c:305: error: implicit declaration of function 'gpiod_get_value_cansleep' drivers/power/sbs-battery.c: In function 'sbs_populate_pdata': >> drivers/power/sbs-battery.c:743: error: implicit declaration of function 'gpio_to_desc' drivers/power/sbs-battery.c:743: warning: assignment makes pointer from integer without a cast drivers/power/sbs-battery.c: In function 'sbs_probe': drivers/power/sbs-battery.c:870: error: implicit declaration of function 'gpiod_to_irq' vim +/gpio_to_desc +743 drivers/power/sbs-battery.c 737 flags, dev_name(&client->dev)); 738 if (rc) { 739 dev_warn(&client->dev, "Failed to get gpio: %d\n", rc); 740 return pdata; 741 } 742 > 743 chip->gpio_detect = gpio_to_desc(pdata->battery_detect); 744 745 return pdata; 746 } --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, [auto build test ERROR on battery/master] [also build test ERROR on v4.7 next-20160729] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Phil-Reid/power-sbs-battery-fixup-gpio-detect-irq-handling/20160725-160137 base: git://git.infradead.org/battery-2.6.git master config: x86_64-randconfig-s3-07310833 (attached as .config) compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/power/sbs-battery.c: In function 'sbs_get_battery_presence_and_health': drivers/power/sbs-battery.c:305:9: error: implicit declaration of function 'gpiod_get_value_cansleep' [-Werror=implicit-function-declaration] ret = gpiod_get_value_cansleep(chip->gpio_detect); ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/power/sbs-battery.c: In function 'sbs_populate_pdata': drivers/power/sbs-battery.c:743:22: error: implicit declaration of function 'gpio_to_desc' [-Werror=implicit-function-declaration] chip->gpio_detect = gpio_to_desc(pdata->battery_detect); ^~~~~~~~~~~~ drivers/power/sbs-battery.c:743:20: warning: assignment makes pointer from integer without a cast [-Wint-conversion] chip->gpio_detect = gpio_to_desc(pdata->battery_detect); ^ drivers/power/sbs-battery.c: In function 'sbs_of_populate_pdata': >> drivers/power/sbs-battery.c:793:22: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration] chip->gpio_detect = devm_gpiod_get_optional(&client->dev, ^~~~~~~~~~~~~~~~~~~~~~~ >> drivers/power/sbs-battery.c:794:26: error: 'GPIOD_IN' undeclared (first use in this function) "sbs,battery-detect", GPIOD_IN); ^~~~~~~~ drivers/power/sbs-battery.c:794:26: note: each undeclared identifier is reported only once for each function it appears in drivers/power/sbs-battery.c: In function 'sbs_probe': drivers/power/sbs-battery.c:870:8: error: implicit declaration of function 'gpiod_to_irq' [-Werror=implicit-function-declaration] irq = gpiod_to_irq(chip->gpio_detect); ^~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/devm_gpiod_get_optional +793 drivers/power/sbs-battery.c 737 flags, dev_name(&client->dev)); 738 if (rc) { 739 dev_warn(&client->dev, "Failed to get gpio: %d\n", rc); 740 return pdata; 741 } 742 > 743 chip->gpio_detect = gpio_to_desc(pdata->battery_detect); 744 745 return pdata; 746 } 747 748 #if defined(CONFIG_OF) 749 750 #include <linux/of_device.h> 751 #include <linux/of_gpio.h> 752 753 static const struct of_device_id sbs_dt_ids[] = { 754 { .compatible = "sbs,sbs-battery" }, 755 { .compatible = "ti,bq20z75" }, 756 { } 757 }; 758 MODULE_DEVICE_TABLE(of, sbs_dt_ids); 759 760 static struct sbs_platform_data *sbs_of_populate_pdata(struct sbs_info *chip) 761 { 762 struct i2c_client *client = chip->client; 763 struct device_node *of_node = client->dev.of_node; 764 struct sbs_platform_data *pdata = NULL; 765 int rc; 766 u32 prop; 767 768 /* verify this driver matches this device */ 769 if (!of_node) 770 return NULL; 771 772 /* first make sure at least one property is set, otherwise 773 * it won't change behavior from running without pdata. 774 */ 775 if (!of_get_property(of_node, "sbs,i2c-retry-count", NULL) && 776 !of_get_property(of_node, "sbs,poll-retry-count", NULL) && 777 !of_get_property(of_node, "sbs,battery-detect-gpios", NULL)) 778 goto of_out; 779 780 pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data), 781 GFP_KERNEL); 782 if (!pdata) 783 return ERR_PTR(-ENOMEM); 784 785 rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop); 786 if (!rc) 787 pdata->i2c_retry_count = prop; 788 789 rc = of_property_read_u32(of_node, "sbs,poll-retry-count", &prop); 790 if (!rc) 791 pdata->poll_retry_count = prop; 792 > 793 chip->gpio_detect = devm_gpiod_get_optional(&client->dev, > 794 "sbs,battery-detect", GPIOD_IN); 795 796 if (IS_ERR(chip->gpio_detect)) { 797 dev_err(&client->dev, "Failed to get gpio: %ld\n", --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Phil, On Mon, Jul 25, 2016 at 10:43:00AM +0800, Phil Reid wrote: > Switch to using new gpio_desc interface and devm gpio get calls to > automatically manage gpio resource. Use gpiod_get_value which handles > active high / low calls. > > If gpio_detect is set then force loading of the driver as it is > reasonable to assume that the battery may not be present. > > Update the is_present flag immediately in the IRQ. I queued PATCH 1-3. Please send a new revision for PATCH 4 (this one) with the following changes: * add the correct gpiod include: <include/linux/gpio/consumer.h> * rebase on power-supply's next branch * remove battery_detect and battery_detect_present from the struct sbs_platform_data and always use the gpiod API. It's data can also come from platform data, see Documentation/gpio/board.txt and Documentation/gpio/consumer.txt -- Sebastian
On 16/08/2016 05:08, Sebastian Reichel wrote: > Hi Phil, > > On Mon, Jul 25, 2016 at 10:43:00AM +0800, Phil Reid wrote: >> Switch to using new gpio_desc interface and devm gpio get calls to >> automatically manage gpio resource. Use gpiod_get_value which handles >> active high / low calls. >> >> If gpio_detect is set then force loading of the driver as it is >> reasonable to assume that the battery may not be present. >> >> Update the is_present flag immediately in the IRQ. > > I queued PATCH 1-3. Please send a new revision for PATCH 4 (this > one) with the following changes: > > * add the correct gpiod include: <include/linux/gpio/consumer.h> > * rebase on power-supply's next branch > * remove battery_detect and battery_detect_present from the > struct sbs_platform_data and always use the gpiod API. It's > data can also come from platform data, see > Documentation/gpio/board.txt and Documentation/gpio/consumer.txt > > -- Sebastian > G'day Sebastian, Thanks, I'll have a look at how that works. I wasn't sure it was acceptable to change the interface to sbs_platform_data.
Hi Phil, On Tue, Aug 16, 2016 at 05:35:09PM +0800, Phil Reid wrote: > On 16/08/2016 05:08, Sebastian Reichel wrote: > > Hi Phil, > > > > On Mon, Jul 25, 2016 at 10:43:00AM +0800, Phil Reid wrote: > > > Switch to using new gpio_desc interface and devm gpio get calls to > > > automatically manage gpio resource. Use gpiod_get_value which handles > > > active high / low calls. > > > > > > If gpio_detect is set then force loading of the driver as it is > > > reasonable to assume that the battery may not be present. > > > > > > Update the is_present flag immediately in the IRQ. > > > > I queued PATCH 1-3. Please send a new revision for PATCH 4 (this > > one) with the following changes: > > > > * add the correct gpiod include: <include/linux/gpio/consumer.h> > > * rebase on power-supply's next branch > > * remove battery_detect and battery_detect_present from the > > struct sbs_platform_data and always use the gpiod API. It's > > data can also come from platform data, see > > Documentation/gpio/board.txt and Documentation/gpio/consumer.txt > > > > -- Sebastian > > > G'day Sebastian, > > Thanks, I'll have a look at how that works. > I wasn't sure it was acceptable to change the interface to > sbs_platform_data. Yes, that API is not exposed by the kernel, so it can be changed. Since there are no users as far as I can see, changing it should be quite simple. Thanks for the driver cleanup. -- Sebastian
diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-battery.c index 31f3e33..c05bec4 100644 --- a/drivers/power/sbs-battery.c +++ b/drivers/power/sbs-battery.c @@ -160,7 +160,7 @@ struct sbs_info { struct power_supply *power_supply; struct sbs_platform_data *pdata; bool is_present; - bool gpio_detect; + struct gpio_desc *gpio_detect; bool enable_detection; int last_state; int poll_time; @@ -301,13 +301,11 @@ static int sbs_get_battery_presence_and_health( s32 ret; struct sbs_info *chip = i2c_get_clientdata(client); - if (psp == POWER_SUPPLY_PROP_PRESENT && - chip->gpio_detect) { - ret = gpio_get_value(chip->pdata->battery_detect); - if (ret == chip->pdata->battery_detect_present) - val->intval = 1; - else - val->intval = 0; + if (psp == POWER_SUPPLY_PROP_PRESENT && chip->gpio_detect) { + ret = gpiod_get_value_cansleep(chip->gpio_detect); + if (ret < 0) + return ret; + val->intval = ret; chip->is_present = val->intval; return ret; } @@ -662,7 +660,12 @@ static irqreturn_t sbs_irq(int irq, void *devid) { struct sbs_info *chip = devid; struct power_supply *battery = chip->power_supply; + int ret; + ret = gpiod_get_value_cansleep(chip->gpio_detect); + if (ret < 0) + return ret; + chip->is_present = ret; power_supply_changed(battery); return IRQ_HANDLED; @@ -717,6 +720,31 @@ static void sbs_delayed_work(struct work_struct *work) } } +static struct sbs_platform_data *sbs_populate_pdata(struct sbs_info *chip) +{ + struct i2c_client *client = chip->client; + struct sbs_platform_data *pdata = client->dev.platform_data; + int rc; + unsigned long flags = GPIOF_DIR_IN; + + if (!gpio_is_valid(pdata->battery_detect)) + return pdata; + + if (!pdata->battery_detect_present) + set_bit(GPIOF_ACTIVE_LOW, &flags); + + rc = devm_gpio_request_one(&client->dev, pdata->battery_detect, + flags, dev_name(&client->dev)); + if (rc) { + dev_warn(&client->dev, "Failed to get gpio: %d\n", rc); + return pdata; + } + + chip->gpio_detect = gpio_to_desc(pdata->battery_detect); + + return pdata; +} + #if defined(CONFIG_OF) #include <linux/of_device.h> @@ -729,12 +757,11 @@ static const struct of_device_id sbs_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, sbs_dt_ids); -static struct sbs_platform_data *sbs_of_populate_pdata( - struct i2c_client *client) +static struct sbs_platform_data *sbs_of_populate_pdata(struct sbs_info *chip) { + struct i2c_client *client = chip->client; struct device_node *of_node = client->dev.of_node; - struct sbs_platform_data *pdata = client->dev.platform_data; - enum of_gpio_flags gpio_flags; + struct sbs_platform_data *pdata = NULL; int rc; u32 prop; @@ -742,10 +769,6 @@ static struct sbs_platform_data *sbs_of_populate_pdata( if (!of_node) return NULL; - /* if platform data is set, honor it */ - if (pdata) - return pdata; - /* first make sure at least one property is set, otherwise * it won't change behavior from running without pdata. */ @@ -757,7 +780,7 @@ static struct sbs_platform_data *sbs_of_populate_pdata( pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data), GFP_KERNEL); if (!pdata) - goto of_out; + return ERR_PTR(-ENOMEM); rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop); if (!rc) @@ -767,27 +790,22 @@ static struct sbs_platform_data *sbs_of_populate_pdata( if (!rc) pdata->poll_retry_count = prop; - if (!of_get_property(of_node, "sbs,battery-detect-gpios", NULL)) { - pdata->battery_detect = -1; - goto of_out; - } - - pdata->battery_detect = of_get_named_gpio_flags(of_node, - "sbs,battery-detect-gpios", 0, &gpio_flags); - - if (gpio_flags & OF_GPIO_ACTIVE_LOW) - pdata->battery_detect_present = 0; - else - pdata->battery_detect_present = 1; + chip->gpio_detect = devm_gpiod_get_optional(&client->dev, + "sbs,battery-detect", GPIOD_IN); + if (IS_ERR(chip->gpio_detect)) { + dev_err(&client->dev, "Failed to get gpio: %ld\n", + PTR_ERR(chip->gpio_detect)); + return ERR_PTR(PTR_ERR(chip->gpio_detect)); + } of_out: return pdata; } #else static struct sbs_platform_data *sbs_of_populate_pdata( - struct i2c_client *client) + struct sbs_info *client) { - return client->dev.platform_data; + return NULL; } #endif @@ -825,7 +843,6 @@ static int sbs_probe(struct i2c_client *client, chip->client = client; chip->enable_detection = false; - chip->gpio_detect = false; psy_cfg.of_node = client->dev.of_node; psy_cfg.drv_data = chip; /* ignore first notification of external change, it is generated @@ -834,38 +851,25 @@ static int sbs_probe(struct i2c_client *client, chip->ignore_changes = 1; chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN; - pdata = sbs_of_populate_pdata(client); - if (pdata) { - chip->gpio_detect = gpio_is_valid(pdata->battery_detect); - chip->pdata = pdata; + pdata = sbs_populate_pdata(chip); + } else { + pdata = sbs_of_populate_pdata(chip); } - i2c_set_clientdata(client, chip); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); - if (!chip->gpio_detect) - goto skip_gpio; + chip->pdata = pdata; - rc = gpio_request(pdata->battery_detect, dev_name(&client->dev)); - if (rc) { - dev_warn(&client->dev, "Failed to request gpio: %d\n", rc); - chip->gpio_detect = false; - goto skip_gpio; - } + i2c_set_clientdata(client, chip); - rc = gpio_direction_input(pdata->battery_detect); - if (rc) { - dev_warn(&client->dev, "Failed to get gpio as input: %d\n", rc); - gpio_free(pdata->battery_detect); - chip->gpio_detect = false; + if (!chip->gpio_detect) goto skip_gpio; - } - irq = gpio_to_irq(pdata->battery_detect); + irq = gpiod_to_irq(chip->gpio_detect); if (irq <= 0) { dev_warn(&client->dev, "Failed to get gpio as irq: %d\n", irq); - gpio_free(pdata->battery_detect); - chip->gpio_detect = false; goto skip_gpio; } @@ -874,8 +878,6 @@ static int sbs_probe(struct i2c_client *client, dev_name(&client->dev), chip); if (rc) { dev_warn(&client->dev, "Failed to request irq: %d\n", rc); - gpio_free(pdata->battery_detect); - chip->gpio_detect = false; goto skip_gpio; } @@ -884,7 +886,7 @@ skip_gpio: * Before we register, we might need to make sure we can actually talk * to the battery. */ - if (!force_load) { + if (!(force_load || chip->gpio_detect)) { rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); if (rc < 0) { @@ -913,9 +915,6 @@ skip_gpio: return 0; exit_psupply: - if (chip->gpio_detect) - gpio_free(pdata->battery_detect); - return rc; } @@ -923,9 +922,6 @@ static int sbs_remove(struct i2c_client *client) { struct sbs_info *chip = i2c_get_clientdata(client); - if (chip->gpio_detect) - gpio_free(chip->pdata->battery_detect); - cancel_delayed_work_sync(&chip->work); return 0;
Switch to using new gpio_desc interface and devm gpio get calls to automatically manage gpio resource. Use gpiod_get_value which handles active high / low calls. If gpio_detect is set then force loading of the driver as it is reasonable to assume that the battery may not be present. Update the is_present flag immediately in the IRQ. Signed-off-by: Phil Reid <preid@electromag.com.au> --- drivers/power/sbs-battery.c | 122 +++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 63 deletions(-)