Hello,
Le Thursday 27 August 2009 16:15:53 Sergei Shtylyov, vous avez écrit :
> Hello.
>
> Florian Fainelli wrote:
> >>>>>This patch adds the board code to register a per-board au1000-eth
> >>>>>platform device to be used wit the au1000-eth platform driver in a
> >>>>>subsequent patch. Note that the au1000-eth driver knows about the
> >>>>>default driver settings such that we do not need to pass any
> >>>>>platform_data informations in most cases except db1x00.
> >>>>
> >>>> Sigh, NAK...
> >>>> Please don't register the SoC device per board, do it in
> >>>>alchemy/common/platfrom.c and find a way to pass the board specific
> >>>>platform data from the board file there instead -- something like
> >>>>arch/arm/mach-davinci/usb.c does.
> >>>
> >>>Ok, like I promised, this was the per-board device registration. Do you
> >>>prefer something like this:
> >>
> >> I certainly do, but still not in this incarnation... :-)
> >>
> >>>+static struct au1000_eth_platform_data au1xxx_eth0_platform_data = {
> >>>+ .phy1_search_mac0 = 1,
> >>>+};
> >>
> >> I'm not sure that the default platfrom data is really a great idea...
> >
> > Can you elaborate a bit more ? We actually need to make the Ethernet MAC
> > driver aware of some PHY settings.
>
> But why do you have the platform data in *this* file, no the board
> files?
The default setting is to search for a PHY on the corresponding MAC, which is
the case for all boards but bosporus, thus it is in this file. No platform
data at all would be fine too.
>
> >>>+#ifndef CONFIG_SOC_AU1100
> >>>+static struct platform_device au1xxx_eth1_device = {
> >>>+ .name = "au1000-eth",
> >>>+ .id = 1,
> >>>+ .num_resources = ARRAY_SIZE(au1xxx_eth1_resources),
> >>>+ .resource = au1xxx_eth1_resources,
> >>
> >> And where's the platfrom data for the second Ethernet?
> >
> > There is no need to, as the driver originally did not override any
> > specific settings on the second MAC (afair).
>
> Specific settings where, in the driver? Shouldn't all such settings be
> bound to the platform data instead?
Yes, platform data should handle that for us, what I was trying to explain is
that the driver did not configure anything specific for MAC1 already, thus
there is no platfo
>
> >>>+};
> >>>+#endif
> >>>+
> >>>+void __init au1xxx_override_eth0_cfg(struct au1000_eth_platform_data
> >>>*eth_data) +{
> >>>+ if (!eth_data)
> >>>+ return;
> >>>+
> >>>+ memcpy(&au1xxx_eth0_platform_data, eth_data,
> >>>+ sizeof(struct au1000_eth_platform_data));
> >>
> >> Why not just set the pointer in au1xxx_eth0_device. And really, why
> >> not make the function more generic, with a prototype like:
> >
> > For the same reasons as explained below, MAC1 did not need any specific
> > change.
>
> So, the driver can get away without platform data? What does it do in
> this case -- I haven't looked at that patch?
In that case it searchs for a PHY attached to the MAC, this is what the driver
did already. Please have a look at the patch, specifically the part which
handles a NULL platform_data.
>
> >>void __init au1xxx_override_eth_cfg(unsigned port, struct
> >> au1000_eth_platform_data *eth_data);
> >>
> >>>+}
> >>>+
> >>> static struct platform_device *au1xxx_platform_devices[] __initdata = {
> >>> &au1xx0_uart_device,
> >>> &au1xxx_usb_ohci_device,
> >>>@@ -351,17 +422,25 @@ static struct platform_device
> >>>*au1xxx_platform_devices[] __initdata = { #ifdef SMBUS_PSC_BASE
> >>> &pbdb_smbus_device,
> >>> #endif
> >>>+ &au1xxx_eth0_device,
> >>> };
> >>>
> >>> static int __init au1xxx_platform_init(void)
> >>> {
> >>> unsigned int uartclk = get_au1x00_uart_baud_base() * 16;
> >>>- int i;
> >>>+ int i, ni;
> >>>
> >>> /* Fill up uartclk. */
> >>> for (i = 0; au1x00_uart_data[i].flags; i++)
> >>> au1x00_uart_data[i].uartclk = uartclk;
> >>>
> >>>+ /* Register second MAC if enabled in pinfunc */
> >>>+#ifndef CONFIG_SOC_AU1100
> >>>+ ni = (int)((au_readl(SYS_PINFUNC) & (u32)(SYS_PF_NI2)) >> 4);
> >>>+ if (!(ni + 1))
> >>
> >> Why so complex, and how can (ni + 1) ever be 0?! :-/
> >
> > This is left-over debugging stub, I will rework it. About complexity,
> > this line is taken directly from the au1000_eth driver.
>
> I don't see !(ni + 1) there, only:
>
> num_ifs = NUM_ETH_INTERFACES - ni;
>
> which is correct, unlike what you've written.
--
Best regards, Florian Fainelli
Email: florian@xxxxxxxxxxx
Web: http://openwrt.org
IRC: [florian] on irc.freenode.net
-------------------------------
