mfd: twl-core: export twl_get_regmap

We want to get rid of global twl_i2c_{write/read}.
As a first step, allow clients to get the regmap and write directly

Signed-off-by Nicolae Rosia <[email protected]>

---
 drivers/mfd/twl-core.c  | 3 ++-
 include/linux/i2c/twl.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

drivers/mfd/twl-core.c +2 -1

index c64615d..49e6a4b 100644
@@ -421,7 +421,7 @@ EXPORT_SYMBOL(twl_rev);
  *
  * Returns the regmap pointer or NULL in case of failure.
  */
-static struct regmap *twl_get_regmap(u8 mod_no)
+struct regmap *twl_get_regmap(u8 mod_no)
 {
 	int sid;
 	struct twl_client *twl;

@@ -440,6 +440,7 @@ static struct regmap *twl_get_regmap(u8 mod_no)
 
 	return twl->regmap;
 }
+EXPORT_SYMBOL(twl_get_regmap);
 
 /**
  * twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0

include/linux/i2c/twl.h +2 -0

index 9ad7828..4c43cdb3 100644
@@ -174,6 +174,8 @@ static inline int twl_class_is_ ##class(void)	\
 TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
 TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
 
+struct regmap *twl_get_regmap(u8 mod_no);
+
 /* Set the regcache bypass for the regmap associated with the nodule */
 int twl_set_regcache_bypass(u8 mod_no, bool enable);
 

Quoted 2 lines · Nicolae Rosia

We want to get rid of global twl_i2c_{write/read}.
As a first step, allow clients to get the regmap and write directly

What's stopping you from passing it through device data?

Quoted 5 lines · Lee Jones

Quoted 2 lines · Nicolae Rosia

We want to get rid of global twl_i2c_{write/read}.
As a first step, allow clients to get the regmap and write directly

Quoted 2 lines

What's stopping you from passing it through device data?

Could you elaborate a bit?
The regmaps are stored in struct twl_client [0], stored in struct twl_private [1], both structs are defined in the source file, not in header.
I could however just fix the problem by reworking the struct, exposing it and use mfd_add_device as real mfd drivers do.

[0] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L152
[1] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L163

It's common place to pass shared resources such as 'regmap' though device data. You can find many examples of *__set_drvdata throughout the kernel.

Passing data between drivers using *_set_drvdata() is a layering violation:

1. Driver data is supposed to be driver private data associated with the currently bound driver.
2. The driver data pointer is NULL'd when the driver unbinds from the device. See __device_release_driver() and the dev_set_drvdata(dev, NULL).
3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a similar reason to (2).

So, do not pass data between drivers using *_set_drvdata() - any examples in the kernel already are founded on bad practice, are fragile, and are already broken for some kernel configurations.

Quoted 11 lines · Russell King - ARM Linux

Passing data between drivers using *_set_drvdata() is a layering violation:

1. Driver data is supposed to be driver private data associated with the currently bound driver.
2. The driver data pointer is NULL'd when the driver unbinds from the device. See __device_release_driver() and the dev_set_drvdata(dev, NULL).
3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a similar reason to (2).

Quoted 3 lines

So, do not pass data between drivers using *_set_drvdata() - any examples in the kernel already are founded on bad practice, are fragile, and are already broken for some kernel configurations.

After inspecting mfd_add_device, it seems that it creates a platform_device which has the parent set to the driver calling the function.
Isn't module unloading forbidden if there is a parent->child relationship in place and you're removing the parent?

What should be the best practice to share data between drivers? Reference counted data?

In the case of TWL, the twl-core is just a simple container for regmaps - all other "sub devices" are using those regmaps to access the I2C device's registers, it makes no sense to remove the parent driver since it does *nothing*.

Quoted 17 lines · Nicolae Rosia

Quoted 11 lines · Russell King - ARM Linux

Passing data between drivers using *_set_drvdata() is a layering violation:

1. Driver data is supposed to be driver private data associated with the currently bound driver.
2. The driver data pointer is NULL'd when the driver unbinds from the device. See __device_release_driver() and the dev_set_drvdata(dev, NULL).
3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a similar reason to (2).

Quoted 3 lines

So, do not pass data between drivers using *_set_drvdata() - any examples in the kernel already are founded on bad practice, are fragile, and are already broken for some kernel configurations.

Quoted 5 lines

After inspecting mfd_add_device, it seems that it creates a platform_device which has the parent set to the driver calling the function.
Isn't module unloading forbidden if there is a parent->child relationship in place and you're removing the parent?

Forget this idea that there's any connection between modules and the struct device relationships - there isn't anything of the kind!

Each struct device is refcounted, and child devices will hold a reference to their parent device, so the parent device doesn't get freed before its children are all gone.

That's a completely separate issue to when a struct device is bound to a struct device_driver - it's entirely possible for parent drivers to be unbound at any time, even when there are child drivers in place.

There are cases where we want that to happen - think of any driver which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These drivers enumerate their children, and destroy their children when the driver is unbound - but the driver has to be in the process of being unbound for that to happen. That process may very well start with the child devices being bound to their drivers.

What makes the child drivers unbind is when the bus driver deletes the child struct devices.

Quoted 2 lines

What should be the best practice to share data between drivers? Reference counted data?

I guess so, but you will still have a race if you do something like:

	struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);

Yes, that'll get the parent's driver private data, but what you don't know is whether the pointer remains valid, and even if you do as the very next step:

	kref_get(&parent_priv->kref);

you don't know whether parent_priv was kfree()d between these two statements.

However, if the parent driver creates the struct device that you're using and deletes the struct device before it frees its private data, then you can be sure that parent_priv will be valid, because the child drivers will be unbound during the parent driver's ->remove function, _before_ the private data is freed.

Quoted 4 lines

In the case of TWL, the twl-core is just a simple container for regmaps - all other "sub devices" are using those regmaps to access the I2C device's registers, it makes no sense to remove the parent driver since it does *nothing*.

I can't comment on what twl-core is doing, I haven't looked at it in ages, but most MFD drivers have the parent device creating and destroying their children, so it should be fine.

My original comment was more along the lines of a parent device poking driver-private data into the child devices it was creating for the child drivers to pick up. However, it's worth discussing the validity cases of the parent's driver data too, as per the above.

I was just curious, and I took a peek at the OMAP/TWL DT files, and I see that it's left to DT to create the children.

I'm converting the driver to use mfd_add_devices and mfd_remove_devices, the subdrivers will access the parent's private data which will remain valid since in the remove method we will be calling mfd_remove_devices first.

After removing all global calls to twl-core.c methods, I will also get rid of the "only one instance" of twl_priv and the "ready" flag.

Thanks for your input,
Nicolae