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

[Paul Cercueil]

Hi,

Le jeu. 13 déc. 2018 à 10:18, Uwe Kleine-König 
<u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> 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.)

I have them here: https://zcrc.me/~paul/jz_docs/

> 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?

I don't really know. We only use this driver for a rumble motor and 
backlight.
Somebody would have to check with a logic analyzer.

> 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?)

There's a bit to toggle between "graceful" shutdown (bit clear) and 
"abrupt"
shutdown (bit set). TCSR bit 9. I think that graceful shutdown will 
complete
the running period, then keep the level active. Abrupt shutdown will 
keep the
current level of the line.

> >  @@ -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.

The idea was that if we don't get the register space we need, that means
the channel is used for something else, hence the EBUSY. Should I switch
it to ENXIO?

> >   	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/  |