| Message ID | 20220602053250.62593-4-chanho61.park@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/6] dt-bindings: phy: samsung,ufs-phy: make pmu-syscon as phandle-array | expand |
On 02/06/2022 07:32, Chanho Park wrote: > To support secondary ufs phy device, we need to get an offset for phy > isolation from the syscon DT node. If the first index argument of the > node is existing, we can read the offset value and set it as isol->offset. > To allow this assignment, we need to change the field so the isol data > needs to be allocated and copied from drvdata. > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > --- > drivers/phy/samsung/phy-samsung-ufs.c | 14 +++++++++++++- > drivers/phy/samsung/phy-samsung-ufs.h | 2 +- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/phy/samsung/phy-samsung-ufs.c b/drivers/phy/samsung/phy-samsung-ufs.c > index b7ddb91a02be..7716b6dc955b 100644 > --- a/drivers/phy/samsung/phy-samsung-ufs.c > +++ b/drivers/phy/samsung/phy-samsung-ufs.c > @@ -288,6 +288,7 @@ static int samsung_ufs_phy_probe(struct platform_device *pdev) > struct phy *gen_phy; > struct phy_provider *phy_provider; > const struct samsung_ufs_phy_drvdata *drvdata; > + u32 isol_offset; > int err = 0; > > match = of_match_node(samsung_ufs_phy_match, dev->of_node); > @@ -324,11 +325,22 @@ static int samsung_ufs_phy_probe(struct platform_device *pdev) > goto out; > } > > + phy->isol = devm_kzalloc(dev, sizeof(struct pmu_isol), GFP_KERNEL); 1. Looks like devm_kmemdup 2. sizeof(*variable), not sizeof(struct) 3. and actually you can simplify all that by storing struct pmu_isol directly in struct samsung_ufs_phy, not as pointer. After all that storing drvdata in samsung_ufs_phy does not make any sense - only one field is left still used (has_symbol_clk), so this should be simplified here as well. > + if (!phy->isol) { > + err = -ENOMEM; > + goto out; > + } > + > drvdata = match->data; > phy->dev = dev; > phy->drvdata = drvdata; > phy->cfg = drvdata->cfg; > - phy->isol = &drvdata->isol; > + memcpy(phy->isol, &drvdata->isol, sizeof(struct pmu_isol)); > + > + if (!of_property_read_u32_index(dev->of_node, "samsung,pmu-syscon", 1, > + &isol_offset)) > + phy->isol->offset = isol_offset; > + > phy->lane_cnt = PHY_DEF_LANE_CNT; > > phy_set_drvdata(gen_phy, phy); > diff --git a/drivers/phy/samsung/phy-samsung-ufs.h b/drivers/phy/samsung/phy-samsung-ufs.h > index 8c3696b3c5ec..d3e1c2016b27 100644 > --- a/drivers/phy/samsung/phy-samsung-ufs.h > +++ b/drivers/phy/samsung/phy-samsung-ufs.h > @@ -122,7 +122,7 @@ struct samsung_ufs_phy { > struct clk *rx1_symbol_clk; > const struct samsung_ufs_phy_drvdata *drvdata; > const struct samsung_ufs_phy_cfg **cfg; > - const struct pmu_isol *isol; > + struct pmu_isol *isol; > u8 lane_cnt; > int ufs_phy_state; > enum phy_mode mode; Best regards, Krzysztof
> > + phy->isol = devm_kzalloc(dev, sizeof(struct pmu_isol), GFP_KERNEL); > > 1. Looks like devm_kmemdup > 2. sizeof(*variable), not sizeof(struct) > > 3. and actually you can simplify all that by storing struct pmu_isol > directly in struct samsung_ufs_phy, not as pointer. > > After all that storing drvdata in samsung_ufs_phy does not make any sense > - only one field is left still used (has_symbol_clk), so this should be > simplified here as well. It would be better storing struct pmu_isol directly and remove drvdata from samsung_ufs_phy as you suggested. Furthermore, definition of struct pmu_isol looks odd in the definition of samsung_ufs_phy_drvdata. So, I'll pop it out and change the naming to "struct samsung_ufs_phy_pmu_isol" Best Regards, Chanho Park
diff --git a/drivers/phy/samsung/phy-samsung-ufs.c b/drivers/phy/samsung/phy-samsung-ufs.c index b7ddb91a02be..7716b6dc955b 100644 --- a/drivers/phy/samsung/phy-samsung-ufs.c +++ b/drivers/phy/samsung/phy-samsung-ufs.c @@ -288,6 +288,7 @@ static int samsung_ufs_phy_probe(struct platform_device *pdev) struct phy *gen_phy; struct phy_provider *phy_provider; const struct samsung_ufs_phy_drvdata *drvdata; + u32 isol_offset; int err = 0; match = of_match_node(samsung_ufs_phy_match, dev->of_node); @@ -324,11 +325,22 @@ static int samsung_ufs_phy_probe(struct platform_device *pdev) goto out; } + phy->isol = devm_kzalloc(dev, sizeof(struct pmu_isol), GFP_KERNEL); + if (!phy->isol) { + err = -ENOMEM; + goto out; + } + drvdata = match->data; phy->dev = dev; phy->drvdata = drvdata; phy->cfg = drvdata->cfg; - phy->isol = &drvdata->isol; + memcpy(phy->isol, &drvdata->isol, sizeof(struct pmu_isol)); + + if (!of_property_read_u32_index(dev->of_node, "samsung,pmu-syscon", 1, + &isol_offset)) + phy->isol->offset = isol_offset; + phy->lane_cnt = PHY_DEF_LANE_CNT; phy_set_drvdata(gen_phy, phy); diff --git a/drivers/phy/samsung/phy-samsung-ufs.h b/drivers/phy/samsung/phy-samsung-ufs.h index 8c3696b3c5ec..d3e1c2016b27 100644 --- a/drivers/phy/samsung/phy-samsung-ufs.h +++ b/drivers/phy/samsung/phy-samsung-ufs.h @@ -122,7 +122,7 @@ struct samsung_ufs_phy { struct clk *rx1_symbol_clk; const struct samsung_ufs_phy_drvdata *drvdata; const struct samsung_ufs_phy_cfg **cfg; - const struct pmu_isol *isol; + struct pmu_isol *isol; u8 lane_cnt; int ufs_phy_state; enum phy_mode mode;
To support secondary ufs phy device, we need to get an offset for phy isolation from the syscon DT node. If the first index argument of the node is existing, we can read the offset value and set it as isol->offset. To allow this assignment, we need to change the field so the isol data needs to be allocated and copied from drvdata. Signed-off-by: Chanho Park <chanho61.park@samsung.com> --- drivers/phy/samsung/phy-samsung-ufs.c | 14 +++++++++++++- drivers/phy/samsung/phy-samsung-ufs.h | 2 +- 2 files changed, 14 insertions(+), 2 deletions(-)