| Message ID | 20180622104930.32050-10-boris.brezillon@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add the I3C subsystem | expand |
Hi, On 06/22/2018 03:49 AM, Boris Brezillon wrote: > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 71c0ab46f216..19ed6006aea1 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -898,6 +898,17 @@ config GPIO_TS4900 > > endmenu > > +menu "I3C GPIO expanders" > + depends on I3C > + > +config GPIO_CDNS_I3C > + tristate "Cadence I3C GPIO expander" > + select GPIOLIB_IRQCHIP > + help > + Say yes here to enabled the driver for Cadence I3C GPIO expander. to enable the driver > + > +endmenu
On Fri, 22 Jun 2018 09:04:47 -0700 Randy Dunlap <rdunlap@infradead.org> wrote: > Hi, > > On 06/22/2018 03:49 AM, Boris Brezillon wrote: > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 71c0ab46f216..19ed6006aea1 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -898,6 +898,17 @@ config GPIO_TS4900 > > > > endmenu > > > > +menu "I3C GPIO expanders" > > + depends on I3C > > + > > +config GPIO_CDNS_I3C > > + tristate "Cadence I3C GPIO expander" > > + select GPIOLIB_IRQCHIP > > + help > > + Say yes here to enabled the driver for Cadence I3C GPIO expander. > > to enable the driver I'll fix this typo Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add a driver for Cadence I3C GPIO expander. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); kmalloc_array() ? > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > + ARRAY_SIZE(xfers)); One line? > +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g, > + unsigned long *mask, > + unsigned long *bits) > +{ > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > + u8 newovr; > + int ret; > + > + newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask); > + if (newovr == gpioc->ovr) > + return; > + > + ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr); > + if (!ret) > + gpioc->ovr = newovr; You don't change mask here, why do you need a pointer to it? > +} > +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g, > + unsigned long *mask, > + unsigned long *bits) > +{ > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > + int ret; > + u8 ivr; > + > + ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr); > + if (ret) > + return ret; > + > + *bits = ivr & *mask & gpioc->dir; > + *bits |= gpioc->ovr & *mask & ~gpioc->dir; Ditto. > + > + return 0; > +} > +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev, > + const struct i3c_ibi_payload *payload) > +{ > + struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev); > + u8 isr = 0; Perhaps you need to check the result of _read_reg() below instead of dummy assignment? > + int i; > + > + cdns_i3c_gpio_read_reg(gpioc, ISR, &isr); > + for (i = 0; i < 8; i++) { > + unsigned int irq; > + > + if (!(BIT(i) & isr & gpioc->imr)) > + continue; for_each_set_bit() ? > + > + irq = irq_find_mapping(gpioc->gpioc.irq.domain, i); > + handle_nested_irq(irq); > + } > +} > +static const struct i3c_device_id cdns_i3c_gpio_ids[] = { > + I3C_DEVICE(0x1c9, 0x0, NULL), > + { /* sentinel */ }, Slightly better without comma. > +}; > +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids);
Hi Andy, On Tue, 26 Jun 2018 22:07:03 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > Add a driver for Cadence I3C GPIO expander. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); > > kmalloc_array() ? > > > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > > + ARRAY_SIZE(xfers)); > > One line? > > > > +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g, > > + unsigned long *mask, > > + unsigned long *bits) > > +{ > > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > > + u8 newovr; > > + int ret; > > + > > + newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask); > > + if (newovr == gpioc->ovr) > > + return; > > + > > + ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr); > > + if (!ret) > > + gpioc->ovr = newovr; > > You don't change mask here, why do you need a pointer to it? What are you talking about??!!! This is part of the gpio_chip interface that drivers have to implement. You can't decide that mask should not be a pointer. And if you actually look at the code, you'll probably see that the reason we're passed a pointer here is that the GPIO chip might expose more GPIOs than can be represented by an unsigned long, hence the array of unsigned long. I'll say it here because this is not the first time I'm pissed off by one of your review. You seem to review everything that is posted on the LKML, and most of the time your reviews are superficial (focused on tiny details or coding style issues). Don't you have better things to do? I mean, I'm fine when I get such comments from people that also do useful comments, but yours are most of the time useless and sometime even wrong because you didn't time to look at the code in details. > > > +} > > > +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g, > > + unsigned long *mask, > > + unsigned long *bits) > > +{ > > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > > + int ret; > > + u8 ivr; > > + > > + ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr); > > + if (ret) > > + return ret; > > + > > + *bits = ivr & *mask & gpioc->dir; > > + *bits |= gpioc->ovr & *mask & ~gpioc->dir; > > Ditto. > > > + > > + return 0; > > +} > > > +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev, > > + const struct i3c_ibi_payload *payload) > > +{ > > + struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev); > > > + u8 isr = 0; > > Perhaps you need to check the result of _read_reg() below instead of > dummy assignment? > > > + int i; > > + > > + cdns_i3c_gpio_read_reg(gpioc, ISR, &isr); > > > + for (i = 0; i < 8; i++) { > > + unsigned int irq; > > + > > + if (!(BIT(i) & isr & gpioc->imr)) > > + continue; > > for_each_set_bit() ? > > > + > > + irq = irq_find_mapping(gpioc->gpioc.irq.domain, i); > > + handle_nested_irq(irq); > > + } > > +} > > > +static const struct i3c_device_id cdns_i3c_gpio_ids[] = { > > + I3C_DEVICE(0x1c9, 0x0, NULL), > > > + { /* sentinel */ }, > > Slightly better without comma. > > > +}; > > +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids); > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 26 Jun 2018 22:07:03 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > Add a driver for Cadence I3C GPIO expander. >> > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> >> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); >> >> kmalloc_array() ? >> >> > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, >> > + ARRAY_SIZE(xfers)); >> >> One line? >> >> You don't change mask here, why do you need a pointer to it? > > What are you talking about??!!! This is part of the gpio_chip interface > that drivers have to implement. You can't decide that mask should not be > a pointer. And if you actually look at the code, you'll probably see > that the reason we're passed a pointer here is that the GPIO chip might > expose more GPIOs than can be represented by an unsigned long, hence > the array of unsigned long. > > I'll say it here because this is not the first time I'm pissed off by > one of your review. Like 'I like offending people, because I think people who get offended should be offended.' ? I'm not that guy, relax. > You seem to review everything that is posted on the LKML, That's not true. > and most of the time your reviews are superficial (focused on tiny > details or coding style issues). Don't you have better things to do? If your code is written using ancient style and / or API it's not good to start with. Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? On top of that you might find more interesting reviews where I pointed out to a memory leak or other nasty stuff. But of course you prefer not to norice that. I understand you. > I mean, I'm fine when I get such comments from people that also do > useful comments, but yours are most of the time useless and sometime > even wrong because you didn't time to look at the code in details. Calm down, please. You would simple ignore them. Your choice. But okay, I would try to avoid your stuff to make you happier. Sorry for disturbing. >> Ditto. In these two comments I'm indeed wrong.
On Tue, 26 Jun 2018 23:44:36 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 26 Jun 2018 22:07:03 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >> > Add a driver for Cadence I3C GPIO expander. > >> > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > >> > >> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); > >> > >> kmalloc_array() ? > >> > >> > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > >> > + ARRAY_SIZE(xfers)); > >> > >> One line? > >> > > >> You don't change mask here, why do you need a pointer to it? > > > > What are you talking about??!!! This is part of the gpio_chip interface > > that drivers have to implement. You can't decide that mask should not be > > a pointer. And if you actually look at the code, you'll probably see > > that the reason we're passed a pointer here is that the GPIO chip might > > expose more GPIOs than can be represented by an unsigned long, hence > > the array of unsigned long. > > > > > I'll say it here because this is not the first time I'm pissed off by > > one of your review. > > Like 'I like offending people, because I think people who get offended > should be offended.' ? > I'm not that guy, relax. I'm not offended, just annoyed, and not because I have things to change in my patchset (I'm used to that and that's something I comply with most of the time), but because the only reviews I had from you were of this kind (nitpicking on minor stuff). > > > You seem to review everything that is posted on the LKML, > > That's not true. > > > and most of the time your reviews are superficial (focused on tiny > > details or coding style issues). Don't you have better things to do? > > If your code is written using ancient style and / or API it's not good > to start with. > Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? Come on! - kzalloc() vs kmalloc_array() - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } vs for_each_bit_set() (which is not even applicable here because I'm not manipulating an unsigned long) Where do you see ancient style APIs here? And that's not even the problem. I'm okay to fix those things, but you only ever do these kind of reviews, and sometime you even act like you know better than the developer or the maintainer how the code should be organized without even digging enough to have a clue (your comment on the fact that mask should not be a pointer is a good example of such situations). > > On top of that you might find more interesting reviews where I pointed > out to a memory leak or other nasty stuff. But of course you prefer > not to norice that. Yes, sometime you have valid point, but it gets lost in all the noise. > I understand you. > > > I mean, I'm fine when I get such comments from people that also do > > useful comments, but yours are most of the time useless and sometime > > even wrong because you didn't time to look at the code in details. > > Calm down, please. Why? Because I say you didn't look at the code in details? Isn't it true? Did you look at what I3C is, how it works or how the I3C framework is architectured? Because that's the kind of reviews I'd love to have on this series. > You would simple ignore them. Your choice. That's not my point. My point is, maybe you should do less reviews but spend more time on each of them so you don't just scratch the surface with comments I could get from a tool like checkpatch. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 26 Jun 2018 23:44:36 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > On Tue, 26 Jun 2018 22:07:03 +0300 >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon >> >> <boris.brezillon@bootlin.com> wrote: >> > I'll say it here because this is not the first time I'm pissed off by >> > one of your review. >> >> Like 'I like offending people, because I think people who get offended >> should be offended.' ? >> I'm not that guy, relax. > > I'm not offended, just annoyed, and not because I have things to change > in my patchset (I'm used to that and that's something I comply with > most of the time), but because the only reviews I had from you were of > this kind (nitpicking on minor stuff). OK, point taken. >> > and most of the time your reviews are superficial (focused on tiny >> > details or coding style issues). Don't you have better things to do? >> >> If your code is written using ancient style and / or API it's not good >> to start with. >> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? > > Come on! > > - kzalloc() vs kmalloc_array() > - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } > vs > for_each_bit_set() (which is not even applicable here because I'm > not manipulating an unsigned long) > > Where do you see ancient style APIs here? Both. kmalloc_array() and for_each_set_bit() were introduced quite long time ago. On top of that you are open coding, if I'm not mistaken, cpu_to_be32() and be32_to_cpu(). ...and more I believe. > And that's not even the problem. I'm okay to fix those things, but you > only ever do these kind of reviews, and sometime you even act like you > know better than the developer or the maintainer how the code should be > organized without even digging enough to have a clue (your comment on > the fact that mask should not be a pointer is a good example of such > situations). "sometime". Yes, I admit my mistakes. >> On top of that you might find more interesting reviews where I pointed >> out to a memory leak or other nasty stuff. But of course you prefer >> not to norice that. > > Yes, sometime you have valid point, but it gets lost in all the noise. Btw, you forgot to call of_node_put() on error path in one case. Did I again missed the details? >> > I mean, I'm fine when I get such comments from people that also do >> > useful comments, but yours are most of the time useless and sometime >> > even wrong because you didn't time to look at the code in details. >> >> Calm down, please. > > Why? Because I say you didn't look at the code in details? Isn't it > true? Did you look at what I3C is, how it works or how the I3C framework > is architectured? Because that's the kind of reviews I'd love to have on > this series. You got me. I have a copy of the spec, this require a bit of time to go through. For now I can offer you to consider IBI implementation using IRQ chip framework. It would give few advantages (like we do this for GPIO), such as IRQ statistics or wake up capabilities. >> You would simple ignore them. Your choice. > > That's not my point. My point is, maybe you should do less reviews but > spend more time on each of them Good point. > so you don't just scratch the surface > with comments I could get from a tool like checkpatch. Perhaps you should run checkpatch yourself then?
Hi Andy, On Wed, 27 Jun 2018 20:53:51 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 26 Jun 2018 23:44:36 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >> > On Tue, 26 Jun 2018 22:07:03 +0300 > >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > >> >> <boris.brezillon@bootlin.com> wrote: > > >> > I'll say it here because this is not the first time I'm pissed off by > >> > one of your review. > >> > >> Like 'I like offending people, because I think people who get offended > >> should be offended.' ? > >> I'm not that guy, relax. > > > > I'm not offended, just annoyed, and not because I have things to change > > in my patchset (I'm used to that and that's something I comply with > > most of the time), but because the only reviews I had from you were of > > this kind (nitpicking on minor stuff). > > OK, point taken. > > >> > and most of the time your reviews are superficial (focused on tiny > >> > details or coding style issues). Don't you have better things to do? > >> > >> If your code is written using ancient style and / or API it's not good > >> to start with. > >> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? > > > > Come on! > > > > - kzalloc() vs kmalloc_array() > > - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } > > vs > > for_each_bit_set() (which is not even applicable here because I'm > > not manipulating an unsigned long) > > > > Where do you see ancient style APIs here? > > Both. kmalloc_array() and for_each_set_bit() were introduced quite > long time ago. I mean, kzalloc() is not deprecated AFAIK and I don't really see the benefit of using kmalloc_array(), but if that makes you happy, let's go for kmalloc_array(). For for_each_bit_set() it would require changing the type of isr + having a temporary variable to get the reg value into an u8. Again, I can do the change, but I don't really see how it improves the code. > > On top of that you are open coding, if I'm not mistaken, cpu_to_be32() > and be32_to_cpu(). Care to point where? > ...and more I believe. Care to point what? > >> On top of that you might find more interesting reviews where I pointed > >> out to a memory leak or other nasty stuff. But of course you prefer > >> not to norice that. > > > > Yes, sometime you have valid point, but it gets lost in all the noise. > > Btw, you forgot to call of_node_put() on error path in one case. In this driver or another patch of the series? I don't see any of_node_get() call in this GPIO driver, but maybe it's something done in one of the GPIO helper. > Did I > again missed the details? > > >> > I mean, I'm fine when I get such comments from people that also do > >> > useful comments, but yours are most of the time useless and sometime > >> > even wrong because you didn't time to look at the code in details. > >> > >> Calm down, please. > > > > Why? Because I say you didn't look at the code in details? Isn't it > > true? Did you look at what I3C is, how it works or how the I3C framework > > is architectured? Because that's the kind of reviews I'd love to have on > > this series. > > You got me. > I have a copy of the spec, this require a bit of time to go through. Cool! > > For now I can offer you to consider IBI implementation using IRQ chip framework. > It would give few advantages (like we do this for GPIO), such as IRQ > statistics or wake up capabilities. Just pasting the comment I made in my cover letter: " Main changes between the initial RFC and this v2 are: - Add a generic infrastructure to support IBIs. It's worth mentioning that I tried exposing IBIs as a regular IRQs, but after several attempts and a discussion with Mark Zyngier, it appeared that it was not really fitting in the Linux IRQ model (the fact that you have payload attached to IBIs, the fact that most of the time an IBI will generate a transfer on the bus which has to be done in an atomic context, ...) The counterpart of this decision is the latency induced by the workqueue approach, but since I don't have real use cases, I don't know if this can be a problem or not. " To sum-up, yes I tried implementing what you suggest, and it ended being messy for 2 main reasons: 1/ IBIs have payload attached to them, and exposing IBIs are regular IRQs meant adding a payload queuing mechanism to the i3c_device so that the I3C device driver could dequeue each payload independently. Clearly not impossible to deal with, but conceptually far from the concept of IRQs we have in Linux. 2/ The I3C APIs are meant to be used in non-atomic context, but if you expose IBIs are regular IRQs, the irqchip will have to mask/unmask the IRQs from an atomic context, and masking/unmasking IRQs almost always implies doing a CCC or priv SDR transfer. Plus, most of the time you'll have to retrieve extra info from the IRQ handler, which again means sending frames on the I3C bus. We could make that work by either supporting async transfers or having the irq chip handle the IRQ handlers from a thread. But both options add extra complexity for no real benefit given that IBIs are already not exactly fitting in the Linux IRQ model. The discussion I had with Mark Zyngier finished convincing me that IBIs would be better exposed with their own API. > > >> You would simple ignore them. Your choice. > > > > That's not my point. My point is, maybe you should do less reviews but > > spend more time on each of them > > Good point. > > > so you don't just scratch the surface > > with comments I could get from a tool like checkpatch. > > Perhaps you should run checkpatch yourself then? > I do run checkpatch --strict and fix most of the thing reported except those hurting readability. I don't remember seeing checkpatch complain about kzalloc() usage, and I guess it's not smart enough to detect that for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" pattern. Anyway, thanks for having a look at the I3C spec and starting the discussion on IBIs. I guess I owe you an apology. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 12:49 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add a driver for Cadence I3C GPIO expander. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> If you need to merge this through the subsystem merge. I see there was some heat in this discussion between you and Andy, and it seems to be resolved. Which is good because you are both senior contributors that I value a lot. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote: > I mean, kzalloc() is not deprecated AFAIK and I don't really see the > benefit of using kmalloc_array(), but if that makes you happy, let's go > for kmalloc_array(). kcalloc > I do run checkpatch --strict and fix most of the thing reported except > those hurting readability. I don't remember seeing checkpatch complain > about kzalloc() usage, and I guess it's not smart enough to detect that > for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" > pattern. That would not an appropriate conversion suggestion in any case. coccinelle could at least look at whether or not x is allocated as a bitmap via DECLARE_BITMAP or bitmap_alloc -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2018 at 1:54 AM, Joe Perches <joe@perches.com> wrote: > On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote: >> I mean, kzalloc() is not deprecated AFAIK and I don't really see the >> benefit of using kmalloc_array(), but if that makes you happy, let's go >> for kmalloc_array(). > > kcalloc IIRC in the original code kmalloc(x*y) is used. >> I do run checkpatch --strict and fix most of the thing reported except >> those hurting readability. I don't remember seeing checkpatch complain >> about kzalloc() usage, and I guess it's not smart enough to detect that >> for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" >> pattern. > > That would not an appropriate conversion suggestion in any case. > coccinelle could at least look at whether or not x is allocated > as a bitmap via DECLARE_BITMAP or bitmap_alloc Huh?! bitmap operations are working against unsigned long *. one long is also a bitmap. So, that coccinelle scripts must be fixed accordingly.
On Thu, 2018-06-28 at 03:00 +0300, Andy Shevchenko wrote: > bitmap operations are working against unsigned long *. one long is > also a bitmap. Should only be so if declared via DECLARE_BITMAP > So, that coccinelle scripts must be fixed accordingly. We disagree about a non-existent script. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I see there was some heat in this discussion between you > and Andy, and it seems to be resolved. Which is good > because you are both senior contributors that I value > a lot. +1
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 71c0ab46f216..19ed6006aea1 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -898,6 +898,17 @@ config GPIO_TS4900 endmenu +menu "I3C GPIO expanders" + depends on I3C + +config GPIO_CDNS_I3C + tristate "Cadence I3C GPIO expander" + select GPIOLIB_IRQCHIP + help + Say yes here to enabled the driver for Cadence I3C GPIO expander. + +endmenu + menu "MFD GPIO expanders" config GPIO_ADP5520 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 1324c8f966a7..020b9171223b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o obj-$(CONFIG_GPIO_BD9571MWV) += gpio-bd9571mwv.o obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o +obj-$(CONFIG_GPIO_CDNS_I3C) += gpio-cdns-i3c.o obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o diff --git a/drivers/gpio/gpio-cdns-i3c.c b/drivers/gpio/gpio-cdns-i3c.c new file mode 100644 index 000000000000..028336754215 --- /dev/null +++ b/drivers/gpio/gpio-cdns-i3c.c @@ -0,0 +1,411 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#include <linux/gpio/driver.h> +#include <linux/i3c/device.h> +#include <linux/module.h> + +#define OVR 0x0 +#define IVR 0x1 +#define DIR_MODE 0x2 +#define IMR 0x3 +#define ISR 0x4 +#define ITR(x) (0x5 + (x)) + +struct cdns_i3c_gpio { + struct gpio_chip gpioc; + struct irq_chip irqc; + struct i3c_device *i3cdev; + struct mutex irq_lock; + u8 dir; + u8 ovr; + u8 imr; + u8 itr[3]; +}; + +static struct cdns_i3c_gpio *gpioc_to_cdns_gpioc(struct gpio_chip *gpioc) +{ + return container_of(gpioc, struct cdns_i3c_gpio, gpioc); +} + +static int cdns_i3c_gpio_read_reg(struct cdns_i3c_gpio *gpioc, u8 reg, + u8 *val) +{ + struct i3c_priv_xfer xfers[2] = { }; + u8 *scratchbuf; + int ret; + + /* + * i3c_device_do_priv_xfers() mandates that buffers passed in xfers be + * DMA-able. This prevents us from using reg and val directly since + * reg is on the stack, and val might be too. + * Allocate a temporary buffer with kmalloc() to solve the problem. + */ + scratchbuf = kmalloc(sizeof(*scratchbuf), GFP_KERNEL); + if (!scratchbuf) + return -ENOMEM; + + scratchbuf[0] = reg; + xfers[0].data.out = scratchbuf; + xfers[0].len = 1; + xfers[1].data.in = scratchbuf; + xfers[1].len = 1; + xfers[1].rnw = true; + + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, + ARRAY_SIZE(xfers)); + if (!ret) + *val = *scratchbuf; + + kfree(scratchbuf); + + return ret; +} + +static int cdns_i3c_gpio_write_reg(struct cdns_i3c_gpio *gpioc, u8 reg, + u8 val) +{ + struct i3c_priv_xfer xfers[2] = { }; + u8 *scratchbuf; + int ret; + + /* + * i3c_device_do_priv_xfers() mandates that buffers passed in xfers be + * DMA-able. This prevents us from using reg and val directly since + * reg is on the stack, and val might be too. + * Allocate a temporary buffer with kmalloc() to solve the problem. + */ + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); + if (!scratchbuf) + return -ENOMEM; + + scratchbuf[0] = reg; + scratchbuf[1] = val; + xfers[0].data.out = scratchbuf; + xfers[0].len = 1; + xfers[1].data.out = scratchbuf + 1; + xfers[1].len = 1; + + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, + ARRAY_SIZE(xfers)); + + kfree(scratchbuf); + + return ret; +} + +static int cdns_i3c_gpio_get_direction(struct gpio_chip *g, + unsigned int offset) +{ + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); + + return !!(gpioc->dir & BIT(offset)); +} + +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g, + unsigned long *mask, + unsigned long *bits) +{ + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); + u8 newovr; + int ret; + + newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask); + if (newovr == gpioc->ovr) + return; + + ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr); + if (!ret) + gpioc->ovr = newovr; +} + +static void cdns_i3c_gpio_set(struct gpio_chip *g, unsigned int offset, + int value) +{ + unsigned long mask = BIT(offset), bits = value ? BIT(offset) : 0; + + cdns_i3c_gpio_set_multiple(g, &mask, &bits); +} + +static int cdns_i3c_gpio_set_dir(struct cdns_i3c_gpio *gpioc, unsigned int pin, + bool in) +{ + u8 newdir; + int ret; + + newdir = gpioc->dir; + if (in) + newdir |= BIT(pin); + else + newdir &= ~BIT(pin); + + if (newdir == gpioc->dir) + return 0; + + gpioc->dir = newdir; + ret = cdns_i3c_gpio_write_reg(gpioc, DIR_MODE, newdir); + if (!ret) + gpioc->dir = newdir; + + return ret; +} + +static int cdns_i3c_gpio_dir_input(struct gpio_chip *g, unsigned int offset) +{ + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); + + return cdns_i3c_gpio_set_dir(gpioc, offset, true); +} + +static int cdns_i3c_gpio_dir_output(struct gpio_chip *g, unsigned int offset, + int val) +{ + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); + + cdns_i3c_gpio_set(g, offset, val); + + return cdns_i3c_gpio_set_dir(gpioc, offset, true); +} + +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g, + unsigned long *mask, + unsigned long *bits) +{ + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); + int ret; + u8 ivr; + + ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr); + if (ret) + return ret; + + *bits = ivr & *mask & gpioc->dir; + *bits |= gpioc->ovr & *mask & ~gpioc->dir; + + return 0; +} + +static int cdns_i3c_gpio_get(struct gpio_chip *g, unsigned int offset) +{ + unsigned long mask = BIT(offset), bits = 0; + int ret; + + ret = cdns_i3c_gpio_get_multiple(g, &mask, &bits); + if (ret) + return ret; + + return mask & bits; +} + +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev, + const struct i3c_ibi_payload *payload) +{ + struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev); + u8 isr = 0; + int i; + + cdns_i3c_gpio_read_reg(gpioc, ISR, &isr); + for (i = 0; i < 8; i++) { + unsigned int irq; + + if (!(BIT(i) & isr & gpioc->imr)) + continue; + + irq = irq_find_mapping(gpioc->gpioc.irq.domain, i); + handle_nested_irq(irq); + } +} + +static void cdns_i3c_gpio_irq_lock(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc); + + mutex_lock(&gpioc->irq_lock); +} + +static void cdns_i3c_gpio_irq_sync_unlock(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc); + int i; + + cdns_i3c_gpio_write_reg(gpioc, IMR, gpioc->imr); + for (i = 0; i < 3; i++) + cdns_i3c_gpio_write_reg(gpioc, ITR(i), gpioc->itr[i]); + + mutex_unlock(&gpioc->irq_lock); +} + +static void cdns_i3c_gpio_irq_unmask(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc); + + gpioc->imr |= BIT(data->hwirq); +} + +static void cdns_i3c_gpio_irq_mask(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc); + + gpioc->imr &= ~BIT(data->hwirq); +} + +static int cdns_i3c_gpio_irq_set_type(struct irq_data *data, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct cdns_i3c_gpio *gpioc = gpiochip_get_data(gc); + + switch (type) { + case IRQ_TYPE_LEVEL_HIGH: + gpioc->itr[0] |= BIT(data->hwirq); + gpioc->itr[1] |= BIT(data->hwirq); + break; + + case IRQ_TYPE_LEVEL_LOW: + gpioc->itr[0] |= BIT(data->hwirq); + gpioc->itr[1] &= ~BIT(data->hwirq); + break; + + case IRQ_TYPE_EDGE_BOTH: + gpioc->itr[0] &= ~BIT(data->hwirq); + gpioc->itr[2] |= BIT(data->hwirq); + break; + + case IRQ_TYPE_EDGE_RISING: + gpioc->itr[0] &= ~BIT(data->hwirq); + gpioc->itr[1] |= BIT(data->hwirq); + gpioc->itr[2] &= ~BIT(data->hwirq); + break; + + case IRQ_TYPE_EDGE_FALLING: + gpioc->itr[0] &= ~BIT(data->hwirq); + gpioc->itr[1] &= ~BIT(data->hwirq); + gpioc->itr[2] &= ~BIT(data->hwirq); + break; + + default: + return -EINVAL; + } + + return 0; +} + +static int cdns_i3c_gpio_probe(struct i3c_device *i3cdev) +{ + struct cdns_i3c_gpio *gpioc; + struct device *parent = i3cdev_to_dev(i3cdev); + struct i3c_ibi_setup ibisetup = { + .max_payload_len = 2, + .num_slots = 1, + .handler = cdns_i3c_gpio_ibi_handler, + }; + int ret; + + gpioc = devm_kzalloc(parent, sizeof(*gpioc), GFP_KERNEL); + if (!gpioc) + return -ENOMEM; + + gpioc->i3cdev = i3cdev; + i3cdev_set_drvdata(i3cdev, gpioc); + + /* Mask all interrupts. */ + ret = cdns_i3c_gpio_write_reg(gpioc, IMR, 0); + if (ret) + return ret; + + /* + * Clear the ISR after reading it, not when the IBI is is Acked by the + * I3C master. This way we make sure we don't lose events. + */ + ret = cdns_i3c_gpio_write_reg(gpioc, ITR(3), 0xff); + if (ret) + return ret; + + ret = cdns_i3c_gpio_read_reg(gpioc, DIR_MODE, &gpioc->dir); + if (ret) + return ret; + + ret = cdns_i3c_gpio_read_reg(gpioc, OVR, &gpioc->ovr); + if (ret) + return ret; + + ret = i3c_device_request_ibi(i3cdev, &ibisetup); + if (ret) + return ret; + + gpioc->gpioc.label = dev_name(parent); + gpioc->gpioc.owner = THIS_MODULE; + gpioc->gpioc.parent = parent; + gpioc->gpioc.base = -1; + gpioc->gpioc.ngpio = 8; + gpioc->gpioc.can_sleep = true; + gpioc->gpioc.get_direction = cdns_i3c_gpio_get_direction; + gpioc->gpioc.direction_input = cdns_i3c_gpio_dir_input; + gpioc->gpioc.direction_output = cdns_i3c_gpio_dir_output; + gpioc->gpioc.get = cdns_i3c_gpio_get; + gpioc->gpioc.get_multiple = cdns_i3c_gpio_get_multiple; + gpioc->gpioc.set = cdns_i3c_gpio_set; + gpioc->gpioc.set_multiple = cdns_i3c_gpio_set_multiple; + + ret = devm_gpiochip_add_data(parent, &gpioc->gpioc, gpioc); + if (ret) + return ret; + + gpioc->irqc.name = dev_name(parent); + gpioc->irqc.parent_device = parent; + gpioc->irqc.irq_unmask = cdns_i3c_gpio_irq_unmask; + gpioc->irqc.irq_mask = cdns_i3c_gpio_irq_mask; + gpioc->irqc.irq_bus_lock = cdns_i3c_gpio_irq_lock; + gpioc->irqc.irq_bus_sync_unlock = cdns_i3c_gpio_irq_sync_unlock; + gpioc->irqc.irq_set_type = cdns_i3c_gpio_irq_set_type; + gpioc->irqc.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND; + + ret = gpiochip_irqchip_add_nested(&gpioc->gpioc, &gpioc->irqc, 0, + handle_simple_irq, IRQ_TYPE_NONE); + if (ret) + goto err_free_ibi; + + ret = i3c_device_enable_ibi(i3cdev); + if (ret) + goto err_free_ibi; + + return 0; + +err_free_ibi: + i3c_device_free_ibi(i3cdev); + + return ret; +} + +static int cdns_i3c_gpio_remove(struct i3c_device *i3cdev) +{ + i3c_device_disable_ibi(i3cdev); + i3c_device_free_ibi(i3cdev); + + return 0; +} + +static const struct i3c_device_id cdns_i3c_gpio_ids[] = { + I3C_DEVICE(0x1c9, 0x0, NULL), + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids); + +static struct i3c_driver cdns_i3c_gpio = { + .driver.name = "cdns-i3c-gpio", + .id_table = cdns_i3c_gpio_ids, + .probe = cdns_i3c_gpio_probe, + .remove = cdns_i3c_gpio_remove, +}; +module_i3c_driver(cdns_i3c_gpio); + +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); +MODULE_DESCRIPTION("Driver for Cadence I3C GPIO expander"); +MODULE_LICENSE("GPL v2");
Add a driver for Cadence I3C GPIO expander. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- Hi Linus, I intentionally did not report your Acked-by because of the scract buffer changes I added in this version (needed to follow the "buffer should be DMA-able" rule) Feel free to add it back if you're okay with this version. Regards, Boris Changes in v5: - Use the !! operator to return 0 or 1 in cdns_i3c_gpio_get_direction() - Use a scratch buffer to make sure the buffers passed to i3c_device_do_priv_xfers() are DMA-able - Fix errors reported by checkpatch Changes in v4: - none Changes in v3: - new --- drivers/gpio/Kconfig | 11 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-cdns-i3c.c | 411 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 423 insertions(+) create mode 100644 drivers/gpio/gpio-cdns-i3c.c