Re: [PATCH 1/2] arm64: dts: ti: k3-pinctrl: Introduce deep sleep macros

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On Fri, Nov 15, 2024 at 09:48:22AM -0600, Rob Herring wrote:

Hello Rob,

> On Tue, Nov 12, 2024 at 05:26:49PM +0530, Siddharth Vadapalli wrote:
> > The behavior of pins in deep sleep mode can be configured by programming
> > the corresponding bits in the respective Pad Configuration register. Add
> > macros to support this.
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >  arch/arm64/boot/dts/ti/k3-pinctrl.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > index 22b8d73cfd32..cac7cccc1112 100644
> > --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > @@ -12,6 +12,12 @@
> >  #define PULLTYPESEL_SHIFT	(17)
> >  #define RXACTIVE_SHIFT		(18)
> >  #define DEBOUNCE_SHIFT		(11)
> > +#define FORCE_DS_EN_SHIFT	(15)
> > +#define DS_EN_SHIFT		(24)
> > +#define DS_OUT_DIS_SHIFT	(25)
> > +#define DS_OUT_VAL_SHIFT	(26)
> > +#define DS_PULLUD_EN_SHIFT	(27)
> > +#define DS_PULLTYPE_SEL_SHIFT	(28)
> >  
> >  #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
> >  #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
> > @@ -38,6 +44,19 @@
> >  #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
> >  #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
> >  
> > +#define PIN_DS_FORCE_DISABLE		(0 << FORCE_DS_EN_SHIFT)
> > +#define PIN_DS_FORCE_ENABLE		(1 << FORCE_DS_EN_SHIFT)
> > +#define PIN_DS_IO_OVERRIDE_DISABLE	(0 << DS_IO_OVERRIDE_EN_SHIFT)
> > +#define PIN_DS_IO_OVERRIDE_ENABLE	(1 << DS_IO_OVERRIDE_EN_SHIFT)
> > +#define PIN_DS_OUT_ENABLE		(0 << DS_OUT_DIS_SHIFT)
> > +#define PIN_DS_OUT_DISABLE		(1 << DS_OUT_DIS_SHIFT)
> > +#define PIN_DS_OUT_VALUE_ZERO		(0 << DS_OUT_VAL_SHIFT)
> > +#define PIN_DS_OUT_VALUE_ONE		(1 << DS_OUT_VAL_SHIFT)
> > +#define PIN_DS_PULLUD_ENABLE		(0 << DS_PULLUD_EN_SHIFT)
> > +#define PIN_DS_PULLUD_DISABLE		(1 << DS_PULLUD_EN_SHIFT)
> > +#define PIN_DS_PULL_DOWN		(0 << DS_PULLTYPE_SEL_SHIFT)
> > +#define PIN_DS_PULL_UP			(1 << DS_PULLTYPE_SEL_SHIFT)
> 
> Are you going to go add the 0 defines to all the existing cases? If you 
> do, it's a lot of pointless churn. If you don't, then it is inconsistent 
> when they do get used. I would drop them all.

The "0 defines" are present for the existing cases as well, namely:
PULL_ENABLE, PULL_DOWN and INPUT_DISABLE are all "0 defines".

Other existing macros are defined in terms of the above, due to which it
might have appeared to be the case that only some of the "0 defines" are
present. For example, the following macros make use of the "0 defines":
PIN_OUTPUT, PIN_OUTPUT_PULLUP, PIN_OUTPUT_PULLDOWN and PIN_INPUT_PULLDOWN

So the current patch is consistent with the existing convention followed
in the k3-pinctrl.h file. Please let me know if I should still drop the
"0 defines" in this patch.

Regards,
Siddharth.