Stephen-

Thanks for all the comments.

On Wed, Oct 30, 2013 at 11:17:55AM -0700, Stephen Boyd wrote:

On 10/28, Josh Cartwright wrote:

quoted

@@ -108,12 +111,17 @@ struct spmi_pmic_arb_dev {
 	void __iomem		*base;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
+	unsigned int		irq;
 	bool			allow_wakeup;
 	spinlock_t		lock;
 	u8			channel;
 	u8			min_apid;
 	u8			max_apid;
 	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
+	int			bus_nr;

This looks unused.

Indeed, will remove (I think this is a holdout from the split qpnpint
handling that exists in the vendor tree).

quoted

+	struct irq_domain	*domain;
+	struct spmi_controller	*spmic;
+	u16			apid_to_ppid[256];
 };

 static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset)

[...]

quoted

+
+static void pmic_arb_chained_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_get_chip(irq);
+	void __iomem *intr = pa->intr;
+	int first = pa->min_apid >> 5;
+	int last = pa->max_apid >> 5;
+	int i, id;
+	u8 ee = 0; /*pa->owner;*/

TODO?

Indeed, I removed this for some reason awhile back, but this really
should be put back into place, and the EE should be a required property
in the bindings.

[..]

quoted

+static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
+					   struct device_node *controller,
+					   const u32 *intspec,
+					   unsigned int intsize,
+					   unsigned long *out_hwirq,
+					   unsigned int *out_type)
+{
+	struct spmi_pmic_arb_dev *pa = d->host_data;
+	struct spmi_pmic_arb_irq_spec spec;
+	int err;
+	u8 apid;
+
+	dev_dbg(&pa->spmic->dev,
+		"intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
+		intspec[0], intspec[1], intspec[2]);
+
+	if (d->of_node != controller)
+		return -EINVAL;
+	if (intsize != 4)
+		return -EINVAL;
+	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
+		return -EINVAL;
+
+	spec.slave = intspec[0];
+	spec.per   = intspec[1];
+	spec.irq   = intspec[2];
+
+	err = search_mapping_table(pa, &spec, &apid);
+	if (err)
+		return err;
+
+	pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
+
+	/* Keep track of {max,min}_apid for bounding search during interrupt */
+	if (apid > pa->max_apid)
+		pa->max_apid = apid;
+	if (apid < pa->min_apid)
+		pa->min_apid = apid;

Ah makes sense now why we set this to the opposite values in
probe.  Please put a comment in patch 4 and maybe move that setup
in patch 4 to this patch.

Indeed, I'll move it and add a comment at init-time.

quoted

+
+	*out_hwirq = spec.slave << 24
+		   | spec.per   << 16
+		   | spec.irq   << 8
+		   | apid;
+	*out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
+
+	dev_dbg(&pa->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
+
+	return 0;
+}
+

[...]

quoted

 static int spmi_pmic_arb_probe(struct platform_device *pdev)
 {
 	struct spmi_pmic_arb_dev *pa;
 	struct spmi_controller *ctrl;
 	struct resource *res;
-	int err, i;
+	int err = 0, i;

 	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
 	if (!ctrl)
 		return -ENOMEM;

 	pa = spmi_controller_get_drvdata(ctrl);
+	pa->spmic = ctrl;

 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
 	pa->base = devm_ioremap_resource(&ctrl->dev, res);

@@ -349,6 +679,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}

+	pa->irq = platform_get_irq(pdev, 0);
+	if (pa->irq < 0) {
+		err = pa->irq;
+		goto err_put_ctrl;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
 		pa->mapping_table[i] = readl_relaxed(
 				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));

@@ -364,15 +700,30 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	ctrl->read_cmd = pmic_arb_read_cmd;
 	ctrl->write_cmd = pmic_arb_write_cmd;

+	dev_dbg(&pdev->dev, "adding irq domain\n");
+	pa->domain = irq_domain_add_tree(pdev->dev.of_node,
+					 &pmic_arb_irq_domain_ops, pa);
+	if (!pa->domain) {
+		dev_err(&pdev->dev, "unable to create irq_domain\n");
+		goto err_put_ctrl;

Why do we silently ignore the error here? Is the irqchip
functionality optional? Setting err here should allow us to skip
intializing err to 0 up at the top of this function.

Nice catch, it wasn't my intent to ignore it.

Thanks again,
   Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation