Received-SPF: pass (google.com: domain of linux-kernel+bounces-197803-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 1 Jun 2024 12:57:48 +0200
From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <ukleinek@kernel.org>
To: Trevor Gamblin <tgamblin@baylibre.com>
Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	michael.hennerich@analog.com, nuno.sa@analog.com, dlechner@baylibre.com, 
	devicetree@vger.kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Subject: Re: [PATCH 2/2 v5] pwm: Add driver for AXI PWM generator
Message-ID: <de2trti4ilp3zw47u6qddg64ghiosvnrmknu3lkce635qushvq@qcoz3nfjuyrx>
References: <20240424125850.4189116-1-tgamblin@baylibre.com>
 <20240424125850.4189116-3-tgamblin@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wyvehzs256ug4w5r"
Content-Disposition: inline
In-Reply-To: <20240424125850.4189116-3-tgamblin@baylibre.com>


--wyvehzs256ug4w5r
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--wyvehzs256ug4w5r
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmZa/poACgkQj4D7WH0S
/k7Ccwf/XbVPD01QvCpVWIitYjlnQFA3OeXZOb78SAPUiKsNRvo4iQ+meWKOoUf8
PGXMMBuVKNkndiKXn0AIIp4XIsmXv+8J8/p2TnTrTLE1OpB6gc3TTtdp0Y8LTRDJ
mFsk+0jrGHwc5GG8OGehTKabJzUdCwJh5K3fYwsbinFob9ud52oiLDf6OPPQg8Wi
m2DE3HLo2tGCzJvaioTEipN/M0vqJpmAkg+B10cNAIBqVEPn+tQxA4/HrD5GF29c
l0YDjYj9se/IsyymwyREnm5/yEzIm6sGed+FVCowawq7P74SPBlIKtfMYEuhKJ+/
tI16LwfQxQtdQKMqIbpdyEOJu8/A/A==
=EJQa
-----END PGP SIGNATURE-----

--wyvehzs256ug4w5r--