| Message ID | 20210430130645.31562-1-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] regmap: add regmap_might_sleep() | expand |
On Fri, Apr 30, 2021 at 03:06:44PM +0200, Michael Walle wrote: > Sometimes a driver needs to know if the underlying regmap could sleep. > For example, consider the gpio-regmap driver which needs to fill the > gpiochip->can_sleep property. Whatever is creating the regmap really ought to know what device it's dealing with... > It might be possible to pass this information via the > gpio_regmap_config, but this has the following drawbacks. First, that > property is redundant and both places might contratict each other. And > secondly, the driver might not even know the type of the regmap because > it just gets an opaque pointer by querying the device tree. If it's a generic GPIO driver from a code correctness point of view it's always got a risk of sleeping...
Am 2021-04-30 17:19, schrieb Mark Brown: > On Fri, Apr 30, 2021 at 03:06:44PM +0200, Michael Walle wrote: > >> Sometimes a driver needs to know if the underlying regmap could sleep. >> For example, consider the gpio-regmap driver which needs to fill the >> gpiochip->can_sleep property. > > Whatever is creating the regmap really ought to know what device it's > dealing with... But creating and using the regmap are two seperate things, no? Consider the gpio-sl28cpld. It will just use whatever regmap the parent has created. How would it know what type of regmap it is? >> It might be possible to pass this information via the >> gpio_regmap_config, but this has the following drawbacks. First, that >> property is redundant and both places might contratict each other. And >> secondly, the driver might not even know the type of the regmap >> because >> it just gets an opaque pointer by querying the device tree. > > If it's a generic GPIO driver from a code correctness point of view > it's > always got a risk of sleeping... I can't follow you here. -michael
On Fri, Apr 30, 2021 at 06:01:49PM +0200, Michael Walle wrote: > Am 2021-04-30 17:19, schrieb Mark Brown: > > Whatever is creating the regmap really ought to know what device it's > > dealing with... > But creating and using the regmap are two seperate things, no? Consider > the gpio-sl28cpld. It will just use whatever regmap the parent has created. > How would it know what type of regmap it is? But that's a driver for a specific device AFAICT which looks like it's only got an I2C binding on the MFD so the driver knows that it's for a device that's on a bus that's going to sleep and doesn't need to infer anything? This looks like the common case I'd expect where there's no variation. > > > It might be possible to pass this information via the > > > gpio_regmap_config, but this has the following drawbacks. First, that > > > property is redundant and both places might contratict each other. And > > > secondly, the driver might not even know the type of the regmap > > > because > > > it just gets an opaque pointer by querying the device tree. > > If it's a generic GPIO driver from a code correctness point of view it's > > always got a risk of sleeping... > I can't follow you here. If users happen to end up with a map flagged as fast they can work on the whatever driver uses this stuff and not realise they're breaking other users of the same driver that end up with slow I/O. The whole point of the flag in GPIO is AIUI warnings to help with that case.
Am 2021-04-30 19:26, schrieb Mark Brown: > On Fri, Apr 30, 2021 at 06:01:49PM +0200, Michael Walle wrote: >> Am 2021-04-30 17:19, schrieb Mark Brown: > >> > Whatever is creating the regmap really ought to know what device it's >> > dealing with... > >> But creating and using the regmap are two seperate things, no? >> Consider >> the gpio-sl28cpld. It will just use whatever regmap the parent has >> created. >> How would it know what type of regmap it is? > > But that's a driver for a specific device AFAICT which looks like it's > only got an I2C binding on the MFD so the driver knows that it's for a > device that's on a bus that's going to sleep and doesn't need to infer > anything? This looks like the common case I'd expect where there's no > variation. You are right, at the moment this driver only has an I2C binding. But the idea was that this IP block and driver can be reused behind any kind of bridge; I2C, SPI or MMIO. Actually, I had the impression that all you need to do to convert it to MMIO is to replace the "kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't that easy. Anyway, the idea is that you don't need to change anything in the gpio-sl28cpld driver, just change the parent. But if we can't ask the regmap what type it is, then we'll have to modify the gpio-sl28cpld driver and we will have to figure it out by some other means. >> > > It might be possible to pass this information via the >> > > gpio_regmap_config, but this has the following drawbacks. First, that >> > > property is redundant and both places might contratict each other. And >> > > secondly, the driver might not even know the type of the regmap >> > > because >> > > it just gets an opaque pointer by querying the device tree. > >> > If it's a generic GPIO driver from a code correctness point of view it's >> > always got a risk of sleeping... > >> I can't follow you here. > > If users happen to end up with a map flagged as fast they can work on > the whatever driver uses this stuff and not realise they're breaking > other users of the same driver that end up with slow I/O. The whole > point of the flag in GPIO is AIUI warnings to help with that case. Hm, but as of now, the only thing which makes the gpio-regmap driver slow i/o is the regmap itself. -michael
On Sat, May 01, 2021 at 12:10:16AM +0200, Michael Walle wrote: > Am 2021-04-30 19:26, schrieb Mark Brown: > > But that's a driver for a specific device AFAICT which looks like it's > > only got an I2C binding on the MFD so the driver knows that it's for a > > device that's on a bus that's going to sleep and doesn't need to infer > > anything? This looks like the common case I'd expect where there's no > > variation. > You are right, at the moment this driver only has an I2C binding. But > the idea was that this IP block and driver can be reused behind any > kind of bridge; I2C, SPI or MMIO. Actually, I had the impression Is this actually a way people are building hardware though? > that all you need to do to convert it to MMIO is to replace the > "kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't > that easy. Anyway, the idea is that you don't need to change anything > in the gpio-sl28cpld driver, just change the parent. But if we can't > ask the regmap what type it is, then we'll have to modify the > gpio-sl28cpld driver and we will have to figure it out by some other > means. Well, you don't need to change anything at all - the driver will work perfectly fine if it's flagging up the GPIOs as potentially sleeping even if they end up not actually sleeping. > > If users happen to end up with a map flagged as fast they can work on > > the whatever driver uses this stuff and not realise they're breaking > > other users of the same driver that end up with slow I/O. The whole > > point of the flag in GPIO is AIUI warnings to help with that case. > Hm, but as of now, the only thing which makes the gpio-regmap driver > slow i/o is the regmap itself. Surely it's just a case of the device that's creating the gpio regmap setting a flag when it instantiates it? It's just one more thing that the parent knows about the device. This doesn't seem insurmountable.
Am 2021-05-06 14:43, schrieb Mark Brown: > On Sat, May 01, 2021 at 12:10:16AM +0200, Michael Walle wrote: >> Am 2021-04-30 19:26, schrieb Mark Brown: > >> > But that's a driver for a specific device AFAICT which looks like it's >> > only got an I2C binding on the MFD so the driver knows that it's for a >> > device that's on a bus that's going to sleep and doesn't need to infer >> > anything? This looks like the common case I'd expect where there's no >> > variation. > >> You are right, at the moment this driver only has an I2C binding. But >> the idea was that this IP block and driver can be reused behind any >> kind of bridge; I2C, SPI or MMIO. Actually, I had the impression > > Is this actually a way people are building hardware though? Granted, this might be a special case because the driver is for an IP inside CPLD/FPGA, thus you could easily switch the bridge. >> that all you need to do to convert it to MMIO is to replace the >> "kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't >> that easy. Anyway, the idea is that you don't need to change anything >> in the gpio-sl28cpld driver, just change the parent. But if we can't >> ask the regmap what type it is, then we'll have to modify the >> gpio-sl28cpld driver and we will have to figure it out by some other >> means. > > Well, you don't need to change anything at all - the driver will work > perfectly fine if it's flagging up the GPIOs as potentially sleeping > even if they end up not actually sleeping. Unless you need the non-sleeping version for a gpio. >> > If users happen to end up with a map flagged as fast they can work on >> > the whatever driver uses this stuff and not realise they're breaking >> > other users of the same driver that end up with slow I/O. The whole >> > point of the flag in GPIO is AIUI warnings to help with that case. > >> Hm, but as of now, the only thing which makes the gpio-regmap driver >> slow i/o is the regmap itself. > > Surely it's just a case of the device that's creating the gpio regmap > setting a flag when it instantiates it? It's just one more thing that > the parent knows about the device. This doesn't seem insurmountable. No its not. It just seemed like it is way easier to just ask the regmap. -michael
On Thu, May 06, 2021 at 03:35:26PM +0200, Michael Walle wrote: > Am 2021-05-06 14:43, schrieb Mark Brown: > > Surely it's just a case of the device that's creating the gpio regmap > > setting a flag when it instantiates it? It's just one more thing that > > the parent knows about the device. This doesn't seem insurmountable. > No its not. It just seemed like it is way easier to just ask the regmap. I'd rather cause a small amount of hoop jumping for one or two users than open up an API that feels like it's going to encourage dodgy usage, there's already enough problems with things like regulator_get_optional() and this feels like it's going down similar roads.
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 297e95be25b3..f0661ff6bdcf 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1847,6 +1847,17 @@ size_t regmap_get_raw_write_max(struct regmap *map) } EXPORT_SYMBOL_GPL(regmap_get_raw_write_max); +/** + * regmap_might_sleep - Returns whether a regmap access might sleep. + * + * @map: Map to check. + */ +bool regmap_might_sleep(struct regmap *map) +{ + return map->can_sleep; +} +EXPORT_SYMBOL_GPL(regmap_might_sleep); + static int _regmap_bus_formatted_write(void *context, unsigned int reg, unsigned int val) { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index f87a11a5cc4a..955fbe61557a 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1079,6 +1079,7 @@ int regmap_reinit_cache(struct regmap *map, const struct regmap_config *config); struct regmap *dev_get_regmap(struct device *dev, const char *name); struct device *regmap_get_device(struct regmap *map); +bool regmap_might_sleep(struct regmap *map); int regmap_write(struct regmap *map, unsigned int reg, unsigned int val); int regmap_write_async(struct regmap *map, unsigned int reg, unsigned int val); int regmap_raw_write(struct regmap *map, unsigned int reg, @@ -1816,6 +1817,12 @@ static inline struct device *regmap_get_device(struct regmap *map) return NULL; } +static inline bool regmap_might_sleep(struct regmap *map) +{ + WARN_ONCE(1, "regmap API is disabled"); + return true; +} + #endif #endif
Sometimes a driver needs to know if the underlying regmap could sleep. For example, consider the gpio-regmap driver which needs to fill the gpiochip->can_sleep property. It might be possible to pass this information via the gpio_regmap_config, but this has the following drawbacks. First, that property is redundant and both places might contratict each other. And secondly, the driver might not even know the type of the regmap because it just gets an opaque pointer by querying the device tree. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/base/regmap/regmap.c | 11 +++++++++++ include/linux/regmap.h | 7 +++++++ 2 files changed, 18 insertions(+)