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
