Re: [PATCH v8 12/26] pwm: jz4740: Allow selection of PWM channels 0 and 1

[Uwe Kleine-König]

On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> The TCU channels 0 and 1 were previously reserved for system tasks, and
> thus unavailable for PWM.
> 
> The driver will now only allow a PWM channel to be requested if memory
> resources corresponding to the register area of the channel were
> supplied to the driver. This allows the TCU channels to be reserved for
> system tasks from within the devicetree.
> 
> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>

While there is someone caring for this driver I'd like to complete (a
bit) my picture about the different capabilities and specialities of the
supported PWMs. So I have a few questions:

Is there a publicly available reference manual for this device? (If
yes, adding a link to the driver would be great.)

jz4740_pwm_config looks as if the currently running period isn't
completed before the new config is in effect. Is that correct? If yes,
can this be fixed? A similar question for set_polarity: Does setting the
JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
immediately or is this shadowed until the next period starts?

How does the device's output behave after the PWM is disabled?
Does it complete the currently running period? How does the output
behave then? (active/inactive/high/low/high-z?)

> @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	char clk_name[16];
>  	int ret;
>  
> -	/*
> -	 * Timers 0 and 1 are used for system tasks, so they are unavailable
> -	 * for use as PWMs.
> -	 */
> -	if (pwm->hwpwm < 2)
> +	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>  		return -EBUSY;

Maybe EBUSY isn't the best choice here. If the needed register space for
the requested pwm is not included in the memory resources provided to
the device I'd prefer ENXIO or ENODEV.

>  	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> @@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	jz4740->parent_res = platform_get_resource(
> +				to_platform_device(dev->parent),
> +				IORESOURCE_MEM, 0);
> +	if (!jz4740->parent_res)
> +		return -EINVAL;
> +
>  	jz4740->chip.dev = dev;
>  	jz4740->chip.ops = &jz4740_pwm_ops;
>  	jz4740->chip.npwm = NUM_PWM;

Thanks
Uwe

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