Re: [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3574 bytes --]

Hi,

On Tue, Nov 15, 2016 at 10:18:51PM +0900, Milo Kim wrote:
> TPS65217 charger driver handles the charger interrupt through the IRQ or
> polling. Both cases can be requested in single function.
> 
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  drivers/power/supply/tps65217_charger.c | 70 ++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 32 deletions(-)

I don't see the advantage of this. It's more lines of codes than
before and does not really increase readability.

-- Sebastian

> 
> diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
> index 9fd019f..04f8322 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -195,12 +195,48 @@ static const struct power_supply_desc tps65217_charger_desc = {
>  	.num_properties		= ARRAY_SIZE(tps65217_ac_props),
>  };
>  
> +static int tps65217_charger_request_interrupt(struct platform_device *pdev)
> +{
> +	struct tps65217_charger *charger = platform_get_drvdata(pdev);
> +	int irq;
> +	int ret;
> +
> +	irq = platform_get_irq_byname(pdev, "AC");
> +	if (irq < 0)
> +		irq = -ENXIO;
> +
> +	charger->irq = irq;
> +
> +	if (irq != -ENXIO) {
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						tps65217_charger_irq, 0,
> +						"tps65217-charger", charger);
> +		if (ret) {
> +			dev_err(charger->dev,
> +				"Unable to register irq %d err %d\n", irq, ret);
> +			return ret;
> +		}
> +
> +		/* Check current state */
> +		tps65217_charger_irq(irq, charger);
> +		return 0;
> +	}
> +
> +	charger->poll_task = kthread_run(tps65217_charger_poll_task, charger,
> +					 "ktps65217charger");
> +	if (IS_ERR(charger->poll_task)) {
> +		ret = PTR_ERR(charger->poll_task);
> +		dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
> +	}
> +
> +	return 0;
> +}
> +
>  static int tps65217_charger_probe(struct platform_device *pdev)
>  {
>  	struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
>  	struct tps65217_charger *charger;
>  	struct power_supply_config cfg = {};
> -	int irq;
>  	int ret;
>  
>  	dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -224,43 +260,13 @@ static int tps65217_charger_probe(struct platform_device *pdev)
>  		return PTR_ERR(charger->ac);
>  	}
>  
> -	irq = platform_get_irq_byname(pdev, "AC");
> -	if (irq < 0)
> -		irq = -ENXIO;
> -	charger->irq = irq;
> -
>  	ret = tps65217_config_charger(charger);
>  	if (ret < 0) {
>  		dev_err(charger->dev, "charger config failed, err %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (irq != -ENXIO) {
> -		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> -						tps65217_charger_irq,
> -						0, "tps65217-charger",
> -						charger);
> -		if (ret) {
> -			dev_err(charger->dev,
> -				"Unable to register irq %d err %d\n", irq,
> -				ret);
> -			return ret;
> -		}
> -
> -		/* Check current state */
> -		tps65217_charger_irq(irq, charger);
> -	} else {
> -		charger->poll_task = kthread_run(tps65217_charger_poll_task,
> -						charger, "ktps65217charger");
> -		if (IS_ERR(charger->poll_task)) {
> -			ret = PTR_ERR(charger->poll_task);
> -			dev_err(charger->dev,
> -				"Unable to run kthread err %d\n", ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return tps65217_charger_request_interrupt(pdev);
>  }
>  
>  static int tps65217_charger_remove(struct platform_device *pdev)
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]