diff mbox series

[v5,5/5] pinctrl: s32: separate const device data from struct s32_pinctrl_soc_info

Message ID 20230327062754.3326-6-clin@suse.com (mailing list archive)
State New, archived
Headers show
Series pinctrl: s32: driver improvements and generic struct use | expand

Commit Message

Chester Lin March 27, 2023, 6:27 a.m. UTC 
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.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Chester Lin <clin@suse.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v5:
- Remove unnecessary (void *) type casting found in pinctrl-s32g2.

Changes in v4:
- Retrieve the matched device data by calling of_device_get_match_data()
  and remove unnecessary type casting. (Merged from the previous v3 series
  [PATCH v3 1/6])

 drivers/pinctrl/nxp/pinctrl-s32.h   | 14 +++++++++-----
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 30 +++++++++++++++++------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c | 14 +++++---------
 3 files changed, 32 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko March 27, 2023, 11:58 a.m. UTC | #1 
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);
Linus Walleij March 27, 2023, 9:39 p.m. UTC | #2 
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
Chester Lin March 29, 2023, 4:25 a.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/drivers/pinctrl/nxp/pinctrl-s32.h b/drivers/pinctrl/nxp/pinctrl-s32.h
index 2f7aecd462e4..add3c77ddfed 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32.h
+++ b/drivers/pinctrl/nxp/pinctrl-s32.h
@@ -34,24 +34,28 @@  struct s32_pin_range {
 	unsigned int end;
 };
 
-struct s32_pinctrl_soc_info {
-	struct device *dev;
+struct s32_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
 	unsigned int npins;
+	const struct s32_pin_range *mem_pin_ranges;
+	unsigned int mem_regions;
+};
+
+struct s32_pinctrl_soc_info {
+	struct device *dev;
+	const struct s32_pinctrl_soc_data *soc_data;
 	struct s32_pin_group *groups;
 	unsigned int ngroups;
 	struct pinfunction *functions;
 	unsigned int nfunctions;
 	unsigned int grp_index;
-	const struct s32_pin_range *mem_pin_ranges;
-	unsigned int mem_regions;
 };
 
 #define S32_PINCTRL_PIN(pin)	PINCTRL_PIN(pin, #pin)
 #define S32_PIN_RANGE(_start, _end) { .start = _start, .end = _end }
 
 int s32_pinctrl_probe(struct platform_device *pdev,
-			struct s32_pinctrl_soc_info *info);
+		      const struct s32_pinctrl_soc_data *soc_data);
 int s32_pinctrl_resume(struct device *dev);
 int s32_pinctrl_suspend(struct device *dev);
 #endif /* __DRIVERS_PINCTRL_S32_H */
diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 8373468719b6..41e024160f36 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -106,7 +106,7 @@  s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	const struct s32_pin_range *pin_range;
-	unsigned int mem_regions = ipctl->info->mem_regions;
+	unsigned int mem_regions = ipctl->info->soc_data->mem_regions;
 	unsigned int i;
 
 	for (i = 0; i < mem_regions; i++) {
@@ -688,8 +688,8 @@  int s32_pinctrl_suspend(struct device *dev)
 	int ret;
 	unsigned int config;
 
-	for (i = 0; i < info->npins; i++) {
-		pin = &info->pins[i];
+	for (i = 0; i < info->soc_data->npins; i++) {
+		pin = &info->soc_data->pins[i];
 
 		if (!s32_pinctrl_should_save(ipctl, pin->number))
 			continue;
@@ -713,8 +713,8 @@  int s32_pinctrl_resume(struct device *dev)
 	struct s32_pinctrl_context *saved_context = &ipctl->saved_context;
 	int ret, i;
 
-	for (i = 0; i < info->npins; i++) {
-		pin = &info->pins[i];
+	for (i = 0; i < info->soc_data->npins; i++) {
+		pin = &info->soc_data->pins[i];
 
 		if (!s32_pinctrl_should_save(ipctl, pin->number))
 			continue;
@@ -831,7 +831,7 @@  static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 	struct resource *res;
 	struct regmap *map;
 	void __iomem *base;
-	int mem_regions = info->mem_regions;
+	unsigned int mem_regions = info->soc_data->mem_regions;
 	int ret;
 	u32 nfuncs = 0;
 	u32 i = 0;
@@ -869,7 +869,7 @@  static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 		}
 
 		ipctl->regions[i].map = map;
-		ipctl->regions[i].pin_range = &info->mem_pin_ranges[i];
+		ipctl->regions[i].pin_range = &info->soc_data->mem_pin_ranges[i];
 	}
 
 	nfuncs = of_get_child_count(np);
@@ -904,20 +904,26 @@  static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 }
 
 int s32_pinctrl_probe(struct platform_device *pdev,
-		      struct s32_pinctrl_soc_info *info)
+		      const struct s32_pinctrl_soc_data *soc_data)
 {
 	struct s32_pinctrl *ipctl;
 	int ret;
 	struct pinctrl_desc *s32_pinctrl_desc;
+	struct s32_pinctrl_soc_info *info;
 #ifdef CONFIG_PM_SLEEP
 	struct s32_pinctrl_context *saved_context;
 #endif
 
-	if (!info || !info->pins || !info->npins) {
+	if (!soc_data || !soc_data->pins || !soc_data->npins) {
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
 
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->soc_data = soc_data;
 	info->dev = &pdev->dev;
 
 	/* Create state holders etc for this driver */
@@ -938,8 +944,8 @@  int s32_pinctrl_probe(struct platform_device *pdev,
 		return -ENOMEM;
 
 	s32_pinctrl_desc->name = dev_name(&pdev->dev);
-	s32_pinctrl_desc->pins = info->pins;
-	s32_pinctrl_desc->npins = info->npins;
+	s32_pinctrl_desc->pins = info->soc_data->pins;
+	s32_pinctrl_desc->npins = info->soc_data->npins;
 	s32_pinctrl_desc->pctlops = &s32_pctrl_ops;
 	s32_pinctrl_desc->pmxops = &s32_pmx_ops;
 	s32_pinctrl_desc->confops = &s32_pinconf_ops;
@@ -960,7 +966,7 @@  int s32_pinctrl_probe(struct platform_device *pdev,
 #ifdef CONFIG_PM_SLEEP
 	saved_context = &ipctl->saved_context;
 	saved_context->pads =
-		devm_kcalloc(&pdev->dev, info->npins,
+		devm_kcalloc(&pdev->dev, info->soc_data->npins,
 			     sizeof(*saved_context->pads),
 			     GFP_KERNEL);
 	if (!saved_context->pads)
diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c
index d9f3ff6794ea..4d01e9a2a23e 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32g2.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c
@@ -721,7 +721,7 @@  static const struct s32_pin_range s32_pin_ranges_siul2[] = {
 	S32_PIN_RANGE(942, 1007),
 };
 
-static struct s32_pinctrl_soc_info s32_pinctrl_info = {
+static struct s32_pinctrl_soc_data s32_pinctrl_data = {
 	.pins = s32_pinctrl_pads_siul2,
 	.npins = ARRAY_SIZE(s32_pinctrl_pads_siul2),
 	.mem_pin_ranges = s32_pin_ranges_siul2,
@@ -730,9 +730,8 @@  static struct s32_pinctrl_soc_info s32_pinctrl_info = {
 
 static const struct of_device_id s32_pinctrl_of_match[] = {
 	{
-
 		.compatible = "nxp,s32g2-siul2-pinctrl",
-		.data = (void *) &s32_pinctrl_info,
+		.data = &s32_pinctrl_data,
 	},
 	{ /* sentinel */ }
 };
@@ -740,14 +739,11 @@  MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match);
 
 static int s32g_pinctrl_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *of_id =
-		of_match_device(s32_pinctrl_of_match, &pdev->dev);
+	const struct s32_pinctrl_soc_data *soc_data;
 
-	if (!of_id)
-		return -ENODEV;
+	soc_data = of_device_get_match_data(&pdev->dev);
 
-	return s32_pinctrl_probe
-			(pdev, (struct s32_pinctrl_soc_info *) of_id->data);
+	return s32_pinctrl_probe(pdev, soc_data);
 }
 
 static const struct dev_pm_ops s32g_pinctrl_pm_ops = {