Received-SPF: pass (google.com: domain of linux-kernel+bounces-17735-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 5 Jan 2024 12:19:17 +0200
From: Abel Vesa <abel.vesa@linaro.org>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Bjorn Andersson <andersson@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] phy: qualcomm: eusb2-repeater: Drop the redundant
 zeroing
Message-ID: <ZZfXpWpFAELlUnUT@linaro.org>
References: <20240104-phy-qcom-eusb2-repeater-fixes-v1-0-047b7b6b8333@linaro.org>
 <20240104-phy-qcom-eusb2-repeater-fixes-v1-2-047b7b6b8333@linaro.org>
 <33b32e2b-fa35-448c-8b4c-c5c51a514922@linaro.org>
 <ZZfI66Mj6v/Qjp5X@linaro.org>
 <e216c027-dc23-415d-8d00-e6b1cbbd4248@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e216c027-dc23-415d-8d00-e6b1cbbd4248@linaro.org>

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