On Fri, Apr 23, 2021 at 07:05:35PM +0200, Thierry Reding wrote: > 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. If this BUG_ON (or your if) triggers we have a compiler or memory problem. The relevant parts of the code are: if (state->period > (0xffff << 3) * 1000) period = (0xffff << 3) * 1000; else period = state->period; period /= 1000; if (period > 0xffff) { pwmc0 = ilog2(period >> 16); BUG_ON(pwmc0 > 3); Given that period is never bigger than 0xffff << 3 when it is used to calculate the argument to ilog2, pwmc0 <= ilog2(7) = 2. Hmm, I wonder if the formula is wrong given that pwmc0 never becomes 3?! Should this better be pwmc0 = fls(period >> 16); ? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
