[U-Boot] [PATCH v2 8/9] tegra: add SPI SLINK driver

[Simon Glass]

Hi Stephen,

On Mon, Jan 14, 2013 at 10:49 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 01/12/2013 09:56 AM, Simon Glass wrote:
>> Hi,
>>
>> On Sat, Jan 12, 2013 at 1:07 AM, Allen Martin <amartin at nvidia.com> wrote:
>>> Add driver for tegra SPI "SLINK" style driver.  This controller is
>>> similar to the tegra20 SPI "SFLASH" controller.  The difference is
>>> that the SLINK controller is a genernal purpose SPI controller and the
>>> SFLASH controller is special purpose and can only talk to FLASH
>>> devices.  In addition there are potentially many instances of an SLINK
>>> controller on tegra and only a single instance of SFLASH.  Tegra20 is
>>> currently ths only version of tegra that instantiates an SFLASH
>>> controller.
>>>
>>> This driver supports basic PIO mode of operation and is configurable
>>> (CONFIG_OF_CONTROL) to be driven off devicetree bindings.  Up to 4
>>> devices per controller may be attached, although typically only a
>>> single chip select line is exposed from tegra per controller so in
>>> reality this is usually limited to 1.
>>>
>>> To enable this driver, use CONFIG_TEGRA_SLINK
>
>>> diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c
>
>>> +#ifdef CONFIG_OF_CONTROL
>>
>> You probably don't need this ifdef
>
> If we did assume CONFIG_OF_CONTROL was on, then we could get rid of the
> previous patch; all the device addresses would come from device tree, so
> there would be no need to add them to any header file:-)

As Allen said I just mean we shouldn't need to put #ifdefs around
#include <fdtdec.h>

>
>>> +void spi_init(void)
>>> +{
>>> +       int node = 0, i;
>>> +       struct tegra_spi_ctrl *ctrl;
>>
>> blank line here
>>
>>> +       for (i = 0; i < CONFIG_TEGRA_SLINK_CTRLS; i++) {
>>> +               ctrl = &spi_ctrls[i];
>>> +#ifdef CONFIG_OF_CONTROL
>>> +               node = fdtdec_next_compatible(gd->fdt_blob, node,
>>> +                                             COMPAT_NVIDIA_TEGRA20_SLINK);
>>> +               if (!node)
>>> +                       break;
>>
>> I think you should be using fdtdec_find_aliases_for_id() so that aliases work.
>
> I strongly believe we shouldn't be using aliases for enumeration, which
> is what fdtdec_find_aliases_for_id() does, IIRC. Instead, we should
> enumerate all the devices in the correct fashion, and then apply aliases
> as a separate step if they exists. Hence, I believe it's not correct to
> use fdtdec_find_aliases_for_id() anywhere.

We might be saying the same thing, but I can't tell. We had quite a
long discussion about this when the function was written and I thought
it was then considered correct.

The function does not ignore nodes without an alias. It simply returns
a list of nodes that your driver should init, in the order in which
U-Boot expects to find them. So I think it is safe to call, and in
fact should be called in any driver that has more than one device and
wants to provide the ability to select device ordering. Please can you
take another look at the function and see what you think?

We should not enumerate devices that are not supposed to be used, so
checking fdtdec_get_is_enabled() might be good too.

Regards,
Simon