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

[Jagan Teki]

Hi Simon,

On 11 October 2014 04:56, Simon Glass <sjg at chromium.org> wrote:
> 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).

Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
so the respective bus get's bind'ed till that fine. But as we use runtime
probe using "sf probe bus:cs" where I would mention
# sf probe 2:0
selecting spi2 which is of third bus on the same controller driver will that
spi_cs_is_valid() will show spi2 is invalid for this particular driver?

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

Just like validating supported cs, through cs_info I assume the same
works for bus as well as this is a pure driver specific theory.

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

Yes ie my point, along with that instead of a separate caller for spi_cs_info()
why can't we call in the probe time, either from command or at the time of
bus_cs binding.

Once the bus_cs binding done, and validate the mentioned bus and cs from
command or devicetree and return.

Does this make sense, please comment?

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

Yes devicetree will help to take but validating should be require as driver
only knows how many bus's with respective supported chip-selects based
on this command user may verify the same. what do think?

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

thanks!
-- 
Jagan.