Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC

[Uwe Kleine-KÃnig]

Hello,

On Thu, Jul 23, 2020 at 12:16:18PM +0800, Tanwar, Rahul wrote:
> On 14/7/2020 3:10 am, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote:
> >> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> >> This PWM controller does not have any other consumer, it is a
> >> dedicated PWM controller for fan attached to the system. Add
> >> driver for this PWM fan controller.
> >>
> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/pwm/Kconfig         |  11 ++
> >>  drivers/pwm/Makefile        |   1 +
> >>  drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 278 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
> 
> [...]
> 
> >> +
> >> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >> +			 const struct pwm_state *state)
> >> +{
> >> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> >> +	u32 duty_cycle, val;
> >> +	unsigned int period;
> >> +
> >> +	if (!state->enabled) {
> >> +		lgm_pwm_enable(chip, 0);
> >> +		return 0;
> >> +	}
> >> +
> >> +	period = min_t(u64, state->period, pc->period);
> >> +
> >> +	if (state->polarity != PWM_POLARITY_NORMAL ||
> >> +	    period < pc->period)
> >> +		return -EINVAL;
> > This check looks wrong. If you refuse period < pc->period there isn't
> > much configuration possible.
> 
> I am kind of stuck here. I made this change of adding a check
> period < pc->period based on your feedback on v2 patch.
> In fact, you had specified this code in v2 review feedback
> and i used the same exact code.

My feedback was nonsense, sorry. Now I don't understand anymore what I
thought. The check you proposed in v2 is perfectly fine.

> How should we handle it when the hardware supports fixed period.
> We don't want user to change period and allow just changing
> duty_cycle. With that intention, i had first added a strict check
> which refused configuration if period != pc->period. Period is
> intended to be a read only value.
> 
> How do you suggest we handle the fixed period hardware support?
> Would you have any reference example of other drivers which also
> supports fixed period? Thanks.
> 
> [...]
> >> +static int lgm_pwm_remove(struct platform_device *pdev)
> >> +{
> >> +	struct lgm_pwm_chip *pc = platform_get_drvdata(pdev);
> >> +	int ret;
> >> +
> >> +	ret = pwmchip_remove(&pc->chip);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	clk_disable_unprepare(pc->clk);
> >> +	reset_control_assert(pc->rst);
> > Please swap the two previous lines to match the error patch of .probe.
> 
> Again, i had made this change based on your below review feedback
> for v1. IMO, reverse of probe makes more sense.
> 
> "In .probe() you first release reset and then enable the clock. It's good
> style to do it the other way round in .remove()."

Again my comment was nonsense, I'm sorry. I think I was irritated by the
fact that reset handling happens in between the clk stuff in probe. You
do there:

	devm_clk_get
	devm_reset_control_get_exclusive
	reset_control_deassert
	clk_prepare_enable

and I noticed that as "in probe clk handling is done first".

Looking at the other feedback I think my other comments are not BS.

Best regards and sorry again,
Uwe

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

Attachment: signature.asc
Description: PGP signature