[U-Boot] [PATCH 4/6] mmc: fsl_esdhc: Add support to force VSELECT set

[Marek Vasut]

On Tuesday, June 17, 2014 at 08:06:38 AM, Michael Trimarchi wrote:
> Hi Marek
> 
> Il 17/giu/2014 08:03 "Marek Vasut" <marex at denx.de> ha scritto:
> > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote:
> > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg
> > > <grinberg at compulab.co.il>
> > 
> > wrote:
> > > > Hi Otavio,
> > > > 
> > > > On 06/16/14 05:24, Otavio Salvador wrote:
> > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex at denx.de> wrote:
> > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote:
> > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex at denx.de>
> 
> wrote:
> > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote:
> > > >>>>> 
> > > >>>>> [...]
> > > >>>>> 
> > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT
> > > >>>>>>>> +     esdhc_setbits32(&regs->vendorspec,
> > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif
> > > >>>>>>> 
> > > >>>>>>> Documentation is missing.
> > > >>>>>> 
> > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include
> 
> it
> 
> > > >>>>>> anywhere.
> > > >>>>> 
> > > >>>>> I'm at loss for words here, really...
> > > >>>>> 
> > > >>>>> I think you know what needs to be done (hint: write the
> > > >>>>> documentation), right ?
> > > >>>> 
> > > >>>> I won't write the full documentation for it. I am sorry.
> > > >>> 
> > > >>> Undocumented configuration option is not acceptable, period.
> > > >> 
> > > >> Who accepted the driver in the first version, without Doc?
> > > >> 
> > > >> I am not in the position to write the full doc.
> > > > 
> > > > I think there is a misunderstanding here...
> > > > I think Marek does not want to say that you need to write the full
> > > > documentation for the driver, but only document the
> > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it
> 
> do
> 
> > > > when you define it and why should one define it).
> > > 
> > > Great but where if it does not exist?
> > > 
> > > should I make a README.fsl-esdhc and include just it?
> > 
> > I briefly looked over the options in the driver, prefixed with hash are
> 
> the ones
> 
> > which are not driver specific:
> > 
> > CONFIG_ESDHC_DETECT_8_BIT_QUIRK
> > CONFIG_ESDHC_DETECT_QUIRK
> > CONFIG_FSL_ESDHC_PIN_MUX
> > CONFIG_FSL_USDHC
> > #CONFIG_MX53
> > #CONFIG_OF_LIBFDT
> > CONFIG_SYS_FSL_ERRATUM_ESDHC111
> > CONFIG_SYS_FSL_ERRATUM_ESDHC135
> > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001
> > CONFIG_SYS_FSL_ESDHC_ADDR
> > CONFIG_SYS_FSL_ESDHC_USE_PIO
> > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33
> > #CONFIG_SYS_MMC_MAX_BLK_COUNT
> > #CONFIG_SYS_SD_VOLTAGE
> > #CONFIG_T4240QDS
> > 
> > It looks like completely ad-hoc addition of new and new config options as
> > whoever seen fit to me, both from their names and git log of the driver.
> 
> This
> 
> > makes the driver completely unmaintainable. I would like to see them
> 
> documented
> 
> Do you think that could be a better option to use flags and runtime detect
> instead having hundred of define?

Eventually, when we transition to DT, this will be indeed needed anyway. But 
before that, we need to understand what those flags do (which we don't because 
previous patches neglected adding proper documentation).