From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Andres Salomon <dilinger@queued.net> Cc: Samuel Ortiz <sameo@linux.intel.com>, Russell King <linux@arm.linux.org.uk>, Mark Brown <broonie@opensource.wolfsonmicro.com>, linux-kernel@vger.kernel.org, Richard Purdie <rpurdie@rpsys.net>, Sascha Hauer <kernel@pengutronix.de>, linux-arm-kernel@lists.infradead.org, Liam Girdwood <lrg@slimlogic.co.uk> Subject: Re: [PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers Date: Fri, 4 Feb 2011 10:34:58 +0100 [thread overview] Message-ID: <20110204093458.GH30452@pengutronix.de> (raw) In-Reply-To: <20110202202015.0fe392a9@queued.net> Hello Andres, On Wed, Feb 02, 2011 at 08:20:15PM -0800, Andres Salomon wrote: > > No need to explicitly set the cell's platform_data/data_size. > > In this case, move the various platform_data pointers > to driver_data. All of the clients which make use of it > are also changed. > > Mfd-core makes a copy of platform_data, but driver_data keeps a pointer > to the original data. Because each cell's platform_data previously > pointed to a local (stack) variable, the various ARM mach types that set > the pdata are updated to keep the memory around. I didn't get this even after reading it 5 times. You wrote in the subject that drivers now have access to mfd_cell. I don't see where e.g. drivers/leds/leds-mc13783.c uses that?! Does this depend on some mfd-changes I don't see and this is just a first step? After reading the changes I think I understood: - You made things that were passed as platform_data before available via driver_data. - Because platform_data is copied and driver_data is not at register time, the data being platform_data cannot be __initdata or stack local anymore, so this needs fixing. In sum this results in .data becoming bigger (which is bad). And I think this patch has a conceptual problem, too. In my opionion platform_data is the point to hand over platform specific data to a driver. driver_data is something that is private to the driver and has to be considered opaque for the platform. The driver was sort of OK before ... > diff --git a/arch/arm/mach-imx/mach-mx27_3ds.c b/arch/arm/mach-imx/mach-mx27_3ds.c > index 1643315..edcecaa 100644 > --- a/arch/arm/mach-imx/mach-mx27_3ds.c > +++ b/arch/arm/mach-imx/mach-mx27_3ds.c > @@ -226,10 +226,14 @@ static struct mc13783_regulator_init_data mx27_3ds_regulators[] = { > }, > }; > > -/* MC13783 */ > -static struct mc13783_platform_data mc13783_pdata __initdata = { > +static struct mc13783_regulator_platform_data mx27_regs = { The prefix mx27 is misleading. If you want to take the machine name into account use mx27_3ds, otherwise just use mc13783_regulator_pdata or similar. (And note that "regs" isn't that good IMHO, too, as it might stand for registers, too. And even mx27_3ds_regulators would be too general. This applies to the changes below, too.) > .regulators = mx27_3ds_regulators, > .num_regulators = ARRAY_SIZE(mx27_3ds_regulators), > +}; > + > +/* MC13783 */ a very minor nitpick is you moved the comment for mx27_3ds, but not for e.g. mx31_3ds. > +static struct mc13783_platform_data mc13783_pdata __initdata = { > + .regulators = &mx27_regs, > .flags = MC13783_USE_REGULATOR, > }; > > diff --git a/arch/arm/mach-imx/mach-pcm038.c b/arch/arm/mach-imx/mach-pcm038.c > index 5056148..b5ae8cc 100644 > --- a/arch/arm/mach-imx/mach-pcm038.c > +++ b/arch/arm/mach-imx/mach-pcm038.c > @@ -262,9 +262,13 @@ static struct mc13783_regulator_init_data pcm038_regulators[] = { > }, > }; > > -static struct mc13783_platform_data pcm038_pmic = { > +static struct mc13783_regulator_platform_data pcm038_regs = { > .regulators = pcm038_regulators, > .num_regulators = ARRAY_SIZE(pcm038_regulators), > +}; > + > +static struct mc13783_platform_data pcm038_pmic = { > + .regulators = &pcm038_regs, > .flags = MC13783_USE_ADC | MC13783_USE_REGULATOR | > MC13783_USE_TOUCHSCREEN, > }; > diff --git a/arch/arm/mach-mx3/mach-mx31_3ds.c b/arch/arm/mach-mx3/mach-mx31_3ds.c > index 0d65db8..3e613ee 100644 > --- a/arch/arm/mach-mx3/mach-mx31_3ds.c > +++ b/arch/arm/mach-mx3/mach-mx31_3ds.c > @@ -156,9 +156,13 @@ static struct mc13783_regulator_init_data mx31_3ds_regulators[] = { > }; > > /* MC13783 */ > -static struct mc13783_platform_data mc13783_pdata __initdata = { > +static struct mc13783_regulator_platform_data mc13783_regs = { > .regulators = mx31_3ds_regulators, > .num_regulators = ARRAY_SIZE(mx31_3ds_regulators), > +}; > + > +static struct mc13783_platform_data mc13783_pdata __initdata = { > + .regulators = &mc13783_regs, > .flags = MC13783_USE_REGULATOR | MC13783_USE_TOUCHSCREEN, > }; > > diff --git a/arch/arm/mach-mx3/mach-mx31moboard.c b/arch/arm/mach-mx3/mach-mx31moboard.c > index 1aa8d65..424fbd9 100644 > --- a/arch/arm/mach-mx3/mach-mx31moboard.c > +++ b/arch/arm/mach-mx3/mach-mx31moboard.c > @@ -267,9 +267,13 @@ static struct mc13783_leds_platform_data moboard_leds = { > .tc2_period = MC13783_LED_PERIOD_10MS, > }; > > -static struct mc13783_platform_data moboard_pmic = { > +static struct mc13783_regulator_platform_data moboard_regs = { > .regulators = moboard_regulators, > .num_regulators = ARRAY_SIZE(moboard_regulators), > +}; > + > +static struct mc13783_platform_data moboard_pmic = { > + .regulators = &moboard_regs, > .leds = &moboard_leds, > .flags = MC13783_USE_REGULATOR | MC13783_USE_RTC | > MC13783_USE_ADC | MC13783_USE_LED, > diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c > index f05bb08..687fb13 100644 > --- a/drivers/leds/leds-mc13783.c > +++ b/drivers/leds/leds-mc13783.c > @@ -30,6 +30,7 @@ struct mc13783_led { > struct mc13783 *master; > enum led_brightness new_brightness; > int id; > + int num_leds; > }; > > #define MC13783_REG_LED_CONTROL_0 51 > @@ -181,9 +182,9 @@ static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current) > return ret; > } > > -static int __devinit mc13783_leds_prepare(struct platform_device *pdev) > +static int __devinit mc13783_leds_prepare(struct platform_device *pdev, > + struct mc13783_leds_platform_data *pdata) > { > - struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev); > struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent); > int ret = 0; > int reg = 0; > @@ -264,7 +265,7 @@ out: > > static int __devinit mc13783_led_probe(struct platform_device *pdev) > { > - struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev); > + struct mc13783_leds_platform_data *pdata = platform_get_drvdata(pdev); > struct mc13783_led_platform_data *led_cur; > struct mc13783_led *led, *led_dat; > int ret, i; > @@ -286,12 +287,15 @@ static int __devinit mc13783_led_probe(struct platform_device *pdev) > return -ENOMEM; > } > > - ret = mc13783_leds_prepare(pdev); > + ret = mc13783_leds_prepare(pdev, pdata); > if (ret) { > dev_err(&pdev->dev, "unable to init led driver\n"); > goto err_free; > } > > + /* no need to save the num of LEDs for any other elements of 'led' */ > + led[0].num_leds = pdata->num_leds; > + So for n leds you introduced n-1 useless ints. So all in all, I don't like that change, maybe just because I don't understand your motivation. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
WARNING: multiple messages have this Message-ID (diff)
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers Date: Fri, 4 Feb 2011 10:34:58 +0100 [thread overview] Message-ID: <20110204093458.GH30452@pengutronix.de> (raw) In-Reply-To: <20110202202015.0fe392a9@queued.net> Hello Andres, On Wed, Feb 02, 2011 at 08:20:15PM -0800, Andres Salomon wrote: > > No need to explicitly set the cell's platform_data/data_size. > > In this case, move the various platform_data pointers > to driver_data. All of the clients which make use of it > are also changed. > > Mfd-core makes a copy of platform_data, but driver_data keeps a pointer > to the original data. Because each cell's platform_data previously > pointed to a local (stack) variable, the various ARM mach types that set > the pdata are updated to keep the memory around. I didn't get this even after reading it 5 times. You wrote in the subject that drivers now have access to mfd_cell. I don't see where e.g. drivers/leds/leds-mc13783.c uses that?! Does this depend on some mfd-changes I don't see and this is just a first step? After reading the changes I think I understood: - You made things that were passed as platform_data before available via driver_data. - Because platform_data is copied and driver_data is not at register time, the data being platform_data cannot be __initdata or stack local anymore, so this needs fixing. In sum this results in .data becoming bigger (which is bad). And I think this patch has a conceptual problem, too. In my opionion platform_data is the point to hand over platform specific data to a driver. driver_data is something that is private to the driver and has to be considered opaque for the platform. The driver was sort of OK before ... > diff --git a/arch/arm/mach-imx/mach-mx27_3ds.c b/arch/arm/mach-imx/mach-mx27_3ds.c > index 1643315..edcecaa 100644 > --- a/arch/arm/mach-imx/mach-mx27_3ds.c > +++ b/arch/arm/mach-imx/mach-mx27_3ds.c > @@ -226,10 +226,14 @@ static struct mc13783_regulator_init_data mx27_3ds_regulators[] = { > }, > }; > > -/* MC13783 */ > -static struct mc13783_platform_data mc13783_pdata __initdata = { > +static struct mc13783_regulator_platform_data mx27_regs = { The prefix mx27 is misleading. If you want to take the machine name into account use mx27_3ds, otherwise just use mc13783_regulator_pdata or similar. (And note that "regs" isn't that good IMHO, too, as it might stand for registers, too. And even mx27_3ds_regulators would be too general. This applies to the changes below, too.) > .regulators = mx27_3ds_regulators, > .num_regulators = ARRAY_SIZE(mx27_3ds_regulators), > +}; > + > +/* MC13783 */ a very minor nitpick is you moved the comment for mx27_3ds, but not for e.g. mx31_3ds. > +static struct mc13783_platform_data mc13783_pdata __initdata = { > + .regulators = &mx27_regs, > .flags = MC13783_USE_REGULATOR, > }; > > diff --git a/arch/arm/mach-imx/mach-pcm038.c b/arch/arm/mach-imx/mach-pcm038.c > index 5056148..b5ae8cc 100644 > --- a/arch/arm/mach-imx/mach-pcm038.c > +++ b/arch/arm/mach-imx/mach-pcm038.c > @@ -262,9 +262,13 @@ static struct mc13783_regulator_init_data pcm038_regulators[] = { > }, > }; > > -static struct mc13783_platform_data pcm038_pmic = { > +static struct mc13783_regulator_platform_data pcm038_regs = { > .regulators = pcm038_regulators, > .num_regulators = ARRAY_SIZE(pcm038_regulators), > +}; > + > +static struct mc13783_platform_data pcm038_pmic = { > + .regulators = &pcm038_regs, > .flags = MC13783_USE_ADC | MC13783_USE_REGULATOR | > MC13783_USE_TOUCHSCREEN, > }; > diff --git a/arch/arm/mach-mx3/mach-mx31_3ds.c b/arch/arm/mach-mx3/mach-mx31_3ds.c > index 0d65db8..3e613ee 100644 > --- a/arch/arm/mach-mx3/mach-mx31_3ds.c > +++ b/arch/arm/mach-mx3/mach-mx31_3ds.c > @@ -156,9 +156,13 @@ static struct mc13783_regulator_init_data mx31_3ds_regulators[] = { > }; > > /* MC13783 */ > -static struct mc13783_platform_data mc13783_pdata __initdata = { > +static struct mc13783_regulator_platform_data mc13783_regs = { > .regulators = mx31_3ds_regulators, > .num_regulators = ARRAY_SIZE(mx31_3ds_regulators), > +}; > + > +static struct mc13783_platform_data mc13783_pdata __initdata = { > + .regulators = &mc13783_regs, > .flags = MC13783_USE_REGULATOR | MC13783_USE_TOUCHSCREEN, > }; > > diff --git a/arch/arm/mach-mx3/mach-mx31moboard.c b/arch/arm/mach-mx3/mach-mx31moboard.c > index 1aa8d65..424fbd9 100644 > --- a/arch/arm/mach-mx3/mach-mx31moboard.c > +++ b/arch/arm/mach-mx3/mach-mx31moboard.c > @@ -267,9 +267,13 @@ static struct mc13783_leds_platform_data moboard_leds = { > .tc2_period = MC13783_LED_PERIOD_10MS, > }; > > -static struct mc13783_platform_data moboard_pmic = { > +static struct mc13783_regulator_platform_data moboard_regs = { > .regulators = moboard_regulators, > .num_regulators = ARRAY_SIZE(moboard_regulators), > +}; > + > +static struct mc13783_platform_data moboard_pmic = { > + .regulators = &moboard_regs, > .leds = &moboard_leds, > .flags = MC13783_USE_REGULATOR | MC13783_USE_RTC | > MC13783_USE_ADC | MC13783_USE_LED, > diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c > index f05bb08..687fb13 100644 > --- a/drivers/leds/leds-mc13783.c > +++ b/drivers/leds/leds-mc13783.c > @@ -30,6 +30,7 @@ struct mc13783_led { > struct mc13783 *master; > enum led_brightness new_brightness; > int id; > + int num_leds; > }; > > #define MC13783_REG_LED_CONTROL_0 51 > @@ -181,9 +182,9 @@ static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current) > return ret; > } > > -static int __devinit mc13783_leds_prepare(struct platform_device *pdev) > +static int __devinit mc13783_leds_prepare(struct platform_device *pdev, > + struct mc13783_leds_platform_data *pdata) > { > - struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev); > struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent); > int ret = 0; > int reg = 0; > @@ -264,7 +265,7 @@ out: > > static int __devinit mc13783_led_probe(struct platform_device *pdev) > { > - struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev); > + struct mc13783_leds_platform_data *pdata = platform_get_drvdata(pdev); > struct mc13783_led_platform_data *led_cur; > struct mc13783_led *led, *led_dat; > int ret, i; > @@ -286,12 +287,15 @@ static int __devinit mc13783_led_probe(struct platform_device *pdev) > return -ENOMEM; > } > > - ret = mc13783_leds_prepare(pdev); > + ret = mc13783_leds_prepare(pdev, pdata); > if (ret) { > dev_err(&pdev->dev, "unable to init led driver\n"); > goto err_free; > } > > + /* no need to save the num of LEDs for any other elements of 'led' */ > + led[0].num_leds = pdata->num_leds; > + So for n leds you introduced n-1 useless ints. So all in all, I don't like that change, maybe just because I don't understand your motivation. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2011-02-04 9:35 UTC|newest] Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-02-03 3:54 [RFC] [PATCH 0/19] mfd sharing support Andres Salomon 2011-02-03 3:55 ` [PATCH 01/19] mfd-core: unconditionally add mfd_cell to every platform_device Andres Salomon 2011-02-03 3:58 ` [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers Andres Salomon 2011-02-03 3:58 ` [lm-sensors] [PATCH 02/19] jz4740: mfd_cell is now implicitly Andres Salomon 2011-02-03 8:09 ` [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers Jean Delvare 2011-02-03 8:09 ` [lm-sensors] [PATCH 02/19] jz4740: mfd_cell is now implicitly Jean Delvare 2011-02-03 4:01 ` [PATCH 03/19] ab3550: mfd_cell is now implicitly available to drivers Andres Salomon 2011-02-03 4:01 ` Andres Salomon 2011-02-04 8:20 ` Mattias Wallin 2011-02-04 8:20 ` Mattias Wallin 2011-02-03 4:03 ` [PATCH 04/19] ab3100: " Andres Salomon 2011-02-03 4:03 ` Andres Salomon 2011-02-03 12:52 ` Linus Walleij 2011-02-03 12:52 ` Linus Walleij 2011-02-03 4:04 ` [PATCH 05/19] asic3: " Andres Salomon 2011-02-03 4:05 ` [PATCH 06/19] htc-pasic3: " Andres Salomon 2011-02-03 4:08 ` [PATCH 07/19] timberdale: " Andres Salomon 2011-02-03 4:08 ` Andres Salomon 2011-03-31 23:05 ` Grant Likely 2011-04-01 11:20 ` Samuel Ortiz 2011-04-01 11:20 ` Samuel Ortiz 2011-04-01 17:47 ` Andres Salomon 2011-04-01 17:47 ` Andres Salomon 2011-04-01 17:56 ` Grant Likely 2011-04-01 17:56 ` Grant Likely 2011-04-01 18:00 ` Grant Likely 2011-04-01 23:52 ` Samuel Ortiz 2011-04-01 23:52 ` Samuel Ortiz 2011-04-01 23:58 ` Grant Likely 2011-04-01 23:58 ` Grant Likely 2011-04-02 0:10 ` Andres Salomon 2011-04-02 0:10 ` Andres Salomon 2011-04-04 10:03 ` Samuel Ortiz 2011-04-04 10:03 ` Samuel Ortiz 2011-04-05 3:04 ` Grant Likely 2011-04-06 15:23 ` Samuel Ortiz 2011-04-06 15:58 ` Greg KH 2011-04-06 15:58 ` Greg KH 2011-04-06 17:05 ` Samuel Ortiz 2011-04-06 17:16 ` Ben Hutchings 2011-04-06 17:51 ` Samuel Ortiz 2011-04-06 17:51 ` Samuel Ortiz 2011-04-06 18:07 ` Ben Hutchings 2011-04-06 18:07 ` Ben Hutchings 2011-04-06 17:56 ` Greg KH 2011-04-06 18:25 ` Andres Salomon 2011-04-06 18:38 ` Greg KH 2011-04-06 18:38 ` Greg KH 2011-04-07 8:04 ` Grant Likely 2011-04-07 8:04 ` Grant Likely 2011-04-06 18:47 ` Samuel Ortiz 2011-04-06 18:59 ` Felipe Balbi 2011-04-06 18:59 ` Felipe Balbi 2011-04-06 22:09 ` Greg KH 2011-04-06 22:09 ` Greg KH 2011-04-07 8:09 ` Felipe Balbi 2011-04-07 13:40 ` Samuel Ortiz 2011-04-07 13:40 ` Samuel Ortiz 2011-04-07 14:35 ` Grant Likely 2011-04-07 15:03 ` Samuel Ortiz 2011-04-07 15:03 ` Samuel Ortiz 2011-04-07 18:06 ` Grant Likely 2011-04-07 18:06 ` Grant Likely 2011-04-07 16:24 ` Grant Likely 2011-04-01 18:26 ` Samuel Ortiz 2011-02-03 4:09 ` [PATCH 08/19] t7166xb: " Andres Salomon 2011-02-03 4:11 ` [PATCH 09/19] wl1273: " Andres Salomon 2011-02-03 4:12 ` [PATCH 10/19] sh_mobile_sdhi: " Andres Salomon 2011-02-03 4:13 ` [PATCH 11/19] tc6393xb: " Andres Salomon 2011-02-03 4:15 ` [PATCH 12/19] twl4030: " Andres Salomon 2011-02-03 6:05 ` Dmitry Torokhov 2011-02-03 6:39 ` Andres Salomon 2011-02-03 6:53 ` Dmitry Torokhov 2011-02-03 7:03 ` Andres Salomon 2011-02-03 7:03 ` Andres Salomon 2011-02-03 9:31 ` Mark Brown 2011-02-03 9:31 ` Mark Brown 2011-02-05 2:39 ` Andres Salomon 2011-02-05 3:25 ` Andres Salomon 2011-02-03 12:23 ` Peter Ujfalusi 2011-02-03 12:23 ` Peter Ujfalusi 2011-02-04 10:41 ` Uwe Kleine-König 2011-02-03 4:16 ` [PATCH 13/19] tc6387xb: " Andres Salomon 2011-02-03 4:17 ` [PATCH 14/19] janz: " Andres Salomon 2011-02-03 4:20 ` [PATCH 15/19] mc13xxx: " Andres Salomon 2011-02-03 4:20 ` Andres Salomon 2011-02-04 9:34 ` Uwe Kleine-König [this message] 2011-02-04 9:34 ` Uwe Kleine-König 2011-02-04 10:13 ` Uwe Kleine-König 2011-02-04 10:13 ` Uwe Kleine-König 2011-02-04 10:16 ` Andres Salomon 2011-02-04 10:16 ` Andres Salomon 2011-02-03 4:21 ` [PATCH 16/19] mfd-core: drop platform_data/data_size from core mfd_cell struct Andres Salomon 2011-02-03 4:22 ` [PATCH 17/19] mfd-core: add refcounting support to mfd_cells Andres Salomon 2011-02-03 4:23 ` [PATCH 18/19] mfd-core: add platform_device sharing support for mfd Andres Salomon 2011-02-03 4:26 ` [PATCH 19/19] cs5535-mfd: add sharing for acpi/pms cells Andres Salomon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110204093458.GH30452@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=broonie@opensource.wolfsonmicro.com \ --cc=dilinger@queued.net \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=lrg@slimlogic.co.uk \ --cc=rpurdie@rpsys.net \ --cc=sameo@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.