>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>> + struct pxa2xx_spi_chip *chip_info)
>
> My understanding is that it is legal to call setup without a defined
> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
> chip is happy with the defaults (only works for a single chip bus that
> needs no CS, a degenerate case, but still legal). Thus you should allow
> for that by always checking for existence (chip_info not NULL) before use.
>
OK.
>> +{
>> + int err = 0;
>> +
>> + if (chip_info == NULL)
>> + return 0;
>> +
>> + /* NOTE: setup() can be called multiple times, possibly with
>> + * different chip_info, previously requested GPIO shall be
>> + * released, stumped :(
>> + */
>
> That is why the GPIO allocation function belongs in the controller
> driver, and not in the master driver. The master serves many
> controllers, and only the controller drivers know when they are loaded
> and unloaded, and thus when they need to allocate and initialize a GPIO
> pin and when to release it. Additionally, on a multi-chip bus (which
> may have chips with CS assertion of differing polarity) it may be that
> only the board init code can configure all the CS pins to an idle state
> to get anything on the bus to work at all. That is why cs_control
> shifts the burden of configuring CS pins outside of the master driver.
> I don't think that this function can work as written; perhaps this can
> be converted to a non-static helper function to be called by either
> board init or controller drivers.
>
This is because chip selects are usually done by GPIO in this case,
what about those SPI masters with a couple of CS(s) controlled
completely by the master registers?
>> + if (gpio_is_valid(chip->gpio_cs))
>> + gpio_free(chip->gpio_cs);
>> +
>> + if (chip_info->cs_control) {
>> + chip->cs_control = chip_info->cs_control;
>> + return 0;
>> + }
>> +
>> + if (gpio_is_valid(chip_info->gpio_cs)) {
>> + err = gpio_request(chip_info->gpio_cs, "SPI_CS");
>> + if (err) {
>> + dev_err(&spi->dev, "failed to request CS GPIO%d\n",
>> + chip_info->gpio_cs);
>
> I do not agree that this is an error, though David may argue that it is.
> Feel free to reduce this to KERN_INFO and eliminate the following
> return. It is not illegal to omit the chip select on a one-chip bus,
> when the chip does not use CS; there is no need to allocate and waste a
> pin if it is not connected to anything.
Well, if chip->gpio_cs is specified and not able to be requested, this
is obviously a serious error. If someone wants to use NULL CS,
chances are he will not make this "gpio_cs" valid.
>
>> + return err;
>> + }
>> +
>> + chip->gpio_cs = chip_info->gpio_cs;
>> + chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
>> +
>> + gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
>> + }
>> +
>> + return err;
>> +}
>> +
>> static int setup(struct spi_device *spi)
>> {
>> struct pxa2xx_spi_chip *chip_info = NULL;
>> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>> return -ENOMEM;
>> }
>>
>> - chip->cs_control = null_cs_control;
>
> Based on cs_assert/cs_deassert, above, chip->cs_control needs to be
> initialized to 0 here. It's initialization should not be removed.
It's already been initialized to 0 by kzalloc(), explicitly.
>
>> + chip->gpio_cs = -1;
>> chip->enable_dma = 0;
>> chip->timeout = 1000;
>> chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
>> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>> /* chip_info isn't always needed */
>> chip->cr1 = 0;
>> if (chip_info) {
>> - if (chip_info->cs_control)
>> - chip->cs_control = chip_info->cs_control;
>> -
>
> This should not be removed. This is where legacy drivers establish the
> pointer for a cs_control routine.
This is done by setup_cs(), FYI.
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
