linux-phy.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework
@ 2024-01-04 14:52 Abel Vesa
  2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Abel Vesa @ 2024-01-04 14:52 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel, Abel Vesa

Found the first issue (from first patch) while adding support
for X Elite (X1E80100) which comes with more than one repeaters.
The second fix is just bonus.

---
Abel Vesa (2):
      phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
      phy: qualcomm: eusb2-repeater: Drop the redundant zeroing

 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
---
base-commit: 0fef202ac2f8e6d9ad21aead648278f1226b9053
change-id: 20240104-phy-qcom-eusb2-repeater-fixes-c9201113032c

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
  2024-01-04 14:52 [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Abel Vesa
@ 2024-01-04 14:52 ` Abel Vesa
  2024-01-04 19:12   ` Abel Vesa
  2024-01-04 22:46   ` Konrad Dybcio
  2024-01-04 14:52 ` [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing Abel Vesa
  2024-01-04 22:58 ` [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Elliot Berman
  2 siblings, 2 replies; 11+ messages in thread
From: Abel Vesa @ 2024-01-04 14:52 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel, Abel Vesa

The global regmap fields offsets currently get incremented with the base
address of the repeater. This issue doesn't get noticed unless the probe
defers or there are multiple repeaters on that platform. So instead of
incrementing the global ones, copy them for each instance of the
repeater.

Fixes: 4ba2e52718c0 ("phy: qualcomm: phy-qcom-eusb2-repeater: Use regmap_fields")
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index a623f092b11f..5f5862a68b73 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -101,6 +101,7 @@ struct eusb2_repeater {
 	struct regmap_field *regs[F_NUM_FIELDS];
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
+	struct reg_field *regfields;
 	const struct eusb2_repeater_cfg *cfg;
 	enum phy_mode mode;
 };
@@ -140,8 +141,8 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
 
 static int eusb2_repeater_init(struct phy *phy)
 {
-	struct reg_field *regfields = eusb2_repeater_tune_reg_fields;
 	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 };
 	u8 override;
@@ -262,15 +263,21 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!regmap)
 		return -ENODEV;
 
+	rptr->regfields = devm_kmemdup(dev, eusb2_repeater_tune_reg_fields,
+				       sizeof(eusb2_repeater_tune_reg_fields),
+				       GFP_KERNEL);
+	if (!rptr->regfields)
+		return -ENOMEM;
+
 	ret = of_property_read_u32(np, "reg", &res);
 	if (ret < 0)
 		return ret;
 
 	for (i = 0; i < F_NUM_FIELDS; i++)
-		eusb2_repeater_tune_reg_fields[i].reg += res;
+		rptr->regfields[i].reg += res;
 
 	ret = devm_regmap_field_bulk_alloc(dev, regmap, rptr->regs,
-					   eusb2_repeater_tune_reg_fields,
+					   rptr->regfields,
 					   F_NUM_FIELDS);
 	if (ret)
 		return ret;

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-04 14:52 [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Abel Vesa
  2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
@ 2024-01-04 14:52 ` Abel Vesa
  2024-01-04 22:50   ` Konrad Dybcio
  2024-01-06 17:30   ` kernel test robot
  2024-01-04 22:58 ` [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Elliot Berman
  2 siblings, 2 replies; 11+ messages in thread
From: Abel Vesa @ 2024-01-04 14:52 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel, Abel Vesa

The local init_tlb is already zero initialized, so the entire zeroing loop
is useless in this case, since the initial values are copied over anyway,
before being written.

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 | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index 5f5862a68b73..3060c0749797 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -156,16 +156,6 @@ static int eusb2_repeater_init(struct phy *phy)
 
 	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))

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
  2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
@ 2024-01-04 19:12   ` Abel Vesa
  2024-01-04 22:46   ` Konrad Dybcio
  1 sibling, 0 replies; 11+ messages in thread
From: Abel Vesa @ 2024-01-04 19:12 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel

On 24-01-04 16:52:11, Abel Vesa wrote:
> The global regmap fields offsets currently get incremented with the base
> address of the repeater. This issue doesn't get noticed unless the probe
> defers or there are multiple repeaters on that platform. So instead of
> incrementing the global ones, copy them for each instance of the
> repeater.
> 
> Fixes: 4ba2e52718c0 ("phy: qualcomm: phy-qcom-eusb2-repeater: Use regmap_fields")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> index a623f092b11f..5f5862a68b73 100644
> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> @@ -101,6 +101,7 @@ struct eusb2_repeater {
>  	struct regmap_field *regs[F_NUM_FIELDS];
>  	struct phy *phy;
>  	struct regulator_bulk_data *vregs;
> +	struct reg_field *regfields;
>  	const struct eusb2_repeater_cfg *cfg;
>  	enum phy_mode mode;
>  };
> @@ -140,8 +141,8 @@ static int eusb2_repeater_init_vregs(struct eusb2_repeater *rptr)
>  
>  static int eusb2_repeater_init(struct phy *phy)
>  {
> -	struct reg_field *regfields = eusb2_repeater_tune_reg_fields;
>  	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
> +	struct reg_field *regfields = rptr->regfields;

Oups, this is not needed. Will drop after I get some more comments on
this.

>  	struct device_node *np = rptr->dev->of_node;
>  	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
>  	u8 override;
> @@ -262,15 +263,21 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
>  	if (!regmap)
>  		return -ENODEV;
>  
> +	rptr->regfields = devm_kmemdup(dev, eusb2_repeater_tune_reg_fields,
> +				       sizeof(eusb2_repeater_tune_reg_fields),
> +				       GFP_KERNEL);
> +	if (!rptr->regfields)
> +		return -ENOMEM;
> +
>  	ret = of_property_read_u32(np, "reg", &res);
>  	if (ret < 0)
>  		return ret;
>  
>  	for (i = 0; i < F_NUM_FIELDS; i++)
> -		eusb2_repeater_tune_reg_fields[i].reg += res;
> +		rptr->regfields[i].reg += res;
>  
>  	ret = devm_regmap_field_bulk_alloc(dev, regmap, rptr->regs,
> -					   eusb2_repeater_tune_reg_fields,
> +					   rptr->regfields,
>  					   F_NUM_FIELDS);
>  	if (ret)
>  		return ret;
> 
> -- 
> 2.34.1
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
  2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
  2024-01-04 19:12   ` Abel Vesa
@ 2024-01-04 22:46   ` Konrad Dybcio
  1 sibling, 0 replies; 11+ messages in thread
From: Konrad Dybcio @ 2024-01-04 22:46 UTC (permalink / raw)
  To: Abel Vesa, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel

On 4.01.2024 15:52, Abel Vesa wrote:
> The global regmap fields offsets currently get incremented with the base
> address of the repeater. This issue doesn't get noticed unless the probe
> defers or there are multiple repeaters on that platform. So instead of
> incrementing the global ones, copy them for each instance of the
> repeater.
> 
> Fixes: 4ba2e52718c0 ("phy: qualcomm: phy-qcom-eusb2-repeater: Use regmap_fields")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
Ohh I wouldn't have thought about this.. Nice spot!

[...]

> -	struct reg_field *regfields = eusb2_repeater_tune_reg_fields;
>  	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
> +	struct reg_field *regfields = rptr->regfields;
Without this:

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-04 14:52 ` [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing Abel Vesa
@ 2024-01-04 22:50   ` Konrad Dybcio
  2024-01-05  9:16     ` Abel Vesa
  2024-01-06 17:30   ` kernel test robot
  1 sibling, 1 reply; 11+ messages in thread
From: Konrad Dybcio @ 2024-01-04 22:50 UTC (permalink / raw)
  To: Abel Vesa, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel

On 4.01.2024 15:52, Abel Vesa wrote:
> The local init_tlb is already zero initialized, so the entire zeroing loop
> is useless in this case, since the initial values are copied over anyway,
> before being written.
> 
> Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

That's another good spot.. partial struct initialization of
pm8550b_init_tbl zeroes out the uninitialized fields.


>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> index 5f5862a68b73..3060c0749797 100644
> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> @@ -156,16 +156,6 @@ static int eusb2_repeater_init(struct phy *phy)
>  
>  	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));

I think this patchset can be made even better, this memcpy is also
useless and we can simply initialize init_tbl=rptr->cfg->init_tbl.

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework
  2024-01-04 14:52 [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Abel Vesa
  2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
  2024-01-04 14:52 ` [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing Abel Vesa
@ 2024-01-04 22:58 ` Elliot Berman
  2 siblings, 0 replies; 11+ messages in thread
From: Elliot Berman @ 2024-01-04 22:58 UTC (permalink / raw)
  To: Abel Vesa, Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: linux-arm-msm, linux-phy, linux-kernel



On 1/4/2024 6:52 AM, Abel Vesa wrote:
> Found the first issue (from first patch) while adding support
> for X Elite (X1E80100) which comes with more than one repeaters.
> The second fix is just bonus.
> 

Tested-by: Elliot Berman <quic_eberman@quicinc.com> # sm8650-qrd

> ---
> Abel Vesa (2):
>       phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances
>       phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
> 
>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> ---
> base-commit: 0fef202ac2f8e6d9ad21aead648278f1226b9053
> change-id: 20240104-phy-qcom-eusb2-repeater-fixes-c9201113032c
> 
> Best regards,

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-04 22:50   ` Konrad Dybcio
@ 2024-01-05  9:16     ` Abel Vesa
  2024-01-05 10:09       ` Konrad Dybcio
  0 siblings, 1 reply; 11+ messages in thread
From: Abel Vesa @ 2024-01-05  9:16 UTC (permalink / raw)
  To: Konrad Dybcio
  Cc: Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I,
	linux-arm-msm, linux-phy, linux-kernel

On 24-01-04 23:50:48, Konrad Dybcio wrote:
> On 4.01.2024 15:52, Abel Vesa wrote:
> > The local init_tlb is already zero initialized, so the entire zeroing loop
> > is useless in this case, since the initial values are copied over anyway,
> > before being written.
> > 
> > Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> 
> That's another good spot.. partial struct initialization of
> pm8550b_init_tbl zeroes out the uninitialized fields.
> 
> 
> >  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> > index 5f5862a68b73..3060c0749797 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> > @@ -156,16 +156,6 @@ static int eusb2_repeater_init(struct phy *phy)
> >  
> >  	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));
> 
> I think this patchset can be made even better, this memcpy is also
> useless and we can simply initialize init_tbl=rptr->cfg->init_tbl.

Actually no. The init_tbl in cfg is a pointer to const. Plus, if you do
that, you will end up with the same situation like in the other patch,
as there are some overrides based on DT values below this.

But now that I've had another look, maybe doing the exact same thing as
the other patch does (kmemdup) will probably look better anyway,
specially if we do that on probe.

> 
> Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-05  9:16     ` Abel Vesa
@ 2024-01-05 10:09       ` Konrad Dybcio
  2024-01-05 10:19         ` Abel Vesa
  0 siblings, 1 reply; 11+ messages in thread
From: Konrad Dybcio @ 2024-01-05 10:09 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I,
	linux-arm-msm, linux-phy, linux-kernel

On 5.01.2024 10:16, Abel Vesa wrote:
> On 24-01-04 23:50:48, Konrad Dybcio wrote:
>> On 4.01.2024 15:52, Abel Vesa wrote:
>>> The local init_tlb is already zero initialized, so the entire zeroing loop
>>> is useless in this case, since the initial values are copied over anyway,
>>> before being written.
>>>
>>> Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>
>> That's another good spot.. partial struct initialization of
>> pm8550b_init_tbl zeroes out the uninitialized fields.
>>
>>
>>>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 10 ----------
>>>  1 file changed, 10 deletions(-)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
>>> index 5f5862a68b73..3060c0749797 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
>>> @@ -156,16 +156,6 @@ static int eusb2_repeater_init(struct phy *phy)
>>>  
>>>  	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));
>>
>> I think this patchset can be made even better, this memcpy is also
>> useless and we can simply initialize init_tbl=rptr->cfg->init_tbl.
> 
> Actually no. The init_tbl in cfg is a pointer to const. Plus, if you do
> that, you will end up with the same situation like in the other patch,
> as there are some overrides based on DT values below this.

Hm, you're right. Maybe we should simple store *base and stop
modifying these tables then. There's not a whole lot of regmap_rw
calls, so making the first argument "rptr->base + rptr->regs[ASDF]"
shouldn't add much fluff. Then we can make the cfg referernce const.

Konrad

> 
> But now that I've had another look, maybe doing the exact same thing as
> the other patch does (kmemdup) will probably look better anyway,
> specially if we do that on probe.
> 
>>
>> Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-05 10:09       ` Konrad Dybcio
@ 2024-01-05 10:19         ` Abel Vesa
  0 siblings, 0 replies; 11+ messages in thread
From: Abel Vesa @ 2024-01-05 10:19 UTC (permalink / raw)
  To: Konrad Dybcio
  Cc: Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I,
	linux-arm-msm, linux-phy, linux-kernel

On 24-01-05 11:09:33, Konrad Dybcio wrote:
> On 5.01.2024 10:16, Abel Vesa wrote:
> > On 24-01-04 23:50:48, Konrad Dybcio wrote:
> >> On 4.01.2024 15:52, Abel Vesa wrote:
> >>> The local init_tlb is already zero initialized, so the entire zeroing loop
> >>> is useless in this case, since the initial values are copied over anyway,
> >>> before being written.
> >>>
> >>> Fixes: 99a517a582fc ("phy: qualcomm: phy-qcom-eusb2-repeater: Zero out untouched tuning regs")
> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> >>> ---
> >>
> >> That's another good spot.. partial struct initialization of
> >> pm8550b_init_tbl zeroes out the uninitialized fields.
> >>
> >>
> >>>  drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c | 10 ----------
> >>>  1 file changed, 10 deletions(-)
> >>>
> >>> diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> >>> index 5f5862a68b73..3060c0749797 100644
> >>> --- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> >>> +++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
> >>> @@ -156,16 +156,6 @@ static int eusb2_repeater_init(struct phy *phy)
> >>>  
> >>>  	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));
> >>
> >> I think this patchset can be made even better, this memcpy is also
> >> useless and we can simply initialize init_tbl=rptr->cfg->init_tbl.
> > 
> > Actually no. The init_tbl in cfg is a pointer to const. Plus, if you do
> > that, you will end up with the same situation like in the other patch,
> > as there are some overrides based on DT values below this.
> 
> Hm, you're right. Maybe we should simple store *base and stop
> modifying these tables then. There's not a whole lot of regmap_rw
> calls, so making the first argument "rptr->base + rptr->regs[ASDF]"
> shouldn't add much fluff. Then we can make the cfg referernce const.
> 

Oh, sorry, did not see your reply in time before sending v2.

Have a look at v2 and we can decide if we want something different then.
https://lore.kernel.org/all/20240105-phy-qcom-eusb2-repeater-fixes-v2-0-775d98e7df05@linaro.org/

Thanks for reviewing.

> Konrad
> 
> > 
> > But now that I've had another look, maybe doing the exact same thing as
> > the other patch does (kmemdup) will probably look better anyway,
> > specially if we do that on probe.
> > 
> >>
> >> Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
  2024-01-04 14:52 ` [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing Abel Vesa
  2024-01-04 22:50   ` Konrad Dybcio
@ 2024-01-06 17:30   ` kernel test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2024-01-06 17:30 UTC (permalink / raw)
  To: Abel Vesa, Bjorn Andersson, Konrad Dybcio, Vinod Koul,
	Kishon Vijay Abraham I
  Cc: oe-kbuild-all, linux-arm-msm, linux-phy, linux-kernel, Abel Vesa

Hi Abel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0fef202ac2f8e6d9ad21aead648278f1226b9053]

url:    https://github.com/intel-lab-lkp/linux/commits/Abel-Vesa/phy-qualcomm-eusb2-repeater-Fix-the-regfields-for-multiple-instances/20240104-225513
base:   0fef202ac2f8e6d9ad21aead648278f1226b9053
patch link:    https://lore.kernel.org/r/20240104-phy-qcom-eusb2-repeater-fixes-v1-2-047b7b6b8333%40linaro.org
patch subject: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing
config: i386-buildonly-randconfig-004-20240106 (https://download.01.org/0day-ci/archive/20240107/202401070143.pnnuXAwC-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240107/202401070143.pnnuXAwC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401070143.pnnuXAwC-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c: In function 'eusb2_repeater_init':
>> drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c:145:20: warning: unused variable 'regfields' [-Wunused-variable]
     struct reg_field *regfields = rptr->regfields;
                       ^~~~~~~~~


vim +/regfields +145 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c

56d77c9a10d97d Abel Vesa     2023-02-08  141  
56d77c9a10d97d Abel Vesa     2023-02-08  142  static int eusb2_repeater_init(struct phy *phy)
56d77c9a10d97d Abel Vesa     2023-02-08  143  {
56d77c9a10d97d Abel Vesa     2023-02-08  144  	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
ac0aae0074102c Abel Vesa     2024-01-04 @145  	struct reg_field *regfields = rptr->regfields;
56156a76e765d3 Konrad Dybcio 2023-09-13  146  	struct device_node *np = rptr->dev->of_node;
56156a76e765d3 Konrad Dybcio 2023-09-13  147  	u32 init_tbl[F_NUM_TUNE_FIELDS] = { 0 };
56156a76e765d3 Konrad Dybcio 2023-09-13  148  	u8 override;
56d77c9a10d97d Abel Vesa     2023-02-08  149  	u32 val;
56d77c9a10d97d Abel Vesa     2023-02-08  150  	int ret;
56d77c9a10d97d Abel Vesa     2023-02-08  151  	int i;
56d77c9a10d97d Abel Vesa     2023-02-08  152  
56d77c9a10d97d Abel Vesa     2023-02-08  153  	ret = regulator_bulk_enable(rptr->cfg->num_vregs, rptr->vregs);
56d77c9a10d97d Abel Vesa     2023-02-08  154  	if (ret)
56d77c9a10d97d Abel Vesa     2023-02-08  155  		return ret;
56d77c9a10d97d Abel Vesa     2023-02-08  156  
4ba2e52718c0ce Konrad Dybcio 2023-09-13  157  	regmap_field_update_bits(rptr->regs[F_EN_CTL1], EUSB2_RPTR_EN, EUSB2_RPTR_EN);
56d77c9a10d97d Abel Vesa     2023-02-08  158  
56156a76e765d3 Konrad Dybcio 2023-09-13  159  	memcpy(init_tbl, rptr->cfg->init_tbl, sizeof(init_tbl));
56156a76e765d3 Konrad Dybcio 2023-09-13  160  
56156a76e765d3 Konrad Dybcio 2023-09-13  161  	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &override))
56156a76e765d3 Konrad Dybcio 2023-09-13  162  		init_tbl[F_TUNE_IUSB2] = override;
56156a76e765d3 Konrad Dybcio 2023-09-13  163  
56156a76e765d3 Konrad Dybcio 2023-09-13  164  	if (!of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &override))
56156a76e765d3 Konrad Dybcio 2023-09-13  165  		init_tbl[F_TUNE_HSDISC] = override;
56156a76e765d3 Konrad Dybcio 2023-09-13  166  
56156a76e765d3 Konrad Dybcio 2023-09-13  167  	if (!of_property_read_u8(np, "qcom,tune-usb2-preem", &override))
56156a76e765d3 Konrad Dybcio 2023-09-13  168  		init_tbl[F_TUNE_USB2_PREEM] = override;
56156a76e765d3 Konrad Dybcio 2023-09-13  169  
56156a76e765d3 Konrad Dybcio 2023-09-13  170  	for (i = 0; i < F_NUM_TUNE_FIELDS; i++)
56156a76e765d3 Konrad Dybcio 2023-09-13  171  		regmap_field_update_bits(rptr->regs[i], init_tbl[i], init_tbl[i]);
56d77c9a10d97d Abel Vesa     2023-02-08  172  
4ba2e52718c0ce Konrad Dybcio 2023-09-13  173  	ret = regmap_field_read_poll_timeout(rptr->regs[F_RPTR_STATUS],
4ba2e52718c0ce Konrad Dybcio 2023-09-13  174  					     val, val & RPTR_OK, 10, 5);
56d77c9a10d97d Abel Vesa     2023-02-08  175  	if (ret)
56d77c9a10d97d Abel Vesa     2023-02-08  176  		dev_err(rptr->dev, "initialization timed-out\n");
56d77c9a10d97d Abel Vesa     2023-02-08  177  
56d77c9a10d97d Abel Vesa     2023-02-08  178  	return ret;
56d77c9a10d97d Abel Vesa     2023-02-08  179  }
56d77c9a10d97d Abel Vesa     2023-02-08  180  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-01-06 17:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-04 14:52 [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Abel Vesa
2024-01-04 14:52 ` [PATCH 1/2] phy: qualcomm: eusb2-repeater: Fix the regfields for multiple instances Abel Vesa
2024-01-04 19:12   ` Abel Vesa
2024-01-04 22:46   ` Konrad Dybcio
2024-01-04 14:52 ` [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing Abel Vesa
2024-01-04 22:50   ` Konrad Dybcio
2024-01-05  9:16     ` Abel Vesa
2024-01-05 10:09       ` Konrad Dybcio
2024-01-05 10:19         ` Abel Vesa
2024-01-06 17:30   ` kernel test robot
2024-01-04 22:58 ` [PATCH 0/2] phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework Elliot Berman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).