diff mbox series

[2/2] phy: qualcomm: eusb2-repeater: Drop the redundant zeroing

Message ID 20240104-phy-qcom-eusb2-repeater-fixes-v1-2-047b7b6b8333@linaro.org (mailing list archive)
State Superseded
Headers show
Series phy: qualcomm: eusb2-repeater: Some fixes after the regmap rework | expand

Commit Message

Abel Vesa Jan. 4, 2024, 2:52 p.m. UTC 
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(-)

Comments

Konrad Dybcio Jan. 4, 2024, 10:50 p.m. UTC | #1 
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
Abel Vesa Jan. 5, 2024, 9:16 a.m. UTC | #2 
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
Konrad Dybcio Jan. 5, 2024, 10:09 a.m. UTC | #3 
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
Abel Vesa Jan. 5, 2024, 10:19 a.m. UTC | #4 
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
kernel test robot Jan. 6, 2024, 5:30 p.m. UTC | #5 
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
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..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))