Re: [PATCH v2 2/2] devicetree: Add devicetree bindings documentation for Cadence SPI

[Michal Simek]

On 04/04/2014 10:09 AM, Geert Uytterhoeven wrote:
> Hi Harini,
> 
> On Fri, Apr 4, 2014 at 5:00 AM, Harini Katakam
> <harinikatakamlinux@xxxxxxxxx> wrote:
>>>> +             num-cs = /bits/ 16 <4>;
>>>
>>> What's going on with the /bits/ - is this something that's required for
>>> the property?
>>
>> The master->num-chipselect property is 16 bit but writing <4> here directly
>> leads to 0 being read in of_property_read (because it's big endian).
>> Instead using of property read u32 and then copying, we decided to do this.
>> This was discussed on v2 between Michal and Rob:
>>>>>> +               num-chip-select = /bits/ 16 <4>;
>>>>
>>>> I was expecting you will comment this a little bit. :-)
>>>> Because all just reading this num-cs as 32bit and then
>>>> assigning this value to master->num_chipselect which is 16bit.
>>>
>>> Well, everyone else has that problem then. Obviously it takes a bit
>>> more care than just reading into a u32, but that is a kernel problem
>>> and not a problem of the binding.
>> They are not reading it directly with read_u32 but they are using
>> intermediate u32 value which is assigned to u16 which is fine.
>> This pattern is in most drivers(maybe all).
>> The point is if binding should or can't simplify driver code.
>> And from your reaction above I expect that it is up to driver
>> owner and binding doc how you want to do it.
> 
> IMHO this "/bits/ 16" doesn't simplify the binding.
> 
> As "num-cs" is a generic spi subsystem binding, it should not be
> restricted to 16 bits for the sake of a driver. As your hardware can drive 4
> chip selects, you could represent it in 3 bits (don't!).
> 
> Simple integers are 32 bit in DT, so use a temporary.

No problem to keep it there to 32bit range. I really appreciate
that discussion we have about it.

Just a note: If "num-cs" is the part of generic spi subsystem binding
maybe it should be moved directly to spi core as is done for
example for timeout-sec in watchdog.
Also this should be listed in any Documentation/devicetree/bindings/spi/spi.txt
file that this is generic spi binding.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachment: signature.asc
Description: OpenPGP digital signature