RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support

[Xiubo Li-B47053]

Hi Thierry,

See the comments below.


> I think the correct capitalizations are: "Freescale", "FTM" and "PWM".
> There are other occurrences in the rest of the driver that I haven't
> explicitly pointed out.
> 

Yes, that's much better.


> > +config PWM_FTM
> 
> Perhaps name this PWM_FSL_FTM to match the driver name?
> 

OK.


> And fix this up to be "pwm-fsl-ftm".

Yes, I will.


> > +#define FTM_CSC_BASE        0x0C
> > +#define FTM_CSC(_CHANNEL) \
> > +	(FTM_CSC_BASE + (_CHANNEL * 0x08))
> 
> I prefer lowercase variables in macros:
> 
> 	#define FTM_CSC(channel) \
> 		(FTM_CSC_BASE + (channel * 8))
> 
Yes, That's better.


> > +#define FTM_PWMMODE         (FTMCnSC_MSB)
> > +#define FTM_HIGH_TRUE       (FTMCnSC_ELSB)
> > +#define FTM_LOW_TRUE        (FTMCnSC_ELSA)
> 
> What's the reason for redefining these? Can't you just use the FTMCnSC_*
> defines directly?
> 

Just for more readable and comprehension.
I will revise it by not losing above two key elements. 

> > +#define FTM_CV(_CHANNEL) \
> > +	(FTM_CV_BASE + (_CHANNEL * 0x08))
> 
> Same comment as for FTM_CSC above.
>

Yes.
 
> > +#define FTM_MAX_CHANNEL     0x08
> 
> There should be no need for this. The only place where you use this is
> when assigning it to pwm_chip.npwm, so you may as well use the literal
> there.
> 

I have recoded the driver, and the macro is used more than one time now.


> > +	struct platform_device *pdev;
> 
> You never use this platform_device. And you have to assign &pdev->dev to
> the pwm_chip.dev anyway, so why not just use that consistently and drop
> the pdev field?
>

Yes, I have droped the pdev field now.
 

> > +	unsigned int	cpwm;
> > +	struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
> 
> Please don't do this. Use pwm_set_chip_data()/pwm_get_chip_data() to
> associate per-channel specific data.
>

I have replaced them now.
 
> > +	/* pinctrl handles */
> 
> Nit: it's only a single handle.
> 

Yes.

> > +	struct pinctrl          *pinctrl;
> 
> You also use tabs and spaces inconsistently here. I suggest you use a
> single space between the data type and the field name, that way it's much
> easier to stay consistent.
> 

Now I have revised all about this.

> > +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm)
> 
> The arguments on the trailing line aren't properly aligned. They should
> be aligned with the arguments on the first line.
> 

The same as the above comment.


> > +	ret = clk_prepare_enable(fpc->clk);
> 
> This should probably be just clk_prepare(). Or is there some reason why
> you can't delay clk_enable() to the .enable() operation?
> 

Firstly, we should be clear that the fpc->clk is chip's work clock.
If so, after the .request() is called and before .enable() is called, the custumer will call .config(), 
in which will read/write the pwm chip registers, if the module clock is still disabled, then the system will hang up.


> > +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> > +			     struct pwm_device *pwm)
> 
> Parameter alignment again. Please also check all other functions.
> 

Yes, I have revise all about this.

> > +{
> > +	int i;
> 
> This should be unsigned int.
> 

I will revise it.

> > +static unsigned long
> > +fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
> 
> The above two lines are 78 characters when joined, so there's no need to
> split them.
> 

OK, I have revise all about this.

> Perhaps time_ns should be "unsigned long"?
> 

Shouldn't this be same with "int duty_ns" and "int period_ns", which are defined by 
struct pwm_ops {
...
	int (*config)(struct pwm_chip *chip,
                    struct pwm_device *pwm,
                    int duty_ns, int period_ns);
...
}  ?


> > +	ps = (0x01 << fpc->clk_ps);
> 
> This is fairly short, so you could do it along with the variable
> declaration. Also there's no need for the parentheses or the hexadecimal
> prefix (0x01 == 1):
> 
> 	unsigned long ps = 1 << fpc->clk_ps;
> 

OK, I will revise it then.

> > +	/* The module clk is HZ/s, the time_ns parameter
> > +	 * is based nanosecond, so here should divide
> > +	 * 1000000000UL.
> > +	 */
> 
> Block comments should be:
> 
> 	/*
> 	 * ...
> 	 * ...
> 	 */
> 
> Also HZ/s isn't a valid unit for a clock frequency. And time_ns also has
> the _ns suffix to designate the unit, so as a whole that comment doesn't
> add any real information and can just as well be dropped.
> 

I will revise it.

> > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
> 
> I think you can safely drop the _channel suffix from the PWM operations.
> 

By adding _channel suffix just for more comprehensave about the pwm's muti-channel operation.
If this is redundant here, I will drop it.


> > +	fpc = to_fsl_chip(chip);
> > +
> > +	if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > +		return -ESHUTDOWN;
> 
> Erm... how do you think this could ever happen? Users need to request a
> PWM to obtain a struct pwm_device, in which case PWMF_REQUESTED will
> always be set. There are a few other occurrences throughout the rest of
> the driver that I haven't pointed out explicitly.
> 

Does the following case is exist ?
The customer in one thread has .free(pwm_1), while in another thread, 
which maybe had slept in for some reason, will call .config/.enable/.disable?

If so, as I have explained before, if the pwm_1 has been freed, the module clock maybe
disabled too, so if the .config is call the system will hang up.



> > +	fpc->fpwms[pwm->hwpwm].period_cycles = period_cycles;
> > +	fpc->fpwms[pwm->hwpwm].duty_cycles = duty_cycles;
> 
> You use these for the sole purpose of passing them into the
> fsl_updata_config() function, so I wonder why you can't just pass them as
> parameters and get rid of struct fsl_pwm as a bonus?
> 

Before I think the fpc has all the information the fsl_update_config() function needed.

Now, I have dropt the fsl_update_config() function.


> > +	fsl_updata_config(fpc, pwm);
> 
> And now that I think about it: perhaps this was supposed to be called
> fsl_update_config() instead of fsl_updat*a*_config()?
> 
Well, a written mistake.


> > +	reg &= ~(0x01 << pwm->hwpwm);
> 
> This would be slightly more canonical as:
> 
> 	reg &= ~BIT(pwm->hwpwm);
> 

Yes, looks much better.

> > +	reg |= (polarity << pwm->hwpwm);
> 
> And perhaps here it would be better to be explicit. I think it's unlikely
> that enum pwm_polarity will even gain a third entry, but better be safe
> than sorry:
> 
> 	if (polarity == PWM_POLARITY_INVERSED)
> 		reg |= BIT(pwm->hwpwm);
> 	else
> 		reg &= ~BIT(pwm->hwpwm);
> 

I will think it over.
While only the polarity's bit0 is used for polarity's statement. If other bit won't has any other
meaning and purpose in the feature, I will replace it derictly.

> > +static int is_any_other_channel_enabled(struct pwm_chip *chip,
> > +				    unsigned int cur)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < chip->npwm; i++) {
> > +		if (i == cur)
> > +			continue;
> > +		if (test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
> > +			return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> This can probably be better done using a reference/enable count. Instead
> of having to iterate over all PWM channels of the chip and checking their
> flags, just keep track of how many times .enable() and .disable() are
> called.
> 
> To make it easier you can probably wrap that into two functions, such as
> fsl_clock_enable() and fsl_clock_disable().
> 
> > +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> > +			      struct pwm_device *pwm)
> > +{
> [...]
> > +	if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> > +		return 0;
> 
> This is where you'd call fsl_clock_enable()...
> 
> > +	reg = readl(fpc->base + FTM_SC);
> > +	reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
> > +			(FTMSC_PS_MASK << FTMSC_PS_OFFSET));
> > +	/* select system clock source */
> > +	reg |= (FTMSC_CLKSYS | fpc->clk_ps);
> > +	writel(reg, fpc->base + FTM_SC);
> 
> ... and run this code in fsl_clock_enable() if the enable count is 0 to
> select the system clock when the first PWM is enabled.
> 
> > +static void fsl_pwm_disable_channel(struct pwm_chip *chip,
> > +				struct pwm_device *pwm)
> > +{
> [...]
> > +	if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> > +		return;
> 
> Then call fsl_clock_disable() here...
> 
> > +	writel(0xFF, fpc->base + FTM_OUTMASK);
> > +	reg = readl(fpc->base + FTM_SC);
> > +	reg &= ~(FTMSC_CLK_MASK << FTMSC_CLK_OFFSET);
> > +	writel(reg, fpc->base + FTM_SC);
> 
> ... and run this code in fsl_clock_disable() if the enable count reaches
> 0 so that the clock is disabled when no PWM channels remain on.
> 

I will think it over and recode about this.


> > +static int fsl_pwm_parse_dt(struct fsl_pwm_chip *fpc) {
> [...]
> > +	int ret = 0;
> > +	u32 chs[FTM_MAX_CHANNEL];
> > +	struct device_node *np = fpc->pdev->dev.of_node;
> > +
> > +	ret = of_property_read_u32(np, "fsl,pwm-clk-ps",
> > +				   &fpc->clk_ps);
> > +	if (ret < 0) {
> > +		dev_err(&fpc->pdev->dev,
> > +				"failed to get pwm "
> > +				"clk prescaler : %d\n",
> > +				ret);
> 
> Perhaps it's more useful to mention the missing property explicitly in
> the error message:
> 
> 		dev_err(fpc->chip.dev,
> 			"failed to parse \"fsl,pwm-clk-ps\" property: %d\n",
> 			ret);
> 

Whil I think the following is better in code. 
 
 		dev_err(fpc->chip.dev,
 			"failed to parse <fsl,pwm-clk-ps> property: %d\n",
 			ret);



> 
> > +		return ret;
> > +	}
> > +	if (fpc->clk_ps > 7 || fpc->clk_ps < 0)
> 
> clk_ps is unsigned and therefore can't be < 0. And a blank line would be
> useful between the previous line ("}") and this line.
> 

I will revise it.

> > +static int fsl_pwm_probe(struct platform_device *pdev) {
> > +	int ret = 0;
> > +	struct fsl_pwm_chip *fpc;
> > +	struct resource *res;
> > +
> > +	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> > +	if (!fpc) {
> > +		dev_err(&pdev->dev,
> > +				"failed to allocate memory\n");
> 
> I don't think that's very useful either. If this should indeed ever fail,
> then the driver core will complain about the probe failing and include
> the error code. Since it's the only location where you return that error
> code you know immediately what went wrong.
> 

Yes, for the devm_kzlloc() have print out the fail reason, this will be redundant.
I have revised all about this.

> > +		return -ENOMEM;
> > +	}
> > +
> > +	mutex_init(&fpc->lock);
> > +
> > +	fpc->pdev = pdev;
> > +
> > +	ret = fsl_pwm_parse_dt(fpc);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	fpc->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(fpc->base)) {
> > +		dev_err(&pdev->dev,
> > +				"failed to ioremap() registers\n");
> 
> No need for this error message. devm_ioremap_resource() prints out errors
> already on failure.
> 

The same comment as the last one.

> > +	fpc->chip.dev = &pdev->dev;
> > +	fpc->chip.ops = &fsl_pwm_ops;
> > +	fpc->chip.of_xlate = of_pwm_xlate_with_flags;
> > +	fpc->chip.of_pwm_n_cells = 3;
> > +	fpc->chip.base = -1;
> > +
> > +	ret = pwmchip_add(&fpc->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev,
> > +				"failed to add ftm0 pwm chip %d\n",
> > +				ret);
> 
> dev_err() will already include the device name in the error message, so I
> don't think you need the "ftm0" here. Also make sure to use the correct
> capitalization for "PWM". And there is no need to split this over so many
> lines, it all fits on a single line just fine. There are other
> occurrences of this, so please double-check.
> 

Yes.
I have revise all about this.



Thanks very much.

--
Best Regards.
Xiubo

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