Re: [PATCH v7 09/10] power: supply: Support ROHM bd99954 charger

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

Hello Again Andy :)

Thanks for looking at this. I appreciate your review work and all the
good tips!

On Tue, 2020-03-31 at 17:19 +0300, Andy Shevchenko wrote:
> 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?

I've used regmap_irq previously for few cases. And I was considering
using it here but noticed pretty soon that defining and requesting all
the different IRQs just so that they could be handled by this same
handler made no sense.

> 
> > +		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.

I don't need ACPI support for now. But if it comes to play a role, then
these comment work as a reminder for me to fix the
power_supply_get_battery_info().

I am not sure if you noticed that this property parsing is done in two
places (here and in power_supply_get_battery_info() ) because we use
properties from two nodes. power_supply_get_battery_info() gets
properties from static battery node if such is present meanwhile this
driver scans it's own (charger) node for charger related (not battery
related) properties. Currently all of these are expected to be in DT -
but I wouldn't be so surprized if ACPI was required at some point. I
still don't want to invest on fixing power_supply_get_battery_info()
for ACPI as I try to keep size of this series somehow reasonable - and
because I don't have test environment for BD99954 with ACPI in use.

> > +	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].rang
> > es,
> > +							  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;
> > +}


Best Regards
	--Matti