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 <[email protected]> > >> --- > >> 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/ |
signature.asc
Description: PGP signature
