Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 5 Nov 2020 14:32:41 +0200 | 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@linaro.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
| |