Hi Nicolas,
On Mon, 2021-01-04 at 17:27 +0800, Nicolas Boichat wrote:
> On Mon, Jan 4, 2021 at 4:51 PM Roger Lu <roger.lu@xxxxxxxxxxxx> wrote:
> >
> >
> > Hi Nicolas,
> >
> > Thanks for all the advices.
> >
> > On Thu, 2020-12-31 at 10:10 +0800, Nicolas Boichat wrote:
> > > On Sun, Dec 27, 2020 at 6:55 PM Roger Lu <roger.lu@xxxxxxxxxxxx> wrote:
> [snip]
> > > > +static int svs_adjust_pm_opp_volts(struct svs_bank *svsb, bool force_update)
> > > > +{
> > > > + int tzone_temp, ret = -EPERM;
> > >
> > > No need to initialize ret.
> >
> > Oh, excuse me, some coding check tool warn that this `ret` might return
> > without being uninitialized. Therefore, I'll keep the initialization.
>
> Oh, you're right, there is a possible path where ret is not set. sgtm then.
>
> >
> > >
> > > > + u32 i, svsb_volt, opp_volt, temp_offset = 0;
> > > > +
> > > > + mutex_lock(&svsb->lock);
> > > > +
> > > > + /*
> > > > + * If svs bank is suspended, it means signed-off voltages are applied.
> > > > + * Don't need to update opp voltage anymore.
> > > > + */
> > > > + if (svsb->suspended && !force_update) {
> > > > + dev_notice(svsb->dev, "bank is suspended\n");
> > > > + ret = -EPERM;
> > > > + goto unlock_mutex;
> > > > + }
> > > > +
> > > > + /* Get thermal effect */
> > > > + if (svsb->phase == SVSB_PHASE_MON) {
> > > > + if (svsb->temp > svsb->temp_upper_bound &&
> > > > + svsb->temp < svsb->temp_lower_bound) {
> > > > + dev_warn(svsb->dev, "svsb temp = 0x%x?\n", svsb->temp);
> > > > + ret = -EINVAL;
> > > > + goto unlock_mutex;
> > > > + }
> > > > +
> > > > + ret = svs_get_bank_zone_temperature(svsb->tzone_name,
> > > > + &tzone_temp);
> > > > + if (ret) {
> > > > + dev_err(svsb->dev, "no \"%s\"?(%d)?\n",
> > > > + svsb->tzone_name, ret);
> > > > + dev_err(svsb->dev, "set signed-off voltage\n");
> > >
> > > Please merge the error message in one line (I'm not sure what "set
> > > signed-off voltage" means here).
> >
> > 1. Ok, I'll merge them. Thanks.
> > 2. signed-off voltages means CPU DVFS default voltages
>
> So just write "default voltages" then? ,-)
Ok, thanks. :)
>
> >
> > >
> [snip]
> > > > +static irqreturn_t svs_isr(int irq, void *data)
> > > > +{
> > > > + struct svs_platform *svsp = (struct svs_platform *)data;
> > >
> > > cast not needed.
> >
> > Ok, I'll remove it. Thanks.
> >
> > >
> > > > + struct svs_bank *svsb = NULL;
> > > > + unsigned long flags;
> > > > + u32 idx, int_sts, svs_en;
> > > > +
> > > > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > > > + svsb = &svsp->banks[idx];
> > > > +
> > > > + spin_lock_irqsave(&mtk_svs_lock, flags);
> > > > + svsp->pbank = svsb;
> > > > +
> > > > + /* Find out which svs bank fires interrupt */
> > > > + if (svsb->int_st & svs_readl(svsp, INTST)) {
> > > > + spin_unlock_irqrestore(&mtk_svs_lock, flags);
> > > > + continue;
> > > > + }
> > > > +
> > > > + if (!svsb->suspended) {
> > > > + svs_switch_bank(svsp);
> > > > + int_sts = svs_readl(svsp, INTSTS);
> > > > + svs_en = svs_readl(svsp, SVSEN);
> > > > +
> > > > + if (int_sts == SVSB_INTSTS_COMPLETE &&
> > > > + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT01))
> > > > + svs_init01_isr_handler(svsp);
> > > > + else if ((int_sts == SVSB_INTSTS_COMPLETE) &&
> > > > + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT02))
> > > > + svs_init02_isr_handler(svsp);
> > > > + else if (!!(int_sts & SVSB_INTSTS_MONVOP))
> > >
> > > !! is not required.
> >
> > Ok, I'll remove it. Thanks.
> >
> > >
> > > > + svs_mon_mode_isr_handler(svsp);
> > > > + else
> > > > + svs_error_isr_handler(svsp);
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&mtk_svs_lock, flags);
> > > > + break;
> > > > + }
> > >
> > > This will panic if svsb is NULL, is that ok or do you want to catch that?
> >
> > Oh, it is fine. Thanks for the heads-up.
>
> I should have been stronger in my statement, I think you want to add a
> BUG_ON(!svsb) to crash in a slightly more predictable manner.
Ok, I'll add BUG_ON(!svsb) to give an evident heads-up. Thanks.
>
> [snip]
> > > > +
> > > > + svsp->tefuse = (u32 *)nvmem_cell_read(cell, &svsp->tefuse_num);
> > >
> > > Cast not needed.
> >
> > Ok, I'll remove it if build/test ok. Because nvmem_cell_read returns
> > (void *).
> >
> > >
> > > Also, this need to be freed somewhere in remove code (kfree(svsp->tefuse)).
> > >
> > > And it seems like svsp->tefuse is only used in this function, can you
> > > just allocate it here?
> >
> > Oh, svsp->tefuse will be used in SVS debug patch for debug purpose. So,
> > I need to save it as struct member.
>
> Oh I missed that, sgtm then. Thanks.
>
> >
> [snip]
