* [PATCH] power: supply: sbs-battery: simplify DT parsing
@ 2016-09-06 13:16 Arnd Bergmann
2016-09-06 23:55 ` Sebastian Reichel
2016-09-19 9:20 ` Phil Reid
0 siblings, 2 replies; 5+ messages in thread
From: Arnd Bergmann @ 2016-09-06 13:16 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Phil Reid, Arnd Bergmann, YH Huang, Joshua Clayton, linux-pm,
linux-kernel
After the change to use the gpio descriptor interface, we get a warning if
-Wmaybe-uninitialized is added back to the build flags (it is currently
disabled:
drivers/power/supply/sbs-battery.c: In function 'sbs_probe':
drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The problem is that if neither the DT properties nor a platform_data
pointer are provided, the chip->pdata pointer gets set to an uninitialized
value.
Looking at the code some more, I found that the sbs_of_populate_pdata
function is more complex than necessary and has confusing calling
conventions of possibly returning a valid pointer, a NULL pointer
or an ERR_PTR pointer (in addition to the uninitialized pointer).
To fix all of that, this gets rid of the chip->pdata pointer and
simply moves the two integers into the sbs_info structure. This
makes it much clearer from reading sbs_probe() what the precedence
of the three possible values are (pdata, DT, hardcoded defaults)
and completely avoids the #ifdef CONFIG_OF guards as
of_property_read_u32() gets replaced with a compile-time stub
when that is disabled, and returns an error if passed a NULL of_node
pointer.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect")
---
drivers/power/supply/sbs-battery.c | 98 ++++++++++++--------------------------
1 file changed, 31 insertions(+), 67 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 8b4c6a8681b0..248a5dd75e22 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -169,6 +169,8 @@ struct sbs_info {
bool enable_detection;
int last_state;
int poll_time;
+ int i2c_retry_count;
+ int poll_retry_count;
struct delayed_work work;
int ignore_changes;
};
@@ -184,7 +186,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address)
int retries = 1;
if (chip->pdata)
- retries = max(chip->pdata->i2c_retry_count + 1, 1);
+ retries = max(chip->i2c_retry_count + 1, 1);
while (retries > 0) {
ret = i2c_smbus_read_word_data(client, address);
@@ -212,8 +214,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
if (chip->pdata) {
- retries_length = max(chip->pdata->i2c_retry_count + 1, 1);
- retries_block = max(chip->pdata->i2c_retry_count + 1, 1);
+ retries_length = max(chip->i2c_retry_count + 1, 1);
+ retries_block = max(chip->i2c_retry_count + 1, 1);
}
/* Adapter needs to support these two functions */
@@ -279,7 +281,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
int retries = 1;
if (chip->pdata)
- retries = max(chip->pdata->i2c_retry_count + 1, 1);
+ retries = max(chip->i2c_retry_count + 1, 1);
while (retries > 0) {
ret = i2c_smbus_write_word_data(client, address,
@@ -741,61 +743,6 @@ static void sbs_delayed_work(struct work_struct *work)
}
}
-#if defined(CONFIG_OF)
-
-#include <linux/of_device.h>
-#include <linux/of_gpio.h>
-
-static const struct of_device_id sbs_dt_ids[] = {
- { .compatible = "sbs,sbs-battery" },
- { .compatible = "ti,bq20z75" },
- { }
-};
-MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-
-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;
- int rc;
- u32 prop;
-
- /* verify this driver matches this device */
- if (!of_node)
- return NULL;
-
- /* first make sure at least one property is set, otherwise
- * it won't change behavior from running without pdata.
- */
- if (!of_get_property(of_node, "sbs,i2c-retry-count", NULL) &&
- !of_get_property(of_node, "sbs,poll-retry-count", NULL))
- goto of_out;
-
- pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data),
- GFP_KERNEL);
- if (!pdata)
- return ERR_PTR(-ENOMEM);
-
- rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop);
- if (!rc)
- pdata->i2c_retry_count = prop;
-
- rc = of_property_read_u32(of_node, "sbs,poll-retry-count", &prop);
- if (!rc)
- pdata->poll_retry_count = prop;
-
-of_out:
- return pdata;
-}
-#else
-static struct sbs_platform_data *sbs_of_populate_pdata(
- struct sbs_info *client)
-{
- return ERR_PTR(-ENOSYS);
-}
-#endif
-
static const struct power_supply_desc sbs_default_desc = {
.type = POWER_SUPPLY_TYPE_BATTERY,
.properties = sbs_properties,
@@ -838,13 +785,23 @@ static int sbs_probe(struct i2c_client *client,
chip->ignore_changes = 1;
chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN;
- if (!pdata)
- pdata = sbs_of_populate_pdata(chip);
-
- if (IS_ERR(pdata))
- return PTR_ERR(pdata);
-
- chip->pdata = pdata;
+ /* use pdata if available, fall back to DT properties,
+ * or hardcoded defaults if not
+ */
+ rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count",
+ &chip->i2c_retry_count);
+ if (rc)
+ chip->i2c_retry_count = 1;
+
+ rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count",
+ &chip->poll_retry_count);
+ if (rc)
+ chip->poll_retry_count = 0;
+
+ if (pdata) {
+ chip->poll_retry_count = pdata->poll_retry_count;
+ chip->i2c_retry_count = pdata->i2c_retry_count;
+ }
chip->gpio_detect = devm_gpiod_get_optional(&client->dev,
"sbs,battery-detect", GPIOD_IN);
@@ -953,13 +910,20 @@ static const struct i2c_device_id sbs_id[] = {
};
MODULE_DEVICE_TABLE(i2c, sbs_id);
+static const struct of_device_id sbs_dt_ids[] = {
+ { .compatible = "sbs,sbs-battery" },
+ { .compatible = "ti,bq20z75" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sbs_dt_ids);
+
static struct i2c_driver sbs_battery_driver = {
.probe = sbs_probe,
.remove = sbs_remove,
.id_table = sbs_id,
.driver = {
.name = "sbs-battery",
- .of_match_table = of_match_ptr(sbs_dt_ids),
+ .of_match_table = sbs_dt_ids,
.pm = SBS_PM_OPS,
},
};
--
2.9.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] power: supply: sbs-battery: simplify DT parsing
2016-09-06 13:16 [PATCH] power: supply: sbs-battery: simplify DT parsing Arnd Bergmann
@ 2016-09-06 23:55 ` Sebastian Reichel
2016-09-07 7:58 ` Arnd Bergmann
2016-09-19 9:20 ` Phil Reid
1 sibling, 1 reply; 5+ messages in thread
From: Sebastian Reichel @ 2016-09-06 23:55 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Phil Reid, YH Huang, Joshua Clayton, linux-pm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]
Hi Arnd,
On Tue, Sep 06, 2016 at 03:16:33PM +0200, Arnd Bergmann wrote:
> After the change to use the gpio descriptor interface, we get a warning if
> -Wmaybe-uninitialized is added back to the build flags (it is currently
> disabled:
>
> drivers/power/supply/sbs-battery.c: In function 'sbs_probe':
> drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The problem is that if neither the DT properties nor a platform_data
> pointer are provided, the chip->pdata pointer gets set to an uninitialized
> value.
>
> Looking at the code some more, I found that the sbs_of_populate_pdata
> function is more complex than necessary and has confusing calling
> conventions of possibly returning a valid pointer, a NULL pointer
> or an ERR_PTR pointer (in addition to the uninitialized pointer).
>
> To fix all of that, this gets rid of the chip->pdata pointer and
> simply moves the two integers into the sbs_info structure. This
> makes it much clearer from reading sbs_probe() what the precedence
> of the three possible values are (pdata, DT, hardcoded defaults)
> and completely avoids the #ifdef CONFIG_OF guards as
> of_property_read_u32() gets replaced with a compile-time stub
> when that is disabled, and returns an error if passed a NULL of_node
> pointer.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect")
Patch looks fine to me. Actually I already asked Phil to
implement your change [0]. I just queued it to power-supply's
for-next branch.
[0] https://marc.info/?l=linux-pm&m=147273382018953&w=2
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] power: supply: sbs-battery: simplify DT parsing
2016-09-06 23:55 ` Sebastian Reichel
@ 2016-09-07 7:58 ` Arnd Bergmann
2016-09-07 13:17 ` Sebastian Reichel
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2016-09-07 7:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Phil Reid, YH Huang, Joshua Clayton, linux-pm, linux-kernel
On Wednesday, September 7, 2016 1:55:23 AM CEST Sebastian Reichel wrote:
>
> Patch looks fine to me. Actually I already asked Phil to
> implement your change [0]. I just queued it to power-supply's
> for-next branch.
>
> [0] https://marc.info/?l=linux-pm&m=147273382018953&w=2
Glad to see we had the same thought. ;-)
I see that we went for different defaults for chip->poll_retry_count:
I concluded that '0' would be the correct default, while you suggested '1'
in your reply.
Can you double-check the code and see which one is right?
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] power: supply: sbs-battery: simplify DT parsing
2016-09-07 7:58 ` Arnd Bergmann
@ 2016-09-07 13:17 ` Sebastian Reichel
0 siblings, 0 replies; 5+ messages in thread
From: Sebastian Reichel @ 2016-09-07 13:17 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Phil Reid, YH Huang, Joshua Clayton, linux-pm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 725 bytes --]
Hi,
On Wed, Sep 07, 2016 at 09:58:10AM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 1:55:23 AM CEST Sebastian Reichel wrote:
> >
> > Patch looks fine to me. Actually I already asked Phil to
> > implement your change [0]. I just queued it to power-supply's
> > for-next branch.
> >
> > [0] https://marc.info/?l=linux-pm&m=147273382018953&w=2
>
> Glad to see we had the same thought. ;-)
>
> I see that we went for different defaults for chip->poll_retry_count:
> I concluded that '0' would be the correct default, while you suggested '1'
> in your reply.
>
> Can you double-check the code and see which one is right?
0 is correct. It will be incremented to 1 later.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] power: supply: sbs-battery: simplify DT parsing
2016-09-06 13:16 [PATCH] power: supply: sbs-battery: simplify DT parsing Arnd Bergmann
2016-09-06 23:55 ` Sebastian Reichel
@ 2016-09-19 9:20 ` Phil Reid
1 sibling, 0 replies; 5+ messages in thread
From: Phil Reid @ 2016-09-19 9:20 UTC (permalink / raw)
To: Arnd Bergmann, Sebastian Reichel
Cc: YH Huang, Joshua Clayton, linux-pm, linux-kernel
G'day Arnd,
Sorry for the delay.
Been distracted and just catching up on mail I was just getting back to
implementing basically this same thing as requested by Sebastian when I saw your change.
Couple of comments below, I haven't tested it.
On 6/09/2016 21:16, Arnd Bergmann wrote:
> After the change to use the gpio descriptor interface, we get a warning if
> -Wmaybe-uninitialized is added back to the build flags (it is currently
> disabled:
>
> drivers/power/supply/sbs-battery.c: In function 'sbs_probe':
> drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The problem is that if neither the DT properties nor a platform_data
> pointer are provided, the chip->pdata pointer gets set to an uninitialized
> value.
>
> Looking at the code some more, I found that the sbs_of_populate_pdata
> function is more complex than necessary and has confusing calling
> conventions of possibly returning a valid pointer, a NULL pointer
> or an ERR_PTR pointer (in addition to the uninitialized pointer).
>
> To fix all of that, this gets rid of the chip->pdata pointer and
> simply moves the two integers into the sbs_info structure. This
> makes it much clearer from reading sbs_probe() what the precedence
> of the three possible values are (pdata, DT, hardcoded defaults)
> and completely avoids the #ifdef CONFIG_OF guards as
> of_property_read_u32() gets replaced with a compile-time stub
> when that is disabled, and returns an error if passed a NULL of_node
> pointer.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect")
> ---
> drivers/power/supply/sbs-battery.c | 98 ++++++++++++--------------------------
> 1 file changed, 31 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 8b4c6a8681b0..248a5dd75e22 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -169,6 +169,8 @@ struct sbs_info {
> bool enable_detection;
> int last_state;
> int poll_time;
> + int i2c_retry_count;
> + int poll_retry_count;
> struct delayed_work work;
> int ignore_changes;
> };
Done we need to remove pdata from here now. It's not set in the probe funciton anymore.
> @@ -184,7 +186,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address)
> int retries = 1;
>
> if (chip->pdata)
> - retries = max(chip->pdata->i2c_retry_count + 1, 1);
> + retries = max(chip->i2c_retry_count + 1, 1);
I don't think this is quite right. as pdata is never set, condition needs to removed.
>
> while (retries > 0) {
> ret = i2c_smbus_read_word_data(client, address);
> @@ -212,8 +214,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
> u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
>
> if (chip->pdata) {
> - retries_length = max(chip->pdata->i2c_retry_count + 1, 1);
> - retries_block = max(chip->pdata->i2c_retry_count + 1, 1);
> + retries_length = max(chip->i2c_retry_count + 1, 1);
> + retries_block = max(chip->i2c_retry_count + 1, 1);
> }
ditto
>
> /* Adapter needs to support these two functions */
> @@ -279,7 +281,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
> int retries = 1;
>
> if (chip->pdata)
> - retries = max(chip->pdata->i2c_retry_count + 1, 1);
> + retries = max(chip->i2c_retry_count + 1, 1);
ditto
>
> while (retries > 0) {
> ret = i2c_smbus_write_word_data(client, address,
> @@ -741,61 +743,6 @@ static void sbs_delayed_work(struct work_struct *work)
> }
> }
>
> -#if defined(CONFIG_OF)
> -
> -#include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> -
> -static const struct of_device_id sbs_dt_ids[] = {
> - { .compatible = "sbs,sbs-battery" },
> - { .compatible = "ti,bq20z75" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, sbs_dt_ids);
> -
> -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;
> - int rc;
> - u32 prop;
> -
> - /* verify this driver matches this device */
> - if (!of_node)
> - return NULL;
> -
> - /* first make sure at least one property is set, otherwise
> - * it won't change behavior from running without pdata.
> - */
> - if (!of_get_property(of_node, "sbs,i2c-retry-count", NULL) &&
> - !of_get_property(of_node, "sbs,poll-retry-count", NULL))
> - goto of_out;
> -
> - pdata = devm_kzalloc(&client->dev, sizeof(struct sbs_platform_data),
> - GFP_KERNEL);
> - if (!pdata)
> - return ERR_PTR(-ENOMEM);
> -
> - rc = of_property_read_u32(of_node, "sbs,i2c-retry-count", &prop);
> - if (!rc)
> - pdata->i2c_retry_count = prop;
> -
> - rc = of_property_read_u32(of_node, "sbs,poll-retry-count", &prop);
> - if (!rc)
> - pdata->poll_retry_count = prop;
> -
> -of_out:
> - return pdata;
> -}
> -#else
> -static struct sbs_platform_data *sbs_of_populate_pdata(
> - struct sbs_info *client)
> -{
> - return ERR_PTR(-ENOSYS);
> -}
> -#endif
> -
> static const struct power_supply_desc sbs_default_desc = {
> .type = POWER_SUPPLY_TYPE_BATTERY,
> .properties = sbs_properties,
> @@ -838,13 +785,23 @@ static int sbs_probe(struct i2c_client *client,
> chip->ignore_changes = 1;
> chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN;
>
> - if (!pdata)
> - pdata = sbs_of_populate_pdata(chip);
> -
> - if (IS_ERR(pdata))
> - return PTR_ERR(pdata);
> -
> - chip->pdata = pdata;
No longer set here.
> + /* use pdata if available, fall back to DT properties,
> + * or hardcoded defaults if not
> + */
> + rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count",
> + &chip->i2c_retry_count);
> + if (rc)
> + chip->i2c_retry_count = 1;
> +
> + rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count",
> + &chip->poll_retry_count);
> + if (rc)
> + chip->poll_retry_count = 0;
> +
> + if (pdata) {
> + chip->poll_retry_count = pdata->poll_retry_count;
> + chip->i2c_retry_count = pdata->i2c_retry_count;
> + }
>
> chip->gpio_detect = devm_gpiod_get_optional(&client->dev,
> "sbs,battery-detect", GPIOD_IN);
> @@ -953,13 +910,20 @@ static const struct i2c_device_id sbs_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, sbs_id);
>
> +static const struct of_device_id sbs_dt_ids[] = {
> + { .compatible = "sbs,sbs-battery" },
> + { .compatible = "ti,bq20z75" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sbs_dt_ids);
> +
> static struct i2c_driver sbs_battery_driver = {
> .probe = sbs_probe,
> .remove = sbs_remove,
> .id_table = sbs_id,
> .driver = {
> .name = "sbs-battery",
> - .of_match_table = of_match_ptr(sbs_dt_ids),
> + .of_match_table = sbs_dt_ids,
> .pm = SBS_PM_OPS,
> },
> };
>
Also sbs_external_power_changed is still referencing chip->pdata->poll_retry_count.
Which I think will result in null pointer dereference now.
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-19 9:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 13:16 [PATCH] power: supply: sbs-battery: simplify DT parsing Arnd Bergmann
2016-09-06 23:55 ` Sebastian Reichel
2016-09-07 7:58 ` Arnd Bergmann
2016-09-07 13:17 ` Sebastian Reichel
2016-09-19 9:20 ` Phil Reid
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).