| Message ID | 1474278372-38077-1-git-send-email-preid@electromag.com.au (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On Mon, Sep 19, 2016 at 05:46:12PM +0800, Phil Reid wrote: > There where still a few lingering references to pdata after commit > power: supply: sbs-battery: simplify DT parsing. > > Remove pdata from struct·sbs_info and conditional checks to ser if this > was set from the i2c read / write functions. > Instead of call max in each function for incrementing poll_retry_count > do it once in the probe function. > Fixup null pointer dereference in to pdata in sbs_external_power_changed. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > drivers/power/supply/sbs-battery.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 248a5dd..3ffa0ec 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -163,7 +163,6 @@ static enum power_supply_property sbs_properties[] = { > struct sbs_info { > struct i2c_client *client; > struct power_supply *power_supply; > - struct sbs_platform_data *pdata; > bool is_present; > struct gpio_desc *gpio_detect; > bool enable_detection; > @@ -185,8 +184,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address) > s32 ret = 0; > int retries = 1; > > - if (chip->pdata) > - retries = max(chip->i2c_retry_count + 1, 1); > + retries = chip->i2c_retry_count + 1; Why + 1? This should already be done in probe(). > while (retries > 0) { > ret = i2c_smbus_read_word_data(client, address); > @@ -213,10 +211,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, > int retries_length = 1, retries_block = 1; > u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; > > - if (chip->pdata) { > - retries_length = max(chip->i2c_retry_count + 1, 1); > - retries_block = max(chip->i2c_retry_count + 1, 1); > - } > + retries_length = chip->i2c_retry_count; > + retries_block = chip->i2c_retry_count; > > /* Adapter needs to support these two functions */ > if (!i2c_check_functionality(client->adapter, > @@ -280,8 +276,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address, > s32 ret = 0; > int retries = 1; > > - if (chip->pdata) > - retries = max(chip->i2c_retry_count + 1, 1); > + retries = max(chip->i2c_retry_count + 1, 1); Here we should be able to drop max() and +1, too. > while (retries > 0) { > ret = i2c_smbus_write_word_data(client, address, > @@ -707,7 +702,7 @@ static void sbs_external_power_changed(struct power_supply *psy) > cancel_delayed_work_sync(&chip->work); > > schedule_delayed_work(&chip->work, HZ); > - chip->poll_time = chip->pdata->poll_retry_count; > + chip->poll_time = chip->poll_retry_count; > } > > static void sbs_delayed_work(struct work_struct *work) > @@ -791,7 +786,7 @@ static int sbs_probe(struct i2c_client *client, > rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count", > &chip->i2c_retry_count); > if (rc) > - chip->i2c_retry_count = 1; > + chip->i2c_retry_count = 0; I think 0 was correct, since you do a + 1 later. > rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count", > &chip->poll_retry_count); > @@ -802,6 +797,7 @@ static int sbs_probe(struct i2c_client *client, > chip->poll_retry_count = pdata->poll_retry_count; > chip->i2c_retry_count = pdata->i2c_retry_count; > } > + chip->i2c_retry_count = max(chip->i2c_retry_count + 1, 1); > > chip->gpio_detect = devm_gpiod_get_optional(&client->dev, > "sbs,battery-detect", GPIOD_IN); I still think we should make i2c_retry_count and poll_retry_count unsigned and skip the max() part completely. -- Sebastian
G'day Sebastian, On 20/09/2016 02:47, Sebastian Reichel wrote: > Hi, > > On Mon, Sep 19, 2016 at 05:46:12PM +0800, Phil Reid wrote: >> There where still a few lingering references to pdata after commit >> power: supply: sbs-battery: simplify DT parsing. >> >> Remove pdata from struct·sbs_info and conditional checks to ser if this >> was set from the i2c read / write functions. >> Instead of call max in each function for incrementing poll_retry_count >> do it once in the probe function. >> Fixup null pointer dereference in to pdata in sbs_external_power_changed. >> >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- >> drivers/power/supply/sbs-battery.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c >> index 248a5dd..3ffa0ec 100644 >> --- a/drivers/power/supply/sbs-battery.c >> +++ b/drivers/power/supply/sbs-battery.c >> @@ -163,7 +163,6 @@ static enum power_supply_property sbs_properties[] = { >> struct sbs_info { >> struct i2c_client *client; >> struct power_supply *power_supply; >> - struct sbs_platform_data *pdata; >> bool is_present; >> struct gpio_desc *gpio_detect; >> bool enable_detection; >> @@ -185,8 +184,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address) >> s32 ret = 0; >> int retries = 1; >> >> - if (chip->pdata) >> - retries = max(chip->i2c_retry_count + 1, 1); >> + retries = chip->i2c_retry_count + 1; > > Why + 1? This should already be done in probe(). Yeap my mistake. > >> while (retries > 0) { >> ret = i2c_smbus_read_word_data(client, address); >> @@ -213,10 +211,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, >> int retries_length = 1, retries_block = 1; >> u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; >> >> - if (chip->pdata) { >> - retries_length = max(chip->i2c_retry_count + 1, 1); >> - retries_block = max(chip->i2c_retry_count + 1, 1); >> - } >> + retries_length = chip->i2c_retry_count; >> + retries_block = chip->i2c_retry_count; >> >> /* Adapter needs to support these two functions */ >> if (!i2c_check_functionality(client->adapter, >> @@ -280,8 +276,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address, >> s32 ret = 0; >> int retries = 1; >> >> - if (chip->pdata) >> - retries = max(chip->i2c_retry_count + 1, 1); >> + retries = max(chip->i2c_retry_count + 1, 1); > > Here we should be able to drop max() and +1, too. > >> while (retries > 0) { >> ret = i2c_smbus_write_word_data(client, address, >> @@ -707,7 +702,7 @@ static void sbs_external_power_changed(struct power_supply *psy) >> cancel_delayed_work_sync(&chip->work); >> >> schedule_delayed_work(&chip->work, HZ); >> - chip->poll_time = chip->pdata->poll_retry_count; >> + chip->poll_time = chip->poll_retry_count; >> } >> >> static void sbs_delayed_work(struct work_struct *work) >> @@ -791,7 +786,7 @@ static int sbs_probe(struct i2c_client *client, >> rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count", >> &chip->i2c_retry_count); >> if (rc) >> - chip->i2c_retry_count = 1; >> + chip->i2c_retry_count = 0; > > I think 0 was correct, since you do a + 1 later. > >> rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count", >> &chip->poll_retry_count); >> @@ -802,6 +797,7 @@ static int sbs_probe(struct i2c_client *client, >> chip->poll_retry_count = pdata->poll_retry_count; >> chip->i2c_retry_count = pdata->i2c_retry_count; >> } >> + chip->i2c_retry_count = max(chip->i2c_retry_count + 1, 1); >> >> chip->gpio_detect = devm_gpiod_get_optional(&client->dev, >> "sbs,battery-detect", GPIOD_IN); > > I still think we should make i2c_retry_count and poll_retry_count > unsigned and skip the max() part completely. > Will do.
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c index 248a5dd..3ffa0ec 100644 --- a/drivers/power/supply/sbs-battery.c +++ b/drivers/power/supply/sbs-battery.c @@ -163,7 +163,6 @@ static enum power_supply_property sbs_properties[] = { struct sbs_info { struct i2c_client *client; struct power_supply *power_supply; - struct sbs_platform_data *pdata; bool is_present; struct gpio_desc *gpio_detect; bool enable_detection; @@ -185,8 +184,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address) s32 ret = 0; int retries = 1; - if (chip->pdata) - retries = max(chip->i2c_retry_count + 1, 1); + retries = chip->i2c_retry_count + 1; while (retries > 0) { ret = i2c_smbus_read_word_data(client, address); @@ -213,10 +211,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, int retries_length = 1, retries_block = 1; u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1]; - if (chip->pdata) { - retries_length = max(chip->i2c_retry_count + 1, 1); - retries_block = max(chip->i2c_retry_count + 1, 1); - } + retries_length = chip->i2c_retry_count; + retries_block = chip->i2c_retry_count; /* Adapter needs to support these two functions */ if (!i2c_check_functionality(client->adapter, @@ -280,8 +276,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address, s32 ret = 0; int retries = 1; - if (chip->pdata) - retries = max(chip->i2c_retry_count + 1, 1); + retries = max(chip->i2c_retry_count + 1, 1); while (retries > 0) { ret = i2c_smbus_write_word_data(client, address, @@ -707,7 +702,7 @@ static void sbs_external_power_changed(struct power_supply *psy) cancel_delayed_work_sync(&chip->work); schedule_delayed_work(&chip->work, HZ); - chip->poll_time = chip->pdata->poll_retry_count; + chip->poll_time = chip->poll_retry_count; } static void sbs_delayed_work(struct work_struct *work) @@ -791,7 +786,7 @@ static int sbs_probe(struct i2c_client *client, rc = of_property_read_u32(client->dev.of_node, "sbs,i2c-retry-count", &chip->i2c_retry_count); if (rc) - chip->i2c_retry_count = 1; + chip->i2c_retry_count = 0; rc = of_property_read_u32(client->dev.of_node, "sbs,poll-retry-count", &chip->poll_retry_count); @@ -802,6 +797,7 @@ static int sbs_probe(struct i2c_client *client, chip->poll_retry_count = pdata->poll_retry_count; chip->i2c_retry_count = pdata->i2c_retry_count; } + chip->i2c_retry_count = max(chip->i2c_retry_count + 1, 1); chip->gpio_detect = devm_gpiod_get_optional(&client->dev, "sbs,battery-detect", GPIOD_IN);
There where still a few lingering references to pdata after commit power: supply: sbs-battery: simplify DT parsing. Remove pdata from struct·sbs_info and conditional checks to ser if this was set from the i2c read / write functions. Instead of call max in each function for incrementing poll_retry_count do it once in the probe function. Fixup null pointer dereference in to pdata in sbs_external_power_changed. Signed-off-by: Phil Reid <preid@electromag.com.au> --- drivers/power/supply/sbs-battery.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)