| Message ID | 20241206103401.1780416-3-heiko@sntech.de |
|---|---|
| State | New |
| Headers | show |
| Series | phy: phy-rockchip-samsung-hdptx: don't use of-alias | expand |
On 12/6/24 12:34 PM, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > The phy needs to know its identity in the system (phy0 or phy1 on rk3588) > for some actions and the driver currently contains code abusing of_alias > for that. > > Devicetree aliases are always optional and should not be used for core > device functionality, so instead keep a list of phys on a soc in the > of_device_data and find the phy-id by comparing against the mapped > register-base. > > Fixes: c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock provider support") > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > --- > .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 50 ++++++++++++++++--- > 1 file changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > index c5c64c209e96..b137f8c4d157 100644 > --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > @@ -385,11 +385,22 @@ enum rk_hdptx_reset { > RST_MAX > }; [...] > + > + /* find the phy-id from the io address */ > + hdptx->phy_id = -ENODEV; > + for (id = 0; id < hdptx->cfgs->num_phys; id++) { > + if (res->start == hdptx->cfgs->phy_ids[id]) { > + hdptx->phy_id = id; > + break; > + } > + } > + > + if (hdptx->phy_id < 0) > + return dev_err_probe(dev, -ENODEV, "no matching device found\n"); Maybe we could simply fallback to assume phy1 doesn't exist in this case, which avoids the need to provide a match data with a single entry. Regardless, Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Hi Cristian, Am Freitag, 6. Dezember 2024, 12:26:35 CET schrieb Cristian Ciocaltea: > On 12/6/24 12:34 PM, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > > > The phy needs to know its identity in the system (phy0 or phy1 on rk3588) > > for some actions and the driver currently contains code abusing of_alias > > for that. > > > > Devicetree aliases are always optional and should not be used for core > > device functionality, so instead keep a list of phys on a soc in the > > of_device_data and find the phy-id by comparing against the mapped > > register-base. > > > > Fixes: c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock provider support") > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > > --- > > .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 50 ++++++++++++++++--- > > 1 file changed, 44 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > > index c5c64c209e96..b137f8c4d157 100644 > > --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > > +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c > > @@ -385,11 +385,22 @@ enum rk_hdptx_reset { > > RST_MAX > > }; > > [...] > > > + > > + /* find the phy-id from the io address */ > > + hdptx->phy_id = -ENODEV; > > + for (id = 0; id < hdptx->cfgs->num_phys; id++) { > > + if (res->start == hdptx->cfgs->phy_ids[id]) { > > + hdptx->phy_id = id; > > + break; > > + } > > + } > > + > > + if (hdptx->phy_id < 0) > > + return dev_err_probe(dev, -ENODEV, "no matching device found\n"); > > Maybe we could simply fallback to assume phy1 doesn't exist in this > case, which avoids the need to provide a match data with a single entry. Personally I'm a fan of consistent behaviour, not things working accidentially ;-) . See the usbdp phy for example, also declaring just one phy using the same mechanism. Also I really don't trust the hdptxphy-grf being stable over time. Rockchip engineers always move bts around in those, so there will be a need for platform-data at some point anyway. > Regardless, > > Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> thanks :-) Heiko
diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c index c5c64c209e96..b137f8c4d157 100644 --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c @@ -385,11 +385,22 @@ enum rk_hdptx_reset { RST_MAX }; +#define MAX_HDPTX_PHY_NUM 2 + +struct rk_hdptx_phy_cfg { + unsigned int num_phys; + unsigned int phy_ids[MAX_HDPTX_PHY_NUM]; +}; + struct rk_hdptx_phy { struct device *dev; struct regmap *regmap; struct regmap *grf; + /* PHY const config */ + const struct rk_hdptx_phy_cfg *cfgs; + int phy_id; + struct phy *phy; struct phy_config *phy_cfg; struct clk_bulk_data *clks; @@ -1857,15 +1868,14 @@ static int rk_hdptx_phy_clk_register(struct rk_hdptx_phy *hdptx) struct device *dev = hdptx->dev; const char *name, *pname; struct clk *refclk; - int ret, id; + int ret; refclk = devm_clk_get(dev, "ref"); if (IS_ERR(refclk)) return dev_err_probe(dev, PTR_ERR(refclk), "Failed to get ref clock\n"); - id = of_alias_get_id(dev->of_node, "hdptxphy"); - name = id > 0 ? "clk_hdmiphy_pixel1" : "clk_hdmiphy_pixel0"; + name = hdptx->phy_id > 0 ? "clk_hdmiphy_pixel1" : "clk_hdmiphy_pixel0"; pname = __clk_get_name(refclk); hdptx->hw.init = CLK_HW_INIT(name, pname, &hdptx_phy_clk_ops, @@ -1908,8 +1918,9 @@ static int rk_hdptx_phy_probe(struct platform_device *pdev) struct phy_provider *phy_provider; struct device *dev = &pdev->dev; struct rk_hdptx_phy *hdptx; + struct resource *res; void __iomem *regs; - int ret; + int ret, id; hdptx = devm_kzalloc(dev, sizeof(*hdptx), GFP_KERNEL); if (!hdptx) @@ -1917,11 +1928,27 @@ static int rk_hdptx_phy_probe(struct platform_device *pdev) hdptx->dev = dev; - regs = devm_platform_ioremap_resource(pdev, 0); + regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); if (IS_ERR(regs)) return dev_err_probe(dev, PTR_ERR(regs), "Failed to ioremap resource\n"); + hdptx->cfgs = device_get_match_data(dev); + if (!hdptx->cfgs) + return dev_err_probe(dev, -EINVAL, "missing match data\n"); + + /* find the phy-id from the io address */ + hdptx->phy_id = -ENODEV; + for (id = 0; id < hdptx->cfgs->num_phys; id++) { + if (res->start == hdptx->cfgs->phy_ids[id]) { + hdptx->phy_id = id; + break; + } + } + + if (hdptx->phy_id < 0) + return dev_err_probe(dev, -ENODEV, "no matching device found\n"); + ret = devm_clk_bulk_get_all(dev, &hdptx->clks); if (ret < 0) return dev_err_probe(dev, ret, "Failed to get clocks\n"); @@ -1981,8 +2008,19 @@ static const struct dev_pm_ops rk_hdptx_phy_pm_ops = { rk_hdptx_phy_runtime_resume, NULL) }; +static const struct rk_hdptx_phy_cfg rk3588_hdptx_phy_cfgs = { + .num_phys = 2, + .phy_ids = { + 0xfed60000, + 0xfed70000, + }, +}; + static const struct of_device_id rk_hdptx_phy_of_match[] = { - { .compatible = "rockchip,rk3588-hdptx-phy", }, + { + .compatible = "rockchip,rk3588-hdptx-phy", + .data = &rk3588_hdptx_phy_cfgs + }, {} }; MODULE_DEVICE_TABLE(of, rk_hdptx_phy_of_match);