Em Thu, 24 Jun 2021 15:08:00 +0100
Lee Jones [off-list ref] escreveu:

quoted

quoted

quoted

+/*
+ * The IRQs are mapped as:
+ *
+ *	======================  =============   ============	=====
+ *	IRQ			MASK REGISTER	IRQ REGISTER	BIT
+ *	======================  =============   ============	=====
+ *	OTMP			0x0202		0x212		bit 0
+ *	VBUS_CONNECT		0x0202		0x212		bit 1
+ *	VBUS_DISCONNECT		0x0202		0x212		bit 2
+ *	ALARMON_R		0x0202		0x212		bit 3
+ *	HOLD_6S			0x0202		0x212		bit 4
+ *	HOLD_1S			0x0202		0x212		bit 5
+ *	POWERKEY_UP		0x0202		0x212		bit 6
+ *	POWERKEY_DOWN		0x0202		0x212		bit 7
+ *
+ *	OCP_SCP_R		0x0203		0x213		bit 0
+ *	COUL_R			0x0203		0x213		bit 1
+ *	SIM0_HPD_R		0x0203		0x213		bit 2
+ *	SIM0_HPD_F		0x0203		0x213		bit 3
+ *	SIM1_HPD_R		0x0203		0x213		bit 4
+ *	SIM1_HPD_F		0x0203		0x213		bit 5
+ *	======================  =============   ============	=====
+ *
+ * Each mask register contains 8 bits. The ancillary macros below
+ * convert a number from 0 to 14 into a register address and a bit mask
+ */
+#define HISI_IRQ_MASK_REG(irq_data)	(SOC_PMIC_IRQ_MASK_0_ADDR + \
+					 (irqd_to_hwirq(irq_data) / BITS_PER_BYTE))
+#define HISI_IRQ_MASK_BIT(irq_data)	BIT(irqd_to_hwirq(irq_data) & (BITS_PER_BYTE - 1))
+#define HISI_8BITS_MASK			GENMASK(BITS_PER_BYTE - 1, 0)    

Are these lines up in real code?  Looks like they're not in the diff.  

Weird. The changes to use those are at patch 3/8. All the above
macros are used at the patch.  

Sorry, that made no sense - it's been a long few days!

I meant to say "do these (the tabs) line up?"

Yes, they line up (and aligned with the parenthesis, in the case of
HISI_IRQ_MASK_REG).

quoted

quoted

quoted

+static const struct mfd_cell hi6421v600_devs[] = {
+	{ .name = "hi6421v600-regulator", },
+};    

Where are the other devices?  

While this is a MFD device, as it has regulators, ADC and other
stuff, right now, only the regulator and the IRQs are implemented. 

The IRQs are at the core of this driver, while the regulator 
is at the separate regulator driver.  

The rule usually goes:

 Drivers don't qualify as MFDs until you register >1 device.

Do you mean that, in order for this to be accepted, should
I move the irq code to a separate driver?

quoted

quoted

quoted

+	for (i = 0; i < PMIC_IRQ_LIST_MAX; i++) {
+		virq = irq_create_mapping(ddata->domain, i);
+		if (!virq) {
+			dev_err(dev, "Failed to map H/W IRQ\n");
+			return -ENOSPC;    

-ENOSPC doesn't seem right here.

Can't find any other uses of it for irq_create_mapping() either.  

There are two drivers returning -ENOSPC:

	arch/powerpc/platforms/pseries/msi.c
	arch/powerpc/sysdev/mpic_u3msi.c  

I only looked in drivers/

quoted

But others return -EIO, -EINVAL, -ENOMEM, -ENODEV, -ENXIO.

I think that -ENODEV would fit better here.  

I think -ENXIO is the most common, followed by -EINVAL.

This doesn't have anything to do with devices per say.

Ok. I'll change it to -ENXIO.

quoted

quoted

quoted

+static void hi6421_spmi_pmic_remove(struct spmi_device *pdev)
+{
+	struct hi6421_spmi_pmic *ddata = dev_get_drvdata(&pdev->dev);
+
+	free_irq(ddata->irq, ddata);    

No devm_* version?  

Are there a devm_* variant for gpio_to_irq()?  

Please refer to Dan's response.

Ok.

Thanks,
Mauro