Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 18 Mar 2019 18:25:35 +0100
From:   Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
To:     Anson Huang <anson.huang@nxp.com>
Cc:     "thierry.reding@gmail.com" <thierry.reding@gmail.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "otavio@ossystems.com.br" <otavio@ossystems.com.br>,
        "stefan@agner.ch" <stefan@agner.ch>,
        Leonard Crestez <leonard.crestez@nxp.com>,
        "schnitzeltony@gmail.com" <schnitzeltony@gmail.com>,
        "jan.tuerk@emtrion.com" <jan.tuerk@emtrion.com>,
        Robin Gong <yibin.gong@nxp.com>,
        "linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH V4 2/5] pwm: Add i.MX TPM PWM driver support
Message-ID: <20190318172535.2yxokxwu52yjf6xf@pengutronix.de>
References: <1552610505-13568-1-git-send-email-Anson.Huang@nxp.com>
 <1552610505-13568-3-git-send-email-Anson.Huang@nxp.com>
 <20190315093532.xw5ivfkxrwvrkvix@pengutronix.de>
 <DB3PR0402MB39161B062231C59842595374F5470@DB3PR0402MB3916.eurprd04.prod.outlook.com>
 <20190318080743.xumj6e72bzumszvp@pengutronix.de>
 <DB3PR0402MB391627A8C741C84A2057E576F5470@DB3PR0402MB3916.eurprd04.prod.outlook.com>
 <DB3PR0402MB39161A9884D5768F7280024FF5470@DB3PR0402MB3916.eurprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <DB3PR0402MB39161A9884D5768F7280024FF5470@DB3PR0402MB3916.eurprd04.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello,

On Mon, Mar 18, 2019 at 02:04:00PM +0000, Anson Huang wrote:
> > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote:
> > > > > > +	val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > > > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > >
> > > > > As this interrupts the output, please only do it if necessary.
> > > >
> > > > OK, will do it ONLY when it is enabled previously.
> > >
> > > I think you only need to do that when the value actually changes.
> > 
> > OK, I will save the period/div count and ONLY do it when the value actually
> > changes.
> 
> After further think, I added below tpm->period to save the current period settings,
> ONLY when the new period to be set is different from the current period, then the 
> pwm_imx_tpm_config_counter() is called, so I think no need to care about the value
> changes, the value is always changed when pwm_imx_tpm_config_counter() is called.
> 
>                  if (tpm->user_count != 1 && state->period != tpm->period)
>                          return -EBUSY;
>                  ret = pwm_imx_tpm_config_counter(chip, state->period);
>                  if (ret)
>                          return ret;

This is still not the optimal thing to do. What you really want is:

	p = round_period_according_to_hw_caps(state->period);
	if (p != actually_configured_period && tpm->user_count != 1)
		return -EBUSY;


> > > > > > +	/* set duty counter */
> > > > > > +	tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > > > > +PWM_IMX_TPM_MOD_MOD_MASK;
> > > > >
> > > > > I recommend storing this value in driver data.
> > > >
> > > > NOT quite understand, as we did NOT use it in other places except
> > > > the get_state, just reading the register once should be OK there.
> > >
> > > I had the impression it is used more than once. Will look again in the
> > > next iteration. But also note that shadowing the value might already
> > > be beneficial for a single call site as driver data might occupy more
> > > RAM than necessary anyhow and reading from RAM is faster than from the
> > > hardware's register.
> > > Probably this is not a fast path, so not worth the optimisation?!
> > 
> > OK, will save it in driver data and avoid accessing register again.

please apply some thought before following my advices. If this is not a
fast path and it hurts readability, don't shadow the register in driver
data.

> > > > > > +		/* disable channel */
> > > > > > +		writel(PWM_IMX_TPM_CnSC_CHF,
> > > > > > +			tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > >
> > > > > Clearing CHF doens't disable the channel as I read the manual.
> > > >
> > > > This write clears CHF as well as writing other bits 0, to disable
> > > > the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid
> > > > confusion.
> > >
> > > Ah, I misinterpreted the value written.
> > >
> > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > +			     struct pwm_state *state) {
> > > > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > > > +	struct pwm_state curstate;
> > > > > > +	u32 duty_cycle = state->duty_cycle;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > > > +
> > > > > > +	mutex_lock(&tpm->lock);
> > > > >
> > > > > What should this lock protect? Does it hurt if the state changes
> > > > > between pwm_imx_tpm_get_state releasing the lock and
> > > > > pwm_imx_tpm_apply taking it?
> > > >
> > > > The idea is to protect the share resourced by multiple channels, but
> > > > I think I can make the mutex_lock includes get_state and remove the
> > > > lock in get_state function.
> > >
> > > You might need it in .get_state to return a consistent state to the
> > > caller. In this case just introduce an unlocked variant of .get_state
> > > to share code between the two functions.
> > >
> > > And BTW the question was honest. I'm not entirely sure that you need
> > > to hold the lock.
> > 
> > Agreed, if the different channel configuration ONLY access its own register,
> > NOT any shared registers, then I think this lock is unnecessary.
> 
> Since all the functions in .apply function will need to access registers and these
> registers are shared by different channels, so I think the lock is necessary. 

I think so, too, but didn't thought it to the end. I can invest some
time here in the next review round.

BTW, one thing that I think is missing is, that .apply must only return
when the newly configured setting is already active.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |