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

[Mike Frysinger]

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 ;).

> >> +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.

> >> +     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

signature.asc
Description: This is a digitally signed message part.

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot