[U-Boot] Remove board specific code from ENC28J60 network driver?

[Ben Warren]

Hi Dirk,

On Sun, Dec 27, 2009 at 10:55 AM, Dirk Behme <dirk.behme at googlemail.com>wrote:

> On 27.12.2009 16:32, Ben Warren wrote:
>
>> Hi Dirk,
>>
>> On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behme<dirk.behme at googlemail.com
>> >wrote:
>>
>>  On 26.12.2009 19:40, Mike Frysinger wrote:
>>>
>>>> On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
>>>>
>>>>> I started to convert the enc28j60.c to common SPI framework. Do you
>>>>> like to have a look at attachment (and maybe test it?)?
>>>>>
>>>>> It is compile tested only. And for the moment it just re-uses the
>>>>> existing driver. When we know that it basically works this way, doing
>>>>> it in a clean way as you describe above would be the next step.
>>>>> CONFIG_NET_MULTI is still missing, too.
>>>>>
>>>>
>>>> spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.
>>>>
>>>  this
>>>
>>>> isnt so much an "interrupts" issue as the process of claiming the bus
>>>> reprograms the controller with the slave settings.
>>>>
>>>>  In your config file you have to set and configure
>>>>>
>>>>> #define CONFIG_xxx_SPI
>>>>> #define CONFIG_ENC28J60
>>>>> #define CONFIG_ENC28J60_SPI_BUS              0
>>>>> #define CONFIG_ENC28J60_SPI_CS               0
>>>>> #define CONFIG_ENC28J60_SPI_CLK              1000000
>>>>> #define CONFIG_CMD_NET
>>>>>
>>>>> for your board.
>>>>>
>>>>
>>>> this is ok with the current design, but broken for NET_MULTI.  when
>>>>
>>> converted
>>>
>>>> to NET_MULTI, the new enc28j60_register() function will take the spi
>>>>
>>> settings
>>>
>>>> as function arguments.  so the function would look something like:
>>>> int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int
>>>>
>>> spi_cs,
>>>
>>>> unsigned int max_hz, unsigned int mode);
>>>>
>>>> and it'd be up to the board to call it with the settings it wants
>>>>
>>>
>>> Both changes, enc28j60_initialize() (NET_MULTI enabled) and
>>> spi_claim_bus/spi_release_bus done in below.
>>>
>>> In the the board file I now have
>>>
>>> int board_eth_init(bd_t *bis)
>>> {
>>>        int rc = 0;
>>> #ifdef CONFIG_ENC28J60
>>>        rc = enc28j60_initialize(bis,
>>>                                 CONFIG_ENC28J60_SPI_BUS,
>>>                                 CONFIG_ENC28J60_SPI_CS,
>>>                                 CONFIG_ENC28J60_SPI_CLK,
>>>                                 SPI_MODE_3);
>>> #endif
>>>        return rc;
>>> }
>>>
>>> This is the right way to do it.  Thanks for taking on this task.  One
>>>
>> comment is that I'm not sure you need to pass 'bis' to the initialize()
>> function.
>>
>
> From functionality point of view it's not needed. The API was proposed by
> Mike this way. But yes, 'bis' can be removed.
>
>
>  Do you like to test? Any further comments?
>>>
>>>  I'm sure you know that converting to CONFIG_NET_MULTI involves a lot
>> more
>> than renaming the initialize() functions.
>>
>
> Could you give some examples or hints, what you like to see? Just in case I
> missed the details ;)
>
> As Mike mentioned, there's documentation.  If you'd like a simple,
properly-done driver as a reference, have a look at drivers/net/cs8900.c

>
>  Looking forward to the full
>> version.
>>
>
> First I'd like to hear if it basically works for someone. If not, it makes
> no sense to go on ;)
>
>
>  I wish I had hardware to help you, but will be happy to review
>> once you're ready.
>>
>
> Mike mentioned that he eventually could test it on Blackfin boards.
>
> Many thanks for review,
>
> Dirk
>
regards,
Ben