On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote: > > > On 09/28/2017 04:56 AM, Thierry Reding wrote: > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > Hi Linus, > > > > here's the latest series of patches that implement the tighter IRQ chip > > integration as well as the banked GPIO infrastructure that we had > > discussed a couple of weeks/months back. > > > > The first couple of patches are mostly preparatory work in order to > > consolidate all IRQ chip related fields in a new structure and create > > the base functionality for adding IRQ chips. > > > > After that, I've added the Tegra186 GPIO support patch that makes use of > > the new tight integration. > > > > To round things off the new banked GPIO infrastructure is added (along > > with some more preparatory work), followed by the conversion of the two > > Tegra GPIO drivers to the new infrastructure. > > Hm. So you've ignored my comments [1]. > > Sry, but I do not agree with this series. > - no prof that it can be re-used by other drivers than tegra > (at least I do not see reasons to re-use it for any TI drivers) I had done some research based on Linus' feedback from an earlier series and identified the following potential candidates[0] that could move to this new infrastructure: - gpio-intel-mid.c - gpio-merrifield.c - gpio-pca953x.c - gpio-stmpe.c - gpio-tc3589x.c - gpio-ws16c48.c Note that this is based on code inspection rather than DT inspection, because that's fundamentally flawed. If you look at this from a DT perspective you're going to be tempted to change the DT bindings, but you can't do that because of backwards compatiblity. This new framework also doesn't address the issues at that level, but rather tries to be some common code that is otherwise duplicated in one way or another in various drivers and therefore hard to maintain. This is what Linus had originally requested, and that's what the series does. > - no split What does this mean? The series is nicely split into separate patches, so each one individually is easy to review. I've also gone through quite some trouble to make sure everything builds fine after each patch, so it's possible to apply individual bits of the series. For example we could opt to apply everything up to the banked GPIO support if that's still contentious. > - all GPIO IRQs mapped statically This series predates your work on the dynamic IRQ mapping, so I hadn't picked up those changes. This should be easily solved by the attached patch, though. > - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins > which is waste of memory It's the only way to generically do this. Also I don't think this wastes that much memory. We have one unsigned int per pin, which even for very large GPIO controllers is unlikely to exceed one 4 KiB page. > - DT binding changes not documented and no DT examples That's because this is completely orthogonal to DT bindings. We can't make any changes to the bindings because of ABI stability. > - below is ugly ;) > + bank = (spec[0] >> gc->of_gpio_bank_mask) & gc->of_gpio_bank_shift; > + pin = (spec[0] >> gc->of_gpio_pin_mask) & gc->of_gpio_pin_shift; If by ugly you mean wrong, then yes, it's actually the wrong way around. It should be: bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask; line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask; Thierry
Attachment:
signature.asc
Description: PGP signature
