Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 12:24:41AM +0300, Yauhen Kharuzhy wrote:
> In some configuration external charger "#charge enable" signal is
> connected to PMIC. Enable it at device probing to allow charging.
> 
> Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
> them at driver unbind to re-enable hardware charging control if it was
> enabled before.
> 
> Tested at Lenovo Yoga Book (YB1-X91L).

My comments below.
After addressing them,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> +#define CHT_WC_CHGDISCTRL_OUTPUT	BIT(0)

Simple _OUT as per documentation.

> +/* 0 - open drain, 1 - regular output */

Regular means push-pull.

> +#define CHT_WC_CHGDISCTRL_DRV_OD_DIS	BIT(4)

_DRV as per documentation.

> +#define CHT_WC_CHGDISCTRL_MODE_HW	BIT(6)

_FN as per documentation.

> +static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext,
> +					  bool enable)
> +{
> +	unsigned int val;
> +	int ret;
> +

> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_OUTPUT;

Can be assigned in definition block.

> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +				 CHT_WC_CHGDISCTRL_OUTPUT, val);
> +	if (ret)
> +		dev_err(ext->dev, "Error updating CHGDISCTRL reg: %d\n", ret);
> +}

> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_MODE_HW;

> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +			CHT_WC_CHGDISCTRL_MODE_HW, val);

Indentation.

> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error setting sw control for charger enable: %d\n",
> +			ret);

> +static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
> +{
> +	int ret;
> +
> +	/*
> +	 * Save the external charger control output state for restoring it at
> +	 * driver unbinding

You may move "at" to the next line and add a period at the end.

> +	 */
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
> +			  &ext->chgdisctrl_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
> +			  &ext->chgrctrl0_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko