Re: [PATCH 5/7] Add MCI card support to barebox

[Juergen Beisert <jbe@pengutronix.de>]

Sascha Hauer wrote:
> On Thu, Oct 07, 2010 at 06:00:06PM +0200, Juergen Beisert wrote:
> > Sascha Hauer wrote:
> > > On Thu, Oct 07, 2010 at 03:24:16PM +0200, Juergen Beisert wrote:
> > > > This adds the basic framework to handle MCI cards in barebox.
> > > >
> > > > Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> > > > ---
> > > >  drivers/Kconfig        |    1 +
> > > >  drivers/Makefile       |    1 +
> > > >  drivers/mci/Kconfig    |   30 ++
> > > >  drivers/mci/Makefile   |    1 +
> > > >  drivers/mci/mci-core.c | 1308
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++ include/mci.h       
> > > >   | 230 +++++++++
> > > >  6 files changed, 1571 insertions(+), 0 deletions(-)
> > > >  create mode 100644 drivers/mci/Kconfig
> > > >  create mode 100644 drivers/mci/Makefile
> > > >  create mode 100644 drivers/mci/mci-core.c
> > > >  create mode 100644 include/mci.h
> > >
> > > This whole patch looks quite good.
> > > Please add some linebreaks in mci-core.c. I don't want strict 80
> > > character lines, but some lines are really long.
> > >
> > > [snip]
> > >
> > > > +static int mci_probe(struct device_d *mci_dev)
> > > > +{
> > > > +	struct mci *mci;
> > > > +	int rc;
> > > > +
> > > > +	mci = xzalloc(sizeof(struct mci));
> > > > +	mci_dev->priv = mci;
> > > > +
> > > > +#ifdef CONFIG_MCI_STARTUP
> > > > +	/* if enabled, probe the attached card immediately */
> > > > +	rc = mci_card_probe(mci_dev);
> > > > +	if (rc == -ENODEV) {
> > > > +		/*
> > > > +		 * If it fails, add the 'probe' parameter to give the user
> > > > +		 * a chance to insert a card and try again. Note: This may fail
> > > > +		 * systems that rely on the MCI card for startup (for the
> > > > +		 * persistant environment for example)
> > > > +		 */
> > > > +		rc = add_mci_parameter(mci_dev);
> > > > +		if (rc != 0) {
> > > > +			pr_err("Failed to add 'probe' parameter to the MCI device\n");
> > > > +			goto on_error;
> > > > +		}
> > > > +	}
> > > > +#endif
> > > > +
> > > > +#ifndef CONFIG_MCI_STARTUP
> > >
> > > #else instead?
> > >
> > > > +	/* add params on demand */
> > > > +	rc = add_mci_parameter(mci_dev);
> > > > +	if (rc != 0) {
> > > > +		pr_err("Failed to add 'probe' parameter to the MCI device\n");
> > > > +		goto on_error;
> > > > +	}
> > > > +#endif
> > > > +
> > > > +	return rc;
> > > > +
> > > > +on_error:
> > > > +	free(mci);
> > > > +	return rc;
> > > > +}
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +/** host information */
> > > > +struct mci_platformdata {
> > > > +	struct device_d *hw_dev;	/**< the host MCI hardware device */
> > > > +	unsigned voltages;
> > > > +	unsigned host_caps;	/**< Host's interface capabilities, refer
> > > > MMC_VDD_* and FIXME */ +	unsigned f_min;		/**< host interface lower
> > > > limit */ +	unsigned f_max;		/**< host interface upper limit */
> > > > +	unsigned clock;		/**< Current clock used to talk to the card */
> > > > +	unsigned bus_width;	/**< used data bus width to the card */
> > > > +
> > > > +	int (*init)(struct device_d*, struct device_d*);	/**< init the host
> > > > interface */ +	void (*set_ios)(struct device_d*, struct device_d*,
> > > > unsigned, unsigned);	/**< change host interface settings */ +	int
> > > > (*send_cmd)(struct device_d*, struct mci_cmd*, struct
> > > > mci_data*);	/**< handle a command */ +};
> > >
> > > I prefer this struct named mci_host, this seems to match better what it
> > > actually is.
> >
> > Hmm, no, its not a "host". It is the MMC/SD card instance. The host is
> > more the interface, isn't it (at least for me)?
>
> It's a host at least in the Linux terminology.

Yes. So, we name the platform like data of the MCI card device 
instance "mci_host".

host device
-----------

  hw_device
     ->priv: points to drivers private data
          names are: "s3c_mci_host" or "stm_mci_host"
     ->platform_data: points to data provided by the platform
          names are: "s3c_mci_platform_data" or "stm_mci_platform_data"

mci device
----------

  mci_device
     ->priv: points to mci layer's private data
           name is: "mci_data"
     ->platform_data: points to data provided by the host device
           name is: "mci_platformdata"
           name should be: "mci_host"

"host" in the name sounds IMHO wrong here. I'm not happy about that, but its 
okay. I will change the name.

> > > For the convenience of drivers set init/set_ios/send_cmd
> > > functions should be passed a pointer to the mci_host, not the device,
> > > because that's what they actually registered. I already prepared a
> > > patch for this, I'll send it in a seperate mail.
> >
> > I tried to layering the devices:
> >
> >  disk_device -> knows how to handle disk drives and partition tables
> >
> >  mci_device -> knows how to probe and manage MMC/SD cards
> >
> >  hw_device -> knows how to transfer data
> >
> > Most functions in the hw_device layer do not need access to any other
> > structure data than their own device (okay, there is a exception). So, I
> > tried to keep it simple.
>
> I didn't change the underlying structure, only the pointer which is
> passed to the functions.

Sure.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox