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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Andy for the review,

On 05/11/2020 12:32, Andy Shevchenko wrote:
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>


I agree with most of the style related comments! will fix them in next version!

...

+#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?

I does, but the custom code can provide additional information (such as pullup/pulldown configuration) which default one does not provide.

Most of the pinctrl drivers have there own version of 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();
?
It looks neat, I will use that!
Thanks for this hint, I never knew we had some function like that!



+               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?

I can try that!

+       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()

make sense, I remember doing this! somehow I missed it in this version!

rest of the comments looks sensible to me, will make sure that those are fixed in next version.


thanks,
srini



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux