[U-Boot] [PATCH v2] dm/mmc: gen_atmel_mci: Add driver model support for mci

[Wenyou.Yang at microchip.com]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: 2016年9月21日 5:15
> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stephen Warren
> <swarren at nvidia.com>
> Subject: Re: [U-Boot] [PATCH v2] dm/mmc: gen_atmel_mci: Add driver model
> support for mci
> 
> On 09/19/2016 09:05 PM, Wenyou Yang wrote:
> > Add the driver model support for Atmel mci while retaining the
> > existing legacy code. This allows the driver to support boards that
> > have converted to driver model as well as those that have not.
> 
> > diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> 
> > +config GENERIC_ATMEL_MCI
> > +	bool "Atmel Multimedia Card Interface support"
> > +	depends on DM_MMC && BLK && DM_MMC_OPS && ARCH_AT91
> > +	help
> > +	  This enables support for Atmel High Speed Multimedia Card Interface
> > +	  (HSMCI), which supports the MultiMedia Card (MMC) Specification V4.3,
> > +	  the SD Memory Card Specification V2.0, the SDIO V2.0 specification
> > +	  and CE-ATA V1.1.
> 
> This seems unrelated to the actual driver conversion. It's more like part of a
> convert-the-option-to-Kconfig patch. I suspect it should be removed for now?

Yes, it is unrelated. It seems not good on this patch.

But here, only add the option, doesn't enable. 

I think it is somewhat acceptable.

> 
> I think you need to remove "#define GENERIC_ATMEL_MCI" from
> include/configs/*.h and add it to configs/*_defconfig as part of the patch that adds
> this Kconfig option.
> 
> > +#ifndef CONFIG_DM_MMC
> >  /* Setup for MCI Clock and Block Size */  static void
> > mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)  {
> >  	struct atmel_mci_priv *priv = mmc->priv;
> > -	atmel_mci_t *mci = priv->mci;
> >  	u32 bus_hz = get_mci_clk_rate();
> > +#else
> > +static void mci_set_mode(struct atmel_mci_priv *priv, u32 hz, u32
> > +blklen) {
> > +	struct mmc *mmc = &priv->mmc;
> > +	u32 bus_hz = priv->bus_clk_rate;
> > +#endif
> 
> It's technically CONFIG_DM_MMC_OPS that changes that function signature, I
> believe. This code could allow someone to enable just CONFIG_DM_MMC but not
> CONFIG_DM_MMC_OPS and then see a build error. Once everything is
> converted to Kconfig that won't be possible due to the "depends on" you added
> above, but while the option is still selected by config.h files, this is possible.
> 
> Still, this is a minor issue so if you're going to do the config.h -> defconfig
> conversion soon, maybe just ignore this.

Yes, I will do the config.h soon.


Best Regards,
Wenyou Yang