From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: In-Reply-To: From: Arnd Bergmann Date: Fri, 16 Jul 2021 11:57:41 +0200 Message-ID: Subject: Re: [PATCH V5 0/2] virtio: Add specification for virtio-gpio Content-Type: text/plain; charset="UTF-8" To: Viresh Kumar Cc: Jason Wang , "Michael S. Tsirkin" , Cornelia Huck , Linus Walleij , Bartosz Golaszewski , Vincent Guittot , Jean-Philippe Brucker , Bill Mills , =?UTF-8?B?QWxleCBCZW5uw6ll?= , "Enrico Weigelt, metux IT consult" , virtio-dev@lists.oasis-open.org, Geert Uytterhoeven List-ID: On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar wrote: > > Hello, > > This patchset adds virtio specification for GPIO devices. It supports basic GPIO > line operations, along with optional interrupts on them (in a separate patch). > > This is an *alternative implementation* of the virtio-gpio specification, > another version of it was earlier posted by Enrico [1]. > > I took back V4 of the specification I posted earlier (on June 17th), to allow > Enrico to come up with something that is robust and works for everyone (as he > started this thing last year), but it didn't go as expected. I couldn't > see any of my review comments being incorporated (or any intentions of them > getting in ever) in the two versions Enrico posted and a month passed since > then. > > And so I am trying to present an alternative approach here to solve the problem, > which I believe is more clear and robust. I am open to suggestions to improve it > further. Hi Viresh, I've looked at the two proposals again now, and yours does look more mature, so I agree it makes sense to continue with your version. > I will let the virtio/gpio maintainers decide on its fate. > > Key differences from Enrico's approach [1]: > > - config structure is rearranged to remove unnecessary fields/padding, and place > gpio_names block in such a way that we can expand the structure easily going > forward, if required. Simplifying is good, maybe we can cut it down even further. I now actually wonder if we shouldn't remove the gpio names from the virtio-gpu spec entirely. In practice I assume this driver only makes sense to be used in combination with firmware describing how it interacts with other drivers (gpio-key, gpio-led, mmc, i2c, ...) that require access to gpio lines for something they do, and these already rely on DT information or some equivalent. As soon as we have a DT node for the device, the names can just be passed according ot the generic DT binding for gpio controllers. > - Supports freeing of a GPIO line after use. I agree it makes sense have both alloc and free as a pair if you have one of them, but as I commented I'm not convinced we do need them at all, so removing both may be better here. > - Interrupt implementation handled with feature bit 0. Either the interrupts are > fully supported or not at all. The interrupt definition looks better than Enrico's version, but the serialization still looks incomplete, especially since you added level triggered mode. I always get confused by those as well, so we'll need to have this reviewed by an irqchip person as well, but let's start with my questions for now. > - Doesn't add any ordering restrictions on the device, it can respond earlier to > the second request, while still processing the first one. > > - Clearly state that two requests can't be initiated for the same line by device > or driver. These are good. The other comments I had regarding the use of errno values, and the split between txq/rxq messages are things you inherited from Enrico's version, so those need to be resolved regardless. Arnd