Re: [PATCH] bq2415x_charger: Allow to load and use driver even if notify device is not registered yet

[Pali RohÃr]

On Saturday 25 July 2015 23:46:56 Pali RohÃr wrote:
> Driver bq2415x_charger works also without notify power supply device for
> charger detection. But when charger detection is specified in DT, then
> bq2415x_charger refused to loaded with -EPROBE_DEFER.
> 
> This patch rewrites code so that notify device for charger detection is
> checked when power supply event is received and not when registering power
> supply device. So this patch allows to use bq2415x_charger driver also when
> kernel is compiled without driver for notify power supply device.
> 
> Now after this patch scheduled workqueue is called after INIT_DELAYED_WORK,
> so it also fix problem when scheduled workqueue was called before init.
> 
> Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
> ---
>  drivers/power/bq2415x_charger.c |  143 +++++++++++++++++++++------------------
>  1 file changed, 79 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c
> index e98dcb6..739ef5b 100644
> --- a/drivers/power/bq2415x_charger.c
> +++ b/drivers/power/bq2415x_charger.c
> @@ -170,7 +170,7 @@ struct bq2415x_device {
>  	struct power_supply *charger;
>  	struct power_supply_desc charger_desc;
>  	struct delayed_work work;
> -	struct power_supply *notify_psy;
> +	struct device_node *notify_node;
>  	struct notifier_block nb;
>  	enum bq2415x_mode reported_mode;/* mode reported by hook function */
>  	enum bq2415x_mode mode;		/* currently configured mode */
> @@ -792,22 +792,47 @@ static int bq2415x_set_mode(struct bq2415x_device *bq, enum bq2415x_mode mode)
>  
>  }
>  
> +static bool bq2415x_update_reported_mode(struct bq2415x_device *bq, int mA)
> +{
> +	enum bq2415x_mode mode;
> +
> +	if (mA == 0)
> +		mode = BQ2415X_MODE_OFF;
> +	else if (mA < 500)
> +		mode = BQ2415X_MODE_NONE;
> +	else if (mA < 1800)
> +		mode = BQ2415X_MODE_HOST_CHARGER;
> +	else
> +		mode = BQ2415X_MODE_DEDICATED_CHARGER;
> +
> +	if (bq->reported_mode == mode)
> +		return false;
> +
> +	bq->reported_mode = mode;
> +	return true;
> +}
> +
>  static int bq2415x_notifier_call(struct notifier_block *nb,
>  		unsigned long val, void *v)
>  {
>  	struct bq2415x_device *bq =
>  		container_of(nb, struct bq2415x_device, nb);
>  	struct power_supply *psy = v;
> -	enum bq2415x_mode mode;
>  	union power_supply_propval prop;
>  	int ret;
> -	int mA;
>  
>  	if (val != PSY_EVENT_PROP_CHANGED)
>  		return NOTIFY_OK;
>  
> -	if (psy != bq->notify_psy)
> -		return NOTIFY_OK;
> +	/* Ignore event if it was not send by notify_node/notify_device */
> +	if (bq->notify_node) {
> +		if (psy->dev.parent &&
> +		    psy->dev.parent->of_node != bq->notify_node)
> +			return NOTIFY_OK;

There is missing branch for case when psy->dev.parent is NULL. Logical
error... I will send new version of patch.

Correct logic should be: ignore psy dev which sent event if it is not
notify dev specified in board/DT config of bq2415x psy dev.

> +	} else if (bq->init_data.notify_device) {
> +		if (strcmp(psy->desc->name, bq->init_data.notify_device) != 0)
> +			return NOTIFY_OK;
> +	}
>  
>  	dev_dbg(bq->dev, "notifier call was called\n");
>  
> @@ -816,22 +841,9 @@ static int bq2415x_notifier_call(struct notifier_block *nb,
>  	if (ret != 0)
>  		return NOTIFY_OK;
>  
> -	mA = prop.intval;
> -
> -	if (mA == 0)
> -		mode = BQ2415X_MODE_OFF;
> -	else if (mA < 500)
> -		mode = BQ2415X_MODE_NONE;
> -	else if (mA < 1800)
> -		mode = BQ2415X_MODE_HOST_CHARGER;
> -	else
> -		mode = BQ2415X_MODE_DEDICATED_CHARGER;
> -
> -	if (bq->reported_mode == mode)
> +	if (!bq2415x_update_reported_mode(bq, prop.intval))
>  		return NOTIFY_OK;
>  
> -	bq->reported_mode = mode;
> -
>  	/* if automode is not enabled do not tell about reported_mode */
>  	if (bq->automode < 1)
>  		return NOTIFY_OK;
> @@ -1536,6 +1548,8 @@ static int bq2415x_probe(struct i2c_client *client,
>  	struct device_node *np = client->dev.of_node;
>  	struct bq2415x_platform_data *pdata = client->dev.platform_data;
>  	const struct acpi_device_id *acpi_id = NULL;
> +	struct power_supply *notify_psy = NULL;
> +	union power_supply_propval prop;
>  
>  	if (!np && !pdata && !ACPI_HANDLE(&client->dev)) {
>  		dev_err(&client->dev, "Neither devicetree, nor platform data, nor ACPI support\n");
> @@ -1569,25 +1583,6 @@ static int bq2415x_probe(struct i2c_client *client,
>  		goto error_2;
>  	}
>  
> -	if (np) {
> -		bq->notify_psy = power_supply_get_by_phandle(np,
> -						"ti,usb-charger-detection");
> -
> -		if (IS_ERR(bq->notify_psy)) {
> -			dev_info(&client->dev,
> -				 "no 'ti,usb-charger-detection' property (err=%ld)\n",
> -				PTR_ERR(bq->notify_psy));
> -			bq->notify_psy = NULL;
> -		} else if (!bq->notify_psy) {
> -			ret = -EPROBE_DEFER;
> -			goto error_2;
> -		}
> -	} else if (pdata && pdata->notify_device) {
> -		bq->notify_psy = power_supply_get_by_name(pdata->notify_device);
> -	} else {
> -		bq->notify_psy = NULL;
> -	}
> -
>  	i2c_set_clientdata(client, bq);
>  
>  	bq->id = num;
> @@ -1607,32 +1602,35 @@ static int bq2415x_probe(struct i2c_client *client,
>  					       "ti,current-limit",
>  					       &bq->init_data.current_limit);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
>  		ret = device_property_read_u32(bq->dev,
>  					"ti,weak-battery-voltage",
>  					&bq->init_data.weak_battery_voltage);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
>  		ret = device_property_read_u32(bq->dev,
>  				"ti,battery-regulation-voltage",
>  				&bq->init_data.battery_regulation_voltage);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
>  		ret = device_property_read_u32(bq->dev,
>  					       "ti,charge-current",
>  					       &bq->init_data.charge_current);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
>  		ret = device_property_read_u32(bq->dev,
>  				"ti,termination-current",
>  				&bq->init_data.termination_current);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
>  		ret = device_property_read_u32(bq->dev,
>  					       "ti,resistor-sense",
>  					       &bq->init_data.resistor_sense);
>  		if (ret)
> -			goto error_3;
> +			goto error_2;
> +		if (np)
> +			bq->notify_node = of_parse_phandle(np,
> +						"ti,usb-charger-detection", 0);
>  	} else {
>  		memcpy(&bq->init_data, pdata, sizeof(bq->init_data));
>  	}
> @@ -1642,56 +1640,72 @@ static int bq2415x_probe(struct i2c_client *client,
>  	ret = bq2415x_power_supply_init(bq);
>  	if (ret) {
>  		dev_err(bq->dev, "failed to register power supply: %d\n", ret);
> -		goto error_3;
> +		goto error_2;
>  	}
>  
>  	ret = bq2415x_sysfs_init(bq);
>  	if (ret) {
>  		dev_err(bq->dev, "failed to create sysfs entries: %d\n", ret);
> -		goto error_4;
> +		goto error_3;
>  	}
>  
>  	ret = bq2415x_set_defaults(bq);
>  	if (ret) {
>  		dev_err(bq->dev, "failed to set default values: %d\n", ret);
> -		goto error_5;
> +		goto error_4;
>  	}
>  
> -	if (bq->notify_psy) {
> +	if (bq->notify_node || bq->init_data.notify_device) {
>  		bq->nb.notifier_call = bq2415x_notifier_call;
>  		ret = power_supply_reg_notifier(&bq->nb);
>  		if (ret) {
>  			dev_err(bq->dev, "failed to reg notifier: %d\n", ret);
> -			goto error_6;
> +			goto error_4;
>  		}
>  
> -		/* Query for initial reported_mode and set it */
> -		bq2415x_notifier_call(&bq->nb, PSY_EVENT_PROP_CHANGED,
> -				      bq->notify_psy);
> -		bq2415x_set_mode(bq, bq->reported_mode);
> -
>  		bq->automode = 1;
> -		dev_info(bq->dev, "automode enabled\n");
> +		dev_info(bq->dev, "automode supported, waiting for events\n");
>  	} else {
>  		bq->automode = -1;
>  		dev_info(bq->dev, "automode not supported\n");
>  	}
>  
> +	/* Query for initial reported_mode and set it */
> +	if (bq->nb.notifier_call) {
> +		if (np) {
> +			notify_psy = power_supply_get_by_phandle(np,
> +						"ti,usb-charger-detection");
> +			if (IS_ERR(notify_psy))
> +				notify_psy = NULL;
> +		} else if (bq->init_data.notify_device) {
> +			notify_psy = power_supply_get_by_name(
> +						bq->init_data.notify_device);
> +		}
> +	}
> +	if (notify_psy) {
> +		ret = power_supply_get_property(notify_psy,
> +					POWER_SUPPLY_PROP_CURRENT_MAX, &prop);
> +		power_supply_put(notify_psy);
> +
> +		if (ret == 0) {
> +			bq2415x_update_reported_mode(bq, prop.intval);
> +			bq2415x_set_mode(bq, bq->reported_mode);
> +		}
> +	}
> +
>  	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
>  	bq2415x_set_autotimer(bq, 1);
>  
>  	dev_info(bq->dev, "driver registered\n");
>  	return 0;
>  
> -error_6:
> -error_5:
> -	bq2415x_sysfs_exit(bq);
>  error_4:
> -	bq2415x_power_supply_exit(bq);
> +	bq2415x_sysfs_exit(bq);
>  error_3:
> -	if (bq->notify_psy)
> -		power_supply_put(bq->notify_psy);
> +	bq2415x_power_supply_exit(bq);
>  error_2:
> +	if (bq->notify_node)
> +		of_node_put(bq->notify_node);
>  	kfree(name);
>  error_1:
>  	mutex_lock(&bq2415x_id_mutex);
> @@ -1707,10 +1721,11 @@ static int bq2415x_remove(struct i2c_client *client)
>  {
>  	struct bq2415x_device *bq = i2c_get_clientdata(client);
>  
> -	if (bq->notify_psy) {
> +	if (bq->nb.notifier_call)
>  		power_supply_unreg_notifier(&bq->nb);
> -		power_supply_put(bq->notify_psy);
> -	}
> +
> +	if (bq->notify_node)
> +		of_node_put(bq->notify_node);
>  
>  	bq2415x_sysfs_exit(bq);
>  	bq2415x_power_supply_exit(bq);

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/