| Message ID | 20230327062754.3326-1-clin@suse.com |
|---|---|
| Headers | show |
| Series | pinctrl: s32: driver improvements and generic struct use | expand |
On Mon, Mar 27, 2023 at 9:28 AM Chester Lin <clin@suse.com> wrote: > > The .data field in struct of_device_id is used as a const member so it's > inappropriate to attach struct s32_pinctrl_soc_info with of_device_id > because some members in s32_pinctrl_soc_info need to be filled by > pinctrl-s32cc at runtime. > > For this reason, struct s32_pinctrl_soc_info must be allocated in > pinctrl-s32cc and then create a new struct s32_pinctrl_soc_data in order > to represent const .data in of_device_id. To combine these two structures, > a s32_pinctrl_soc_data pointer is introduced in s32_pinctrl_soc_info. > > Besides, use of_device_get_match_data() instead of of_match_device() since > the driver only needs to retrieve the .data from of_device_id. ... > -static struct s32_pinctrl_soc_info s32_pinctrl_info = { > +static struct s32_pinctrl_soc_data s32_pinctrl_data = { I'm wondering why it's not const. But don't resend too quickly, let's wait for Linus to comment on this and other stuff. It might be that he can amend this when applying. ... > + const struct s32_pinctrl_soc_data *soc_data; > > + soc_data = of_device_get_match_data(&pdev->dev);
On Mon, Mar 27, 2023 at 8:28 AM Chester Lin <clin@suse.com> wrote: > Improve error/return code handlings and config checks in order to have > better reliability and simplify driver codes such as removing/changing > improper macros, blanks, print formats and helper calls. > > Signed-off-by: Chester Lin <clin@suse.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Patch applied! Yours, Linus Walleij
On Mon, Mar 27, 2023 at 8:28 AM Chester Lin <clin@suse.com> wrote: > Use generic data structure to describe pin control groups in S32 SoC family > and drop duplicated struct members. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Chester Lin <clin@suse.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Patch applied! Yours, Linus Walleij
Hi Linus and Andy, On Mon, Mar 27, 2023 at 11:39:18PM +0200, Linus Walleij wrote: > On Mon, Mar 27, 2023 at 1:59 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 27, 2023 at 9:28 AM Chester Lin <clin@suse.com> wrote: > > > > > > The .data field in struct of_device_id is used as a const member so it's > > > inappropriate to attach struct s32_pinctrl_soc_info with of_device_id > > > because some members in s32_pinctrl_soc_info need to be filled by > > > pinctrl-s32cc at runtime. > > > > > > For this reason, struct s32_pinctrl_soc_info must be allocated in > > > pinctrl-s32cc and then create a new struct s32_pinctrl_soc_data in order > > > to represent const .data in of_device_id. To combine these two structures, > > > a s32_pinctrl_soc_data pointer is introduced in s32_pinctrl_soc_info. > > > > > > Besides, use of_device_get_match_data() instead of of_match_device() since > > > the driver only needs to retrieve the .data from of_device_id. > > > > ... > > > > > -static struct s32_pinctrl_soc_info s32_pinctrl_info = { > > > +static struct s32_pinctrl_soc_data s32_pinctrl_data = { > > > > I'm wondering why it's not const. > > > > But don't resend too quickly, let's wait for Linus to comment on this > > and other stuff. It might be that he can amend this when applying. > > I don't dare to add const here given the compiler warnings it > can easily spawn. > > Chester can you investigate if these can be static const? > > You would only need to resend this patch 5/5 because I applied > all the others to lower your patch stack. > > Thanks for fixing! > Yours, > Linus Walleij Thanks for reviewing this patch and Andy's suggestion looks good to me. The s32_pinctrl_data should be const as well since the 'data' pointer in of_device_id is declared as const. Anyway, I have resent a revised 5/5 under the same mail thread: https://lore.kernel.org/all/20230329041630.8011-1-clin@suse.com/T/#u It can be compiled by the following two gcc versions without a warning on drivers/pinctrl/nxp - aarch64-native: - gcc version 7.5.0 (SUSE Linux) <aarch64-native> - cross-compilation on x86-64 - gcc version 13.0.1 20230127 (experimental) [revision ca8fb0096713a8477614ef874f16ba5bf16c48bc] (SUSE Linux) Thanks! Regards, Chester
Hello, This patch series contains some improvements for s32 pinctrl drivers suggested by upstream[1], such as - Fix error shadowings and improve return value handlings. - Fix print format. - Remove unnecessary blanks. - Use proper macros and helpers to simplify codes. - Refactor config param parsing and remove config arguments that are never used. - Use generic struct pingroup and struct pinfunction to describe pin data. Regards, Chester [1] https://lore.kernel.org/all/20230220023320.3499-1-clin@suse.com/ Changes in v5: - Remove unnecessary (void *) type casting found in pinctrl-s32g2. Changes in v4: - Link: https://lore.kernel.org/lkml/20230324143626.16336-1-clin@suse.com/ - Merge the of_device_get_match_data() patch [v3 1/6] into the last patch [v4 5/5, "pinctrl: s32: separate const device data ..."] in order to solve compiler warning properly. Changes in v3: - Link: https://lore.kernel.org/lkml/20230323144833.28562-1-clin@suse.com/ - Remove unnecessary type casting and correct type qualifiers. - Split the previous generic-struct patch [v2 4/4] into two separate patches. - Add a new patch [v3 6/6] to attach a real const .data with of_device_id. Changes in v2: - Link: https://lore.kernel.org/lkml/20230320163823.886-1-clin@suse.com/ - Use of_device_get_match_data() to get matched of_device_id data. - Enhance sizeof() arguments. - Fix blanks and remove unnecessary parentheses. - Drop unnecessary marcos and s32_pin_config() implemented in v1 and set/clear mask/config values transparently. - Put pull-function related cases together in s32_pin_set_pull(). - Simply use generic 'struct pinfunction' rather than having extra 'struct s32_pmx_func'. Chester Lin (5): pinctrl: s32: refine error/return/config checks and simplify driver codes pinctrl: s32cc: refactor pin config parsing pinctrl: s32cc: embed generic struct pingroup pinctrl: s32cc: Use generic struct data to describe pin function pinctrl: s32: separate const device data from struct s32_pinctrl_soc_info drivers/pinctrl/nxp/pinctrl-s32.h | 40 ++-- drivers/pinctrl/nxp/pinctrl-s32cc.c | 282 ++++++++++++++++------------ drivers/pinctrl/nxp/pinctrl-s32g2.c | 17 +- 3 files changed, 178 insertions(+), 161 deletions(-)