Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <90830c22-0437-591e-cee0-67b16214bc55@gmail.com>
Date:   Wed, 12 Jul 2023 09:45:08 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.0
Subject: Re: [PATCH v3] pwm: atmel: Enable clk when pwm already enabled in
 bootloader
To:     =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     claudiu.beznea@microchip.com, thierry.reding@gmail.com,
        nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
        linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230711200905.6464-1-aarongt.shen@gmail.com>
 <20230711203017.cdfe2nrjx7lt25tm@pengutronix.de>
Content-Language: en-US
From:   Guiting Shen <aarongt.shen@gmail.com>
In-Reply-To: <20230711203017.cdfe2nrjx7lt25tm@pengutronix.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
>> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
>> +{
>> +	unsigned int i;
>> +	int err;
>> +	u32 sr;
>> +
>> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
>> +	if (!sr)
>> +		return 0;
>> +
>> +	for (i = 0; i < atmel_pwm->chip.npwm; i++) {
>> +		if (!(sr & (1 << i)))
>> +			continue;
>> +
>> +		err = clk_enable(atmel_pwm->clk);
>> +		if (err) {
>> +			dev_err(atmel_pwm->chip.dev,
>> +				"failed to enable clock: %pe\n", ERR_PTR(err));
> 
> Here you leak possibly a few enables. While it's not likely that the
> (say) third enable goes wrong, it's also not that hard to handle?!

The driver used the enable_count member of struct clk_core to count the
PWM channels(4 channels). It will enable hardware clock only when one of
the PWM channels becomed on from all PWM channels off which maybe return
error. And in second/third/fourth times to clk_enable(), it just
increased the enable_count of struct clk_core which would never return
error.

It maybe confused at first time to view the code.
Do it need to add something like that: ?

for (i = 0; i < atmel_pwm->chip.npwm; i++) {
	if (!(sr & (1 << i)))
		continue;

	err = clk_enable(atmel_pwm->clk);
	if (err) {
		dev_err(atmel_pwm->chip.dev,
			"failed to enable clock: %pe\n", ERR_PTR(err));

		for (i = 0; i < cnt; i++)
			clk_disable(atmel_pwm->clk);
		return err;
	}
	cnt++;
}

-- 
Regards,
Guiting Shen