platform data and mfd design question

platform data and mfd design question

[Abhijeet Dharmapurikar]

Currently all the mfd devices declare their struct mfd_cell 
sub_devices[] array within the core driver. The platform data to them is 
either passed in as a part of the core driver's platform data.

Msm on the other hand declares the struct mfd_cell subdevice[] array in 
the board file and passes this on to the core driver via platfom data.
This code can be found here (sorry for the long url - it is convinient 
to click on it),
https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=blob;f=arch/arm/mach-msm/board-msm8x60.c;h=ed9e9a7674b5ee443f25af828a0044ff99fac483;hb=refs/heads/android-msm-2.6.35
look for static struct mfd_cell pm8058_subdevs[]

This gives one the convenience of changing the mfd_cells and their 
platform data in the board file itself. There are boards where the 
platform data of some cells changes and in some cases we dont even add a 
particular cell.

This design makes the core driver very light weight. All it does is 
calls mfd_add_devices on the cell array passed from its platform data.

Will this be acceptable in mainline OR do we need to change to follow 
how others in drivers/mfd do it which is to define the mfd_cell array in 
the core file itself and manipulate their platform data before doing 
mfd_add_devices.


--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm 
Innovation Center, Inc. is a member of the Code Aurora Forum.

platform data and mfd design question

[Abhijeet Dharmapurikar]

Currently all the mfd devices declare their struct mfd_cell 
sub_devices[] array within the core driver. The platform data to them is 
either passed in as a part of the core driver's platform data.

Msm on the other hand declares the struct mfd_cell subdevice[] array in 
the board file and passes this on to the core driver via platfom data.
This code can be found here (sorry for the long url - it is convinient 
to click on it),
https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=blob;f=arch/arm/mach-msm/board-msm8x60.c;h=ed9e9a7674b5ee443f25af828a0044ff99fac483;hb=refs/heads/android-msm-2.6.35
look for static struct mfd_cell pm8058_subdevs[]

This gives one the convenience of changing the mfd_cells and their 
platform data in the board file itself. There are boards where the 
platform data of some cells changes and in some cases we dont even add a 
particular cell.

This design makes the core driver very light weight. All it does is 
calls mfd_add_devices on the cell array passed from its platform data.

Will this be acceptable in mainline OR do we need to change to follow 
how others in drivers/mfd do it which is to define the mfd_cell array in 
the core file itself and manipulate their platform data before doing 
mfd_add_devices.


--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm 
Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: platform data and mfd design question

[Mark Brown]

On Tue, Feb 15, 2011 at 01:51:16PM -0800, Abhijeet Dharmapurikar wrote:

> This code can be found here (sorry for the long url - it is convinient to
> click on it),

I can't actually render this URL on a single line without fiddling with
the font size for my terminal...  When pasting gitweb URLs you almost
always want to edit out the head ID and just rely on the branch.

> This gives one the convenience of changing the mfd_cells and their
> platform data in the board file itself. There are boards where the
> platform data of some cells changes and in some cases we dont even
> add a particular cell.

My main question is why you're trying to do this.  Even if some features
aren't used on a given board one would strongly expect that the silicon
will be the same anyway - what's the goal in changing the devices?

> This design makes the core driver very light weight. All it does is
> calls mfd_add_devices on the cell array passed from its platform
> data.

The downside is that it then becomes impossible for the MFD to pass in
resources to the devices (IRQs being the most obvious example) and every
single board needs to replicate the definitions of all the subdevices
which gets tedious, especially if you want to change them for some
reason (eg, an IP becomes used in a wider range of chips so a rename is
appropriate).

platform data and mfd design question

[Mark Brown]

On Tue, Feb 15, 2011 at 01:51:16PM -0800, Abhijeet Dharmapurikar wrote:

> This code can be found here (sorry for the long url - it is convinient to
> click on it),

I can't actually render this URL on a single line without fiddling with
the font size for my terminal...  When pasting gitweb URLs you almost
always want to edit out the head ID and just rely on the branch.

> This gives one the convenience of changing the mfd_cells and their
> platform data in the board file itself. There are boards where the
> platform data of some cells changes and in some cases we dont even
> add a particular cell.

My main question is why you're trying to do this.  Even if some features
aren't used on a given board one would strongly expect that the silicon
will be the same anyway - what's the goal in changing the devices?

> This design makes the core driver very light weight. All it does is
> calls mfd_add_devices on the cell array passed from its platform
> data.

The downside is that it then becomes impossible for the MFD to pass in
resources to the devices (IRQs being the most obvious example) and every
single board needs to replicate the definitions of all the subdevices
which gets tedious, especially if you want to change them for some
reason (eg, an IP becomes used in a wider range of chips so a rename is
appropriate).

Re: platform data and mfd design question

[Linus Walleij]

On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> Msm on the other hand declares the struct mfd_cell subdevice[] array in
> the board file and passes this on to the core driver via platfom data.
>    

This way the platform data tells the core driver what kind of
silicon it has "hey, PM8058, guess what, you have an RTC!"
which looks backwards to me, especially given that it does
not need any fancy platform data at all, just two IRQ numbers
which the core driver can very well handle.

For example: if the platform data (which is about how the
components are connected on the board etc) does not
provide the RTC resource, all of a sudden it appears to the
system as if the PM8058 does not have an RTC, but it does...


Yours,
Linus Walleij

Re: platform data and mfd design question

[Linus Walleij]

On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> Msm on the other hand declares the struct mfd_cell subdevice[] array in
> the board file and passes this on to the core driver via platfom data.
>    

This way the platform data tells the core driver what kind of
silicon it has "hey, PM8058, guess what, you have an RTC!"
which looks backwards to me, especially given that it does
not need any fancy platform data at all, just two IRQ numbers
which the core driver can very well handle.

For example: if the platform data (which is about how the
components are connected on the board etc) does not
provide the RTC resource, all of a sudden it appears to the
system as if the PM8058 does not have an RTC, but it does...


Yours,
Linus Walleij

platform data and mfd design question

[Linus Walleij]

On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> Msm on the other hand declares the struct mfd_cell subdevice[] array in
> the board file and passes this on to the core driver via platfom data.
>    

This way the platform data tells the core driver what kind of
silicon it has "hey, PM8058, guess what, you have an RTC!"
which looks backwards to me, especially given that it does
not need any fancy platform data at all, just two IRQ numbers
which the core driver can very well handle.

For example: if the platform data (which is about how the
components are connected on the board etc) does not
provide the RTC resource, all of a sudden it appears to the
system as if the PM8058 does not have an RTC, but it does...


Yours,
Linus Walleij

Re: platform data and mfd design question

[Mark Brown]

On Wed, Feb 16, 2011 at 09:48:33AM +0100, Linus Walleij wrote:
> On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> >Msm on the other hand declares the struct mfd_cell subdevice[] array in
> >the board file and passes this on to the core driver via platfom data.

> This way the platform data tells the core driver what kind of
> silicon it has "hey, PM8058, guess what, you have an RTC!"
> which looks backwards to me, especially given that it does
> not need any fancy platform data at all, just two IRQ numbers
> which the core driver can very well handle.

Indeed, and the RTC would still be useful even without IRQ support I
imagine (it should still be able to tell you the time).

Re: platform data and mfd design question

[Mark Brown]

On Wed, Feb 16, 2011 at 09:48:33AM +0100, Linus Walleij wrote:
> On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> >Msm on the other hand declares the struct mfd_cell subdevice[] array in
> >the board file and passes this on to the core driver via platfom data.

> This way the platform data tells the core driver what kind of
> silicon it has "hey, PM8058, guess what, you have an RTC!"
> which looks backwards to me, especially given that it does
> not need any fancy platform data at all, just two IRQ numbers
> which the core driver can very well handle.

Indeed, and the RTC would still be useful even without IRQ support I
imagine (it should still be able to tell you the time).

platform data and mfd design question

[Mark Brown]

On Wed, Feb 16, 2011 at 09:48:33AM +0100, Linus Walleij wrote:
> On 02/15/2011 10:51 PM, Abhijeet Dharmapurikar wrote:
> >Msm on the other hand declares the struct mfd_cell subdevice[] array in
> >the board file and passes this on to the core driver via platfom data.

> This way the platform data tells the core driver what kind of
> silicon it has "hey, PM8058, guess what, you have an RTC!"
> which looks backwards to me, especially given that it does
> not need any fancy platform data at all, just two IRQ numbers
> which the core driver can very well handle.

Indeed, and the RTC would still be useful even without IRQ support I
imagine (it should still be able to tell you the time).