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

[Uwe Kleine-König]

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