Re: [PATCH v5 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Thierry,

(2015/08/17 23:15), Thierry Reding wrote:
> Sorry for taking an awful long time to get around to this. The driver
> looks generally okay, but I have a few minor comments...

Thank you for the review!

> On Mon, Jun 15, 2015 at 06:08:44PM +0900, Yoshihiro Shimoda wrote:
>> This patch adds support for R-Car SoCs PWM Timer.
> 
> This could be a little more verbose. You could say for example how many
> channels the driver exposes, or mention typical use-cases (if any).

Yes, I will add the following comment.

The PWM timer of R-Car H2 has 7 channels. So, we can use the channels if we
describe device tree nodes.

>> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> [...]
>> +static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
>> +{
>> +	int div;
> 
> Can be unsigned int.

I will fix it.

>> +	unsigned long clk_rate = clk_get_rate(rp->clk);
>> +	unsigned long long max;	/* max cycle / nanoseconds */
>> +
>> +	if (!clk_rate)
> 
> I prefer it when these are explicit: clk_rate == 0. This goes for
> numerical comparisons. For booleans, or NULL pointer comparisons the
> !expression is fine.

I will fix it.

>> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			   int duty_ns, int period_ns)
>> +{
>> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
>> +	int div = rcar_pwm_get_clock_division(rp, period_ns);
>> +	int ret;
>> +
>> +	if (div < 0)
>> +		return div;
>> +
>> +	/* Let the core driver set pwm->period if disabled and duty_ns == 0 */
>> +	if (!test_bit(PWMF_ENABLED, &pwm->flags) && !duty_ns)
>> +		return 0;
>> +
>> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
>> +	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
>> +	rcar_pwm_set_clock_control(rp, div);
>> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
>> +	u32 pwmcnt;
>> +
>> +	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
>> +	pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
>> +	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
>> +	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
>> +		return -EINVAL;
> 
> This looks wrong. Any errors in configuration should've been caught by
> the ->config() implementation. Why can't you return -EINVAL on this
> condition in ->config()? ->enable() failing should only be the case if
> truly the PWM can't be enabled, not if it's badly configured.

I would like to avoid a "prohibition setting" when the PWM is enabled.
The datasheet said "setting 0x000 is prohibited" in CYC0 and PH0.
However, the initial value of each field is 0x000.
So, I am thinking this is truly the PWM can't be enabled.
If this driver sets any value to the register in probe() to avoid
"prohibition setting", I can remove the condtion in ->enable().
What do you think?

About the ->config(), it already has such a condition check by rcar_pwm_set_counter():

+	/* Avoid prohibited setting */
+	if (cyc && ph)
+		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
+
+	return (cyc && ph) ? 0 : -EINVAL;

However it may be unreadable code. So, I will fix it as the followings:

+	/* Avoid prohibited setting */
+	if (cyc != 0 && ph != 0) {
+		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
+		return 0;
+	} else {
+		return -EINVAL;
+	}

>> +static struct platform_driver rcar_pwm_driver = {
>> +	.probe		= rcar_pwm_probe,
>> +	.remove		= rcar_pwm_remove,
>> +	.driver		= {
>> +		.name	= "pwm-rcar",
>> +		.of_match_table = of_match_ptr(rcar_pwm_of_table),
>> +	}
>> +};
> 
> This doesn't need the artificial padding. A single space around = is
> enough.

I will fix it.

Best regards,
Yoshihiro Shimoda

> Thierry
>