lkml.org 
[lkml]   [2018]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH v3 05/17] irqchip/sun4i: add support for suniv interrupt controller
On Thu, Nov 22, 2018 at 06:02:00PM +0300, Mesih Kilinc wrote:
> > > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> > > index e3e5b91..7ca4a4d 100644
> > > --- a/drivers/irqchip/irq-sun4i.c
> > > +++ b/drivers/irqchip/irq-sun4i.c
> > > @@ -28,11 +28,21 @@
> > > #define SUN4I_IRQ_NMI_CTRL_REG 0x0c
> > > #define SUN4I_IRQ_PENDING_REG(x) (0x10 + 0x4 * x)
> > > #define SUN4I_IRQ_FIQ_PENDING_REG(x) (0x20 + 0x4 * x)
> > > -#define SUN4I_IRQ_ENABLE_REG(x) (0x40 + 0x4 * x)
> > > -#define SUN4I_IRQ_MASK_REG(x) (0x50 + 0x4 * x)
> > > +#define SUN4I_IRQ_ENABLE_REG(x) (irq_ic_data->enable_req_offset + 0x4 * x)
> > > +#define SUN4I_IRQ_MASK_REG(x) (irq_ic_data->mask_req_offset + 0x4 * x)
> >
> > You shouldn't have all the values you use passed as argument, so
> > irq_ic_data should be one of them here.
> >
>
> Could you elaborate it a little bit?

You should change your macro to something like:

SUN4I_IRQ_ENABLE_REG(data, x) ((data)->enable_req_offset + 0x4 * x)

That way, you are transparent about the parameters that the macro
expects, and don't rely on some unwritten rule about the variable
name.

> > > +
> > > +static int __init suniv_ic_of_init(struct device_node *node,
> > > + struct device_node *parent)
> > > +{
> > > + irq_ic_data = kzalloc(sizeof(struct sunxi_irq_chip_data), GFP_KERNEL);
> > > + if (!irq_ic_data) {
> > > + pr_err("kzalloc failed!\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + irq_ic_data->enable_req_offset = SUNIV_IRQ_ENABLE_REG_OFFSET;
> > > + irq_ic_data->mask_req_offset = SUNIV_IRQ_MASK_REG_OFFSET;
> > > +
> > > + return sun4i_of_init(node, parent);
> > > +}
> > > +
> > > +IRQCHIP_DECLARE(allwinner_sunvi_ic, "allwinner,suniv-f1c100s-ic", suniv_ic_of_init);
> >
> > You can even split that addition to a new patch as well.
>
> OK. I will do 3 patches. First one will add a struct that holds only
> base and domain. Second one will add register offsets to that struct.
> Third one will add f1c100s support. Is that ok?

Sounds perfect :)

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-11-22 17:01    [W:0.059 / U:0.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site