linux-kernel - Re: [PATCH 2/2 v5] pwm: Add driver for AXI PWM generator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sat, 1 Jun 2024 12:57:48 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Trevor Gamblin <tgamblin@...libre.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	michael.hennerich@...log.com, nuno.sa@...log.com, dlechner@...libre.com, 
	devicetree@...r.kernel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org
Subject: Re: [PATCH 2/2 v5] pwm: Add driver for AXI PWM generator

Hello Trevor,

thanks for your patience waiting for me to come around looking at your
patch. I only found some trivial stuff to fix this time around.

On Wed, Apr 24, 2024 at 08:58:48AM -0400, Trevor Gamblin wrote:
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> new file mode 100644
> index 000000000000..e0bf90cc2ba3
> --- /dev/null
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AXI PWM generator
> + *
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 Baylibre SAS

If there is public documentation available for the device, extra points
for a link to it here.

> [...]
> +static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(regmap, AXI_PWMGEN_REG_CORE_MAGIC, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != AXI_PWMGEN_REG_CORE_MAGIC_VAL)
> +		return dev_err_probe(dev, -ENODEV,
> +			"failed to read expected value from register: got %08x, expected %08x\n",
> +			val,
> +			AXI_PWMGEN_REG_CORE_MAGIC_VAL);

Join these two lines, please.

> +	ret = regmap_read(regmap, AXI_PWMGEN_REG_CORE_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (ADI_AXI_PCORE_VER_MAJOR(val) != 2) {
> +		return dev_err_probe(dev, -ENODEV, "Unsupported peripheral version %u.%u.%u\n",
> +			ADI_AXI_PCORE_VER_MAJOR(val),
> +			ADI_AXI_PCORE_VER_MINOR(val),
> +			ADI_AXI_PCORE_VER_PATCH(val));
> +	}
> +
> +	/* Enable the core */
> +	ret = regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_REG_CONFIG_RESET, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Return the number of PWMs */
> +	return val;

If the AXI_PWMGEN_REG_NPWM register contains a value >= 0x80000000, the
return value is considered a negative error code. Not sure how necessary
it is to check for this case. I'll leave that to you.

> +}
> [...]
> +static int axi_pwmgen_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	struct pwm_chip *chip;
> +	struct axi_pwmgen_ddata *ddata;
> +	struct clk *clk;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	io_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	regmap = devm_regmap_init_mmio(dev, io_base, &axi_pwmgen_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "failed to init register map\n");
> +
> +	ret = axi_pwmgen_setup(regmap, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip = devm_pwmchip_alloc(dev, ret, sizeof(*ddata));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	ddata = pwmchip_get_drvdata(chip);
> +	ddata->regmap = regmap;
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	ret = devm_clk_rate_exclusive_get(dev, clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
> +
> +	ret = devm_add_action_or_reset(dev, axi_pwmgen_clk_rate_exclusive_put, clk);
> +	if (ret)
> +		return ret;

this should be dropped as devm_clk_rate_exclusive_get() already takes
care for calling clk_rate_exclusive_put().

> +	ddata->clk_rate_hz = clk_get_rate(clk);
> +	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
> +
> +	chip->ops = &axi_pwmgen_pwm_ops;
> +	chip->atomic = true;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "could not add PWM chip\n");
> +
> +	return 0;
> +}

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)