Re: [PATCH] can: grcan: Add device driver for GRCAN and GRHCAN cores

[Andreas Larsson <andreas@gaisler.com>]

On 2012-10-11 13:28, Marc Kleine-Budde wrote:
> On 10/11/2012 01:22 PM, Andreas Larsson wrote:
>> On 2012-10-11 12:04, Marc Kleine-Budde wrote:
>>> On 10/04/2012 11:45 AM, Marc Kleine-Budde wrote:
>>>> Hello,
>>>>
>>>> On 10/02/2012 04:38 PM, Andreas Larsson wrote:
>>>>> This driver supports GRCAN and CRHCAN CAN controllers available in
>>>>> the GRLIB
>>>>> VHDL IP core library.
>>>>>
>>>>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>>>>> ---
>>>>>    drivers/net/can/Kconfig  |    6 +
>>>>>    drivers/net/can/Makefile |    1 +
>>>>>    drivers/net/can/grcan.c  | 1283
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    drivers/net/can/grcan.h  |  273 ++++++++++
>>>>>    4 files changed, 1563 insertions(+), 0 deletions(-)
>>>>>    create mode 100644 drivers/net/can/grcan.c
>>>>>    create mode 100644 drivers/net/can/grcan.h
>>>>
>>>> Some general remarks while scrolling though the driver:
>>>> - please provide a device tree binding documentation
>>>>     Documentation/devicetree/bindings/net/can/grcan.txt
>>>>     and add devicetree-discuss@lists.ozlabs.org on Cc
>>>> - please provide a documentation for the sysfs entries:
>>>>     Documentation/ABI/testing/sys-XXX-grcan.txt
>>>> - please implement a NAPI
>>>> - please include the .h file into the C code,
>>>>     as the C code is the one and only user of the .h file
>>>> - please give every macro a GRCAN_ prefix
>>>
>>> Your driver should work on little as well as on big endian systems. You
>>> need probably two implementations of the grcan_{read,write}_reg
>>> functions, you can take the flexcan driver [1] as an example.
>>
>> Adding that is easy of course, but that gives the impression of support
>> for little endian systems. If the core would be instantiated in a little
>> endian fashion in a little endian environment, the DMA accessing parts
>> of the hardware would still not work in a little endian system without
>> modifications to the hardware design.
>
> Can you add this to the Kconfig help text.
>
>>  From the driver point of view either way works for me, but I am not sure
>> what is best.
>
> In this case you should limit the driver to big endian systems via kconfig.

That would of course be nice. One problem with that is that there seems 
to be no Kconfig configuration option to depend on to match big endian 
systems in general.

One solution is to depend on SPARC (the standard environment of the 
core) and having anyone instantiating the core outside of a sparc 
environment patch his or her kernel.

Another solution is of course to do the ifdef on endianness solution 
like in flexcan and have a note in Kconfig text about the hardware not 
being compatible with little endian in an unchanged fashion.

I am not comfortable to try to come up with some hairy dependency that 
tries to catch all kinds of architectures that might or might not be 
configured big endian at compile time.

Cheers,
Andreas