lkml.org 
[lkml]   [2011]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC] [PATCH] mfd: Fetch cell pointer from platform_device->mfd_cell
    This looks fine to me; just some minor points below.

    On
    Fri, 8 Apr 2011 02:40:09 +0200 Samuel Ortiz <sameo@linux.intel.com>
    wrote:

    >
    > In order for MFD drivers to fetch their cell pointer but also their
    > platform data one, an mfd cell pointer is t aed to the
    > platform_device structure.
    > That allows all MFD sub devices drivers to be MFD agnostic, unless
    > they really need to access their MFD cell data. Most of them don't,
    > especially the ones for IPs used by both MFD and non MFD SoCs.
    >
    > Cc: Greg KH <gregkh@suse.de>
    > Cc: Grant Likely <grant.likely@secretlab.ca>
    > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
    > ---
    > drivers/base/platform.c | 1 +
    > drivers/mfd/mfd-core.c | 16 ++++++++++++++--
    > include/linux/mfd/core.h | 8 ++++++--
    > include/linux/platform_device.h | 5 +++++
    > 4 files changed, 26 insertions(+), 4 deletions(-)
    >
    > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
    > index f051cff..6c3a2bd 100644
    > --- a/drivers/base/platform.c
    > +++ b/drivers/base/platform.c
    > @@ -149,6 +149,7 @@ static void platform_device_release(struct device
    > *dev)
    > of_device_node_put(&pa->pdev.dev);
    > kfree(pa->pdev.dev.platform_data);
    > + kfree(pa->pdev.mfd_cell);

    Hm, given that most platform devices won't be mfd devices (and thus
    mfd_cell will be NULL), is it better to rely on kfree's
    unlikely(ZERO_OR_NULL_PTR(...)), or have this be "if
    (pa->pdev.mfd_cell) kfree(pa->pdev.mfd_cell);"?

    > kfree(pa->pdev.resource);
    > kfree(pa);
    > }
    > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
    > index d01574d..f4c8c84 100644
    > --- a/drivers/mfd/mfd-core.c
    > +++ b/drivers/mfd/mfd-core.c
    > @@ -55,6 +55,19 @@ int mfd_cell_disable(struct platform_device *pdev)
    > }
    > EXPORT_SYMBOL(mfd_cell_disable);
    >
    > +static int mfd_platform_add_cell(struct platform_device *pdev,
    > + const struct mfd_cell *cell)
    > +{
    > + if (!cell)
    > + return 0;
    > +
    > + pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
    > + if (!pdev->mfd_cell)
    > + return -ENOMEM;
    > +
    > + return 0;
    > +}
    > +
    > static int mfd_add_device(struct device *parent, int id,
    > const struct mfd_cell *cell,
    > struct resource *mem_base,
    > @@ -75,7 +88,7 @@ static int mfd_add_device(struct device *parent,
    > int id,
    > pdev->dev.parent = parent;
    >
    > - ret = platform_device_add_data(pdev, cell, sizeof(*cell));
    > + ret = mfd_platform_add_cell(pdev, cell);
    > if (ret)
    > goto fail_res;
    >
    > @@ -123,7 +136,6 @@ static int mfd_add_device(struct device *parent,
    > int id,
    > return 0;
    >
    > -/* platform_device_del(pdev); */
    > fail_res:
    > kfree(res);
    > fail_device:
    > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
    > index ad1b19a..ee4731b 100644
    > --- a/include/linux/mfd/core.h
    > +++ b/include/linux/mfd/core.h
    > @@ -86,16 +86,20 @@ extern int mfd_clone_cell(const char *cell, const
    > char **clones, */
    > static inline const struct mfd_cell *mfd_get_cell(struct
    > platform_device *pdev) {
    > - return pdev->dev.platform_data;
    > + return pdev->mfd_cell;
    > }
    >
    > /*
    > * Given a platform device that's been created by mfd_add_devices(),
    > fetch
    > * the .mfd_data entry from the mfd_cell that created it.
    > + * Otherwise just return the platform_data pointer.

    I'd also suggest describing why we fall back to
    platform_data; to the casual reader, it would be confusing. Perhaps
    something to the effect of, "This maintains compatibility with
    platform drivers whose devices aren't created by the mfd layer, and
    expect platform_data to contain what would've otherwise been in
    mfd_data."

    > */
    > static inline void *mfd_get_data(struct platform_device *pdev)
    > {
    > - return mfd_get_cell(pdev)->mfd_data;
    > + if (pdev->mfd_cell)
    > + return mfd_get_cell(pdev)->mfd_data;
    > + else
    > + return pdev->dev.platform_data;

    Not much point checking pdev->mfd_cell and then using an abstraction.
    I'd just do "const struct mfd_cell *cell = mfd_get_cell(pdev); if
    (cell) return cell->mfd_data;" or "if (pdev->mfd_cell) return
    pdev->mfd_cell->mfd_data;"


    > }
    >
    > extern int mfd_add_devices(struct device *parent, int id,
    > diff --git a/include/linux/platform_device.h
    > b/include/linux/platform_device.h index d96db98..744942c 100644
    > --- a/include/linux/platform_device.h
    > +++ b/include/linux/platform_device.h
    > @@ -14,6 +14,8 @@
    > #include <linux/device.h>
    > #include <linux/mod_devicetable.h>
    >
    > +struct mfd_cell;
    > +
    > struct platform_device {
    > const char * name;
    > int id;
    > @@ -23,6 +25,9 @@ struct platform_device {
    >
    > const struct platform_device_id *id_entry;
    >
    > + /* MFD cell pointer */
    > + struct mfd_cell *mfd_cell;
    > +
    > /* arch specific additions */
    > struct pdev_archdata archdata;
    > };



    \
     
     \ /
      Last update: 2011-04-08 04:41    [W:0.033 / U:0.092 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site