Message-ID: <529D4B58.9020700@atmel.com>
Date:	Tue, 3 Dec 2013 11:09:12 +0800
From:	Bo Shen <voice.shen@...el.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	Bo Shen <voice.shen@...el.com>, <linux-pwm@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <nicolas.ferre@...el.com>,
	<linux-kernel@...r.kernel.org>,
	<alexandre.belloni@...e-electrons.com>, <galak@...eaurora.org>,
	<plagnioj@...osoft.com>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 1/2] PWM: atmel-pwm: add PWM controller driver

Hi Thierry,

On 12/02/2013 06:59 PM, Thierry Reding wrote:
> On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote:
> [...]
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> [...]
>> +/* Max value for duty and period
>
> Block comments should be of this form:
>
> 	/*
> 	 * Max value ...
> 	 * ...
> 	 */

OK, I will use this style.

>> +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +		int duty_ns, int period_ns)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +	unsigned long clk_rate, prd, dty;
>> +	unsigned long long div;
>> +	int ret, pres = 0;
>> +
>> +	clk_rate = clk_get_rate(atmel_pwm->clk);
>> +	div = clk_rate;
>> +
>> +	/* Calculate the period cycles */
>> +	while (div > PWM_MAX_PRD) {
>> +		div = clk_rate / (1 << pres);
>> +		div = div * period_ns;
>> +		/* 1/Hz = 100000000 ns */
>
> I don't think that comment is needed.

This is asked to be added.
And, I think keep it and it won't hurt, what do you think?

>> +		do_div(div, 1000000000);
>> +
>> +		if (pres++ > PRD_MAX_PRES) {
>> +			dev_err(chip->dev, "pres exceed the maximum value\n");
>
> "exceeds"

Thanks for correct it.

>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	/* Calculate the duty cycles */
>> +	prd = div;
>> +	div *= duty_ns;
>> +	do_div(div, period_ns);
>> +	dty = div;
>> +
>> +	ret = clk_enable(atmel_pwm->clk);
>> +	if (ret) {
>> +		dev_err(chip->dev, "failed to enable pwm clock\n");
>
> "PWM clock"

OK, I will change all low case pwm to upper case PWM.

>> +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				int dty, int prd)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +	unsigned int val;
>> +
>> +	/*
>> +	 * If the PWM channel is disabled, write value to duty and period
>> +	 * registers directly.
>> +	 * If the PWM channel is enabled, using the update register, it needs
>> +	 * to set bit 10 of CMR to 0
>> +	 */
>
> I think it would make sense to split this comment and move each part
> into the respective conditional branch.

OK, I will split them.

>> +	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>> +
>> +		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>> +		val &= ~PWM_CMR_UPD_CDTY;
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> +	} else {
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>> +	}
>> +}
>> +
>> +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				int dty, int prd)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +
>> +	/*
>> +	 * If the PWM channel is disabled, write value to duty and period
>> +	 * registers directly.
>> +	 * If the PWM channel is enabled, using the duty update register to
>> +	 * update the value.
>> +	 */
>
> Same here.
>
>> +	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
>> +	} else {
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd);
>> +	}
>> +}
>
> Neither version 1 nor version 2 seem to be able to change the period
> while the channel is enabled. Perhaps that should be checked for in
> atmel_pwm_config() and an error (-EBUSY) returned?

The period is configured in dt in device tree, or platform data in non 
device tree. Nowhere will update period. So, not code to update period.
Am I right? If not, please figure out.

>> +
>> +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> +		enum pwm_polarity polarity)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +	u32 val = 0;
>> +	int ret;
>> +
>> +	if (polarity == PWM_POLARITY_NORMAL)
>> +		val &= ~PWM_CMR_CPOL;
>> +	else
>> +		val |= PWM_CMR_CPOL;
>
> I think I've mentioned this before, but val is always assigned to 0, so
> clearing a bit is a superfluous. Perhaps you need to readl the CMR
> register first before toggling the bit here?

Thanks, we should read CMR, and set the CPOL accordingly.

>> +
>> +	ret = clk_enable(atmel_pwm->clk);
>> +	if (ret) {
>> +		dev_err(chip->dev, "failed to enable pwm clock\n");
>
> "PWM clock"
>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id atmel_pwm_dt_ids[] = {
>> +	{
>> +		.compatible = "atmel,at91sam9rl-pwm",
>> +		.data = &atmel_pwm_data_v1,
>> +	}, {
>> +		.compatible = "atmel,sama5d3-pwm",
>> +		.data = &atmel_pwm_data_v2,
>> +	}, {
>> +		/* sentinel */
>> +	},
>> +};
>> +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>> +#endif
>
> I don't think you can do this. You use this table in a call to
> of_match_device() later on, in code which isn't protected by a
> corresponding #ifdef.

I will remove #ifdef.

>> +static inline const struct atmel_pwm_data * __init
>> +	atmel_pwm_get_driver_data(struct platform_device *pdev)
>
> I don't think __init is warranted here. In fact I think this will give
> you a build warning, because this code is called from atmel_pwm_probe(),
> which in turn isn't marked __init.

OK, I will remove __init.

> Also it's probably not worth marking this inline explicitly. It isn't
> all that short, and the compiler will likely inline it anyway since it's
> only called once.

It only called one, so, it can be inline.

>> +{
>> +	if (pdev->dev.of_node) {
>> +		const struct of_device_id *match;
>> +		match = of_match_device(atmel_pwm_dt_ids, &pdev->dev);
>
> Blank line between the above two for readability.

OK, I will add one blank line.

>> +		if (match == NULL)
>> +			return NULL;
>> +		return match->data;
>
> Same here. And "if (!match)" rather than "if (match == NULL)".

OK, I will change like this.

>> +	}
>> +
>> +	return (struct atmel_pwm_data *)
>> +		platform_get_device_id(pdev)->driver_data;
>
> Please use a temporary variable here to make this more readable, like
> so:
>
> 	struct platform_device_id *id = platform_get_device_id(pdev);
>
> 	...
>
> 	return (struct atmel_pwm_data *)id->driver;
>
>> +}

OK, I will change like this.

>> +static int atmel_pwm_probe(struct platform_device *pdev)
>> +{
>> +	const struct atmel_pwm_data *data;
>> +	struct atmel_pwm_chip *atmel_pwm;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL);
>> +	if (!atmel_pwm)
>> +		return -ENOMEM;
>
> You could move this further down, so that memory doesn't get allocated
> if atmel_pwm_get_driver_data() or platform_get_resource() fails.

OK, I will move this down.

>> +
>> +	data = atmel_pwm_get_driver_data(pdev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -ENODEV;
>
> No need to check the return value here. devm_ioremap_resource() checks
> it for you.

OK, I will remove this check.

>> +
>> +	atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(atmel_pwm->base))
>> +		return PTR_ERR(atmel_pwm->base);
>> +
>> +	atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(atmel_pwm->clk))
>> +		return PTR_ERR(atmel_pwm->clk);
>> +
>> +	ret = clk_prepare(atmel_pwm->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to prepare pwm clock\n");
>
> "PWM clock"
>
>> +		return ret;
>> +	}
>> +
>> +	atmel_pwm->chip.dev = &pdev->dev;
>> +	atmel_pwm->chip.ops = &atmel_pwm_ops;
>> +	if (pdev->dev.of_node) {
>
> Blank line between the above two for readability.

OK, I will add blank line.

>> +		atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> +		atmel_pwm->chip.of_pwm_n_cells = 3;
>> +		atmel_pwm->chip.base = -1;
>> +	} else {
>> +		atmel_pwm->chip.base = pdev->id;
>
> That's not correct. The chip cannot be tied to pdev->id, because that ID
> is the instance number of the device. So typically you would have
> devices name like this:
>
> 	atmel-pwm.0
> 	atmel-pwm.1
> 	...
>
> Now, if you have that, then you won't be able to register the second
> instance because the first instance will already have requested PWMs
> 0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which
> intersects with the range of the first instance.
>
> The same applies of course if you have other PWM controllers in the
> system which have similar instance names.
>
> So the right thing to do here is to provide that number via platform
> data so that platform code can define it, knowing in advance all ranges
> for all other PWM controllers and thereby make sure there's no
> intersection.

OK, I will fix this.

>> +	}
>> +	atmel_pwm->chip.npwm = 4;
>
> Blank line between the above two for readability.

OK, I will add blank line.

>> +	atmel_pwm->config = data->config;
>> +
>> +	ret = pwmchip_add(&atmel_pwm->chip);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
>
> "PWM chip"
>
> Thierry

Best Regards,
Bo Shen

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists