| 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
| ||
|
Date: Thu, 5 Nov 2020 14:32:41 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Andy Gross <agross@...nel.org>, linux-arm-msm@...r.kernel.org,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver
On Thu, Nov 5, 2020 at 2:06 PM Srinivas Kandagatla
<srinivas.kandagatla@...aro.org> wrote:
>
> Add initial pinctrl driver to support pin configuration for
> LPASS (Low Power Audio SubSystem) LPI (Low Power Island) pinctrl
> on SM8250.
> +config PINCTRL_LPASS_LPI
> + tristate "Qualcomm Technologies Inc LPASS LPI pin controller driver"
> + depends on GPIOLIB && OF
> + help
> + This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> + Qualcomm Technologies Inc LPASS (Low Power Audio SubSystem) LPI
> + (Low Power Island) found on the Qualcomm Technologies Inc SoCs.
> +#include <linux/of_device.h>
> +#include <linux/of.h>
...
> + val = lpi_gpio_read(pctrl, pin, LPI_GPIO_REG_VAL_CTL);
> + val &= ~(LPI_GPIO_REG_FUNCTION_MASK);
Redundant parentheses.
> + val |= i << LPI_GPIO_REG_FUNCTION_SHIFT;
> + lpi_gpio_write(pctrl, pin, LPI_GPIO_REG_VAL_CTL, val);
...
> +static unsigned int lpi_drive_to_regval(u32 arg)
> +{
> + return (arg/2 - 1);
Ditto. On top, use spaces.
> +}
...
> + case PIN_CONFIG_SLEW_RATE:
> + if (arg > LPI_SLEW_RATE_MAX) {
> + dev_err(pctldev->dev, "%s: invalid slew rate %u for pin: %d\n",
> + __func__, arg, pin);
__func__ is not needed.
> + goto set_gpio;
> + }
...
> + for (i = 0; i < LPI_SLEW_BITS_SIZE; i++) {
> + if (arg & 0x01)
> + set_bit(offset, &val);
> + else
> + clear_bit(offset, &val);
assign_bit(, arg & BIT(i))
> + offset++;
> + arg = arg >> 1;
No need on a separate line, see above.
> + }
...
> +done:
Useless label.
> + return ret;
...
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/seq_file.h>
> +#else
> +#define lpi_gpio_dbg_show NULL
> +#endif
Hmm... Doesn't pin control provide a wrapper for this?
...
> + int ret, npins;
> + struct clk *core_vote = NULL;
> + struct clk *audio_vote = NULL;
> +
> + struct lpi_pinctrl *pctrl;
> + const struct lpi_pinctrl_variant_data *data;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
Redundant blank line. Can you keep them in reversed xmas tree order?
...
> + core_vote = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(core_vote)) {
> + dev_dbg(&pdev->dev, "%s: clk get %s failed %d\n",
> + __func__, "core_vote", ret);
First of all you missed the deferred probe issue, second, __func__ is
redundant for *_dbg() calls (okay, when Dynamic Debug is enabled).
That said why not
return dev_err_probe();
?
> + return PTR_ERR(core_vote);
> + }
...
> + audio_vote = devm_clk_get(&pdev->dev, "audio");
> + if (IS_ERR(audio_vote)) {
> + dev_dbg(&pdev->dev, "%s: clk get %s failed %d\n",
> + __func__, "audio_vote", ret);
> + return PTR_ERR(audio_vote);
Ditto/
> + }
Why is it not a bulk?
> + clk_prepare_enable(pctrl->core_vote);
> + clk_prepare_enable(pctrl->audio_vote);
Either from them may return an error. Also, when you go devm_*() the
rule of thumb is either all or none. Because here you will have
ordering issue on ->remove().
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pctrl->tlmm_base = devm_ioremap_resource(&pdev->dev, res);
devm_platform_ioremap_resource()
> + if (IS_ERR(pctrl->tlmm_base)) {
> + ret = PTR_ERR(pctrl->tlmm_base);
> + goto err;
> + }
> +
> +
One blank line is enough.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + pctrl->slew_base = devm_ioremap_resource(&pdev->dev, res);
As above.
> + if (IS_ERR(pctrl->slew_base)) {
> + ret = PTR_ERR(pctrl->slew_base);
> + goto err;
> + }
...
> + ret = gpiochip_add_data(&pctrl->chip, pctrl);
Not devm_?
> + if (ret) {
> + dev_err(pctrl->dev, "can't add gpio chip\n");
> + goto err_pinctrl;
> + }
> + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(dev), 0, 0, npins);
Why not to define a proper callback?
> + if (ret) {
> + dev_err(dev, "failed to add pin range\n");
> + goto err_range;
> + }
...
> +err_range:
> + gpiochip_remove(&pctrl->chip);
> +err_pinctrl:
> + mutex_destroy(&pctrl->slew_access_lock);
> +err:
> + clk_disable_unprepare(pctrl->core_vote);
> + clk_disable_unprepare(pctrl->audio_vote);
> +
> + return ret;
These are not needed for devm_ case.
...
> +static int lpi_pinctrl_remove(struct platform_device *pdev)
> +{
> + struct lpi_pinctrl *pctrl = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&pctrl->chip);
> + mutex_destroy(&pctrl->slew_access_lock);
> + clk_disable_unprepare(pctrl->core_vote);
> + clk_disable_unprepare(pctrl->audio_vote);
Ditto. It also has ordering issues.
> + return 0;
> +}
...
> +static const struct of_device_id lpi_pinctrl_of_match[] = {
> + {
> + .compatible = "qcom,sm8250-lpass-lpi-pinctrl",
> + .data = &sm8250_lpi_data,
> + },
> + { },
Comma is not needed here.
> +};
> +
Extra blank line/
> +MODULE_DEVICE_TABLE(of, lpi_pinctrl_of_match);
...
> +static struct platform_driver lpi_pinctrl_driver = {
> +};
> +
Extra blank line.
> +module_platform_driver(lpi_pinctrl_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists