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

[Tomasz Figa <tomasz.figa@gmail.com>]

On 11.06.2014 20:23, Naveen Krishna Ch wrote:
> Hello Tomasz,
> 
> On 11 June 2014 23:20, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 11.06.2014 19:27, Javier Martinez Canillas wrote:
>>> On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote:
>>>> On 11 June 2014 16:43, Javier Martinez Canillas
>>>> <javier.martinez@collabora.co.uk> wrote:
>>>>> On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:
>>
>> [snip]
>>
>>>>>
>>>>>>               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;

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