Message-ID: <20200331141933.GK1922688@smile.fi.intel.com>
Date:   Tue, 31 Mar 2020 17:19:33 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mazziesaccount@...il.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mikko Mutanen <mikko.mutanen@...rohmeurope.com>,
        Markus Laine <markus.laine@...rohmeurope.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ard Biesheuvel <ardb@...nel.org>, Borislav Petkov <bp@...e.de>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Changbin Du <changbin.du@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        David Gow <davidgow@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        devicetree@...r.kernel.org, Gary Hook <Gary.Hook@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Mark Rutland <mark.rutland@....com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mikhail Zaslonko <zaslonko@...ux.ibm.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Rob Herring <robh+dt@...nel.org>,
        Sebastian Reichel <sre@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Tal Gilboa <talgi@...lanox.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Uwe Kleine-König <uwe@...ine-koenig.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Vladimir Oltean <olteanv@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>
Subject: Re: [PATCH v7 09/10] power: supply: Support ROHM bd99954 charger

On Tue, Mar 31, 2020 at 03:28:17PM +0300, Matti Vaittinen wrote:
> The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-Ion
> secondary battery intended to be used in space-constraint equipment such
> as Low profile Notebook PC, Tablets and other applications. BD99954
> provides a Dual-source Battery Charger, two port BC1.2 detection and a
> Battery Monitor.
> 
> Support ROHM BD99954 Charger IC.

...

> +static irqreturn_t bd9995x_irq_handler_thread(int irq, void *private)
> +{
> +	struct bd9995x_device *bd = private;
> +	int ret, status, mask, i;
> +	unsigned long tmp;
> +	struct bd9995x_state state;
> +
> +	/*
> +	 * The bd9995x does not seem to generate big amount of interrupts.
> +	 * The logic regarding which interrupts can cause relevant
> +	 * status changes seem to be pretty complex.
> +	 *
> +	 * So lets implement really simple and hopefully bullet-proof handler:
> +	 * It does not really matter which IRQ we handle, we just go and
> +	 * re-read all interesting statuses + give the framework a nudge.
> +	 *
> +	 * Other option would be building a _complex_ and error prone logic
> +	 * trying to decide what could have been changed (resulting this IRQ
> +	 * we are now handling). During the normal operation the BD99954 does
> +	 * not seem to be generating much of interrupts so benefit from such
> +	 * logic would probably be minimal.
> +	 */
> +
> +	ret = regmap_read(bd->rmap, INT0_STATUS, &status);
> +	if (ret) {
> +		dev_err(bd->dev, "Failed to read IRQ status\n");
> +		return IRQ_NONE;
> +	}
> +
> +	ret = regmap_field_read(bd->rmap_fields[F_INT0_SET], &mask);
> +	if (ret) {
> +		dev_err(bd->dev, "Failed to read IRQ mask\n");
> +		return IRQ_NONE;
> +	}
> +
> +	/* Handle only IRQs that are not masked */
> +	status &= mask;
> +	tmp = status;
> +
> +	/* Lowest bit does not represent any sub-registers */
> +	tmp >>= 1;
> +
> +	/*
> +	 * Mask and ack IRQs we will handle (+ the idiot bit)
> +	 */
> +	ret = regmap_field_write(bd->rmap_fields[F_INT0_SET], 0);
> +	if (ret) {
> +		dev_err(bd->dev, "Failed to mask F_INT0\n");
> +		return IRQ_NONE;
> +	}
> +
> +	ret = regmap_write(bd->rmap, INT0_STATUS, status);
> +	if (ret) {
> +		dev_err(bd->dev, "Failed to ack F_INT0\n");
> +		goto err_umask;
> +	}
> +
> +	for_each_set_bit(i, &tmp, 7) {
> +		int sub_status, sub_mask;
> +		int sub_status_reg[] = {
> +			INT1_STATUS, INT2_STATUS, INT3_STATUS, INT4_STATUS,
> +			INT5_STATUS, INT6_STATUS, INT7_STATUS,
> +		};
> +		struct regmap_field *sub_mask_f[] = {
> +			bd->rmap_fields[F_INT1_SET],
> +			bd->rmap_fields[F_INT2_SET],
> +			bd->rmap_fields[F_INT3_SET],
> +			bd->rmap_fields[F_INT4_SET],
> +			bd->rmap_fields[F_INT5_SET],
> +			bd->rmap_fields[F_INT6_SET],
> +			bd->rmap_fields[F_INT7_SET],
> +		};
> +
> +		/* Clear sub IRQs */
> +		ret = regmap_read(bd->rmap, sub_status_reg[i], &sub_status);
> +		if (ret) {
> +			dev_err(bd->dev, "Failed to read IRQ sub-status\n");
> +			goto err_umask;
> +		}


Looking into it makes me thing that you perhaps need regmap IRQ chip?
Have you chance to look at drivers/mfd/intel_soc_pmic_bxtwc.c, for example?

> +		ret = regmap_field_read(sub_mask_f[i], &sub_mask);
> +		if (ret) {
> +			dev_err(bd->dev, "Failed to read IRQ sub-mask\n");
> +			goto err_umask;
> +		}
> +
> +		/* Ack active sub-statuses */
> +		sub_status &= sub_mask;
> +
> +		ret = regmap_write(bd->rmap, sub_status_reg[i], sub_status);
> +		if (ret) {
> +			dev_err(bd->dev, "Failed to ack sub-IRQ\n");
> +			goto err_umask;
> +		}
> +	}
> +
> +	ret = regmap_field_write(bd->rmap_fields[F_INT0_SET], mask);
> +	if (ret)
> +		/* May as well retry once */
> +		goto err_umask;
> +
> +	/* Read whole chip state */
> +	ret = bd9995x_get_chip_state(bd, &state);
> +	if (ret < 0) {
> +		dev_err(bd->dev, "Failed to read chip state\n");
> +	} else {
> +		mutex_lock(&bd->lock);
> +		bd->state = state;
> +		mutex_unlock(&bd->lock);
> +
> +		power_supply_changed(bd->charger);
> +	}
> +
> +	return IRQ_HANDLED;
> +
> +err_umask:
> +	ret = regmap_field_write(bd->rmap_fields[F_INT0_SET], mask);
> +	if (ret)
> +		dev_err(bd->dev,
> +		"Failed to un-mask F_INT0 - IRQ permanently disabled\n");
> +
> +	return IRQ_NONE;
> +}

...

> +static int bd9995x_fw_probe(struct bd9995x_device *bd)
> +{
> +	int ret;
> +	struct power_supply_battery_info info;
> +	u32 property;
> +	int i;
> +	int regval;
> +	bool found;
> +	struct bd9995x_init_data *init = &bd->init_data;
> +	struct battery_init battery_inits[] = {
> +		{
> +			.name = "trickle-charging current",
> +			.info_data = &info.tricklecharge_current_ua,
> +			.range = &charging_current_ranges[0],
> +			.ranges = 2,
> +			.data = &init->itrich_set,
> +		}, {
> +			.name = "pre-charging current",
> +			.info_data = &info.precharge_current_ua,
> +			.range = &charging_current_ranges[0],
> +			.ranges = 2,
> +			.data = &init->iprech_set,
> +		}, {
> +			.name = "pre-to-trickle charge voltage threshold",
> +			.info_data = &info.precharge_voltage_max_uv,
> +			.range = &trickle_to_pre_threshold_ranges[0],
> +			.ranges = 2,
> +			.data = &init->vprechg_th_set,
> +		}, {
> +			.name = "charging termination current",
> +			.info_data = &info.charge_term_current_ua,
> +			.range = &charging_current_ranges[0],
> +			.ranges = 2,
> +			.data = &init->iterm_set,
> +		}, {
> +			.name = "charging re-start voltage",
> +			.info_data = &info.charge_restart_voltage_uv,
> +			.range = &charge_voltage_regulation_ranges[0],
> +			.ranges = 2,
> +			.data = &init->vrechg_set,
> +		}, {
> +			.name = "battery overvoltage limit",
> +			.info_data = &info.overvoltage_limit_uv,
> +			.range = &charge_voltage_regulation_ranges[0],
> +			.ranges = 2,
> +			.data = &init->vbatovp_set,
> +		}, {
> +			.name = "fast-charging max current",
> +			.info_data = &info.constant_charge_current_max_ua,
> +			.range = &fast_charge_current_ranges[0],
> +			.ranges = 1,
> +			.data = &init->ichg_set,
> +		}, {
> +			.name = "fast-charging voltage",
> +			.info_data = &info.constant_charge_voltage_max_uv,
> +			.range = &charge_voltage_regulation_ranges[0],
> +			.ranges = 2,
> +			.data = &init->vfastchg_reg_set1,
> +		},
> +	};
> +	struct dt_init props[] = {
> +		{
> +			.prop = "rohm,vsys-regulation-microvolt",
> +			.range = &vsys_voltage_regulation_ranges[0],
> +			.ranges = 2,
> +			.data = &init->vsysreg_set,
> +		}, {
> +			.prop = "rohm,vbus-input-current-limit-microamp",
> +			.range = &input_current_limit_ranges[0],
> +			.ranges = 1,
> +			.data = &init->ibus_lim_set,
> +		}, {
> +			.prop = "rohm,vcc-input-current-limit-microamp",
> +			.range = &input_current_limit_ranges[0],
> +			.ranges = 1,
> +			.data = &init->icc_lim_set,
> +		},
> +	};
> +

> +	/*
> +	 * The power_supply_get_battery_info() does not support getting values
> +	 * from ACPI. Let's fix it if ACPI is required here.
> +	 */

Previously we discussed this and you told that you don't need ACPI support. Did
I get your wrong or something has been changed? If the latter, perhaps
converting power supply core to use device property API is not harder than what
you have done below.

> +	ret = power_supply_get_battery_info(bd->charger, &info);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(battery_inits); i++) {
> +		int val = *battery_inits[i].info_data;
> +		const struct linear_range *range = battery_inits[i].range;
> +		int ranges = battery_inits[i].ranges;
> +
> +		if (val == -EINVAL)
> +			continue;
> +
> +		ret = linear_range_get_selector_low_array(range, ranges, val,
> +							  &regval, &found);
> +		if (ret) {
> +			dev_err(bd->dev, "Unsupported value for %s\n",
> +				battery_inits[i].name);
> +
> +			power_supply_put_battery_info(bd->charger, &info);
> +			return -EINVAL;
> +		}
> +		if (!found) {
> +			dev_warn(bd->dev,
> +				 "Unsupported value for %s - using smaller\n",
> +				 battery_inits[i].name);
> +		}
> +		*(battery_inits[i].data) = regval;
> +	}
> +
> +	power_supply_put_battery_info(bd->charger, &info);
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		ret = device_property_read_u32(bd->dev, props[i].prop,
> +					       &property);
> +		if (ret < 0) {
> +			dev_err(bd->dev, "failed to read %s", props[i].prop);
> +
> +			return ret;
> +		}
> +
> +		ret = linear_range_get_selector_low_array(props[i].range,
> +							  props[i].ranges,
> +							  property, &regval,
> +							  &found);
> +		if (ret) {
> +			dev_err(bd->dev, "Unsupported value for '%s'\n",
> +				props[i].prop);
> +
> +			return -EINVAL;
> +		}
> +
> +		if (!found) {
> +			dev_warn(bd->dev,
> +				 "Unsupported value for '%s' - using smaller\n",
> +				 props[i].prop);
> +		}
> +
> +		*(props[i].data) = regval;
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists