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
