On 16.09.2015 02:58, Andreas Dannenberg wrote:
> A new optional device property called "ti,current-limit" is introduced
> to allow disabling the D+/D- USB signal-based charger type auto-
> detection algorithm used to set the input current limit and instead to
> use a fixed input current limit.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@xxxxxx>
> ---
> drivers/power/bq24257_charger.c | 102 +++++++++++++++++++++++++++++++---------
> 1 file changed, 81 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> index fdfe855..55e4ee4 100644
> --- a/drivers/power/bq24257_charger.c
> +++ b/drivers/power/bq24257_charger.c
> @@ -74,6 +74,7 @@ struct bq24257_init_data {
> u8 ichg; /* charge current */
> u8 vbat; /* regulation voltage */
> u8 iterm; /* termination current */
> + u8 in_ilimit; /* input current limit */
> };
>
> struct bq24257_state {
> @@ -100,6 +101,8 @@ struct bq24257_device {
> struct bq24257_state state;
>
> struct mutex lock; /* protect state data */
> +
> + bool in_ilimit_autoset_disable;
> };
>
> static bool bq24257_is_volatile_reg(struct device *dev, unsigned int reg)
> @@ -188,6 +191,12 @@ static const u32 bq24257_iterm_map[] = {
>
> #define BQ24257_ITERM_MAP_SIZE ARRAY_SIZE(bq24257_iterm_map)
>
> +static const u32 bq24257_iilimit_map[] = {
Too many ii? bq24257_ilimit_map? Or is it a shortcut for "in_ilimit"?
New fields have pattern like "in_ilimit*".
Anyway it's a confusing so maybe use everywhere the same pattern?
> + 100000, 150000, 500000, 900000, 1500000, 2000000
> +};
> +
> +#define BQ24257_IILIMIT_MAP_SIZE ARRAY_SIZE(bq24257_iilimit_map)
ditto: ILIMIT_MAP_SIZE?
> +
> static int bq24257_field_read(struct bq24257_device *bq,
> enum bq24257_fields field_id)
> {
> @@ -479,24 +488,35 @@ static void bq24257_handle_state_change(struct bq24257_device *bq,
> old_state = bq->state;
> mutex_unlock(&bq->lock);
>
> - if (!new_state->power_good) { /* power removed */
> - cancel_delayed_work_sync(&bq->iilimit_setup_work);
> -
> - /* activate D+/D- port detection algorithm */
> - ret = bq24257_field_write(bq, F_DPDM_EN, 1);
> - if (ret < 0)
> - goto error;
> + /*
> + * Handle BQ2425x state changes observing whether the D+/D- based input
> + * current limit autoset functionality is enabled.
> + */
> + if (!new_state->power_good) {
> + dev_dbg(bq->dev, "Power removed\n");
> + if (!bq->in_ilimit_autoset_disable) {
> + cancel_delayed_work_sync(&bq->iilimit_setup_work);
>
> - reset_iilimit = true;
> - } else if (!old_state.power_good) { /* power inserted */
> - config_iilimit = true;
> - } else if (new_state->fault == FAULT_NO_BAT) { /* battery removed */
> - cancel_delayed_work_sync(&bq->iilimit_setup_work);
> + /* activate D+/D- port detection algorithm */
> + ret = bq24257_field_write(bq, F_DPDM_EN, 1);
> + if (ret < 0)
> + goto error;
>
> - 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?
> + 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.
> + 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?
>
> @@ -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".
> +
> + /* 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.
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
