Re: [PATCH RESEND v6 6/8] mfd: hi6421-spmi-pmic: move driver from staging

[Mauro Carvalho Chehab]

Em Thu, 24 Jun 2021 15:08:00 +0100
Lee Jones <lee.jones@xxxxxxxxxx> escreveu:

> > > > +/*
> > > > + * 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).

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

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

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