| Message ID | 20200608134300.76091-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | mfd: Make use of software nodes | expand |
On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > Some devices would need to have a hierarchy of properties and > child nodes passed to the child or children of MFD. For such case > we may utilize software nodes, which is superior on device properties. > > Add support of software nodes to MFD core and convert one driver > to show how it looks like. This allows to get rid of legacy platform > data. > > The change has been tested on Intel Galileo Gen 2. I am wondering whether we could move the {gpio_base, ngpio, irq_shared} part into the gpio-dwapb.c driver and use either the ACPI-based or platform_device_id-based matching to get the device-specific resources info through the driver_data field. By doing so you wouldn't need to introduce a new "snps,gpio-base"-like property and propagate software_node-based properties, but still you could get rid of the dwapb_platform_data structure since all the info would be locally available. If ACPI-based matching doesn't uniquely address the Quark GPIO node, then you could just replace the intel_quark_mfd_cells[0].name with something like "gpio-dwapb-quark", which then by the MFD core will be copied to the corresponding platform_device->name due to calling platform_device_alloc() with cell-name passed. That name will be used to match a platform_driver with id_table having that new name added. -Sergey > > Andy Shevchenko (5): > gpio: dwapb: Replace irq_shared flag with fwnode type check > gpio: dwapb: Read GPIO base from snps,gpio-base property > mfd: intel_quark_i2c_gpio: Convert to use software nodes > gpio: dwapb: Get rid of legacy platform data > gpio: dwapb: Define magic number for IRQ and GPIO lines > > Heikki Krogerus (1): > mfd: core: Propagate software node group to the sub devices > > drivers/gpio/gpio-dwapb.c | 44 +++++++++------- > drivers/mfd/intel_quark_i2c_gpio.c | 64 +++++++++++------------- > drivers/mfd/mfd-core.c | 31 ++++++++++-- > include/linux/mfd/core.h | 3 ++ > include/linux/platform_data/gpio-dwapb.h | 23 --------- > 5 files changed, 85 insertions(+), 80 deletions(-) > delete mode 100644 include/linux/platform_data/gpio-dwapb.h > > -- > 2.27.0.rc2 >
On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > > Some devices would need to have a hierarchy of properties and > > child nodes passed to the child or children of MFD. For such case > > we may utilize software nodes, which is superior on device properties. > > > > Add support of software nodes to MFD core and convert one driver > > to show how it looks like. This allows to get rid of legacy platform > > data. > > > > The change has been tested on Intel Galileo Gen 2. > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > part into the gpio-dwapb.c driver and use either the ACPI-based or > platform_device_id-based matching to get the device-specific resources > info through the driver_data field. By doing so you wouldn't need to > introduce a new "snps,gpio-base"-like property and propagate > software_node-based properties, but still you could get rid of the > dwapb_platform_data structure since all the info would be locally > available. The idea is to get rid of the driver being dependent on some quirks when we may do it clearly and nicely. We, by applying this series, make (keep) layers independent: board code vs. driver code. Mixing them more is the opposite to what I propose. WRT property. snps,gpio-base can be easily changed to *already in use* gpio-base or being both converted to linux,gpio-base to explicitly show people that this is internal stuff that must not be present in firmware nodes. > If ACPI-based matching doesn't uniquely address the Quark GPIO node, > then you could just replace the intel_quark_mfd_cells[0].name with > something like "gpio-dwapb-quark", which then by the MFD core will be > copied to the corresponding platform_device->name due to calling > platform_device_alloc() with cell-name passed. That name will be used > to match a platform_driver with id_table having that new name added. Oh, that doesn't sound right. It makes things ugly.
On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > > > Some devices would need to have a hierarchy of properties and > > > child nodes passed to the child or children of MFD. For such case > > > we may utilize software nodes, which is superior on device properties. > > > > > > Add support of software nodes to MFD core and convert one driver > > > to show how it looks like. This allows to get rid of legacy platform > > > data. > > > > > > The change has been tested on Intel Galileo Gen 2. > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > > part into the gpio-dwapb.c driver and use either the ACPI-based or > > platform_device_id-based matching to get the device-specific resources > > info through the driver_data field. By doing so you wouldn't need to > > introduce a new "snps,gpio-base"-like property and propagate > > software_node-based properties, but still you could get rid of the > > dwapb_platform_data structure since all the info would be locally > > available. > > The idea is to get rid of the driver being dependent on some quirks > when we may do it clearly and nicely. Yes, I've got that and in most of the aspects I like what you suggested in this parchset. But it seems to me that the maintainers are mostly prone to having some of the platform-specifics being locally (in-driver) defined. So I proposed an alternative solution, which might do to satisfy their requirement. Note saying that you want to get rid of the quirks and introducing something like "gpio-base" firmware property seems contradicting a bit. > We, by applying this series, make (keep) layers independent: board > code vs. driver code. Mixing them more is the opposite to what I > propose. > > WRT property. > snps,gpio-base can be easily changed to *already in use* gpio-base or > being both converted to linux,gpio-base to explicitly show people that > this is internal stuff that must not be present in firmware nodes. > As I see it the part with "gpio-base" and the irq_shared can be moved to the gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in the driver. * Note I don't really like replacing the irq_shared flag with to_of_node() because in general to_of_node() doesn't mean the IRQ line is shared, so selecting the shared and non-shared interrupt request paths based on that macro seems hackish. > > If ACPI-based matching doesn't uniquely address the Quark GPIO node, > > then you could just replace the intel_quark_mfd_cells[0].name with > > something like "gpio-dwapb-quark", which then by the MFD core will be > > copied to the corresponding platform_device->name due to calling > > platform_device_alloc() with cell-name passed. That name will be used > > to match a platform_driver with id_table having that new name added. > > Oh, that doesn't sound right. It makes things ugly. I may have said that a bit unclearly. The only thing you'd need to do is to add an unique name to the Quark GPIO cell, like: drivers/mfd/intel_quark_i2c_gpio.c: static struct mfd_cell intel_quark_mfd_cells[] = { { .name = "gpio-dwapb-quark", } Then make the gpio-dwapb.c driver being compatible with that device by declaring the id_table with that device name and passing the table to the DW APB GPIO "struct platform_driver" descriptor. The MFD/platform cores already provide the functionality of matching those two device and driver. If ACPI node uniquely defines the Quark GPIO with all that quirks applicable then you wouldn't even need to do the platform_device-driver-based matching. Just use the acpi_device_id to get the quirks flags/descriptors. The rest of the patchset concerning passing software_node's down to the individual MFD sub-device and declaring "reg" and "snps,nr-gpios" in there seems pretty much acceptable to me. * Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in the DT schema and have the "ngpios" supported as well. -Sergey > > -- > With Best Regards, > Andy Shevchenko
On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote: > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > > > > Some devices would need to have a hierarchy of properties and > > > > child nodes passed to the child or children of MFD. For such case > > > > we may utilize software nodes, which is superior on device properties. > > > > > > > > Add support of software nodes to MFD core and convert one driver > > > > to show how it looks like. This allows to get rid of legacy platform > > > > data. > > > > > > > > The change has been tested on Intel Galileo Gen 2. > > > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > > > part into the gpio-dwapb.c driver and use either the ACPI-based or > > > platform_device_id-based matching to get the device-specific resources > > > info through the driver_data field. By doing so you wouldn't need to > > > introduce a new "snps,gpio-base"-like property and propagate > > > software_node-based properties, but still you could get rid of the > > > dwapb_platform_data structure since all the info would be locally > > > available. > > > > The idea is to get rid of the driver being dependent on some quirks > > when we may do it clearly and nicely. > > Yes, I've got that and in most of the aspects I like what you suggested > in this parchset. But it seems to me that the maintainers are mostly prone > to having some of the platform-specifics being locally (in-driver) defined. You are a maintainer of the dwapb-gpio. Is it what you insist? > So I proposed an alternative solution, which might do to satisfy their > requirement. I'm puzzled whom about you are talking. > Note saying that you want to get rid of the quirks and > introducing something like "gpio-base" firmware property seems contradicting > a bit. Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver, so, it wouldn't care about what platform(s) require base and what do not. > > We, by applying this series, make (keep) layers independent: board > > code vs. driver code. Mixing them more is the opposite to what I > > propose. > > > > WRT property. > > snps,gpio-base can be easily changed to *already in use* gpio-base or > > being both converted to linux,gpio-base to explicitly show people that > > this is internal stuff that must not be present in firmware nodes. > > As I see it the part with "gpio-base" and the irq_shared can be moved to the > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in > the driver. Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks. > * Note I don't really like replacing the irq_shared flag with to_of_node() > because in general to_of_node() doesn't mean the IRQ line is shared, so > selecting the shared and non-shared interrupt request paths based on that macro > seems hackish. This I can understand, but can you propose better alternative? > > > If ACPI-based matching doesn't uniquely address the Quark GPIO node, > > > then you could just replace the intel_quark_mfd_cells[0].name with > > > something like "gpio-dwapb-quark", which then by the MFD core will be > > > copied to the corresponding platform_device->name due to calling > > > platform_device_alloc() with cell-name passed. That name will be used > > > to match a platform_driver with id_table having that new name added. > > > > Oh, that doesn't sound right. It makes things ugly. > > I may have said that a bit unclearly. The only thing you'd need to do is to > add an unique name to the Quark GPIO cell, like: > drivers/mfd/intel_quark_i2c_gpio.c: > static struct mfd_cell intel_quark_mfd_cells[] = { > { > .name = "gpio-dwapb-quark", > } > > Then make the gpio-dwapb.c driver being compatible with that device by declaring > the id_table with that device name and passing the table to the DW APB GPIO > "struct platform_driver" descriptor. The MFD/platform cores already provide the > functionality of matching those two device and driver. If ACPI node uniquely > defines the Quark GPIO with all that quirks applicable then you wouldn't even > need to do the platform_device-driver-based matching. Just use the acpi_device_id > to get the quirks flags/descriptors. What you are talking about? Can you provide a code we can discuss? > * Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in > the DT schema and have the "ngpios" supported as well. Actually I used to have that piece in my first patches, but decided to remove due to this property being (semi-)internal. I would like to hear GPIO maintainers about this. Bart, Linus?
On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote: > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote: > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > > > > > Some devices would need to have a hierarchy of properties and > > > > > child nodes passed to the child or children of MFD. For such case > > > > > we may utilize software nodes, which is superior on device properties. > > > > > > > > > > Add support of software nodes to MFD core and convert one driver > > > > > to show how it looks like. This allows to get rid of legacy platform > > > > > data. > > > > > > > > > > The change has been tested on Intel Galileo Gen 2. > > > > > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > > > > part into the gpio-dwapb.c driver and use either the ACPI-based or > > > > platform_device_id-based matching to get the device-specific resources > > > > info through the driver_data field. By doing so you wouldn't need to > > > > introduce a new "snps,gpio-base"-like property and propagate > > > > software_node-based properties, but still you could get rid of the > > > > dwapb_platform_data structure since all the info would be locally > > > > available. > > > > > > The idea is to get rid of the driver being dependent on some quirks > > > when we may do it clearly and nicely. > > > > Yes, I've got that and in most of the aspects I like what you suggested > > in this parchset. But it seems to me that the maintainers are mostly prone > > to having some of the platform-specifics being locally (in-driver) defined. > > You are a maintainer of the dwapb-gpio. Is it what you insist? That was a discussable suggestion of how the patches could be improved to still have the software-node functionality and to get rid of the legacy platform data passing around. > > > So I proposed an alternative solution, which might do to satisfy their > > requirement. > > I'm puzzled whom about you are talking. I don't have a habit to say of myself like that) So obviously I was talking about Linus and Lee in the previous sentence mostly referring to the Linus' words regarding setting the GPIO base and having the "gpio-base" property at all. > > > Note saying that you want to get rid of the quirks and > > introducing something like "gpio-base" firmware property seems contradicting > > a bit. > > Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver, > so, it wouldn't care about what platform(s) require base and what do not. My point is that no mater how you pass the gpio-base value it will always be a quirk. The same thing is with the irq_shared flag. Both of them are specific to the Quark-platform. They are used to tune the DW APB GPIO driver up so one would create a GPIO device working well with the Quark-specific DW APB GPIO block. > > > > We, by applying this series, make (keep) layers independent: board > > > code vs. driver code. Mixing them more is the opposite to what I > > > propose. > > > > > > WRT property. > > > snps,gpio-base can be easily changed to *already in use* gpio-base or > > > being both converted to linux,gpio-base to explicitly show people that > > > this is internal stuff that must not be present in firmware nodes. > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in > > the driver. > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks. As I said above having the gpio-base value set is the platform-specific thing in any case. So no mater whether you pass it via the software_node/properties or the private platform-data structure, the DW APB GPIO driver will have to handle those parameters in some special way, which is quirk-prone since normal platforms don't have such peculiar requirements. > > > * Note I don't really like replacing the irq_shared flag with to_of_node() > > because in general to_of_node() doesn't mean the IRQ line is shared, so > > selecting the shared and non-shared interrupt request paths based on that macro > > seems hackish. > > This I can understand, but can you propose better alternative? Alas I can't at the moment. But see my comment below. > > > > > If ACPI-based matching doesn't uniquely address the Quark GPIO node, > > > > then you could just replace the intel_quark_mfd_cells[0].name with > > > > something like "gpio-dwapb-quark", which then by the MFD core will be > > > > copied to the corresponding platform_device->name due to calling > > > > platform_device_alloc() with cell-name passed. That name will be used > > > > to match a platform_driver with id_table having that new name added. > > > > > > Oh, that doesn't sound right. It makes things ugly. > > > > I may have said that a bit unclearly. The only thing you'd need to do is to > > add an unique name to the Quark GPIO cell, like: > > drivers/mfd/intel_quark_i2c_gpio.c: > > static struct mfd_cell intel_quark_mfd_cells[] = { > > { > > .name = "gpio-dwapb-quark", > > } > > > > Then make the gpio-dwapb.c driver being compatible with that device by declaring > > the id_table with that device name and passing the table to the DW APB GPIO > > "struct platform_driver" descriptor. The MFD/platform cores already provide the > > functionality of matching those two device and driver. If ACPI node uniquely > > defines the Quark GPIO with all that quirks applicable then you wouldn't even > > need to do the platform_device-driver-based matching. Just use the acpi_device_id > > to get the quirks flags/descriptors. > > What you are talking about? Can you provide a code we can discuss? Here is what I suggest in step-by-step: Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition to the gpio-dwapb.c driver. Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB GPIO block living in the Quark platform (by means of either ACPI or the platform-device ID data). Patch 3) Introduce the software_node propagation functionality (AFAIU Lee wants it to be a part of the platform-device core instead of being created on the MFD level). Patch 4) Convert the intel_quark_i2c_gpio driver to using the software nodes instead of creating the dwapb_platform_data instance (nearly matches your patch 4 except it won't introduce any "gpio-base"-like property). Patch 5) Get rid of the global dwapb_platform_data utilization (matches your patch 5). Patch 6) Introduce the IRQ and GPIO lines macro (matches your patch 6). By doing as I suggested you don't have to pass around the GPIO-base value and the IRQ-shared flag. They will be initialized and utilized locally in the DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1 and 2). In that case a software-node created in Patch 3 and 4 will be a normal firmware node described by the DW bindings. You'll still be able to get rid of the legacy global platform-data in the Patch 5. The spirit of your original patchset will still be preserved: introduce the software-nodes propagation interface and use it to pass the generic DW APB GPIO parameters. The only difference is that we'd move the GPIO-base and IRQ-shared functionality fully into the DW APB GPIO driver. By doing so we'd have: 1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver. In the current implementation we have them distributed between two drivers: intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters, gpio-dwapb uses them for setup. Note there is no any other platform with DW APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do we need to have such complicated interface then if we can identify whether the particular DW APB GPIO is a Quark-specific block or not? Moreover having those specifics in both drivers in fact means having quirks in both of them. 2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO device with software-nodes fully compatible with DT binding. DW APB GPIO driver will detect whether the block is Quark-specific and will activate the corresponding quirks if it is. 3) Still legacy global platform-data removed. -Sergey > > > * Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in > > the DT schema and have the "ngpios" supported as well. > > Actually I used to have that piece in my first patches, but decided to remove > due to this property being (semi-)internal. I would like to hear GPIO > maintainers about this. > > Bart, Linus? > > -- > With Best Regards, > Andy Shevchenko > >
On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote: > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote: > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote: > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: ... > > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > > > > > part into the gpio-dwapb.c driver and use either the ACPI-based or > > > > > platform_device_id-based matching to get the device-specific resources > > > > > info through the driver_data field. By doing so you wouldn't need to > > > > > introduce a new "snps,gpio-base"-like property and propagate > > > > > software_node-based properties, but still you could get rid of the > > > > > dwapb_platform_data structure since all the info would be locally > > > > > available. > > > > > > > > The idea is to get rid of the driver being dependent on some quirks > > > > when we may do it clearly and nicely. > > > > > > > Yes, I've got that and in most of the aspects I like what you suggested > > > in this parchset. But it seems to me that the maintainers are mostly prone > > > to having some of the platform-specifics being locally (in-driver) defined. > > > > You are a maintainer of the dwapb-gpio. Is it what you insist? > > That was a discussable suggestion of how the patches could be improved to > still have the software-node functionality and to get rid of the legacy > platform data passing around. > > > So I proposed an alternative solution, which might do to satisfy their > > > requirement. > > > > I'm puzzled whom about you are talking. > > I don't have a habit to say of myself like that) So obviously I was talking > about Linus and Lee in the previous sentence mostly referring to the Linus' > words regarding setting the GPIO base and having the "gpio-base" property > at all. Linus replied to three messages out of 7, seems you read only the first one. That's why I have been puzzled. > > > Note saying that you want to get rid of the quirks and > > > introducing something like "gpio-base" firmware property seems contradicting > > > a bit. > > > > Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver, > > so, it wouldn't care about what platform(s) require base and what do not. > > My point is that no mater how you pass the gpio-base value it will always be a > quirk. The same thing is with the irq_shared flag. Both of them are specific to > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would > create a GPIO device working well with the Quark-specific DW APB GPIO block. Precisely! > > > > We, by applying this series, make (keep) layers independent: board > > > > code vs. driver code. Mixing them more is the opposite to what I > > > > propose. > > > > > > > > WRT property. > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or > > > > being both converted to linux,gpio-base to explicitly show people that > > > > this is internal stuff that must not be present in firmware nodes. > > > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in > > > the driver. > > > > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks. > > As I said above having the gpio-base value set is the platform-specific thing in > any case. So no mater whether you pass it via the software_node/properties or > the private platform-data structure, the DW APB GPIO driver will have to handle > those parameters in some special way, which is quirk-prone since normal platforms > don't have such peculiar requirements. Seems you are proposing layering violation without seeing it. Let me explain the design of the drivers in Linux kernel. There are basically few possible concepts how drivers are handling quirks (because we know that most of the hardware support is a set of quirks here and there): 1) platform provides a board files and via platform data structures supplies quirks to the certain driver 2) platform provides a firmware node (ACPI, DT, ...) and unified driver handles it thru the fwnode interface 3) driver is split to be a library (or core part) + glue drivers full of quirks 4) driver has embedded quirks and supplies them based on IDs (PCI ID, ACPI ID, compatible string, ID table, etc) 5) ...missed something? ... What I'm proposing is turn 1 to 2 for Quark case, what you are proposing is absent on the picture, i.e.: x) platform provides a board file (intel_quark_i2c_gpio.c) and the driver has embedded quirks based on ID. This is not what we want, definitely. ... > > What you are talking about? Can you provide a code we can discuss? > > Here is what I suggest in step-by-step: > Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition > to the gpio-dwapb.c driver. Layering violation: spreading board code over the kernel. > Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB > GPIO block living in the Quark platform (by means of either ACPI or > the platform-device ID data). Same. (The rest doesn't matter. I dropped it because it's basically the same what I proposed) > By doing as I suggested you don't have to pass around the GPIO-base value and > the IRQ-shared flag. They will be initialized and utilized locally in the > DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1 > and 2). In that case a software-node created in Patch 3 and 4 will be a normal > firmware node described by the DW bindings. You'll still be able to get rid of > the legacy global platform-data in the Patch 5. > > The spirit of your original patchset will still be preserved: introduce the > software-nodes propagation interface and use it to pass the generic DW APB GPIO > parameters. The only difference is that we'd move the GPIO-base and IRQ-shared > functionality fully into the DW APB GPIO driver. By doing so we'd have: > 1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver. > In the current implementation we have them distributed between two drivers: > intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters, > gpio-dwapb uses them for setup. Note there is no any other platform with DW > APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do > we need to have such complicated interface then if we can identify whether > the particular DW APB GPIO is a Quark-specific block or not? Moreover having > those specifics in both drivers in fact means having quirks in both of them. > 2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO > device with software-nodes fully compatible with DT binding. DW APB GPIO > driver will detect whether the block is Quark-specific and will activate > the corresponding quirks if it is. > 3) Still legacy global platform-data removed. I see. But you mistakenly made a conclusion 1). It will be spread to two drivers. And in 2) you considered (wrongly) that MFD driver is not a board code. Again MFD driver _is_ a board code or i.o.w. quirk driver for specific Quark (MFD) device.
On Sat, Jun 20, 2020 at 01:13:56PM +0300, Andy Shevchenko wrote: > On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote: > > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared} > > > > > > part into the gpio-dwapb.c driver and use either the ACPI-based or > > > > > > platform_device_id-based matching to get the device-specific resources > > > > > > info through the driver_data field. By doing so you wouldn't need to > > > > > > introduce a new "snps,gpio-base"-like property and propagate > > > > > > software_node-based properties, but still you could get rid of the > > > > > > dwapb_platform_data structure since all the info would be locally > > > > > > available. > > > > > > > > > > The idea is to get rid of the driver being dependent on some quirks > > > > > when we may do it clearly and nicely. > > > > > > > > > > Yes, I've got that and in most of the aspects I like what you suggested > > > > in this parchset. But it seems to me that the maintainers are mostly prone > > > > to having some of the platform-specifics being locally (in-driver) defined. > > > > > > You are a maintainer of the dwapb-gpio. Is it what you insist? > > > > That was a discussable suggestion of how the patches could be improved to > > still have the software-node functionality and to get rid of the legacy > > platform data passing around. > > > > > So I proposed an alternative solution, which might do to satisfy their > > > > requirement. > > > > > > I'm puzzled whom about you are talking. > > > > I don't have a habit to say of myself like that) So obviously I was talking > > about Linus and Lee in the previous sentence mostly referring to the Linus' > > words regarding setting the GPIO base and having the "gpio-base" property > > at all. > > Linus replied to three messages out of 7, seems you read only the first one. > That's why I have been puzzled. Actually I've read all of them and from what I could see, he didn't like a code setting gpio-base regardless whether it has been created before your patchset or your explanation why it was necessary. > > > > > Note saying that you want to get rid of the quirks and > > > > introducing something like "gpio-base" firmware property seems contradicting > > > > a bit. > > > > > > Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver, > > > so, it wouldn't care about what platform(s) require base and what do not. > > > > My point is that no mater how you pass the gpio-base value it will always be a > > quirk. The same thing is with the irq_shared flag. Both of them are specific to > > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would > > create a GPIO device working well with the Quark-specific DW APB GPIO block. > > Precisely! > > > > > > We, by applying this series, make (keep) layers independent: board > > > > > code vs. driver code. Mixing them more is the opposite to what I > > > > > propose. > > > > > > > > > > WRT property. > > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or > > > > > being both converted to linux,gpio-base to explicitly show people that > > > > > this is internal stuff that must not be present in firmware nodes. > > > > > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the > > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a > > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in > > > > the driver. > > > > > > > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks. > > > > As I said above having the gpio-base value set is the platform-specific thing in > > any case. So no mater whether you pass it via the software_node/properties or > > the private platform-data structure, the DW APB GPIO driver will have to handle > > those parameters in some special way, which is quirk-prone since normal platforms > > don't have such peculiar requirements. > > Seems you are proposing layering violation without seeing it. > Let me explain the design of the drivers in Linux kernel. > > There are basically few possible concepts how drivers are handling > quirks (because we know that most of the hardware support is a set of > quirks here and there): > 1) platform provides a board files and via platform data structures > supplies quirks to the certain driver > 2) platform provides a firmware node (ACPI, DT, ...) and unified > driver handles it thru the fwnode interface > 3) driver is split to be a library (or core part) + glue drivers full of quirks > 4) driver has embedded quirks and supplies them based on IDs (PCI ID, > ACPI ID, compatible string, ID table, etc) > 5) ...missed something? ... > > What I'm proposing is turn 1 to 2 for Quark case, what you are > proposing is absent on the picture, i.e.: > x) platform provides a board file (intel_quark_i2c_gpio.c) and the > driver has embedded quirks based on ID. > > This is not what we want, definitely. From the list you provided it's still not obvious why what I suggested wasn't good, because it's perfectly fine to have both ACPI/DT-based firmware node (entry 2) and quirks based on IDs (entry 4), which plenty of the kernel drivers do. The difference between the most of those drivers and what I suggested is that by bonding a device with a driver they provide !device!-quirks, but not the !platform!-quirks. While the platform quirks and platform properties are normally provided by means of the platform data, firmware nodes, glue layers, etc. Moving some of the platform-quirks to a driver you called "layering violation". Well, I've never met that definition in the kernel before (have you just come up with it?), but at least I see what you meant. Anyway there are GPIO-drivers which still use the device IDs to get the platform quirks or get the GPIO-base from an of node alias (gpio-zynq.c, gpio-vf610.c, gpio-zx.c, gpio-mxc.c, gpio-mxs.c). There are even some, which either use a static variable to redistribute the GPIO-base between all available GPIO-chips (gpio-brcmstb.c, gpio-sta2x11.c, gpio-omap.c) or set a fixed GPIO-based value (like gpio-xlp.c, gpio-iop.c, gpio-ep93xx.c, gpio-vx855.c, gpio-cs5535.c, gpio-sch311x.c, gpio-loongson.c, gpio-loongson1.c, gpio-ath79.c, gpio-octeon.c), or even get the GPIO-base value from some hardware register (gpio-merrifield.c, gpio-intel-mid.c). I am pretty sure the examples of having the locally-defined platform quirks and the concepts 1, 2, 3 and 4 at some extent utilized in a single driver can be found in another subsystems too. I am not saying, that the approaches utilized in those drivers are ideal. Those are just examples, that the platform specifics can be reflected in the corresponding drivers and the so called "layering violation" is allowed at some circumstances. Linus, correct me if I am wrong. Getting back to this patchset. As I see it, the main problem here is connected with two parameters: - GPIO-base. You suggest to update the gpio-dwapb.c driver so one would support a firmware property like "gpio-base". It's not good, since the property will be implicitly supported by OF API as well and nothing will prevent a user from using it. Even though you said that we won't advertise that property, some user may try to define it in dts anyway, which can be easily missed on review. - IRQ-shared. As I said before it's not good to replace the irq_shared flag with the to_of_node() macro. Because having to_of_node() returned an of-node doesn't mean the IRQs can't be shared. As I see it the convenience provided by your patchset in relation to the GPIO-base and IRQ-shared properties doesn't overcome the problems denoted above. IMO it would be better either to move the GPIO-base and the IRQ-shared parameters definition to the gpio-dwapb.c driver despite of the so called "layering violation" or just leave them in the MFD driver. Linus, please join the discussion. Do you have any better idea of what to do with these properties? -Sergey > > ... > > > > What you are talking about? Can you provide a code we can discuss? > > > > Here is what I suggest in step-by-step: > > Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition > > to the gpio-dwapb.c driver. > > Layering violation: spreading board code over the kernel. > > > Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB > > GPIO block living in the Quark platform (by means of either ACPI or > > the platform-device ID data). > > Same. (The rest doesn't matter. I dropped it because it's basically > the same what I proposed) > > > By doing as I suggested you don't have to pass around the GPIO-base value and > > the IRQ-shared flag. They will be initialized and utilized locally in the > > DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1 > > and 2). In that case a software-node created in Patch 3 and 4 will be a normal > > firmware node described by the DW bindings. You'll still be able to get rid of > > the legacy global platform-data in the Patch 5. > > > > The spirit of your original patchset will still be preserved: introduce the > > software-nodes propagation interface and use it to pass the generic DW APB GPIO > > parameters. The only difference is that we'd move the GPIO-base and IRQ-shared > > functionality fully into the DW APB GPIO driver. By doing so we'd have: > > 1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver. > > In the current implementation we have them distributed between two drivers: > > intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters, > > gpio-dwapb uses them for setup. Note there is no any other platform with DW > > APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do > > we need to have such complicated interface then if we can identify whether > > the particular DW APB GPIO is a Quark-specific block or not? Moreover having > > those specifics in both drivers in fact means having quirks in both of them. > > 2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO > > device with software-nodes fully compatible with DT binding. DW APB GPIO > > driver will detect whether the block is Quark-specific and will activate > > the corresponding quirks if it is. > > 3) Still legacy global platform-data removed. > > I see. But you mistakenly made a conclusion 1). It will be spread to > two drivers. > And in 2) you considered (wrongly) that MFD driver is not a board code. > > Again MFD driver _is_ a board code or i.o.w. quirk driver for specific > Quark (MFD) device. > > -- > With Best Regards, > Andy Shevchenko
On Tue, Jun 23, 2020 at 4:49 AM Serge Semin <fancer.lancer@gmail.com> wrote: > On Sat, Jun 20, 2020 at 01:13:56PM +0300, Andy Shevchenko wrote: > > On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote: > > > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote: > > > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote: > > > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote: ... > > Linus replied to three messages out of 7, seems you read only the first one. > Actually I've read all of them and from what I could see, he didn't like a code > setting gpio-base regardless whether it has been created before your patchset > or your explanation why it was necessary. Yes, and then he realized what happened in the past. TBH I also do not like this kind of thing, but we may not get rid of it because we will get a regression. The compromise (which is used in several cases of my knowledge, but I believe there are more) is to use internal property. This keeps layers separate and individual drivers nicer and neater. ... > > > My point is that no mater how you pass the gpio-base value it will always be a > > > quirk. The same thing is with the irq_shared flag. Both of them are specific to > > > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would > > > create a GPIO device working well with the Quark-specific DW APB GPIO block. > > > > Precisely! > > > > > > > > We, by applying this series, make (keep) layers independent: board > > > > > > code vs. driver code. Mixing them more is the opposite to what I > > > > > > propose. > > > > > > > > > > > > WRT property. > > > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or > > > > > > being both converted to linux,gpio-base to explicitly show people that > > > > > > this is internal stuff that must not be present in firmware nodes. > > > > > > > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the > > > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a > > > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in > > > > > the driver. > > > > > > > > > > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks. > > > > > > As I said above having the gpio-base value set is the platform-specific thing in > > > any case. So no mater whether you pass it via the software_node/properties or > > > the private platform-data structure, the DW APB GPIO driver will have to handle > > > those parameters in some special way, which is quirk-prone since normal platforms > > > don't have such peculiar requirements. > > > > > Seems you are proposing layering violation without seeing it. > > Let me explain the design of the drivers in Linux kernel. > > > > There are basically few possible concepts how drivers are handling > > quirks (because we know that most of the hardware support is a set of > > quirks here and there): > > 1) platform provides a board files and via platform data structures > > supplies quirks to the certain driver > > 2) platform provides a firmware node (ACPI, DT, ...) and unified > > driver handles it thru the fwnode interface > > 3) driver is split to be a library (or core part) + glue drivers full of quirks > > 4) driver has embedded quirks and supplies them based on IDs (PCI ID, > > ACPI ID, compatible string, ID table, etc) > > 5) ...missed something? ... > > > > What I'm proposing is turn 1 to 2 for Quark case, what you are > > proposing is absent on the picture, i.e.: > > x) platform provides a board file (intel_quark_i2c_gpio.c) and the > > driver has embedded quirks based on ID. > > > > This is not what we want, definitely. > > From the list you provided it's still not obvious why what I suggested wasn't > good, because it's perfectly fine to have both ACPI/DT-based firmware node > (entry 2) and quirks based on IDs (entry 4), which plenty of the kernel > drivers do. And I'm not talking about combinations of 2+4, what I'm talking about is 1+4 which *is* layering violation and a bad idea to start with. > The difference between the most of those drivers and what I > suggested is that by bonding a device with a driver they provide > !device!-quirks, but not the !platform!-quirks. While the platform quirks > and platform properties are normally provided by means of the platform data, > firmware nodes, glue layers, etc. Moving some of the platform-quirks to a > driver you called "layering violation". Well, I've never met that definition > in the kernel before > (have you just come up with it?), Nope, I heard it many times during reviews. > but at least I see > what you meant. > Anyway there are GPIO-drivers which still use the device IDs to get the > platform quirks Maybe you missed that we have more players here than simple dwapb-gpio? How many of them are part of MFD (being used as MFD cells)? > or get the GPIO-base from an of node alias (gpio-zynq.c, > gpio-vf610.c, gpio-zx.c, gpio-mxc.c, gpio-mxs.c). There are even some, > which either use a static variable to redistribute the GPIO-base between > all available GPIO-chips (gpio-brcmstb.c, gpio-sta2x11.c, gpio-omap.c) > or set a fixed GPIO-based value (like gpio-xlp.c, gpio-iop.c, gpio-ep93xx.c, > gpio-vx855.c, gpio-cs5535.c, gpio-sch311x.c, gpio-loongson.c, gpio-loongson1.c, > gpio-ath79.c, gpio-octeon.c), So, the question is above. > or even get the GPIO-base value from some > hardware register (gpio-merrifield.c, gpio-intel-mid.c). These examples are not good, they are not part of MFD and the latter one actually should be a glue driver to gpio-pxa.c. > I am pretty sure > the examples of having the locally-defined platform quirks and the concepts > 1, 2, 3 and 4 at some extent utilized in a single driver can be found in another > subsystems too. I'm pretty sure layering violation can be found in many places in the kernel. It doesn't mean we have to take bad or different examples as suitable. > I am not saying, that the approaches utilized in those drivers are ideal. > Those are just examples, that the platform specifics can be reflected in > the corresponding drivers and the so called "layering violation" is allowed > at some circumstances. Linus, correct me if I am wrong. It depends how a certain driver is being used. In our case we don't need to spread board code over the kernel, we may be smarter than that! > Getting back to this patchset. As I see it, the main problem here is connected > with two parameters: > - GPIO-base. You suggest to update the gpio-dwapb.c driver so one would > support a firmware property like "gpio-base". It's not good, since the > property will be implicitly supported by OF API as well and nothing will > prevent a user from using it. Even though you said that we won't advertise > that property, some user may try to define it in dts anyway, which can be > easily missed on review. No, if we don't advertise that and if we add "linux," prefix (see DWC3 for example) to be explicit what this is used for and why. > - IRQ-shared. As I said before it's not good to replace the irq_shared flag > with the to_of_node() macro. Because having to_of_node() returned an > of-node doesn't mean the IRQs can't be shared. This is a valid point, but I would ask you, as a more familiar guy with the OF/DT system, what suggestion would be better? > As I see it the convenience provided by your patchset in relation to the > GPIO-base and IRQ-shared properties doesn't overcome the problems denoted > above. IMO it would be better either to move the GPIO-base and the IRQ-shared > parameters definition to the gpio-dwapb.c driver despite of the so called > "layering violation" or just leave them in the MFD driver. Linus, please join > the discussion. Do you have any better idea of what to do with these > properties? Moving them to gpio-dwapb is a silly move. Better to do nothing if you insist, but I consider that is non-constructive. -- With Best Regards, Andy Shevchenko