[U-Boot] [PATCH v2 3/8] tegra2: spi: Add SPI driver for Tegra2 SOC

[Simon Glass]

Hi Mike,

On Sat, Nov 5, 2011 at 10:13 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Saturday 05 November 2011 10:36:30 Simon Glass wrote:
>> On Thu, Nov 3, 2011 at 6:36 PM, Mike Frysinger wrote:
>> > On Thursday 03 November 2011 18:41:34 Simon Glass wrote:
>> >> --- /dev/null
>> >> +++ b/drivers/spi/tegra2_spi.c
>> >>
>> >> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
>> >> +{
>> >> +     /* Tegra2 SPI-Flash - only 1 device ('bus/cs') */
>> >> +     if (bus > 0 && cs != 0)
>> >> +             return 0;
>> >> +     else
>> >> +             return 1;
>> >> +}
>> >
>> > shouldn't that be "||" and not "&&" ?
>>
>> This function should be removed as it doesn't print enough errors.
>
> this func is part of the SPI API.  you can't remove it ;).

OK - it seems to only be used by the mmc_spi command which Tegra isn't
using. But I will add it back in. (Should it be called in
spi_flash_probe(), for example?)

>
>> >> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>> >> +             unsigned int max_hz, unsigned int mode)
>> >> +{
>> >> +     struct tegra_spi_slave *spi;
>> >> +
>> >> +     if (!spi_cs_is_valid(bus, cs))
>> >> +             return NULL;
>> >> +
>> >> +     if (bus != 0) {
>> >> +             printf("SPI error: unsupported bus %d\n", bus);
>> >> +             return NULL;
>> >> +     }
>> >> +     if (cs != 0) {
>> >> +             printf("SPI error: unsupported chip select %d on bus
>> >> %d\n", +                    cs, bus);
>> >> +             return NULL;
>> >> +     }
>> >
>> > doesn't spi_cs_is_valid() make these two later checks redundant ?
>>
>> Yes - have removed the function.
>
> i think this is the wrong direction ... other SPI buses are not warning about
> invalid bus/cs combos and that seems to be fine.  i don't think the tegra spi
> bus needs to be uniquely verbose.

OK, I was lead astray by the OMAP code which does this. I will revert
and resend this patch.

>
>> >> +     reg = readl(&regs->status);
>> >> +     writel(reg, &regs->status);     /* Clear all SPI events via R/W */
>> >
>> > are these R1C or W1C bits ?  if the latter, you could just write -1 and
>> > avoid the read altogether ...
>>
>> The next line is:
>>
>>       debug("spi_xfer entry: STATUS = %08x\n", reg);
>>
>> and I didn't want to remove that, so I need to keep the read
>> unfortunately. It could perhaps be this if you are keen:
>>
>> writel(-1, &regs->status);     /* Clear all SPI events via R/W */
>> debug("spi_xfer entry: STATUS = %08x\n", readl(&reg->status));
>
> what you have now is fine if you prefer it that way
> -mike
>

OK - I will leave it. I have already modified Tom's driver more than is polite.

Regards,
Simon