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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6375 bytes --]

Hello,

On Fri, Sep 18, 2020 at 07:31:40AM +0900, Nobuhiro Iwamatsu wrote:
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index 000000000000..601450111166
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0

The SPDX guys deprecated "GPL-2.0" as identifier and recommend
"GPL-2.0-only" instead. As in the kernel both are allowed I prefer the
latter.

> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + * Copyright (c) 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> + *
> + */

If there is a publically available manual, please add a link here.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
> +
> +#define PWMC0_PWMACT BIT(5)
> +
> +#define REG_PCSR(ch) (0x400 + 4 * (ch))
> +#define REG_PDUT(ch) (0x420 + 4 * (ch))
> +#define REG_PWM0(ch) (0x440 + 4 * (ch))

Please us a prefix for the register defines. Also it would be great if
it would be obvious from the naming to which register the PWMACT bit
belongs.

> +struct visconti_pwm_chip {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +#define to_visconti_chip(chip) \
> +	container_of(chip, struct visconti_pwm_chip, chip)
> +
> +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> +	u32 period, duty, pwmc0;
> +
> +	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> +		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
> +	if (state->enabled) {
> +		period = state->period / 1000;
> +		duty = state->duty_cycle / 1000;
> +		if (period < 0x10000)
> +			pwmc0 = 0;
> +		else if (period < 0x20000)
> +			pwmc0 = 1;
> +		else if (period < 0x40000)
> +			pwmc0 = 2;
> +		else if (period < 0x80000)
> +			pwmc0 = 3;
> +		else
> +			return -EINVAL;
> +
> +		if (pwmc0) {
> +			period /= BIT(pwmc0);
> +			duty /= BIT(pwmc0);
> +		}

You can drop the if and just make this:

	period <<= pwmc0;
	duty <<= pwmc0;

as this is a noop if pwmc0 is zero.

> +		if (state->polarity == PWM_POLARITY_INVERSED)
> +			pwmc0 |= PWMC0_PWMACT;
> +
> +		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
> +		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
> +		writel(period, priv->base + REG_PCSR(pwm->hwpwm));

Some comments about the function of the hardware would be good.
Something like (I assume the optimal hardware here, please adapt to
reality):

	pwmc is a 2-bit divider for the input clock running at 1 MHz.
	When the settings of the PWM are modified, the new values are
	shadowed in hardware until the period register (PCSR) is written
	and the currently running period is completed. This way the
	hardware switches atomically from the old setting to the new.
	Also disabling the hardware completes the currently running
	period and then the output drives the inactive state.

(I'm sure however this is wrong because you don't consider
state->polarity in the !state-enabled case.)

If your hardware doesn't behave as indicated please add a Limitations
paragraph at the beginning of the driver as is done for several other
drivers already describing the shortcomings.

> +	} else {
> +		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops visconti_pwm_ops = {
> +	.apply = visconti_pwm_apply,

Please implement .get_state(). (And test it using PWM_DEBUG.)

> +	.owner = THIS_MODULE,
> +};
> +
> +static int visconti_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct visconti_pwm_chip *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;

You can better use

	priv->dev = dev;

here. (But I agree to the previous review that it makes little sense to
keep this member in struct visconti_pwm_chip.)

> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "unable to map I/O space\n");

devm_platform_ioremap_resource already emits an error message on failure,
so no need to add another.

> +		return PTR_ERR(priv->base);
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &visconti_pwm_ops;
> +	priv->chip.base = -1;
> +	priv->chip.npwm = 4;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);

Please use dev_err_probe here or %pe for the error code.

> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "visconti PWM registered\n");

Please degrade this to dev_dbg.

> +	return 0;
> +}
> +
> +static int visconti_pwm_remove(struct platform_device *pdev)
> +{
> +	struct visconti_pwm_chip *priv = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&priv->chip);
> +}
> +
> +static const struct of_device_id visconti_pwm_of_match[] = {
> +	{ .compatible = "toshiba,pwm-visconti", },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, visconti_pwm_of_match);

Please drop the empty line before MODULE_DEVICE_TABLE.

> +static struct platform_driver visconti_pwm_driver = {
> +	.driver = {
> +		.name = "pwm-visconti",
> +		.of_match_table = visconti_pwm_of_match,
> +	},
> +	.probe = visconti_pwm_probe,
> +	.remove = visconti_pwm_remove,
> +};
> +
> +module_platform_driver(visconti_pwm_driver);

The empty line before module_platform_driver is also unusual.

> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Toshiba");
> +MODULE_ALIAS("platform:visconti-pwm");

This is wrong; as the driver name is pwm-visconti this should be
MODULE_ALIAS("platform:pwm-visconti");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]