Hi Andy,

On Wed, Mar 08, 2023 at 03:21:00PM +0200, Andy Shevchenko wrote:

On Wed, Mar 8, 2023 at 7:03 AM Chester Lin [off-list ref] wrote:

quoted

On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@gmail.com wrote:

quoted

Mon, Feb 20, 2023 at 10:33:19AM +0800, Chester Lin kirjoitti:

...

quoted

but the driver patch
has been merged into the maintainer's for-next so I would not change this part
unless the driver patch needs to be reverted and re-submitted in the end.

As I said you have to keep it in mind for all your future
contributions to the Linux kernel independently on the destiny of this
one.

...

quoted

quoted

quoted

+   depends on ARCH_S32 && OF

Is OF necessary? Can it be

I think it's required since the driver file refers to of_* APIs.

And? Is it functional or compilation dependency? If the latter is the
case, what API exactly isn't providing a stub?

I was wrong. Looks like the ARM64 arch Kconfig always select OF so it's not
really necessary to have OF here.

quoted

quoted

      depends OF || COMPILE_TEST

?

So?

Since the OF dependency is not really necessary here, to fulfill the compile test
purpose, the possible dependency might be (ARCH_S32 || COMPILE_TEST), but it
could meet a compiling failure on the reference of pinconf_generic_parse_dt_config()
for those architectures which do not select OF by default since there's no stub
for this function. [pinconf_generic_parse_dt_config() is called in pinctrl-s32cc.c]

...

quoted

quoted

quoted

+   depends on ARCH_S32 && OF

Ditto.

Based on the previous assumption [OF is not needed and PINCTRL_S32CC doesn't
depend on COMPILE_TEST], selecting PINCTRL_S32G2 wouldn't work if it simply
depends on (ARCH_S32 || COMPILE_TEST), for example:

  WARNING: unmet direct dependencies detected for PINCTRL_S32CC
    Depends on [n]: PINCTRL [=y] && ARCH_S32
      Selected by [y]:
        - PINCTRL_S32G2 [=y] && PINCTRL [=y] && (ARCH_S32 || COMPILE_TEST [=y])

So the better solutions is to still have OF in PINCTRL_S32CC, such as:

config PINCTRL_S32CC
	bool
	depends on ARCH_S32 || (OF && COMPILE_TEST)
	.....

config PINCTRL_S32G2
	depends on ARCH_S32 || COMPILE_TEST
	.....

Regards,
Chester

quoted

quoted

quoted

+/**
+ * struct s32_pin_group - describes an S32 pin group
+ * @name: the name of this specific pin group
+ * @npins: the number of pins in this group array, i.e. the number of
+ *         elements in pin_ids and pin_sss so we can iterate over that array
+ * @pin_ids: an array of pin IDs in this group
+ * @pin_sss: an array of source signal select configs paired with pin_ids
+ */
+struct s32_pin_group {
+   const char *name;
+   unsigned int npins;
+   unsigned int *pin_ids;
+   unsigned int *pin_sss;

Why didn't you embed struct pingroup?

I did think about that but there's an additional 'pin_sss' which could make code
a bit messy. For example:

        s32_regmap_update(pctldev, grp->grp.pins[i],
                          S32_MSCR_SSS_MASK, grp->pin_sss[i]);

We specifically provide those data types to separate generic things
with custom ones. I don't think about the code getting longer, the
access to the proper data seems reasonable to me. Look into other
drivers that utilise these data types.

Will change it, thanks for your suggestions.

quoted

quoted

quoted

+};
+
+/**
+ * struct s32_pmx_func - describes S32 pinmux functions
+ * @name: the name of this specific function
+ * @groups: corresponding pin groups
+ * @num_groups: the number of groups
+ */
+struct s32_pmx_func {
+   const char *name;
+   const char **groups;
+   unsigned int num_groups;
+};

struct pinfunction.

Thanks for your information. I was not aware of this new struct since it just got
merged recently.

That's why the rule is to keep an eye on the subsystem development by
regular rebasing on top of its tip (pinctrl tree, devel branch in this
case).

...

quoted

quoted

quoted

+#ifdef CONFIG_PM_SLEEP
+int __maybe_unused s32_pinctrl_resume(struct device *dev);
+int __maybe_unused s32_pinctrl_suspend(struct device *dev);
+#endif

Please, consider using new PM macros, like pm_ptr().

Maybe pm_sleep_ptr() is more accurate?

You are the author, choose what you think fits the best!

...

quoted

quoted

quoted

+   case PIN_CONFIG_BIAS_PULL_UP:
+           if (arg)
+                   *config |= S32_MSCR_PUS;
+           else
+                   *config &= ~S32_MSCR_PUS;

quoted

+           fallthrough;

It's quite easy to miss this and tell us about how is it supposed to work with PU + PD?

I admit that it's ambiguous and should be improved in order to have better readability.

In a S32G2 MSCR register, there are two register bits related to pull up/down functions:

PUE (Pull Enable, MSCR[13]): 0'b: Disabled,  1'b: Enabled
PUS (Pull Select, MSCR[12]): 0'b: Pull Down, 1'b: Pull Up

The dt properties could be like these:

1) 'bias-pull-up' or 'bias-pull-up: true'  --> arg = 1
   In this case both PUE and PUS are set.

2) 'bias-pull-up: false'  --> arg = 0
   In this case both PUE and PUS are cleared since the pull-up function must be disabled.

So, split it to a separate function where you do the enabling only once.
I can point to drivers/pinctrl/intel/pinctrl-intel.c for the idea to take from.

Will do.

quoted

quoted

quoted

+   case PIN_CONFIG_BIAS_PULL_DOWN:
+           if (arg)
+                   *config |= S32_MSCR_PUE;
+           else
+                   *config &= ~S32_MSCR_PUE;
+           *mask |= S32_MSCR_PUE | S32_MSCR_PUS;
+           break;
+   case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+           *config &= ~(S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE);
+           *mask |= S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE;
+           fallthrough;

Ditto.

It's similar to the case 'PIN_CONFIG_BIAS_PULL_UP' although the PUS bit is assumed
as 0 via the config variable so only the PUE bit needs to be configured, for example:

1) 'bias-pull-down' or 'bias-pull-down: true'  --> arg = 1
   PUE is set and PUS is cleared.

2) 'bias-pull-down: false'  --> arg = 0
   In this case both PUE and PUS are cleared since the pull-down function must be disabled.

quoted

quoted

+   case PIN_CONFIG_BIAS_DISABLE:
+           *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
+           *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+           break;

Ditto.

...

I assume that non-commented is equal to silent agreement and will be
addressed accordingly. If it's not the case, reply again with your
objections.

-- 
With Best Regards,
Andy Shevchenko