From: "Tanwar, Rahul" <rahul.tanwar@linux.intel.com> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: thierry.reding@gmail.com, p.zabel@pengutronix.de, linux-pwm@vger.kernel.org, robh+dt@kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, andriy.shevchenko@intel.com, songjun.Wu@intel.com, cheol.yong.kim@intel.com, qi-ming.wu@intel.com Subject: Re: [PATCH v1 2/2] Add PWM driver for LGM Date: Wed, 27 May 2020 14:28:53 +0800 [thread overview] Message-ID: <3a1f1e83-2d9d-ddbf-e2e6-9c8bab87372b@linux.intel.com> (raw) In-Reply-To: <20200522085613.ktb2ruw2virj337v@pengutronix.de> Hi Uwe, Thanks for review. On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: > Hello, > > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: >> Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC. >> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> >> --- >> drivers/pwm/Kconfig | 9 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 366 insertions(+) >> create mode 100644 drivers/pwm/pwm-intel-lgm.c >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index eebbc917ac97..a582214f50b2 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -232,6 +232,15 @@ config PWM_IMX_TPM >> To compile this driver as a module, choose M here: the module >> will be called pwm-imx-tpm. >> >> +config PWM_INTEL_LGM >> + tristate "Intel LGM PWM support" >> + depends on X86 || COMPILE_TEST >> + help >> + Generic PWM framework driver for LGM SoC. [...] >> +}; >> + >> +static void tach_work(struct work_struct *work) >> +{ >> + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip, >> + work.work); >> + struct regmap *regmap = pc->regmap; >> + u32 fan_tach, fan_dc, val; >> + s32 diff; >> + static u32 fanspeed_err_cnt, time_window, delta_dc; >> + >> + /* >> + * Fan speed is tracked by reading the active duty cycle of PWM output >> + * from the active duty cycle register. Some variance in the duty cycle >> + * register value is expected. So we set a time window of 30 seconds and >> + * if we detect inaccurate fan speed 6 times within 30 seconds then we >> + * mark it as fan speed problem and fix it by readjusting the duty cycle. >> + */ > I'm a unhappy to have this in the PWM driver. The PWM driver is supposed > to be generic and I think this belongs into a dedicated driver. Well noted about all other review concerns. I will rework the driver in v2. However, i am not very sure about the above point - of having a separate dedicated driver for tach_work because its logic is tightly coupled with this driver. Regards, Rahul
WARNING: multiple messages have this Message-ID (diff)
From: "Tanwar, Rahul" <rahul.tanwar-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> To: "Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org, linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, songjun.Wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, cheol.yong.kim-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, qi-ming.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org Subject: Re: [PATCH v1 2/2] Add PWM driver for LGM Date: Wed, 27 May 2020 14:28:53 +0800 [thread overview] Message-ID: <3a1f1e83-2d9d-ddbf-e2e6-9c8bab87372b@linux.intel.com> (raw) In-Reply-To: <20200522085613.ktb2ruw2virj337v-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Hi Uwe, Thanks for review. On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: > Hello, > > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: >> Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC. >> >> Signed-off-by: Rahul Tanwar <rahul.tanwar-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> >> --- >> drivers/pwm/Kconfig | 9 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 366 insertions(+) >> create mode 100644 drivers/pwm/pwm-intel-lgm.c >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index eebbc917ac97..a582214f50b2 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -232,6 +232,15 @@ config PWM_IMX_TPM >> To compile this driver as a module, choose M here: the module >> will be called pwm-imx-tpm. >> >> +config PWM_INTEL_LGM >> + tristate "Intel LGM PWM support" >> + depends on X86 || COMPILE_TEST >> + help >> + Generic PWM framework driver for LGM SoC. [...] >> +}; >> + >> +static void tach_work(struct work_struct *work) >> +{ >> + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip, >> + work.work); >> + struct regmap *regmap = pc->regmap; >> + u32 fan_tach, fan_dc, val; >> + s32 diff; >> + static u32 fanspeed_err_cnt, time_window, delta_dc; >> + >> + /* >> + * Fan speed is tracked by reading the active duty cycle of PWM output >> + * from the active duty cycle register. Some variance in the duty cycle >> + * register value is expected. So we set a time window of 30 seconds and >> + * if we detect inaccurate fan speed 6 times within 30 seconds then we >> + * mark it as fan speed problem and fix it by readjusting the duty cycle. >> + */ > I'm a unhappy to have this in the PWM driver. The PWM driver is supposed > to be generic and I think this belongs into a dedicated driver. Well noted about all other review concerns. I will rework the driver in v2. However, i am not very sure about the above point - of having a separate dedicated driver for tach_work because its logic is tightly coupled with this driver. Regards, Rahul
next prev parent reply other threads:[~2020-05-27 6:28 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-22 7:41 [PATCH v1 0/2] pwm: intel: Add PWM driver for new SoC Rahul Tanwar 2020-05-22 7:41 ` Rahul Tanwar 2020-05-22 7:41 ` [PATCH v1 1/2] Add YAML schema for a new PWM driver Rahul Tanwar 2020-05-28 23:31 ` Rob Herring 2020-05-28 23:31 ` Rob Herring 2020-05-22 7:41 ` [PATCH v1 2/2] Add PWM driver for LGM Rahul Tanwar 2020-05-22 8:56 ` Uwe Kleine-König 2020-05-22 9:18 ` Andy Shevchenko 2020-05-22 9:18 ` Andy Shevchenko 2020-05-22 9:32 ` Uwe Kleine-König 2020-05-27 6:28 ` Tanwar, Rahul [this message] 2020-05-27 6:28 ` Tanwar, Rahul 2020-05-27 9:15 ` Andy Shevchenko 2020-05-28 5:45 ` Tanwar, Rahul 2020-05-28 5:45 ` Tanwar, Rahul
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3a1f1e83-2d9d-ddbf-e2e6-9c8bab87372b@linux.intel.com \ --to=rahul.tanwar@linux.intel.com \ --cc=andriy.shevchenko@intel.com \ --cc=cheol.yong.kim@intel.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=p.zabel@pengutronix.de \ --cc=qi-ming.wu@intel.com \ --cc=robh+dt@kernel.org \ --cc=songjun.Wu@intel.com \ --cc=thierry.reding@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.