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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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