Re: [PATCH v2 1/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver

From: Andy Shevchenko
Date: Thu Nov 05 2020 - 07:31:54 EST


On Thu, Nov 5, 2020 at 2:06 PM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> 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