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

[Michael Trimarchi]

Hi

On Tue, Jun 17, 2014 at 5:49 PM, Marek Vasut <marex at denx.de> wrote:
> 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).

I suggest as Stefano said to add this option/flags now in preparation
of DT transition
and start to document flags as you said.

Michael