Re: [PATCH V2 3/4] drivers: pwm: pwm-bcm-kona: Add cygnus-pwm support

[Uwe Kleine-König]

Hello,

On Thu, Jan 17, 2019 at 01:45:15AM +0530, Sheetal Tigadoli wrote:
> From: Praveen Kumar B <praveen.b@xxxxxxxxxxxx>
> 
> Add support for new version of pwm-cygnus.
> Add support to make PWM changes configured and stable.
> 
> Signed-off-by: Praveen Kumar B <praveen.b@xxxxxxxxxxxx>
> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
> Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@xxxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 95 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index fe63289..143843f 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -15,6 +15,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/ioport.h>
>  #include <linux/math64.h>
>  #include <linux/module.h>
> @@ -65,10 +66,19 @@
>  #define DUTY_CYCLE_HIGH_MIN			(0x00000000)
>  #define DUTY_CYCLE_HIGH_MAX			(0x00ffffff)
>  
> +#define PWM_MONITOR_OFFSET			0xb0
> +#define PWM_MONITOR_TIMEOUT_US			5
> +
> +enum kona_pwmc_ver {
> +	KONA_PWM = 1,
> +	CYGNUS_PWM
> +};
> +
>  struct kona_pwmc {
>  	struct pwm_chip chip;
>  	void __iomem *base;
>  	struct clk *clk;
> +	enum kona_pwmc_ver version;
>  };
>  
>  static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
> @@ -76,10 +86,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
>  	return container_of(_chip, struct kona_pwmc, chip);
>  }
>  
> +static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan,
> +				 unsigned int kona_ver)
> +{
> +	struct kona_pwmc *kp = to_kona_pwmc(chip);
> +	unsigned int value;
> +
> +	switch (kona_ver) {
> +	case KONA_PWM:
> +		/*
> +		 * There must be a min 400ns delay between clearing trigger and
> +		 * settingit. Failing to do this may result in no PWM signal.
> +		 */
> +		ndelay(400);
> +		return 0;
> +
> +	case CYGNUS_PWM:
> +		return readl_poll_timeout(kp->base + PWM_MONITOR_OFFSET, value,
> +					  !(value & (BIT(chan))), 0,
> +					  PWM_MONITOR_TIMEOUT_US);
> +
> +	default:
> +		return -ENODEV;
> +
> +	}
> +}

This function is the only difference between these two otherwise similar
implementations. If you do the following instead:

	static int kona_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan)
	{
		ndelay(400);
		return 0;
	}

	static int cygnus_pwmc_wait_stable(struct pwm_chip *chip, unsigned int chan)
	{
		return readl_poll_timeout(...);
	}

and then maintain a

	int (*wait_stable)(struct pwm_chip *, unsigned int);

in struct kona_pwmc that is initialized in .probe you save quite some
checks for the version.

> +
>  /*
>   * Clear trigger bit but set smooth bit to maintain old output.
>   */
> -static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> +static int kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>  	unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> @@ -88,14 +124,10 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>  	value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/*
> -	 * There must be a min 400ns delay between clearing trigger and setting
> -	 * it. Failing to do this may result in no PWM signal.
> -	 */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +static int kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  {
>  	unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>  
> @@ -104,8 +136,7 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  	value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>  	writel(value, kp->base + PWM_CONTROL_OFFSET);
>  
> -	/* Trigger bit must be held high for at least 400 ns. */
> -	ndelay(400);
> +	return kona_pwmc_wait_stable(&kp->chip, chan, kp->version);
>  }
>  
>  static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -113,8 +144,13 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	unsigned int chan = pwm->hwpwm;
>  	unsigned int value;
> +	int ret;
>  
> -	kona_pwmc_prepare_for_settings(kp, chan);
> +	ret = kona_pwmc_prepare_for_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to prepare pwm settings: %d\n", ret);
> +		return;
> +	}
>  
>  	/* Simulate a disable by configuring for zero duty */
>  	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> @@ -125,7 +161,11 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	value &= ~PRESCALE_MASK(chan);
>  	writel(value, kp->base + PRESCALE_OFFSET);
>  
> -	kona_pwmc_apply_settings(kp, chan);
> +	ret = kona_pwmc_apply_settings(kp, chan);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to apply pwm settings: %d\n", ret);
> +		return;

I think it would make sense to change kona_pwmc_disable to return an
error code and then use that in the callers.

> +	}
>  
>  	clk_disable_unprepare(kp->clk);
>  }
> @@ -188,7 +228,12 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			}
>  		}
>  
> -		kona_pwmc_prepare_for_settings(kp, chan);
> +		ret = kona_pwmc_prepare_for_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev,
> +				"failed to prepare pwm settings: %d\n", ret);
> +			return ret;
> +		}
>  
>  		value = readl(kp->base + PRESCALE_OFFSET);
>  		value &= ~PRESCALE_MASK(chan);
> @@ -208,7 +253,12 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			writel(value, kp->base + PWM_CONTROL_OFFSET);
>  		}
>  
> -		kona_pwmc_apply_settings(kp, chan);
> +		ret = kona_pwmc_apply_settings(kp, chan);
> +		if (ret < 0) {
> +			dev_err(chip->dev,
> +				"failed to prepare pwm settings: %d\n", ret);
> +			return ret;
> +		}
>  	} else if (cstate.enabled) {
>  		kona_pwmc_disable(chip, pwm);
>  	}
> @@ -221,14 +271,26 @@ static int kona_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	.owner = THIS_MODULE,
>  };
>  
> +static const struct of_device_id bcm_kona_pwmc_dt[] = {
> +	{ .compatible = "brcm,kona-pwm", .data = (void *)KONA_PWM},
> +	{ .compatible = "brcm,cygnus-pwm", .data = (void *)CYGNUS_PWM},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> +
>  static int kona_pwmc_probe(struct platform_device *pdev)
>  {
>  	struct kona_pwmc *kp;
>  	struct resource *res;
> +	const struct of_device_id *of_id;
>  	unsigned int chan;
>  	unsigned int value = 0;
>  	int ret = 0;
>  
> +	of_id = of_match_node(bcm_kona_pwmc_dt, pdev->dev.of_node);
> +	if (!of_id)
> +		return -ENODEV;
> +
>  	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
>  	if (kp == NULL)
>  		return -ENOMEM;
> @@ -241,6 +303,7 @@ static int kona_pwmc_probe(struct platform_device *pdev)
>  	kp->chip.npwm = 6;
>  	kp->chip.of_xlate = of_pwm_xlate_with_flags;
>  	kp->chip.of_pwm_n_cells = 3;
> +	kp->version = (enum kona_pwmc_ver)of_id->data;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	kp->base = devm_ioremap_resource(&pdev->dev, res);
> @@ -287,12 +350,6 @@ static int kona_pwmc_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&kp->chip);
>  }
>  
> -static const struct of_device_id bcm_kona_pwmc_dt[] = {
> -	{ .compatible = "brcm,kona-pwm" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> -
>  static struct platform_driver kona_pwmc_driver = {
>  	.driver = {
>  		.name = "bcm-kona-pwm",

Best regards
Uwe

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