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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YIL+Xwjbk1EE04Sm@orome.fritz.box>
Date:   Fri, 23 Apr 2021 19:05:35 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Lee Jones <lee.jones@...aro.org>,
        devicetree@...r.kernel.org, linux-pwm@...r.kernel.org,
        punit1.agrawal@...hiba.co.jp, yuji2.ishikawa@...hiba.co.jp,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM
 support

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)