mbox series

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

Message ID 20210419000007.1944301-1-nobuhiro1.iwamatsu@toshiba.co.jp
Headers show
Series [v6,1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller | expand

Message

Nobuhiro Iwamatsu April 19, 2021, midnight UTC 
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

Comments

Thierry Reding April 23, 2021, 5:05 p.m. UTC | #1 
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
Uwe Kleine-König April 23, 2021, 5:20 p.m. UTC | #2 
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