On 17.09.2015 04:23, Andreas Dannenberg wrote:
> On Wed, Sep 16, 2015 at 09:41:49AM +0900, Krzysztof Kozlowski wrote:
>>> - reset_iilimit = true;
>>> - } else if (old_state.fault == FAULT_NO_BAT) { /* battery connected */
>>> - config_iilimit = true;
>>> - } else if (new_state->fault == FAULT_TIMER) { /* safety timer expired */
>>> + reset_iilimit = true;
>>> + }
>>> + } else if (!old_state.power_good) {
>>> + dev_dbg(bq->dev, "Power inserted\n");
>>> + config_iilimit = !bq->in_ilimit_autoset_disable;
>>> + } else if (new_state->fault == FAULT_NO_BAT) {
>>> + dev_warn(bq->dev, "Battery removed\n");
>>
>> dev_warn? This wasn't here before... It's a bit too serious. Is removing
>> a battery an error condition? Maybe user just unplugged it?
>> dev_dbg or dev_info should be sufficient.
>>
>> BTW, it is useful to quickly find boot regressions with "dmesg -l
>> warn,err". Removing a battery probably is not an error?
>
> I would argue that most devices that use a Li-Ion battery have the
> battery built-in and it's not user removable. Therefore, if the battery
> would ever go disconnected I figured it'll most likely be something
> serious such as a contact breaking loose or something else dramatic,
> warranting at least a warning. Plus, many devices with built in Li-Ion
> batteries are actually designed in a way that they don't really function
> properly with the battery taken out as the HW is often designed to draw
> supplemental current from the battery during times of load in addition
> to the A/C supply (key feature of many charger chips).
Okay, I guess if there ever will be an user annoyed by dmesg's after
removing the battery we can always revisit this. :)
>
>>
>>> + if (!bq->in_ilimit_autoset_disable) {
>>> + cancel_delayed_work_sync(&bq->iilimit_setup_work);
>>> + reset_iilimit = true;
>>> + }
>>> + } else if (old_state.fault == FAULT_NO_BAT) {
>>> + dev_warn(bq->dev, "Battery connected\n");
>>
>> Definitely not a warn. Inserting a battery is not an error condition.
>
> Same as above.
OK
>
>>> + config_iilimit = !bq->in_ilimit_autoset_disable;
>>> + } else if (new_state->fault == FAULT_TIMER) {
>>> dev_err(bq->dev, "Safety timer expired! Battery dead?\n");
>>> }
>>
>> Don't you have a schedule_delayed_work() call here which now will be
>> executed always? Even when work was not INIT and nothing will cancel it?
>
> It'll be more obvious when looking at the merged code but the schedule_
> delayed_work() call only happens if config_iilimit==true which only
> happens when the input limit current autoset functionality is not
> disabled. If that's the case (autoset functionality is enabled) the INIT
> for that work is executed during probe.
>
OK
>>>
>>> @@ -581,7 +601,16 @@ static int bq24257_hw_init(struct bq24257_device *bq)
>>> bq->state = state;
>>> mutex_unlock(&bq->lock);
>>>
>>> - if (!state.power_good)
>>> + if (bq->in_ilimit_autoset_disable) {
>>> + dev_dbg(bq->dev, "manually setting iilimit = %d\n",
>>> + bq->init_data.in_ilimit);
>>
>> Nit, no actual difference but makes more sense to me because field is
>> u8: "%u".
>
> Yes that should be changed.
>
>>> +
>>> + /* program fixed input current limit */
>>> + ret = bq24257_field_write(bq, F_IILIMIT,
>>> + bq->init_data.in_ilimit);
>>> + if (ret < 0)
>>> + return ret;
>>> + } else if (!state.power_good)
>>> /* activate D+/D- detection algorithm */
>>> ret = bq24257_field_write(bq, F_DPDM_EN, 1);
>>> else if (state.fault != FAULT_NO_BAT)
>>> @@ -659,6 +688,7 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
>>> int ret;
>>> u32 property;
>>>
>>> + /* Required properties */
>>> ret = device_property_read_u32(bq->dev, "ti,charge-current", &property);
>>> if (ret < 0)
>>> return ret;
>>> @@ -682,6 +712,24 @@ static int bq24257_fw_probe(struct bq24257_device *bq)
>>> bq->init_data.iterm = bq24257_find_idx(property, bq24257_iterm_map,
>>> BQ24257_ITERM_MAP_SIZE);
>>>
>>> + /* Optional properties. If not provided use reasonable default. */
>>> + ret = device_property_read_u32(bq->dev, "ti,current-limit",
>>> + &property);
>>> + if (ret < 0)
>>> + /*
>>> + * Explicitly set a default value which will be needed for
>>> + * devices that don't support the automatic setting of the input
>>> + * current limit through the charger type detection mechanism.
>>> + */
>>> + bq->init_data.in_ilimit = IILIMIT_500;
>>> + else {
>>> + bq->in_ilimit_autoset_disable = true;
>>> + bq->init_data.in_ilimit =
>>> + bq24257_find_idx(property,
>>> + bq24257_iilimit_map,
>>> + BQ24257_IILIMIT_MAP_SIZE);
>>> + }
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -740,8 +788,6 @@ static int bq24257_probe(struct i2c_client *client,
>>>
>>> i2c_set_clientdata(client, bq);
>>>
>>> - INIT_DELAYED_WORK(&bq->iilimit_setup_work, bq24257_iilimit_setup_work);
>>> -
>>> if (!dev->platform_data) {
>>> ret = bq24257_fw_probe(bq);
>>> if (ret < 0) {
>>> @@ -752,6 +798,18 @@ static int bq24257_probe(struct i2c_client *client,
>>> return -ENODEV;
>>> }
>>>
>>> + /*
>>> + * The BQ24250 doesn't support the D+/D- based charger type detection
>>> + * used for the automatic setting of the input current limit setting so
>>> + * explicitly disable that feature.
>>> + */
>>> + if (bq->chip == BQ24250)
>>> + bq->in_ilimit_autoset_disable = true;
>>> +
>>> + if (!bq->in_ilimit_autoset_disable)
>>
>> In most places you have quite obfuscated negation of
>> "autoset_disable"... So maybe just "bq->in_ilimit_autoset" or
>> "bq->in_ilimit_autoset_enable" and the negation won't be needed? It
>> could be more readable.
>
> This stems from the fact that this was initially tied to a boolean DT
> property with the same name which is no longer there. The other reason
> was setting this property to non-zero changes the default behavior of
> the driver (that used to have autoset always enabled) so I wanted to
> reflect this behavior in the logic. The driver was tested pretty well so
> unless you feel strongly about this I would rather leave it as-is.
IMHO the code would be more readable but I don't insist.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
