2021-04-19 00:06:07

by Nobuhiro Iwamatsu

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

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

--
2.30.0.rc2


2021-04-23 17:06:38

by Thierry Reding

[permalink] [raw]
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


Attachments:
(No filename) (4.55 kB)
signature.asc (849.00 B)
Download all attachments

2021-04-23 17:21:54

by Uwe Kleine-König

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

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/ |


Attachments:
(No filename) (5.49 kB)
signature.asc (499.00 B)
Download all attachments