[02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode
diff mbox

Message ID 1386350983-13281-3-git-send-email-wens@csie.org
State New, archived
Headers show

Commit Message

Chen-Yu Tsai Dec. 6, 2013, 5:29 p.m. UTC
"snps,force_sf_dma_mode" is documented in stmmac device tree bindings,
but is never handled by the driver.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 +
 1 file changed, 1 insertion(+)

Comments

David Miller Dec. 6, 2013, 9:26 p.m. UTC | #1
From: Chen-Yu Tsai <wens@csie.org>
Date: Sat,  7 Dec 2013 01:29:35 +0800

> @@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
>  		plat->bus_id = 0;
>  
>  	of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
> +	plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");

Will this do the right thing for when the property is not present?
Right now the force_sf_dma_mode value is always false.

In fact won't it override the explicit settings done elsewhere in the
driver?
Chen-Yu Tsai Dec. 7, 2013, 1:23 a.m. UTC | #2
Resending reply due to incorrect MIME type.

On Sat, Dec 7, 2013 at 5:26 AM, David Miller <davem@davemloft.net> wrote:
>
> From: Chen-Yu Tsai <wens@csie.org>
> Date: Sat,  7 Dec 2013 01:29:35 +0800
>
> > @@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
> >               plat->bus_id = 0;
> >
> >       of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
> > +     plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");
>
> Will this do the right thing for when the property is not present?
> Right now the force_sf_dma_mode value is always false.

of_property_read_bool will return false when the property is not present.

> In fact won't it override the explicit settings done elsewhere in the
> driver?

Point taken. The current implementation will override settings passed from
platform data. ORing the two would be better.


Thanks
Maxime Ripard Dec. 7, 2013, 10:07 a.m. UTC | #3
On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> Point taken. The current implementation will override settings passed from
> platform data. ORing the two would be better.

Platform_data and DT-based configuration are pretty unlikely to be
used together, so ORing it doesn't have much sense.

Maxime
Tomasz Figa Dec. 7, 2013, 11:06 a.m. UTC | #4
On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> > Point taken. The current implementation will override settings passed from
> > platform data. ORing the two would be better.
> 
> Platform_data and DT-based configuration are pretty unlikely to be
> used together, so ORing it doesn't have much sense.

In fact, the recommended way is to always use platform data alone if it is
present or try to parse DT otherwise, so no mixing of data from these two
sources should be done.

Best regards,
Tomasz
Chen-Yu Tsai Dec. 9, 2013, 2:59 a.m. UTC | #5
On Sat, Dec 7, 2013 at 7:06 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
>> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
>> > Point taken. The current implementation will override settings passed from
>> > platform data. ORing the two would be better.
>>
>> Platform_data and DT-based configuration are pretty unlikely to be
>> used together, so ORing it doesn't have much sense.
>
> In fact, the recommended way is to always use platform data alone if it is
> present or try to parse DT otherwise, so no mixing of data from these two
> sources should be done.

Would binding platform data with compatibles, as I did so in this patch
series, be a bad idea then?
Maxime Ripard Dec. 10, 2013, 8:10 p.m. UTC | #6
On Mon, Dec 09, 2013 at 10:59:38AM +0800, Chen-Yu Tsai wrote:
> On Sat, Dec 7, 2013 at 7:06 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
> >> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> >> > Point taken. The current implementation will override settings passed from
> >> > platform data. ORing the two would be better.
> >>
> >> Platform_data and DT-based configuration are pretty unlikely to be
> >> used together, so ORing it doesn't have much sense.
> >
> > In fact, the recommended way is to always use platform data alone if it is
> > present or try to parse DT otherwise, so no mixing of data from these two
> > sources should be done.
> 
> Would binding platform data with compatibles, as I did so in this patch
> series, be a bad idea then?

What I meant was that you'll either be probed will pdev->dev.of_node
or pdev->dev.platform_data filled, but not both at the same time, so
ORing it isn't really sensible.

But I don't see why you couldn't reuse the stmmac_platform_data
structure in your patch.

Maxime

Patch
diff mbox

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 51c9069..74c7aef 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -47,6 +47,7 @@  static int stmmac_probe_config_dt(struct platform_device *pdev,
 		plat->bus_id = 0;
 
 	of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
+	plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");
 
 	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
 					   sizeof(struct stmmac_mdio_bus_data),