Re: [PATCH v6 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Thierry Reding]

On Mon, Apr 19, 2021 at 09:00:05AM +0900, Nobuhiro Iwamatsu wrote:
> Hi,
> 
> This series is the PWM driver for Toshiba's ARM SoC, Visconti[0].
> This provides DT binding documentation and device driver.
> 
> [0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html
> 
> Updates:
> 
>   dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
>     v5 -> v6:
>       - No update.
>     v4 -> v5:
>       - No update.
>     v3 -> v4:
>       - No update.
>     v2 -> v3:
>       - Change compatible to toshiba,visconti-pwm
>       - Change filename to toshiba,visconti-pwm.yaml.
>       - Add Reviewed-by tag from Rob.
>     v1 -> v2:
>       - Change SPDX-License-Identifier to GPL-2.0-only OR BSD-2-Clause.
>       - Set compatible toshiba,pwm-visconti only.
>       - Drop unnecessary comments.
> 
>   pwm: visconti: Add Toshiba Visconti SoC PWM support
>     v5 -> v6:
>      - Update year in copyright.
>      - Update limitations.
>      - Fix coding style, used braces for both branches.
>     v4 -> v5:
>       - Droped checking PIPGM_PCSR from visconti_pwm_get_state.
>       - Changed from to_visconti_chip to visconti_pwm_from_chip.
>       - Removed pwmchip_remove return value management.
>       - Add limitations of this device.
>       - Add 'state->enabled = true' to visconti_pwm_get_state().
>     v3 -> v4:
>       - Sorted alphabetically include files.
>       - Changed container_of to using static inline functions.
>       - Dropped unnecessary dev_dbg().
>       - Drop Initialization of chip.base.
>       - Drop commnet "period too small".
>       - Rebased for-next. 
>     v2 -> v3:
>       - Change compatible to toshiba,visconti-pwm.
>       - Fix MODULE_ALIAS to platform:pwm-visconti, again.
>       - Align continuation line to the opening parenthesis.
>       - Rewrite the contents of visconti_pwm_apply() based on the contents suggested by Uwe.
>     v1 -> v2:
>       - Change SPDX-License-Identifier to GPL-2.0-only.
>       - Add prefix for the register defines.
>       - Drop struct device from struct visconti_pwm_chip.
>       - Use '>>' instead of '/'.
>       - Drop error message by devm_platform_ioremap_resource().
>       - Use dev_err_probe instead of dev_err.
>       - Change dev_info to dev_dbg.
>       - Remove some empty lines.
>       - Fix MODULE_ALIAS to platform:pwm-visconti.
>       - Add .get_state() function.
>       - Use the author name and email address to MODULE_AUTHOR.
>       - Add more comment to function of the hardware.
>       - Support .get_status() function.
>       - Use NSEC_PER_USEC instead of 1000.
>       - Alphabetically sorted for Makefile and Kconfig.
>       - Added check for set value in visconti_pwm_apply().
> 
> Nobuhiro Iwamatsu (2):
>   dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
>   pwm: visconti: Add Toshiba Visconti SoC PWM support
> 
>  .../bindings/pwm/toshiba,pwm-visconti.yaml    |  43 ++++
>  drivers/pwm/Kconfig                           |   9 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-visconti.c                    | 189 ++++++++++++++++++
>  4 files changed, 242 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
>  create mode 100644 drivers/pwm/pwm-visconti.c

Both patches applied, thanks.

checkpatch did complain when I applied:

> WARNING: please write a paragraph that describes the config symbol fully
> #9: FILE: drivers/pwm/Kconfig:604:
> +config PWM_VISCONTI

That seems a bit excessive. The paragraph is perhaps not a poster child
for Kconfig, but there are others that aren't better, so I think that's
fine.

> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #32: 
> new file mode 100644

Fine, too.

> WARNING: 'loosing' may be misspelled - perhaps 'losing'?
> #112: FILE: drivers/pwm/pwm-visconti.c:76:
>  +	 * NSEC_PER_SEC / CLKFREQ = 1000 without loosing precision.
>   	                                         ^^^^^^^

I've fixed that up while applying.

> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
> #127: FILE: drivers/pwm/pwm-visconti.c:91:
> +		BUG_ON(pwmc0 > 3);

I think that one is legit. I've turned that into:

	if (WARN_ON(pwmc0 > 3))
		return -EINVAL;

so that requests for too big period will be rejected rather than crash
the system. Next time, please run checkpatch before submitting and
eliminate at least the big warnings.

Thierry

Attachment: signature.asc
Description: PGP signature