Eric Miao wrote:
>>> +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?
ON MORE CAREFUL THOUGHT (sorry): I missed something; I was thrown off
track by your original "stumped" comment, above. I thought your
original concern was that you could only maintain one gpio_cs at a time,
and that is what cannot work. I forgot that you are maintaining one
gpio_cs per chip, which is right, and is the way cs_control is managed.
In light of that, it seems like your original "stumped" is easily dealt
with by only freeing chip->gpio_cs if it does not match the new GPIO value:
if (chip->gpio_cs != chip_info->gpio_cs
|| !chip_info->cs_control)
if (gpio_is_valid(chip->gpio_cs))
gpio_free(chip->gpio_cs);
to replace the next two lines. Of course, the two ifs can be combined
if the call to gpio_is_valid is not expensive.
By the way, I am not familiar with these gpio_ calls. Is gpio_is_valid
really the right call here, or is there something like
gpio_is_allocated. Since you call gpio_is_valid on an unallocated GPIO,
below, the call would succeed on an unallocated GPIO, and I wonder if
freeing an unallocated GPIO is legal.
>>> + 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.
Oops. What I was thinking is that it is not an error for
chip_info->gpio_cs to remain in its initialized state of -1. You are
right, of course, that to request a specific pin that can't be allocated
is an error. And it is too late at night, so I missed the logic that
err remains zero if gpio_is_valid fails. Sorry.
>>> + 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.
Oops, I keep forgetting that. I was looking at the auto declaration.
>
>>> + 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.
I guess that is right, too. I did miss that, but in the back of my mind
I was thinking that you will not end up calling setup_cs() from the
setup() routine (because I was arguing that GPIO allocation does not
belong here).
So now I am whittled down to a suggestion for your "stumped" comment.
Clearly, I should have left this post for tomorrow. :-)
--
Ned Forrester [EMAIL PROTECTED]
Oceanographic Systems Lab 508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079
-------------------------------------------------------------------------
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
