Thread (34 messages) 34 messages, 7 authors, 2015-05-13

Re: [PATCH V1 2/6] regulator: da9062: DA9062 regulator driver

From: Mark Brown <broonie@kernel.org>
Date: 2015-04-18 11:48:59
Also in: linux-devicetree, linux-rtc, linux-watchdog, lkml

On Fri, Apr 17, 2015 at 03:23:32PM +0100, S Twiss wrote:
+/* Regulator interrupt handlers */
+static irqreturn_t da9062_ldo_lim_event(int irq, void *data)
+{
+	struct da9062_regulators *regulators = data;
+	struct da9062 *hw = regulators->regulator[0].hw;
+	struct da9062_regulator *regl;
+	int bits, i, ret;
+
+	ret = regmap_read(hw->regmap, DA9062AA_STATUS_D, &bits);
+	if (ret < 0)
+		return IRQ_NONE;
Please log an error for this, if we're having trouble talking to the
device that seems like a serious issue.
+	for (i = regulators->n_regulators - 1; i >= 0; i--) {
+		regl = &regulators->regulator[i];
+		if (regl->info->oc_event.reg != DA9062AA_STATUS_D)
+			continue;
+
+		if (BIT(regl->info->oc_event.lsb) & bits)
+			regulator_notifier_call_chain(regl->rdev,
+					REGULATOR_EVENT_OVER_CURRENT, NULL);
+	}
+
+	return IRQ_HANDLED;
This will return IRQ_HANDLED even if none of the regulators were
flagginng an event.
+static irqreturn_t da9062_vdd_warn_event(int irq, void *data)
+{
+	return IRQ_HANDLED;
+}
Ignoring an interrupt is not usefully handling it - at the *least* this
should be generating a log message.
+static struct da9062_regulators_pdata *da9062_parse_regulators_dt(
+		struct platform_device *pdev,
+		struct of_regulator_match **reg_matches)
+{
+	struct da9062_regulators_pdata *pdata;
+	struct da9062_regulator_data *rdata;
+	struct device_node *node;
+	int i, n, num;
+
+	node = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
+	if (!node) {
+		dev_err(&pdev->dev, "Regulators device node not found\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	num = of_regulator_match(&pdev->dev, node, da9062_matches,
+				 ARRAY_SIZE(da9062_matches));
Don't open code this, describe the DT names in the regualtor_desc and
let the core register.
+	if (IS_ERR(pdata) || pdata->n_regulators == 0) {
+		dev_err(&pdev->dev,
+			"No regulators defined for the platform\n");
+		return PTR_ERR(pdata);
+	}
+
+	n_regulators = ARRAY_SIZE(local_regulator_info),
This is broken, the set of regulators in the silicon is not a property
of the platform.  The driver should just register all the regualtors
that are present in the silicon.  I'm fairly sure I've been through this
before...
+	ret = request_threaded_irq(irq,
+				   NULL, da9062_vdd_warn_event,
+				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+				   "VDD_WARN", regulators);
devm_request_threaded_irq().

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help