| Message ID | 20190822211514.19288-3-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 37b4100180641968056cb4e034cebc38338e8652 |
| Headers | show |
| Series | Poll mode for NXP DSPI driver | expand |
On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote: > The DSPI interrupt can be shared between two controllers at least on the > LX2160A. In that case, the driver for one controller might misbehave and > consume the other's interrupt. Fix this by actually checking if any of > the bits in the status register have been asserted. It would be better to have done this as the first patch before the restructuring, that way we could send this as a fix - the refactoring while good doesn't really fit with stable.
Hi Mark, On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote: > > The DSPI interrupt can be shared between two controllers at least on the > > LX2160A. In that case, the driver for one controller might misbehave and > > consume the other's interrupt. Fix this by actually checking if any of > > the bits in the status register have been asserted. > > It would be better to have done this as the first patch before > the restructuring, that way we could send this as a fix - the > refactoring while good doesn't really fit with stable. Did you see this? https://lkml.org/lkml/2019/8/22/1542 Regards, -Vladimir
On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote: > On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote: > > It would be better to have done this as the first patch before > > the restructuring, that way we could send this as a fix - the > > refactoring while good doesn't really fit with stable. > Did you see this? > https://lkml.org/lkml/2019/8/22/1542 I'm not online enough to readily follow that link right now, I did apply another patch for a similar issue though. If that's a different version of the same change please don't do that, sending multiple conflicting versions of the same thing creates conflicts and makes everything harder to work with. Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read.
On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote: > On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote: > > Did you see this? > > https://lkml.org/lkml/2019/8/22/1542 > I'm not online enough to readily follow that link right now, I > did apply another patch for a similar issue though. If that's > a different version of the same change please don't do that, > sending multiple conflicting versions of the same thing creates > conflicts and makes everything harder to work with. Oh, I guess this was due to there being an existing refactoring in -next that meant the fix wouldn't apply directly. I sorted that out now I think, but in general the same thing applies - it's better to put fixes before anything else in the series, it'll flag up when reviewing.
Hi Mark, On Fri, 23 Aug 2019 at 13:59, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote: > > On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote: > > > > Did you see this? > > > https://lkml.org/lkml/2019/8/22/1542 > > > I'm not online enough to readily follow that link right now, I > > did apply another patch for a similar issue though. If that's > > a different version of the same change please don't do that, > > sending multiple conflicting versions of the same thing creates > > conflicts and makes everything harder to work with. > > Oh, I guess this was due to there being an existing refactoring > in -next that meant the fix wouldn't apply directly. I sorted > that out now I think, but in general the same thing applies - > it's better to put fixes before anything else in the series, > it'll flag up when reviewing. I try to require as little attention span as possible from you and I apologize that I'm sending patches like a noob, but I'm not used to this sort of interaction with a maintainer. It's taking me some time to adjust expectations. - You left change requests in the initial patchset I submitted, but you partially applied the series anyway. You didn't give me a chance to respin the whole series and put the shared IRQ fix on top, so it applies on old trees as well. No problem, I sent two versions of the patch. - On my previous series you left this comment: > Please don't include all the extra tags you've got in your subject > lines. In my inbox this series looks like: > > 790 N T 08/18 Vladimir Oltean ( 16K) ├─>[PATCH spi for-5.4 01/14] spi: spi-f > > so I can't tell what it's actually about just from looking at it. Just > [PATCH 01/14] would be enough, putting a target version in or versioning > the patch series can be OK but you usually don't use a target version > for -next and adding spi in there is redundant given that it's also in > the changelog. So I didn't put any target version in the patch titles this time, although arguably it would have been clearer to you that there's a patch for-5.4 and another version of it for-4.20 (which i *think* is how I should submit a fix, I don't see any branch for inclusion in stable trees per se). No problem, I explained in the cover letters that one patchset is for -next and the other is for inclusion in stable trees. Maintainers do read cover letters, right? Message from the -next cover letter: > The series also contains a bug fix for the shared IRQ of the DSPI > driver. I am going to respin a version of it as a separate patch for > inclusion in stable trees, independent of this patchset. Message from the fix's cover letter: > This patch is taken out of the "Poll mode for NXP DSPI driver" series > and respun against the "for-4.20" branch. > $(git describe --tags 13aed2392741) shows: > v4.20-rc1-18-g13aed2392741 Yes, I did send a cover letter for a single patch. I thought it's harder to miss than a note hidden under patch 2/5 of one series, and in the note section of the other's. I think you could have also made an argument about me not doing it the other way around. In the end, you can not read a note just as well as not read a cover letter, and there's little I can do. No problem, you missed the link between the two. I sent you a link to the lkml archive. You said "I'm not online enough to readily follow that link right now". Please teach me - I really don't know - how can I make links between patchsets easier for you to follow, if you don't read cover letters and you can't access lkml? I promise I'll use that method next time. > I do frequently catch up on my mail on flights or while otherwise > travelling so this is even more pressing for me than just being about > making things a bit easier to read. Maybe you simply should do something else while traveling, just saying. Regards, -Vladimir
On Fri, Aug 23, 2019 at 03:06:52PM +0300, Vladimir Oltean wrote: > - You left change requests in the initial patchset I submitted, but > you partially applied the series anyway. You didn't give me a chance > to respin the whole series and put the shared IRQ fix on top, so it > applies on old trees as well. No problem, I sent two versions of the > patch. Right, and this is fine. A big part of this is that it's just generally bad practice to not have fixes at the front of the series, I'd flag this up as a problem even if the code was all new and there was no question of applying as a bug fix. It's something that's noticable just at the level of looking at the shape of the series without even looking at the contents of the patches, if the fix is actually a good one or anything like that. In the context of this it made it look like the reason you'd had to do two versions. > So I didn't put any target version in the patch titles this time, > although arguably it would have been clearer to you that there's a > patch for-5.4 and another version of it for-4.20 (which i *think* is > how I should submit a fix, I don't see any branch for inclusion in > stable trees per se). Not for 4.20, for v5.3 - we basically only fix Linus' tree directly, anything else gets backported from there unless it's super important. I don't think anyone is updating v4.20 at all these days, the version number change from v4 to v5 was totally arbatrary. > Yes, I did send a cover letter for a single patch. I thought it's > harder to miss than a note hidden under patch 2/5 of one series, and > in the note section of the other's. I think you could have also made If you're sending a multi-patch series it's of course good to send a cover letter, it's just single patches where it's adding overhead. > No problem, you missed the link between the two. I sent you a link to > the lkml archive. You said "I'm not online enough to readily follow > that link right now". Please teach me - I really don't know - how can It's not that I missed the link between them, it's that what I'd expected to see was the fix being the first patch in the series for -next and for that fix to look substantially the same with at most some context difference. I wasn't expecting to see a completely different patch that wasn't at the start of the series, had the fix been at the start of the series it'd have been fairly clear what was going on but the refactoring patch looked like the main reason you'd needed different versions (it's certainly why they don't visually resemble each other). In other words it looked like you'd sent a different fix because the fix you'd done for -next was based on the first patch in the series rather than there also being some context changes. > I make links between patchsets easier for you to follow, if you don't > read cover letters and you can't access lkml? I promise I'll use that > method next time. Like I said include a plain text description of what you're linking to (eg, the subject line from a mail). > > I do frequently catch up on my mail on flights or while otherwise > > travelling so this is even more pressing for me than just being about > > making things a bit easier to read. > Maybe you simply should do something else while traveling, just saying. I could also add in the coffee shop I sometimes work from which doesn't have WiFi or mobile coverage. Besides, like that part of the text does say it's also a usability thing, having to fire up a web browser to figure out what's being described is a stumbling block.
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index c90db7db4121..6ef2279a3699 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -659,7 +659,7 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id) regmap_write(dspi->regmap, SPI_SR, spi_sr); if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF))) - return IRQ_HANDLED; + return IRQ_NONE; /* Get transfer counter (in number of SPI transfers). It was * reset to 0 when transfer(s) were started.
The DSPI interrupt can be shared between two controllers at least on the LX2160A. In that case, the driver for one controller might misbehave and consume the other's interrupt. Fix this by actually checking if any of the bits in the status register have been asserted. Fixes: 13aed2392741 ("spi: spi-fsl-dspi: use IRQF_SHARED mode to request IRQ") Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/spi/spi-fsl-dspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)