[linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Thu, Jan 2, 2014 at 9:11 PM, srinivas kandagatla
<srinivas.kandagatla at st.com> wrote:
> Hi Chen,
>
> On 24/12/13 03:27, Chen-Yu Tsai wrote:
>> Srinivas,
>>
>> Let's keep platform data as of_data, so SoC compatibles can pass
>> hardware feature flags for cores that don't support auto-detection.
>
> I understand your concern here, But making platform_data as of_data
> would just open a wide door for other hacky stuff too. So other glue
> drivers would just use this interface instead, ignoring standard
> interfaces. Also there would be issues on who takes the priority if the
> parameters are specified in multiple places.
>
>
> Can't this field/s be one of the variable in the struct stmmac_of_data
> rather than reusing platform data?

We (sunxi) need .has_gmac and .tx_coe.

To be generic and usable to other SoCs, it seems like I would be
adding most of the fields from platform data. But maybe future
glue layers will only need the callbacks. I can not be sure.

Also since snps,phy-addr should be deprecated, I am thinking of
changing the default phy address to auto-detect, until
Giuseppe merges his phy node support work.

I will post a v2 series this week.

>>
>> We can adjust the callbacks as you suggested, and I added a .free
>> to complement .setup. Driver private data and platform data are
>> off limits to the callbacks. My version of the callbacks:
>>
>>         void (*fix_mac_speed)(void *priv, unsigned int speed);
>>         void (*bus_setup)(void __iomem *ioaddr);
>
> In reply to your question in last email:
> bus_setup this is something very specific to ST which configures the bus
> parameters of AMBA-to-STBUS converter. This register falls inside the
> memory map of stmmac.

I see. IMHO it could be merged into .init?

>
>>         void *(*setup)(struct platform_device *pdev);
>>         void (*free)(struct platform_device *pdev, void *priv);
>>         int (*init)(struct platform_device *pdev, void *priv);
>>         void (*exit)(struct platform_device *pdev, void *priv);
>>
>
> The callbacks to Ok to me, unless Peppe has more comments on this.

Ok.


Cheers
ChenYu