Prev: winbond-cir: fix suspend/resume
Next: Regulators: wm8994 - clean up driver data after removal
From: Mark Brown on 24 Feb 2010 05:30
On Tue, Feb 23, 2010 at 11:38:50PM -0800, Dmitry Torokhov wrote:
> Driver data set by platform_set_drvdata() is for private use of
> the driver currently bound to teh device and not for use by parent,
> subsystem and anyone else.
>
> Also have wm8400_register_regulator() accept 'sturct wm8400 *'
> instead of generic device structure.

Nack due to this change - this change would make it impossible for
callers to actually call the function. Note that nothing including only
wm8400.h even has a struct declaration, much less defniition, for struct
wm8400.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 24 Feb 2010 14:20
On Wed, Feb 24, 2010 at 11:02:34AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 24, 2010 at 10:25:49AM +0000, Mark Brown wrote:

> > Nack due to this change - this change would make it impossible for
> > callers to actually call the function. Note that nothing including only
> > wm8400.h even has a struct declaration, much less defniition, for struct
> > wm8400.

> If you notice I added forward declaration of "struct wm8400" to wm8400.h
> thus users can pass around pointer to the structure.

This doesn't help unless you also provide a way for users to obtain a
struct wm8400.

> I really think we should refrain from passing naked 'struct device *'
> pointers as much as possible since it is error prone. Otherwise
> wm8400_register_regulator has no way of ensuting that passed 'dev' is
> indeed wm8400.

Right, there's no type safety here (and this whole method of registering
regulators is fairly deprecated anyway in favour of just straight
platform data) but really it doesn't buy us much - the users get exactly
the struct device they need to use passed in to them, there's really
very little chance for them to get confused about what they're talking
about.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 24 Feb 2010 15:50
On Wed, Feb 24, 2010 at 11:21:26AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 24, 2010 at 07:14:03PM +0000, Mark Brown wrote:

> > This doesn't help unless you also provide a way for users to obtain a
> > struct wm8400.

> Why would they need it? Only code that creates instances of wm8400 needs
> to know the definition of the sturcture, the rest can simply pass the
> pointer around.

> I guess there is disconnect between us and I do not see any users of
> wm8400_register_regulator() in linux-next... Is there another tree I
> could peek at?

There are no users in mainline. This would be called by board specific
code from the init callback of the wm8400 - you'd need to pass that
callback the struct wm8400.

In any case, this is clearly an unrelated change to whatever else you
were doing to the driver so should be split off into a separate patch,
but if this is being changed at all then it'd be much more sensible to
change it to use a more modern pattern which completely removes the
wm8400_register_regulator() function and just uses platform data.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 25 Feb 2010 05:50
On Thu, Feb 25, 2010 at 01:55:37AM -0800, Dmitry Torokhov wrote:

> Fair enough, I removed the offending part, updated patch below.

Acked-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com>

> --
> Dmitry
>
> regulator: wm8400 - cleanup platform driver data handling

but note that including the patch after your sig separator means that a
lot of MUAs will hide the patch from users which makes it a bit hard to
read.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: winbond-cir: fix suspend/resume
Next: Regulators: wm8994 - clean up driver data after removal