Re: [PATCH v5 2/3] pinctrl: qcom: Refactor target specific pinctrl driver

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

 




On 5/3/2023 3:11 PM, Andy Shevchenko wrote:
On Wed, May 3, 2023 at 8:39 AM Rohit Agarwal <quic_rohiagar@xxxxxxxxxxx> wrote:
Update the msm_function and msm_pingroup structure to reuse the generic
structures

pinfunction and pingroup structures. Also refactor pinctrl drivers to adjust
the new macro and updated structure defined in pinctrl.h and pinctrl_msm.h
respectively.
Thanks for this, my comments below.

...

  #define FUNCTION(fname)                                        \
         [APQ_MUX_##fname] = {                           \
-               .name = #fname,                         \
-               .groups = fname##_groups,               \
-               .ngroups = ARRAY_SIZE(fname##_groups),  \
-       }
+               .func = PINCTRL_PINFUNCTION(#fname,                     \
+                                       fname##_groups,                 \
+                                       ARRAY_SIZE(fname##_groups))             \
+                       }
Does it really make sense to keep an additional wrapper data type that
does not add any value? Can't we simply have
This was done as part of embeding the pinfunction structure in msm_function.
Will drop this in the next.
   #define FUNCTION(fname)      [...fname] = PINCTRL_PINFUNCTION(...)

?

...

+               .grp = PINCTRL_PINGROUP("gpio"#id, gpio##id##_pins,     \
+                       (unsigned int)ARRAY_SIZE(gpio##id##_pins)),     \
Why do you need this casting? Same Q to all the rest of the similar cases.
Will drop it.
...

+#include <linux/pinctrl/pinctrl.h>
Keep it separate, and below the generic ones...
Sure

  #include <linux/pm.h>
  #include <linux/types.h>

...like here (note also a blank line).

...

  /**
   * struct msm_function - a pinmux function
- * @name:    Name of the pinmux function.
- * @groups:  List of pingroups for this function.
- * @ngroups: Number of entries in @groups.
+ * @func: Generic data of the pin function (name and groups of pins)
   */
  struct msm_function {
-       const char *name;
-       const char * const *groups;
-       unsigned ngroups;
+       struct pinfunction func;
  };
But why? Just kill the entire structure.
Got it. Can we have a typedef for pinfunction to msm_function in the msm header file?
...

  #define FUNCTION(fname)                                        \
This definition appears in many files, instead you can make a generic
to this drivers one and use it here

#define QCOM_FUNCTION(_prefix_, _fname_)
   [_prefix_##_fname_] = PINCTRL_PINFUNCTION(...)

#define FUNCTION(fname) QCOM_FUNCTION(msm_mux, fname)

(this just a pseudocode, might not even be compilable)

         [msm_mux_##fname] = {                           \
-               .name = #fname,                         \
-               .groups = fname##_groups,               \
-               .ngroups = ARRAY_SIZE(fname##_groups),  \
+               .func = PINCTRL_PINFUNCTION(#fname,                     \
+                                       fname##_groups,                 \
+                                       ARRAY_SIZE(fname##_groups))             \
         }
Got your point. Maybe your option 2 of using MSM_PIN_FUNCTION seems more generic, as there wont be any redefinition of any macro FUNCTION altogether in the target specific files.

Thanks,
Rohit.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux