On Tue, 2013-12-24 at 12:45 +0000, Mark Brown wrote:
> I'd note that I wasn't CCed on most of this series so I'm not entirely
> sure what it's trying to do.
Apologies. The series is quite long and I didn't want to bother too many
people with mostly irrelevant changes. Will copy you on the whole thing
next time.
> > Bus-less maps (ones with reg_read and reg_write functions
> > defined in regmap_config) were given the context passed
> > in regmap_init(), but it was still called "bus_context".
> >
> > This patch formalises this aspect by renaming it to simple
> > "context" and adds the missing link, free_context function
> > in regmap_config, which allows bus-less maps to use the
> > context in classic way.
>
> This should be two patches, one patch to do the rename and one to add
> the operation.
Sure, will do.
> The obvious question here is why is this callback useful
> - what is being allocated in a regmap specific context that needs to be
> lifetime managed separately to the thing doing the creation? I can't
> see any obvious reason why this would ever get used.
First of all, it's just a generalization of the free_context already
existing in regmap_bus (and used by regmap-mmio). And in case of this
series it is being used to release extra resource added allocated for a
"busless" regmap_config. Briefly, I'm using devm_regmap_init() to
"attach" a custom regmap configuration to a device when it is being
created (which is then dev_get_regmap()-ed in the driver, as you saw in
the regulator patch) and its context is a pointer to kzallocated data.
free_context is used to release it when devm resource is being removed.
Does it make any sense?
Paweł
On Thu, Jan 09, 2014 at 01:08:31PM +0000, Pawel Moll wrote:
> On Tue, 2013-12-24 at 12:45 +0000, Mark Brown wrote:
> > The obvious question here is why is this callback useful
> > - what is being allocated in a regmap specific context that needs to be
> > lifetime managed separately to the thing doing the creation? I can't
> > see any obvious reason why this would ever get used.
> First of all, it's just a generalization of the free_context already
> existing in regmap_bus (and used by regmap-mmio). And in case of this
> series it is being used to release extra resource added allocated for a
> "busless" regmap_config. Briefly, I'm using devm_regmap_init() to
> "attach" a custom regmap configuration to a device when it is being
> created (which is then dev_get_regmap()-ed in the driver, as you saw in
> the regulator patch) and its context is a pointer to kzallocated data.
> free_context is used to release it when devm resource is being removed.
> Does it make any sense?
To be honest not really - the above sounds like you should've allocated
the memory using devm_kzalloc() or just embedding the allocated data in
the driver data for the parent. Internal things need to clean up after
themselves but users should already have a larger context of some kind.
On Thu, 2014-01-09 at 13:34 +0000, Mark Brown wrote:
> > First of all, it's just a generalization of the free_context already
> > existing in regmap_bus (and used by regmap-mmio). And in case of this
> > series it is being used to release extra resource added allocated for a
> > "busless" regmap_config. Briefly, I'm using devm_regmap_init() to
> > "attach" a custom regmap configuration to a device when it is being
> > created (which is then dev_get_regmap()-ed in the driver, as you saw in
> > the regulator patch) and its context is a pointer to kzallocated data.
> > free_context is used to release it when devm resource is being removed.
>
> > Does it make any sense?
>
> To be honest not really - the above sounds like you should've allocated
> the memory using devm_kzalloc() or just embedding the allocated data in
> the driver data for the parent. Internal things need to clean up after
> themselves but users should already have a larger context of some kind.
I've started with regmap_bus so the free_context did fit well like in
regmap-mmio, but you're right - with the current approach I should be
able to simply use devm_kzalloc(). I'll give it a try and if it works,
I'll drop this patch completely.
Paweł
On Thu, 09 Jan 2014 13:08:31 +0000, Pawel Moll <[email protected]> wrote:
> On Tue, 2013-12-24 at 12:45 +0000, Mark Brown wrote:
> > I'd note that I wasn't CCed on most of this series so I'm not entirely
> > sure what it's trying to do.
>
> Apologies. The series is quite long and I didn't want to bother too many
> people with mostly irrelevant changes. Will copy you on the whole thing
> next time.
>
> > > Bus-less maps (ones with reg_read and reg_write functions
> > > defined in regmap_config) were given the context passed
> > > in regmap_init(), but it was still called "bus_context".
> > >
> > > This patch formalises this aspect by renaming it to simple
> > > "context" and adds the missing link, free_context function
> > > in regmap_config, which allows bus-less maps to use the
> > > context in classic way.
> >
> > This should be two patches, one patch to do the rename and one to add
> > the operation.
>
> Sure, will do.
>
> > The obvious question here is why is this callback useful
> > - what is being allocated in a regmap specific context that needs to be
> > lifetime managed separately to the thing doing the creation? I can't
> > see any obvious reason why this would ever get used.
>
> First of all, it's just a generalization of the free_context already
> existing in regmap_bus (and used by regmap-mmio). And in case of this
> series it is being used to release extra resource added allocated for a
> "busless" regmap_config. Briefly, I'm using devm_regmap_init() to
> "attach" a custom regmap configuration to a device when it is being
> created (which is then dev_get_regmap()-ed in the driver, as you saw in
> the regulator patch) and its context is a pointer to kzallocated data.
> free_context is used to release it when devm resource is being removed.
Have you thought through all the implications here? What you've
described effectively changes the devm model. devm operates under the
assumption that devm data only exists between probe() and remove() time.
If you 'preload' devm data then the preloaded data will get discarded at
remove() time which breaks if the driver is remove and probed again at
runtime.
g.
On Thu, 2014-01-16 at 17:09 +0000, Grant Likely wrote:
> > First of all, it's just a generalization of the free_context already
> > existing in regmap_bus (and used by regmap-mmio). And in case of this
> > series it is being used to release extra resource added allocated for a
> > "busless" regmap_config. Briefly, I'm using devm_regmap_init() to
> > "attach" a custom regmap configuration to a device when it is being
> > created (which is then dev_get_regmap()-ed in the driver, as you saw in
> > the regulator patch) and its context is a pointer to kzallocated data.
> > free_context is used to release it when devm resource is being removed.
>
> Have you thought through all the implications here? What you've
> described effectively changes the devm model. devm operates under the
> assumption that devm data only exists between probe() and remove() time.
> If you 'preload' devm data then the preloaded data will get discarded at
> remove() time which breaks if the driver is remove and probed again at
> runtime.
Uh. Right, you're correct, I've missed that (obvious) fact :-(
I will get the drivers back to "vexpress_regmap_init()" model or try to
find a way of "attaching" a regmap pointer(s?) to a struct device. If it
makes sense at all...
Paweł