diff mbox series

[v2,2/2] phy: qualcomm: eusb2-repeater: Rework init to drop redundant zero-out loop

Message ID 20240105-phy-qcom-eusb2-repeater-fixes-v2-2-775d98e7df05@linaro.org
State Changes Requested
Headers show
Series phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework | expand

Commit Message

Abel Vesa Jan. 5, 2024, 10:16 a.m. UTC 
The device match config init table already has zero values, so rework
the container struct to hold a copy of the init table that can be
override be the DT specified values. By doing this, only the number of
vregs remain in the device match config that will be later needed, so
instead of holding the cfg after probe, store the number of vregs in the
container struct.

Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 42 ++++++++++++--------------
 1 file changed, 19 insertions(+), 23 deletions(-)

Comments

Konrad Dybcio Jan. 5, 2024, 11:44 a.m. UTC | #1 
On 5.01.2024 11:16, Abel Vesa wrote:
> The device match config init table already has zero values, so rework
> the container struct to hold a copy of the init table that can be
> override be the DT specified values. By doing this, only the number of
> vregs remain in the device match config that will be later needed, so
> instead of holding the cfg after probe, store the number of vregs in the
> container struct.
> 
> Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

This looks good as-is, though I think my proposal of storing the
peripheral base reg instead is still better, as it'd require less
memory (no kmemdup as the regs wouldn't be modified).

Konrad
Dmitry Baryshkov Jan. 5, 2024, 12:22 p.m. UTC | #2 
On Fri, 5 Jan 2024 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 5.01.2024 11:16, Abel Vesa wrote:
> > The device match config init table already has zero values, so rework
> > the container struct to hold a copy of the init table that can be
> > override be the DT specified values. By doing this, only the number of
> > vregs remain in the device match config that will be later needed, so
> > instead of holding the cfg after probe, store the number of vregs in the
> > container struct.
> >
> > Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
>
> This looks good as-is, though I think my proposal of storing the
> peripheral base reg instead is still better, as it'd require less
> memory (no kmemdup as the regs wouldn't be modified).

I'd second this. We usually handle such cases via the base + offset
rather than patching the data. If regfields can not handle this, then
the regfield should be fixed.
Abel Vesa Jan. 8, 2024, 10:29 a.m. UTC | #3 
On 24-01-05 14:22:50, Dmitry Baryshkov wrote:
> On Fri, 5 Jan 2024 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> > On 5.01.2024 11:16, Abel Vesa wrote:
> > > The device match config init table already has zero values, so rework
> > > the container struct to hold a copy of the init table that can be
> > > override be the DT specified values. By doing this, only the number of
> > > vregs remain in the device match config that will be later needed, so
> > > instead of holding the cfg after probe, store the number of vregs in the
> > > container struct.
> > >
> > > Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> >
> > This looks good as-is, though I think my proposal of storing the
> > peripheral base reg instead is still better, as it'd require less
> > memory (no kmemdup as the regs wouldn't be modified).
> 
> I'd second this. We usually handle such cases via the base + offset
> rather than patching the data. If regfields can not handle this, then
> the regfield should be fixed.
> 

Sure. Will do that instead.

> -- 
> With best wishes
> Dmitry
diff mbox series

Patch

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index 5f5862a68b73..d28106e71ce3 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -102,7 +102,8 @@  struct eusb2_repeater {
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
 	struct reg_field *regfields;
-	const struct eusb2_repeater_cfg *cfg;
+	u32 *init_tbl;
+	int num_vregs;
 	enum phy_mode mode;
 };
 
@@ -123,9 +124,10 @@  static const struct eusb2_repeater_cfg pm8550b_eusb2_cfg = {
 	.num_vregs	= ARRAY_SIZE(pm8550b_vreg_l),
 };
 
-static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
+static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr,
+				     const struct eusb2_repeater_cfg *cfg)
 {
-	int num = rptr->cfg->num_vregs;
+	int num = cfg->num_vregs;
 	struct device *dev = rptr->dev;
 	int i;
 
@@ -134,7 +136,7 @@  static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 		return -ENOMEM;
 
 	for (i = 0; i < num; i++)
-		rptr->vregs[i].supply = rptr->cfg->vreg_list[i];
+		rptr->vregs[i].supply = cfg->vreg_list[i];
 
 	return devm_regulator_bulk_get(dev, num, rptr->vregs);
 }
@@ -142,32 +144,19 @@  static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 static int eusb2_repeater_init(struct phy *phy)
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
-	struct reg_field *regfields = rptr->regfields;
 	struct device_node *np = rptr->dev->of_node;
-	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
+	u32 *init_tbl = rptr->init_tbl;
 	u8 override;
 	u32 val;
 	int ret;
 	int i;
 
-	ret = regulator_bulk_enable(rptr->cfg->num_vregs, rptr->vregs);
+	ret = regulator_bulk_enable(rptr->num_vregs, rptr->vregs);
 	if (ret)
 		return ret;
 
 	regmap_field_update_bits(rptr->regs[F_EN_CTL1], EUSB2_RPTR_EN, EUSB2_RPTR_EN);
 
-	for (i = 0; i < F_NUM_TUNE_FIELDS; i++) {
-		if (init_tbl[i]) {
-			regmap_field_update_bits(rptr->regs[i], init_tbl[i], init_tbl[i]);
-		} else {
-			/* Write 0 if there's no value set */
-			u32 mask = GENMASK(regfields[i].msb, regfields[i].lsb);
-
-			regmap_field_update_bits(rptr->regs[i], mask, 0);
-		}
-	}
-	memcpy(init_tbl, rptr->cfg->init_tbl, sizeof(init_tbl));
-
 	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &override))
 		init_tbl[F_TUNE_IUSB2] = override;
 
@@ -228,7 +217,7 @@  static int eusb2_repeater_exit(struct phy *phy)
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 
-	return regulator_bulk_disable(rptr->cfg->num_vregs, rptr->vregs);
+	return regulator_bulk_disable(rptr->num_vregs, rptr->vregs);
 }
 
 static const struct phy_ops eusb2_repeater_ops = {
@@ -240,6 +229,7 @@  static const struct phy_ops eusb2_repeater_ops = {
 
 static int eusb2_repeater_probe(struct platform_device *pdev)
 {
+	const struct eusb2_repeater_cfg *cfg;
 	struct eusb2_repeater *rptr;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
@@ -255,8 +245,8 @@  static int eusb2_repeater_probe(struct platform_device *pdev)
 	rptr->dev = dev;
 	dev_set_drvdata(dev, rptr);
 
-	rptr->cfg = of_device_get_match_data(dev);
-	if (!rptr->cfg)
+	cfg = of_device_get_match_data(dev);
+	if (!cfg)
 		return -EINVAL;
 
 	regmap = dev_get_regmap(dev->parent, NULL);
@@ -269,6 +259,12 @@  static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!rptr->regfields)
 		return -ENOMEM;
 
+	rptr->init_tbl = devm_kmemdup(dev, cfg->init_tbl,
+				       sizeof(cfg->init_tbl),
+				       GFP_KERNEL);
+	if (!rptr->init_tbl)
+		return -ENOMEM;
+
 	ret = of_property_read_u32(np, "reg", &res);
 	if (ret < 0)
 		return ret;
@@ -282,7 +278,7 @@  static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = eusb2_repeater_init_vregs(rptr);
+	ret = eusb2_repeater_init_vregs(rptr, cfg);
 	if (ret < 0) {
 		dev_err(dev, "unable to get supplies\n");
 		return ret;