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
State New
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. 5, 2024, 10:09 a.m. UTC | #1 
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 | #2 
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 | #3 
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))