| Message ID | 20220923052828.16581-5-jianguo.zhang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Mediatek ethernet patches for mt8188 | expand |
Il 23/09/22 07:28, Jianguo Zhang ha scritto: > Update the name of property 'clk_csr' as 'snps,clk-csr' to align with > the property name in the binding file. > > Signed-off-by: Jianguo Zhang <jianguo.zhang@mediatek.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 9f5cac4000da..18f9952d667f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -444,7 +444,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > * or get clk_csr from device tree. > */ > plat->clk_csr = -1; > - of_property_read_u32(np, "clk_csr", &plat->clk_csr); > + of_property_read_u32(np, "snps,clk-csr", &plat->clk_csr); This is going to break MT2712e on old devicetrees. The right way of doing that is to check the return value of of_property_read_u32() for "snps,clk-csr": if the property is not found, fall back to the old "clk_csr". Regards, Angelo > > /* "snps,phy-addr" is not a standard property. Mark it as deprecated > * and warn of its use. Remove this when phy node support is added.
On 23/09/2022 11:10, AngeloGioacchino Del Regno wrote: > Il 23/09/22 07:28, Jianguo Zhang ha scritto: >> Update the name of property 'clk_csr' as 'snps,clk-csr' to align with >> the property name in the binding file. >> >> Signed-off-by: Jianguo Zhang <jianguo.zhang@mediatek.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 9f5cac4000da..18f9952d667f 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -444,7 +444,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) >> * or get clk_csr from device tree. >> */ >> plat->clk_csr = -1; >> - of_property_read_u32(np, "clk_csr", &plat->clk_csr); >> + of_property_read_u32(np, "snps,clk-csr", &plat->clk_csr); > > This is going to break MT2712e on old devicetrees. > > The right way of doing that is to check the return value of of_property_read_u32() > for "snps,clk-csr": if the property is not found, fall back to the old "clk_csr". I must admit - I don't care. That's the effect when submitter bypasses DT bindings review (81311c03ab4d ("net: ethernet: stmmac: add management of clk_csr property")). If anyone wants ABI, please document the properties. If out-of-tree users complain, please upstream your DTS or do not use undocumented features... Best regards, Krzysztof
On 23/09/2022 20:14, Krzysztof Kozlowski wrote: >> This is going to break MT2712e on old devicetrees. >> >> The right way of doing that is to check the return value of of_property_read_u32() >> for "snps,clk-csr": if the property is not found, fall back to the old "clk_csr". > > I must admit - I don't care. That's the effect when submitter bypasses > DT bindings review (81311c03ab4d ("net: ethernet: stmmac: add management > of clk_csr property")). > > If anyone wants ABI, please document the properties. > > If out-of-tree users complain, please upstream your DTS or do not use > undocumented features... > OTOH, as Angelo pointed out, handling old and new properties is quite easy to achieve, so... :) Best regards, Krzysztof
Dear Krzysztof, Thanks for your comment. On Fri, 2022-09-23 at 20:15 +0200, Krzysztof Kozlowski wrote: > On 23/09/2022 20:14, Krzysztof Kozlowski wrote: > > > This is going to break MT2712e on old devicetrees. > > > > > > The right way of doing that is to check the return value of > > > of_property_read_u32() > > > for "snps,clk-csr": if the property is not found, fall back to > > > the old "clk_csr". > > > > I must admit - I don't care. That's the effect when submitter > > bypasses > > DT bindings review (81311c03ab4d ("net: ethernet: stmmac: add > > management > > of clk_csr property")). > > > > If anyone wants ABI, please document the properties. > > > > If out-of-tree users complain, please upstream your DTS or do not > > use > > undocumented features... > > > > OTOH, as Angelo pointed out, handling old and new properties is quite > easy to achieve, so... :) > So, the conclusion is as following: 1. add new property 'snps,clk-csr' and document it in binding file. 2. parse new property 'snps,clk-csr' firstly, if failed, fall back to old property 'clk_csr' in driver. Is my understanding correct? > Best regards, > Krzysztof > BRS Jianguo
Dear Krzysztof, Thanks for your comment. On Fri, 2022-09-23 at 20:15 +0200, Krzysztof Kozlowski wrote: > On 23/09/2022 20:14, Krzysztof Kozlowski wrote: > > > This is going to break MT2712e on old devicetrees. > > > > > > The right way of doing that is to check the return value of > > > of_property_read_u32() > > > for "snps,clk-csr": if the property is not found, fall back to > > > the old "clk_csr". > > > > I must admit - I don't care. That's the effect when submitter > > bypasses > > DT bindings review (81311c03ab4d ("net: ethernet: stmmac: add > > management > > of clk_csr property")). > > > > If anyone wants ABI, please document the properties. > > > > If out-of-tree users complain, please upstream your DTS or do not > > use > > undocumented features... > > > > OTOH, as Angelo pointed out, handling old and new properties is quite > easy to achieve, so... :) > So, the conclusion is as following: 1. add new property 'snps,clk-csr' and document it in binding file. 2. parse new property 'snps,clk-csr' firstly, if failed, fall back to old property 'clk_csr' in driver. Is my understanding correct? > Best regards, > Krzysztof > BRS Jianguo
Il 27/09/22 10:44, Jianguo Zhang ha scritto: > Dear Krzysztof, > Thanks for your comment. > > On Fri, 2022-09-23 at 20:15 +0200, Krzysztof Kozlowski wrote: >> On 23/09/2022 20:14, Krzysztof Kozlowski wrote: >>>> This is going to break MT2712e on old devicetrees. >>>> >>>> The right way of doing that is to check the return value of >>>> of_property_read_u32() >>>> for "snps,clk-csr": if the property is not found, fall back to >>>> the old "clk_csr". >>> >>> I must admit - I don't care. That's the effect when submitter >>> bypasses >>> DT bindings review (81311c03ab4d ("net: ethernet: stmmac: add >>> management >>> of clk_csr property")). >>> >>> If anyone wants ABI, please document the properties. >>> >>> If out-of-tree users complain, please upstream your DTS or do not >>> use >>> undocumented features... >>> >> >> OTOH, as Angelo pointed out, handling old and new properties is quite >> easy to achieve, so... :) >> > So, the conclusion is as following: > > 1. add new property 'snps,clk-csr' and document it in binding file. > 2. parse new property 'snps,clk-csr' firstly, if failed, fall back to > old property 'clk_csr' in driver. > > Is my understanding correct? Yes, please. I think that bindings should also get a 'clk_csr' with deprecated: true, but that's Krzysztof's call. Regards, Angelo > >> Best regards, >> Krzysztof >> > BRS > Jianguo >
On 27/09/2022 12:44, AngeloGioacchino Del Regno wrote: >>> OTOH, as Angelo pointed out, handling old and new properties is quite >>> easy to achieve, so... :) >>> >> So, the conclusion is as following: >> >> 1. add new property 'snps,clk-csr' and document it in binding file. >> 2. parse new property 'snps,clk-csr' firstly, if failed, fall back to >> old property 'clk_csr' in driver. >> >> Is my understanding correct? > > Yes, please. > > I think that bindings should also get a 'clk_csr' with deprecated: true, > but that's Krzysztof's call. The property was never documented, so I think we can skip it as deprecated. Best regards, Krzysztof
Dear Krzysztof, Thanks for your comment. On Wed, 2022-09-28 at 09:17 +0200, Krzysztof Kozlowski wrote: > On 27/09/2022 12:44, AngeloGioacchino Del Regno wrote: > > > > > OTOH, as Angelo pointed out, handling old and new properties is > > > > quite > > > > easy to achieve, so... :) > > > > > > > > > > So, the conclusion is as following: > > > > > > 1. add new property 'snps,clk-csr' and document it in binding > > > file. > > > 2. parse new property 'snps,clk-csr' firstly, if failed, fall > > > back to > > > old property 'clk_csr' in driver. > > > > > > Is my understanding correct? > > > > Yes, please. > > > > I think that bindings should also get a 'clk_csr' with deprecated: > > true, > > but that's Krzysztof's call. > > The property was never documented, so I think we can skip it as > deprecated. > We will send next version patches according to the conclusion. > Best regards, > Krzysztof > BRS Jianguo
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 9f5cac4000da..18f9952d667f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -444,7 +444,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) * or get clk_csr from device tree. */ plat->clk_csr = -1; - of_property_read_u32(np, "clk_csr", &plat->clk_csr); + of_property_read_u32(np, "snps,clk-csr", &plat->clk_csr); /* "snps,phy-addr" is not a standard property. Mark it as deprecated * and warn of its use. Remove this when phy node support is added.
Update the name of property 'clk_csr' as 'snps,clk-csr' to align with the property name in the binding file. Signed-off-by: Jianguo Zhang <jianguo.zhang@mediatek.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)