[U-Boot] [PATCH v3 05/29] dm: spi: Add a uclass for SPI

[Simon Glass]

Hi Jagan,

On 10 October 2014 12:37, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On 10 October 2014 23:35, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> Can you please see my comments below, I'm relating more about cs and bus
>>> valid scenario which is available on current drivers wrt dm-spi.
>>>
>>> On 30 September 2014 01:05, Simon Glass <sjg at chromium.org> wrote:
>>>> Add a uclass which provides access to SPI buses and includes operations
>>>> required by SPI.
>>>>
>>>> For a time driver model will need to co-exist with the legacy SPI interface
>>>> so some parts of the header file are changed depending on which is in use.
>>>> The exports are adjusted also since some functions are not available with
>>>> driver model.
>>>>
>>>> Boards must define CONFIG_DM_SPI to use driver model for SPI.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Add a cs_info() method to the driver model SPI API
>>>
>>> I think this gives the enough info regarding cs on particular controller what
>>> about the bus usage - which is also specific to controller it self, right?
>>>
>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>
>>> And also I saw tegra20_sflash.c validates only cs
>>>
>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>                            struct spi_cs_info *info)
>>>
>>> What about bus number check this particular tegra spi controller is concern.
>>
>> The bus device is passed in as a pointer. The bus number gets looked
>> up to find the device for that bus number. See spi_get_bus_and_cs()
>> for that.
>>
>> This is good because drivers no longer need to validate the bus number.
>
> Agreed, but for controlling "sf probe bus:cs " options bus number
> should get from
> the respective driver.

I wonder if the issue here is a difference of understanding about bus
and chip select. In my mind, bus is just a device. We use the bus
number to specify which device we mean, but it is just a a
convenience. If there were some other way of finding the device we
would use it (as we do with device tree, for example).

Similarly, chip select is just a device. We look at the bus device,
find the appropriate chip select number, and end up with a child
device which is referred to by that chip select.

>
> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
> we have a function calls for uclass_get_device_by_seq() and
> spi_find_bus_and_cs()
> along with that ops->cs_info(bus, cs, info) by adding bus number check
> for the driver.
>
> So-that there is no requirement to call spi_cs_is_valid from any other location
> as bus and cs validates through commands itself.

At present spi_cs_info() calls spi_find_chip_select(). I think I see
what you are getting at - you don't want a device to be created unless
cs_info() allows it, right? I will take a look.

>
> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?

Only if someone calls spi_cs_info(). I don't think there are any
callers at present. With tegra using device tree I'm not sure it would
have any purpose - the chip selects and buses are all known.

>
>>
>>>
>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>> - Add missing comments to spi.h
>>>> - Correct typo where 'slave' should say 'bus'
>>>> - Fix two comment typos
>>>> - Put the cs member back into spi_slave
>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>
>>>> Changes in v2:
>>>> - Add missing comments for struct spi_slave
>>>> - Fix code nits from Daniel Schwierzeck
>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>
>>>>  common/exports.c         |   4 +-
>>>>  drivers/spi/Makefile     |   4 +
>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/dm/uclass-id.h   |   2 +
>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>
>>
>>
>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>
>>> Who is the caller for this?
>>
>> This can be called by boards, or anywhere really. It replaces the
>> function with the same purpose in the old SPI framework.
>
> Please see above.
>
OK

Regards,
Simon