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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux