From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Mon, 11 Apr 2016 11:18:07 +0530
Subject: [U-Boot] [PATCH] dm: spi: Read default speed,
	mode values from DT
In-Reply-To: <570B2B24.9090903@ti.com>
References: <1460043146-27918-1-git-send-email-vigneshr@ti.com>
 <20160408194546.GM23166@bill-the-cat> <570B2B24.9090903@ti.com>
Message-ID: <570B3A97.5050908@ti.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday 11 April 2016 10:12 AM, Vignesh R wrote:
> 
> 
> On 04/09/2016 01:15 AM, Tom Rini wrote:
>> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote:
>>
>>> In case of DT boot, don't read default speed and mode for SPI from
>>> CONFIG_*, instead read from DT node. This will make sure that boards
>>> with multiple SPI/QSPI controllers can be probed at different
>>> bus frequencies and modes.
>>>
>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>> ---
>>>  cmd/sf.c                 | 2 ++
>>>  drivers/spi/spi-uclass.c | 8 ++++++--
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/sf.c b/cmd/sf.c
>>> index 42862d9d921a..4c40d5104422 100644
>>> --- a/cmd/sf.c
>>> +++ b/cmd/sf.c
>>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>>>  #ifdef CONFIG_DM_SPI_FLASH
>>>  	struct udevice *new, *bus_dev;
>>>  	int ret;
>>> +	/* In DM mode defaults wiil be taken from DT */
>>> +	speed = -1, mode = -1;

speed and mode are uint and it is assigned with -1 which is not making
sense. Assigning 0 will be a better option.

Regards
Mugunthan V N

>>>  #else
>>>  	struct spi_flash *new;
>>>  #endif
>>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
>>> index 5561f36762f9..55ebb5ef19b7 100644
>>> --- a/drivers/spi/spi-uclass.c
>>> +++ b/drivers/spi/spi-uclass.c
>>> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  		       struct udevice **busp, struct spi_slave **devp)
>>>  {
>>>  	struct udevice *bus, *dev;
>>> +	struct dm_spi_slave_platdata *plat;
>>>  	bool created = false;
>>>  	int ret;
>>>  
>>> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  	 * SPI flash chip - we will bind to the correct driver.
>>>  	 */
>>>  	if (ret == -ENODEV && drv_name) {
>>> -		struct dm_spi_slave_platdata *plat;
>>> -
>>>  		debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
>>>  		      __func__, dev_name, busnum, cs, drv_name);
>>>  		ret = device_bind_driver(bus, drv_name, dev_name, &dev);
>>> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  		slave->dev = dev;
>>>  	}
>>>  
>>> +	plat = dev_get_parent_platdata(dev);
>>> +	if (speed == -1)
>>> +		speed = plat->max_hz;
>>> +	if (mode == -1)
>>> +		mode = plat->mode;
>>>  	ret = spi_set_speed_mode(bus, speed, mode);
>>>  	if (ret)
>>>  		goto err;
>>
>> I like the concept but if it can fail, passing -1 for speed/mode seems
>> like a bad idea and we should just fail.
>>
> I didn't quite understand your comment. Could you explain what can fail?
> I don't think mode/speed will be -1 when calling spi_set_speed_mode()
> after above change. speed/mode will either be command line arg passed to
> sf probe cmd or CONFIG_SF_DEFAULT_* in case of non DM driver or
> mode/speed specified in DT node in case of DM.
> 
> 
>