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
