| Message ID | 20211218214954.109755-9-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for VSC75XX control over SPI | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Sat, 18 Dec 2021, Colin Foster wrote: > Some drivers will need to create regmaps differently based on whether they > are a child of an MFD or a standalone device. An example of this would be > if a regmap were directly memory-mapped or an external bus. In the > memory-mapped case a call to devm_regmap_init_mmio would return the correct > regmap. In the case of an MFD, the regmap would need to be requested from > the parent device. > > This addition allows the driver to correctly reason about these scenarios. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > drivers/mfd/mfd-core.c | 5 +++++ > include/linux/mfd/core.h | 10 ++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > index 684a011a6396..905f508a31b4 100644 > --- a/drivers/mfd/mfd-core.c > +++ b/drivers/mfd/mfd-core.c > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { > .name = "mfd_device", > }; > > +int device_is_mfd(struct platform_device *pdev) > +{ > + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); > +} > + Why is this device different to any other that has ever been mainlined?
On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote: > On Sat, 18 Dec 2021, Colin Foster wrote: > > > Some drivers will need to create regmaps differently based on whether they > > are a child of an MFD or a standalone device. An example of this would be > > if a regmap were directly memory-mapped or an external bus. In the > > memory-mapped case a call to devm_regmap_init_mmio would return the correct > > regmap. In the case of an MFD, the regmap would need to be requested from > > the parent device. > > > > This addition allows the driver to correctly reason about these scenarios. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > drivers/mfd/mfd-core.c | 5 +++++ > > include/linux/mfd/core.h | 10 ++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > > index 684a011a6396..905f508a31b4 100644 > > --- a/drivers/mfd/mfd-core.c > > +++ b/drivers/mfd/mfd-core.c > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { > > .name = "mfd_device", > > }; > > > > +int device_is_mfd(struct platform_device *pdev) > > +{ > > + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); > > +} > > + > > Why is this device different to any other that has ever been > mainlined? Hi Lee, First, let me apologize for not responding to your response from the related RFC from earlier this month. It had been blocked by my spam filter and I had not seen it until just now. I'll have to check that more diligently now. Moving on... That's a question I keep asking myself. Either there's something I'm missing, or there's something new I'm doing. This is taking existing drivers that work via MMIO regmaps and making them interface-independent. As Vladimir pointed out here: https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/ device_is_mfd could be dropped in lieu of an mfd-specific probe function. If there's something I'm missing, please let me know. But it feels like devm_get_regmap_from_resource at the end of the day would be the best solution to the design, and that doesn't exist. And implementing something like that is a task that I feel I'm not capable of tackling at this time. > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
On Wed, 29 Dec 2021, Colin Foster wrote: > On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote: > > On Sat, 18 Dec 2021, Colin Foster wrote: > > > > > Some drivers will need to create regmaps differently based on whether they > > > are a child of an MFD or a standalone device. An example of this would be > > > if a regmap were directly memory-mapped or an external bus. In the > > > memory-mapped case a call to devm_regmap_init_mmio would return the correct > > > regmap. In the case of an MFD, the regmap would need to be requested from > > > the parent device. > > > > > > This addition allows the driver to correctly reason about these scenarios. > > > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > > --- > > > drivers/mfd/mfd-core.c | 5 +++++ > > > include/linux/mfd/core.h | 10 ++++++++++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > > > index 684a011a6396..905f508a31b4 100644 > > > --- a/drivers/mfd/mfd-core.c > > > +++ b/drivers/mfd/mfd-core.c > > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { > > > .name = "mfd_device", > > > }; > > > > > > +int device_is_mfd(struct platform_device *pdev) > > > +{ > > > + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); > > > +} > > > + > > > > Why is this device different to any other that has ever been > > mainlined? > > Hi Lee, > > First, let me apologize for not responding to your response from the > related RFC from earlier this month. It had been blocked by my spam > filter and I had not seen it until just now. I'll have to check that > more diligently now. > > Moving on... > > That's a question I keep asking myself. Either there's something I'm > missing, or there's something new I'm doing. > > This is taking existing drivers that work via MMIO regmaps and making > them interface-independent. As Vladimir pointed out here: > https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/ > device_is_mfd could be dropped in lieu of an mfd-specific probe > function. > > If there's something I'm missing, please let me know. But it feels like > devm_get_regmap_from_resource at the end of the day would be the best > solution to the design, and that doesn't exist. And implementing > something like that is a task that I feel I'm not capable of tackling at > this time. I'm really not a fan of leaking any MFD API outside of drivers/mfd. MFD isn't a tangible thing. It's a Linuxiusm, something we made up, a figment of your imagination. What happens if you were to all dev_get_regmap() in the non-MFD case or when you call devm_regmap_init_mmio() when the driver was registered via the MFD framework?
On Thu, Dec 30, 2021 at 01:43:53PM +0000, Lee Jones wrote: > On Wed, 29 Dec 2021, Colin Foster wrote: > > > On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote: > > > On Sat, 18 Dec 2021, Colin Foster wrote: > > > > > > > Some drivers will need to create regmaps differently based on whether they > > > > are a child of an MFD or a standalone device. An example of this would be > > > > if a regmap were directly memory-mapped or an external bus. In the > > > > memory-mapped case a call to devm_regmap_init_mmio would return the correct > > > > regmap. In the case of an MFD, the regmap would need to be requested from > > > > the parent device. > > > > > > > > This addition allows the driver to correctly reason about these scenarios. > > > > > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > > > --- > > > > drivers/mfd/mfd-core.c | 5 +++++ > > > > include/linux/mfd/core.h | 10 ++++++++++ > > > > 2 files changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > > > > index 684a011a6396..905f508a31b4 100644 > > > > --- a/drivers/mfd/mfd-core.c > > > > +++ b/drivers/mfd/mfd-core.c > > > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { > > > > .name = "mfd_device", > > > > }; > > > > > > > > +int device_is_mfd(struct platform_device *pdev) > > > > +{ > > > > + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); > > > > +} > > > > + > > > > > > Why is this device different to any other that has ever been > > > mainlined? > > > > Hi Lee, > > > > First, let me apologize for not responding to your response from the > > related RFC from earlier this month. It had been blocked by my spam > > filter and I had not seen it until just now. I'll have to check that > > more diligently now. > > > > Moving on... > > > > That's a question I keep asking myself. Either there's something I'm > > missing, or there's something new I'm doing. > > > > This is taking existing drivers that work via MMIO regmaps and making > > them interface-independent. As Vladimir pointed out here: > > https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/ > > device_is_mfd could be dropped in lieu of an mfd-specific probe > > function. > > > > If there's something I'm missing, please let me know. But it feels like > > devm_get_regmap_from_resource at the end of the day would be the best > > solution to the design, and that doesn't exist. And implementing > > something like that is a task that I feel I'm not capable of tackling at > > this time. > > I'm really not a fan of leaking any MFD API outside of drivers/mfd. > MFD isn't a tangible thing. It's a Linuxiusm, something we made up, a > figment of your imagination. > > What happens if you were to all dev_get_regmap() in the non-MFD case > or when you call devm_regmap_init_mmio() when the driver was > registered via the MFD framework? I'd imagine dev_get_regmap in a non-MFD case would be the same as dev_get_and_ioremap_resource() followed by devm_regmap_init_mmio(). In the MFD case it would possibly request the regmap from the parent, which could reason about how to create the regmap. As you understand, this is exactly the behavior I created in this patch set. I did it by way of ocelot_get_regmap_from_resource, and admit it isn't the best way. But it certainly seems there isn't an existing method that I'm missing. I'm coming from a pretty narrow field of view, but believe my use-case is a valid one. If that is true, and there isn't another design I should use... this is the opportunity to create it. Implementing ocelot_get_regmap_from_resource is a way to achieve my needs without affecting anyone else. Going one step further and implementing mfd_get_regmap_from_parent (or similar) would creep into the design of MFD. I don't know enough about MFD and the users to suggest this. I wouldn't want to start venturing down that path without blessing from the community. And this would indirectly affect every MFD driver. Going all in and implementing device_get_regmap_from_resource... I don't know that I'd be comfortable even starting down that path knowing that it would affect every device. Perhaps it would have to utilize something like IORESOURCE_REG that seems to only get utilized in a handful of files: https://elixir.bootlin.com/linux/v5.16-rc7/C/ident/IORESOURCE_REG > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
On Thu, 30 Dec 2021, Colin Foster wrote: > On Thu, Dec 30, 2021 at 01:43:53PM +0000, Lee Jones wrote: > > On Wed, 29 Dec 2021, Colin Foster wrote: > > > > > On Wed, Dec 29, 2021 at 03:25:55PM +0000, Lee Jones wrote: > > > > On Sat, 18 Dec 2021, Colin Foster wrote: > > > > > > > > > Some drivers will need to create regmaps differently based on whether they > > > > > are a child of an MFD or a standalone device. An example of this would be > > > > > if a regmap were directly memory-mapped or an external bus. In the > > > > > memory-mapped case a call to devm_regmap_init_mmio would return the correct > > > > > regmap. In the case of an MFD, the regmap would need to be requested from > > > > > the parent device. > > > > > > > > > > This addition allows the driver to correctly reason about these scenarios. > > > > > > > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > > > > --- > > > > > drivers/mfd/mfd-core.c | 5 +++++ > > > > > include/linux/mfd/core.h | 10 ++++++++++ > > > > > 2 files changed, 15 insertions(+) > > > > > > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > > > > > index 684a011a6396..905f508a31b4 100644 > > > > > --- a/drivers/mfd/mfd-core.c > > > > > +++ b/drivers/mfd/mfd-core.c > > > > > @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { > > > > > .name = "mfd_device", > > > > > }; > > > > > > > > > > +int device_is_mfd(struct platform_device *pdev) > > > > > +{ > > > > > + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); > > > > > +} > > > > > + > > > > > > > > Why is this device different to any other that has ever been > > > > mainlined? > > > > > > Hi Lee, > > > > > > First, let me apologize for not responding to your response from the > > > related RFC from earlier this month. It had been blocked by my spam > > > filter and I had not seen it until just now. I'll have to check that > > > more diligently now. > > > > > > Moving on... > > > > > > That's a question I keep asking myself. Either there's something I'm > > > missing, or there's something new I'm doing. > > > > > > This is taking existing drivers that work via MMIO regmaps and making > > > them interface-independent. As Vladimir pointed out here: > > > https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/ > > > device_is_mfd could be dropped in lieu of an mfd-specific probe > > > function. > > > > > > If there's something I'm missing, please let me know. But it feels like > > > devm_get_regmap_from_resource at the end of the day would be the best > > > solution to the design, and that doesn't exist. And implementing > > > something like that is a task that I feel I'm not capable of tackling at > > > this time. > > > > I'm really not a fan of leaking any MFD API outside of drivers/mfd. > > MFD isn't a tangible thing. It's a Linuxiusm, something we made up, a > > figment of your imagination. > > > > What happens if you were to all dev_get_regmap() in the non-MFD case > > or when you call devm_regmap_init_mmio() when the driver was > > registered via the MFD framework? > > I'd imagine dev_get_regmap in a non-MFD case would be the same as > dev_get_and_ioremap_resource() followed by devm_regmap_init_mmio(). > > In the MFD case it would possibly request the regmap from the parent, > which could reason about how to create the regmap. As you understand, > this is exactly the behavior I created in this patch set. I did it by > way of ocelot_get_regmap_from_resource, and admit it isn't the best way. > But it certainly seems there isn't an existing method that I'm missing. > > I'm coming from a pretty narrow field of view, but believe my use-case > is a valid one. If that is true, and there isn't another design I should > use... this is the opportunity to create it. Implementing > ocelot_get_regmap_from_resource is a way to achieve my needs without > affecting anyone else. > > Going one step further and implementing mfd_get_regmap_from_parent (or > similar) would creep into the design of MFD. I don't know enough about > MFD and the users to suggest this. I wouldn't want to start venturing > down that path without blessing from the community. And this would > indirectly affect every MFD driver. > > Going all in and implementing device_get_regmap_from_resource... I don't > know that I'd be comfortable even starting down that path knowing that > it would affect every device. Perhaps it would have to utilize something > like IORESOURCE_REG that seems to only get utilized in a handful of > files: > https://elixir.bootlin.com/linux/v5.16-rc7/C/ident/IORESOURCE_REG Let's speak to Mark and see if he can provide any insight.
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index 684a011a6396..905f508a31b4 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -33,6 +33,11 @@ static struct device_type mfd_dev_type = { .name = "mfd_device", }; +int device_is_mfd(struct platform_device *pdev) +{ + return (!strcmp(pdev->dev.type->name, mfd_dev_type.name)); +} + int mfd_cell_enable(struct platform_device *pdev) { const struct mfd_cell *cell = mfd_get_cell(pdev); diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h index 0bc7cba798a3..c0719436b652 100644 --- a/include/linux/mfd/core.h +++ b/include/linux/mfd/core.h @@ -10,6 +10,7 @@ #ifndef MFD_CORE_H #define MFD_CORE_H +#include <generated/autoconf.h> #include <linux/platform_device.h> #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) @@ -123,6 +124,15 @@ struct mfd_cell { int num_parent_supplies; }; +#ifdef CONFIG_MFD_CORE +int device_is_mfd(struct platform_device *pdev); +#else +static inline int device_is_mfd(struct platform_device *pdev) +{ + return 0; +} +#endif + /* * Convenience functions for clients using shared cells. Refcounting * happens automatically, with the cell's enable/disable callbacks
Some drivers will need to create regmaps differently based on whether they are a child of an MFD or a standalone device. An example of this would be if a regmap were directly memory-mapped or an external bus. In the memory-mapped case a call to devm_regmap_init_mmio would return the correct regmap. In the case of an MFD, the regmap would need to be requested from the parent device. This addition allows the driver to correctly reason about these scenarios. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- drivers/mfd/mfd-core.c | 5 +++++ include/linux/mfd/core.h | 10 ++++++++++ 2 files changed, 15 insertions(+)