| Message ID | cover.1602612067.git.vijayakannan.ayyathurai@intel.com |
|---|---|
| Headers | show |
| Series | Add PWM support for Intel Keem Bay SoC | expand |
Hello, sorry, I still found a problem that I want to have addressed. I'll point out a few minor things en passant. But after that I really have a good feeling and like the driver now. On Wed, Oct 14, 2020 at 02:14:12AM +0800, vijayakannan.ayyathurai@intel.com wrote: > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + unsigned long long pwm_h_count, pwm_l_count; > + unsigned long clk_rate; > + u32 buff; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + if (buff & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle */ > + buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC; > + pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC; <minor nit>: The variable names are not optimal. I'd use "highlow" instead of "buff". pwm_l_count would be appropriate for FIELD_GET(KMB_PWM_LOW_MASK, buff); when multiplied with NSEC_PER_SEC it's not really matching. Maybe just use "low"?! (and "high" instead of pwm_h_count) > + state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate); > + state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate); state->polarity = PWM_POLARITY_NORMAL; (That's the important bit here.) > +} > + > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + struct pwm_state current_state; > + u16 pwm_h_count, pwm_l_count; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); A comment describing the effect of this register would be great. > + keembay_pwm_get_state(chip, pwm, ¤t_state); > + > + if (!state->enabled) { > + if (current_state.enabled) > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > [...] Best regards Uwe
Hi Uwe, Thanks for reviewing this patch. -----Original Message----- From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Wednesday, 14 October, 2020 12:19 PM To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux-pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com> Subject: Re: [PATCH v11 1/2] pwm: Add PWM driver for Intel Keem Bay Hello, sorry, I still found a problem that I want to have addressed. I'll point out a few minor things en passant. But after that I really have a good feeling and like the driver now. On Wed, Oct 14, 2020 at 02:14:12AM +0800, vijayakannan.ayyathurai@intel.com wrote: > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + unsigned long long pwm_h_count, pwm_l_count; > + unsigned long clk_rate; > + u32 buff; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + if (buff & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle */ > + buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC; > + pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC; <minor nit>: The variable names are not optimal. I'd use "highlow" instead of "buff". pwm_l_count would be appropriate for FIELD_GET(KMB_PWM_LOW_MASK, buff); when multiplied with NSEC_PER_SEC it's not really matching. Maybe just use "low"?! (and "high" instead of pwm_h_count) Yes. I have also thought of it. So it will look like below in the next version. highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); low = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC; high = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC; Also in other places of it as well. > + state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate); > + state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, > +clk_rate); state->polarity = PWM_POLARITY_NORMAL; (That's the important bit here.) Ok. I will incorporate in next version. > +} > + > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) { > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + struct pwm_state current_state; > + u16 pwm_h_count, pwm_l_count; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); A comment describing the effect of this register would be great. This register is for setting up the number of repetition and leadin low time for that particular channel. I will update it accordingly in the next version. > + keembay_pwm_get_state(chip, pwm, ¤t_state); > + > + if (!state->enabled) { > + if (current_state.enabled) > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > [...] Best regards Uwe
From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> Hi, This patch set enables the support for PWM in the Intel Keem Bay SoC. Keem Bay is an ARM based SoC, and the GPIO module allows configuration of 6 PWM outputs. Patch 1 adds the PWM driver and Patch 2 is for the required Device Tree bindings documentation. This driver was tested on the Keem Bay evaluation module board. Thank you. Regards, Vijay Changes since v10: - Update low time calculation formula as per Uwe. - During distruct remove pwmchip first then disable the clock. Changes since v9: - Remove Reported-by tag from the commit log. Changes since v8: - Fix the compilation error reported by kernel test robot. - Add the tag Reported-by: kernel test robot <lkp@intel.com> - Minor correction in the pwm low time calculation formula. - Rebase with 5.9-rc7 Changes since v7: - Change the dependency as ARCH_KEEMBAY instead of ARM64 in Kconfig. - Use DIV_ROUND_DOWN_ULL instead of DIV_ROUND_CLOSEST_ULL. - Update the right formula as per Uwe. - List the tags in chronological order. - Add clk_disable_unprepare in the error paths. Changes since v6: - Add reviewed-by tag Changes since v5: - Reorder symbols/Kconfig in drivers/pwm/Kconfig and drivers/pwm/Makefile - Use "Limitations" for consistency - Add clk_prepare_enable() - Reorder keembay_pwm_get_state() function call - Rework if conditional for channel disablement in .apply() - Remove channel disabling from .probe(), and clear LEADIN register bits in .apply instead - Update commit message for Patch 1 Changes since v4: - Add co-developed-by tag - Include mod_devicetable.h and remove of.h - Update comment with correct calulation for high/low time - Fix missing return from dev_err_probe Changes since v3: - Removed variable for address and calculate in place instead - Utilized u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET - Utilized dev_err_probe() for error reporting - Updated comments to use physical units - Updated error check for pwmchip_add() Changes since v2: - Include documentation about HW limitation/behaviour - Use hex values for KMB_PWM_COUNT_MAX - Redefine register macros - Utilize FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and pwm_count - Round up duty cycle/period values - Get current hardware state in .apply instead of cached values - Do a polarity check before .enabled - Round high time/low time to closest value - Set enable bit in KMB_PWM_LEADIN_OFFSET to 0 in probe - Correct the naming for MODULE_ALIAS - Add additionalProperties: false in DT bindings Changes since v1: - Updated licensing info, "clocks" property and example in DT bindings - Updated name of DT bindings document to match compatible string - Removed 1 patch for addition of new sysfs attribute "count" - Added support for COMPILE_TEST in Kconfig - Updated naming of defines and regmap attribute - Updated calculation of waveform high time and low time - Added range checking for waveform high/low time - Implemented .get_state - Removed register writes for lead-in and count values (left to default) - Updated register access to single-access - Folded keembay_pwm_enable/disable_channel, keembay_pwm_config_period/duty_cycle, and keembay_pwm_config into keembay_pwm_apply - Updated error messages/error codes - Removed pwm_disable from keembay_pwm_remove - Removed clk_prepare/clk_enable/clk_disable from driver Lai, Poey Seng (1): pwm: Add PWM driver for Intel Keem Bay Vineetha G. Jaya Kumaran (1): dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM .../bindings/pwm/intel,keembay-pwm.yaml | 47 ++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-keembay.c | 228 ++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml create mode 100644 drivers/pwm/pwm-keembay.c base-commit: 549738f15da0e5a00275977623be199fbbf7df50 prerequisite-patch-id: 0a348762b660d0d817b8e70cc71647e83173c78c prerequisite-patch-id: 0c6072cfe492b078c44ec864b8f9d1c76eada93b prerequisite-patch-id: 12b93428ee51a3d92ca973b928c0e0989f5d585e