[PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"

On 11.06.2014 20:23, Naveen Krishna Ch wrote:

Hello Tomasz,

On 11 June 2014 23:20, Tomasz Figa [off-list ref] wrote:

quoted

On 11.06.2014 19:27, Javier Martinez Canillas wrote:

quoted

On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote:

quoted

On 11 June 2014 16:43, Javier Martinez Canillas
[off-list ref] wrote:

quoted

On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:

[snip]

quoted

quoted

quoted

quoted

              return ERR_PTR(-EINVAL);
      }
+     cs->line = spi->cs_gpio;

I wonder why are you keeping cs->line? AFAICT it's only used in
s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device
pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

I see, then I prefer the opposite and do something like this on s3c64xx_spi_probe():

if (!pdev->dev.of_node)
       spi->cs_gpio = cs->line;

Hmm, as far as I understand, spi here is spi_device, not spi_master. I
don't think you have access to SPI devices on your bus in controller
probe().

What I think could work is reworking the driver to:

- in DT case, don't do anything in the driver about the GPIO chip
select, because it will be handled automatically by the core.

But, i see that gpio_request_one is needed in _setup function in the driver.
Except that no other gpio related operation is needed in the driver.

This should be handled by SPI core as well, but apparently it isn't. So
the scheme of work in s3c64xx_spi_setup() changes as in pseudocode below:

	if (!DT)
		spi->cs_gpio = cs->line;

	if (gpio_is_valid(spi->cs_gpio))
		gpio_request_one(spi->cs_gpio);

and in s3c64xx_spi_cleanup():

	if (gpio_is_valid(spi->cs_gpio))
		gpio_free(spi->cs_gpio);

	if (!DT)
		spi->cs_gpio = -ENOENT;

quoted

- in non-DT case, in s3c64xx_spi_setup(), just take the GPIO pin from
s3c64xx_spi_csinfo struct passed through spi->controller_data, request
it and save it to spi->cs_gpio,

- in non-DT case, in s3c64xx_spi_cleanup(), free the GPIO requested in
s3c64xx_spi_setup() and set spi->cs_gpio to -ENOENT (as done initially
in spi_alloc_device()).

Its better going back to the working way.

Hmm?

Best regards,
Tomasz