Re: [PATCH v14 0/2] Add PWM support for Intel Keem Bay SoC

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5516 bytes --]

On Thu, Oct 22, 2020 at 03:14:45PM +0800, vijayakannan.ayyathurai@intel.com wrote:
> 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 v13:
> - Fix indentation error in dt-binding.
> - Add maintainer name in dt-binding.
> 
> Changes since v12:
> - Use devm_add_action_or_reset() as per Andy suggestion.
> - Do the clk_prepare_enable() after all devm_ stuff as per Uwe suggestion.
> - Optimize keembay_pwm_remove function.
> - Simplify the error handling for pwmchip_add.
> - In Kconfig, Use depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> - Fix indentation error in dt-binding.
> - Add Uwe's Reviewed-by tag and Vijay's Co-developed-by tag.
> 
> Changes since v11:
> - Minor variable name change to match the api needs.
> - Setup polarity as PWM_POLARITY_NORMAL.
> - Add comment for LEADIN register usage.
> 
> 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                     | 240 ++++++++++++++++++
>  4 files changed, 297 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-keembay.c

Both patches applied, though I did change the ordering so that
checkpatch doesn't complain about the compatible string not being
defined. I also changed the -ENOSYS error code to -EINVAL for the
unsupported polarity case because checkpatch doesn't like it when
we use -ENOSYS other than for system calls.

I'm also going to follow up with a patch that makes the return value
more consistent for these cases in other drivers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]