Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Fri, May 23, 2008 at 11:19:28PM -0600, Grant Likely wrote:
> Yup, I like this approach better.  I had been thinking about putting
> this all in the same file (drivers/mmc/host/mmc_spi.c) instead of
> exporting the probe/remove symbols and by using clear comment blocks
> to divide the sections, but I've got no issues with this approach.
> 
> This is good work.  Some comments below.  (I won't repeat Stephen's comments)

Thanks!

> On Fri, May 23, 2008 at 12:28 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > This patch depends on the Grant Likely's SPI patches, so this is
> > for RFC only.
> >
> > Also, later we'll able to remove OF_GPIO dependency.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  Documentation/powerpc/booting-without-of.txt |   24 ++++
> >  drivers/mmc/host/Kconfig                     |    7 ++
> >  drivers/mmc/host/Makefile                    |    2 +-
> >  drivers/mmc/host/of_mmc_spi.c                |  151 ++++++++++++++++++++++++++
> >  4 files changed, 183 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/mmc/host/of_mmc_spi.c
> >
> > diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> > index 423cb2b..7d0ef80 100644
> > --- a/Documentation/powerpc/booting-without-of.txt
> > +++ b/Documentation/powerpc/booting-without-of.txt
> > @@ -3151,7 +3151,31 @@ platforms are moved over to use the flattened-device-tree model.
> >                        };
> >                };
> >
> > +    ...) MMC-over-SPI
> >
> > +      Required properties:
> > +      - #address-cells : should be 0.
> > +      - #size-cells : should be 0.
> 
> Are these properties required at all?  Will this node have any children.

Yeah, my weakness for #a/#s, I always insert it where unnecessary. :-)
Will fix.

> > +      - compatible : should be "linux,mmc-spi".
> > +      - linux,modalias  - should be "of_mmc_spi".
> 
> I'm not even sure if the whole linux,modalias is even a good idea.  I
> had kind of thrown it in there as a convenient way to override
> compatible when needed, but I haven't really thought it out very well
> and I think it is rather a hack.
> 
> The real problem is we don't yet have good method (or place) to apply
> a translation table from compatible values to modaliases.  Ideally,
> the translations should be part of the drivers themselves, but that
> causes a chicken and egg problem of needing to load the driver to get
> access to the table to know if it is the correct driver... Of course,
> I'm really not very familiar with the whole module autoloading
> mechanism.  Regardless; binding should be based on compatible, not on
> a hacky and bogus linux,modalias property.

I fully agree. Though, I just tried to use your spi_of with a belief
that you'll implement compatible->modalias translation in spi_of. :-)

> > +      - reg : should specify SPI address (chip-select number).
> > +      - max-speed : (optional) maximum frequency for this device (Hz).
> > +      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
> > +        (slot voltage).
> 
> Should this property be better defined?

Yes, will do. Was a bit lazy to do this for the RFC.

[...]
> > +static int mmc_get_cd(struct device *dev)
> > +{
> > +       struct of_mmc_spi *oms = dev->archdata.of_node->data;
> > +
> > +       return gpio_get_value(oms->cd_gpio);
> > +}
> > +
> > +static int of_mmc_spi_probe(struct spi_device *spi)
> > +{
> > +       int ret = -EINVAL;
> > +       struct device_node *np = spi->dev.archdata.of_node;
> > +       struct device *dev = &spi->dev;
> > +       struct of_mmc_spi *oms = kzalloc(sizeof(*oms), GFP_KERNEL);
> > +       const u32 *ocr_mask;
> > +       int size;
> > +
> > +       if (!oms)
> > +               return -ENOMEM;
> > +
> > +       /* Somebody occupied node's data? */
> > +       WARN_ON(spi->dev.archdata.of_node->data);
> 
> Perhaps bail at this point to avoid corruption?  Would it be better to
> use container_of() instead for getting a pointer back to the private
> structure from the pdata pointer?

Using platform_data with container_of isn't always safe (e.g. for
platform bus we can't use it, this is done so that board's platform_data
definitions could be made __initdata).

David, would you [not] suggest us to use platform_data with
container_of, that is, are there any plans to make SPI core copy
the platform_data instead of passing a pointer directly?

> > +
> > +       /*
> > +        * mmc_spi_probe will use drvdata, so we can't use it. Use node's
> > +        * data instead.
> > +        */
> > +       spi->dev.archdata.of_node->data = oms;
> > +
[...]

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2