[PATCHv2] drivers: tpm2: update reset gpio semantics

[PATCHv2] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz]

Use the more generic reset-gpios propery name.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
  v2: kept gpio-reset as legacy

 .../tpm2/tis-tpm2-spi.txt                     |  2 +-
 drivers/tpm/tpm2_tis_spi.c                    | 21 ++++++++++++-------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
index 3a2ee4bd17..bbcd12950f 100644
--- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
+++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
@@ -6,7 +6,7 @@ Required properties:
 - reg			: SPI Chip select
 
 Optional properties:
-- gpio-reset		: Reset GPIO (if not connected to the SoC reset line)
+- reset-gpios		: Reset GPIO (if not connected to the SoC reset line)
 - spi-max-frequency	: See spi-bus.txt
 
 Example:
diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
index 4b33ac8fd3..1f9f89f68f 100644
--- a/drivers/tpm/tpm2_tis_spi.c
+++ b/drivers/tpm/tpm2_tis_spi.c
@@ -589,18 +589,23 @@ static int tpm_tis_spi_probe(struct udevice *dev)
 	if (CONFIG_IS_ENABLED(DM_GPIO)) {
 		struct gpio_desc reset_gpio;
 
-		ret = gpio_request_by_name(dev, "gpio-reset", 0,
+		ret = gpio_request_by_name(dev, "reset-gpios", 0,
 					   &reset_gpio, GPIOD_IS_OUT);
 		if (ret) {
-			log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
-			    __func__);
-		} else {
-			dm_gpio_set_value(&reset_gpio, 1);
-			mdelay(1);
-			dm_gpio_set_value(&reset_gpio, 0);
+			/* legacy reset */
+			ret = gpio_request_by_name(dev, "gpio-reset", 0,
+						   &reset_gpio, GPIOD_IS_OUT);
+			if (ret) {
+				log(LOGC_NONE, LOGL_NOTICE,
+				    "%s: missing reset GPIO\n",  __func__);
+				goto init;
+			}
 		}
+		dm_gpio_set_value(&reset_gpio, 1);
+		mdelay(1);
+		dm_gpio_set_value(&reset_gpio, 0);
 	}
-
+init:
 	/* Ensure a minimum amount of time elapsed since reset of the TPM */
 	mdelay(drv_data->time_before_first_cmd_ms);
 
-- 
2.31.1

Re: [PATCHv2] drivers: tpm2: update reset gpio semantics

[Michal Simek]

On 6/1/21 8:09 AM, Jorge Ramirez-Ortiz wrote:
> Use the more generic reset-gpios propery name.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>   v2: kept gpio-reset as legacy
> 
>  .../tpm2/tis-tpm2-spi.txt                     |  2 +-
>  drivers/tpm/tpm2_tis_spi.c                    | 21 ++++++++++++-------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> index 3a2ee4bd17..bbcd12950f 100644
> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> @@ -6,7 +6,7 @@ Required properties:
>  - reg			: SPI Chip select
>  
>  Optional properties:
> -- gpio-reset		: Reset GPIO (if not connected to the SoC reset line)

As I said you shouldn't remove this. Just extend description that it is
deprecated and reset-gpios should be used instead.

> +- reset-gpios		: Reset GPIO (if not connected to the SoC reset line)
>  - spi-max-frequency	: See spi-bus.txt
>  
>  Example:
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index 4b33ac8fd3..1f9f89f68f 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -589,18 +589,23 @@ static int tpm_tis_spi_probe(struct udevice *dev)
>  	if (CONFIG_IS_ENABLED(DM_GPIO)) {
>  		struct gpio_desc reset_gpio;
>  
> -		ret = gpio_request_by_name(dev, "gpio-reset", 0,
> +		ret = gpio_request_by_name(dev, "reset-gpios", 0,
>  					   &reset_gpio, GPIOD_IS_OUT);
>  		if (ret) {
> -			log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
> -			    __func__);
> -		} else {
> -			dm_gpio_set_value(&reset_gpio, 1);
> -			mdelay(1);
> -			dm_gpio_set_value(&reset_gpio, 0);
> +			/* legacy reset */
> +			ret = gpio_request_by_name(dev, "gpio-reset", 0,
> +						   &reset_gpio, GPIOD_IS_OUT);
> +			if (ret) {
> +				log(LOGC_NONE, LOGL_NOTICE,
> +				    "%s: missing reset GPIO\n",  __func__);
> +				goto init;
> +			}

And here it is clear that gpio-reset is used which should deprecated
that's why you should print message about it here.


>  		}
> +		dm_gpio_set_value(&reset_gpio, 1);
> +		mdelay(1);
> +		dm_gpio_set_value(&reset_gpio, 0);
>  	}

What about this to remove that goto?

ret = gpio_request_by_name(dev, "reset-gpios", 0,
			   &reset_gpio, GPIOD_IS_OUT);
if (ret) {
	/* trying legacy reset */
	ret = gpio_request_by_name(dev, "gpio-reset", 0,
				   &reset_gpio, GPIOD_IS_OUT);
	if (!ret)
		log(LOGC_NONE, LOGL_NOTICE,
		    "%s: Using deprecated gpio-reset switch to reset-gpios\n",  __func__);
}

if (!ret) {
	dm_gpio_set_value(&reset_gpio, 1);
	mdelay(1);
	dm_gpio_set_value(&reset_gpio, 0);
}

> -
> +init:
>  	/* Ensure a minimum amount of time elapsed since reset of the TPM */
>  	mdelay(drv_data->time_before_first_cmd_ms);
>  
> 

M

Re: [PATCHv2] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz, Foundries]

On 01/06/21, Michal Simek wrote:
> 
> 
> On 6/1/21 8:09 AM, Jorge Ramirez-Ortiz wrote:
> > Use the more generic reset-gpios propery name.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >   v2: kept gpio-reset as legacy
> > 
> >  .../tpm2/tis-tpm2-spi.txt                     |  2 +-
> >  drivers/tpm/tpm2_tis_spi.c                    | 21 ++++++++++++-------
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> > index 3a2ee4bd17..bbcd12950f 100644
> > --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> > +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> > @@ -6,7 +6,7 @@ Required properties:
> >  - reg			: SPI Chip select
> >  
> >  Optional properties:
> > -- gpio-reset		: Reset GPIO (if not connected to the SoC reset line)
> 
> As I said you shouldn't remove this. Just extend description that it is
> deprecated and reset-gpios should be used instead.

I dont really agree with that. IMO we should remove the documentation
since it is obsolete after this commit and anyone reading it should
not care about the gpio-reset property.

> 
> > +- reset-gpios		: Reset GPIO (if not connected to the SoC reset line)
> >  - spi-max-frequency	: See spi-bus.txt
> >  
> >  Example:
> > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > index 4b33ac8fd3..1f9f89f68f 100644
> > --- a/drivers/tpm/tpm2_tis_spi.c
> > +++ b/drivers/tpm/tpm2_tis_spi.c
> > @@ -589,18 +589,23 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> >  	if (CONFIG_IS_ENABLED(DM_GPIO)) {
> >  		struct gpio_desc reset_gpio;
> >  
> > -		ret = gpio_request_by_name(dev, "gpio-reset", 0,
> > +		ret = gpio_request_by_name(dev, "reset-gpios", 0,
> >  					   &reset_gpio, GPIOD_IS_OUT);
> >  		if (ret) {
> > -			log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
> > -			    __func__);
> > -		} else {
> > -			dm_gpio_set_value(&reset_gpio, 1);
> > -			mdelay(1);
> > -			dm_gpio_set_value(&reset_gpio, 0);
> > +			/* legacy reset */
> > +			ret = gpio_request_by_name(dev, "gpio-reset", 0,
> > +						   &reset_gpio, GPIOD_IS_OUT);
> > +			if (ret) {
> > +				log(LOGC_NONE, LOGL_NOTICE,
> > +				    "%s: missing reset GPIO\n",  __func__);
> > +				goto init;
> > +			}
> 
> And here it is clear that gpio-reset is used which should deprecated
> that's why you should print message about it here.

yes, I can do that. makes sense

> 
> 
> >  		}
> > +		dm_gpio_set_value(&reset_gpio, 1);
> > +		mdelay(1);
> > +		dm_gpio_set_value(&reset_gpio, 0);
> >  	}
> 
> What about this to remove that goto?

um, what is the problem with the goto (IMO tidier than yet another
conditional); it is not as if this goto is making the code obscure.

with the change below you just removed previous functionality
(ie indicating that there is no GPIO reset provided, hence why at
first sight might look cleaner than a goto)

> 
> ret = gpio_request_by_name(dev, "reset-gpios", 0,
> 			   &reset_gpio, GPIOD_IS_OUT);
> if (ret) {
> 	/* trying legacy reset */
> 	ret = gpio_request_by_name(dev, "gpio-reset", 0,
> 				   &reset_gpio, GPIOD_IS_OUT);
> 	if (!ret)
> 		log(LOGC_NONE, LOGL_NOTICE,
> 		    "%s: Using deprecated gpio-reset switch to reset-gpios\n",  __func__);
> }
> 
> if (!ret) {
> 	dm_gpio_set_value(&reset_gpio, 1);
> 	mdelay(1);
> 	dm_gpio_set_value(&reset_gpio, 0);
> }
> 
> > -
> > +init:
> >  	/* Ensure a minimum amount of time elapsed since reset of the TPM */
> >  	mdelay(drv_data->time_before_first_cmd_ms);
> >  
> > 
> 
> M

Re: [PATCHv2] drivers: tpm2: update reset gpio semantics

[Michal Simek]

On 6/1/21 9:35 AM, Jorge Ramirez-Ortiz, Foundries wrote:
> On 01/06/21, Michal Simek wrote:
>>
>>
>> On 6/1/21 8:09 AM, Jorge Ramirez-Ortiz wrote:
>>> Use the more generic reset-gpios propery name.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> ---
>>>   v2: kept gpio-reset as legacy
>>>
>>>  .../tpm2/tis-tpm2-spi.txt                     |  2 +-
>>>  drivers/tpm/tpm2_tis_spi.c                    | 21 ++++++++++++-------
>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
>>> index 3a2ee4bd17..bbcd12950f 100644
>>> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
>>> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
>>> @@ -6,7 +6,7 @@ Required properties:
>>>  - reg			: SPI Chip select
>>>  
>>>  Optional properties:
>>> -- gpio-reset		: Reset GPIO (if not connected to the SoC reset line)
>>
>> As I said you shouldn't remove this. Just extend description that it is
>> deprecated and reset-gpios should be used instead.
> 
> I dont really agree with that. IMO we should remove the documentation
> since it is obsolete after this commit and anyone reading it should
> not care about the gpio-reset property.

Run this on linux kernel and you will see that normal style is to keep
it there.
git grep deprecated Documentation/devicetree/bindings

> 
>>
>>> +- reset-gpios		: Reset GPIO (if not connected to the SoC reset line)
>>>  - spi-max-frequency	: See spi-bus.txt
>>>  
>>>  Example:
>>> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
>>> index 4b33ac8fd3..1f9f89f68f 100644
>>> --- a/drivers/tpm/tpm2_tis_spi.c
>>> +++ b/drivers/tpm/tpm2_tis_spi.c
>>> @@ -589,18 +589,23 @@ static int tpm_tis_spi_probe(struct udevice *dev)
>>>  	if (CONFIG_IS_ENABLED(DM_GPIO)) {
>>>  		struct gpio_desc reset_gpio;
>>>  
>>> -		ret = gpio_request_by_name(dev, "gpio-reset", 0,
>>> +		ret = gpio_request_by_name(dev, "reset-gpios", 0,
>>>  					   &reset_gpio, GPIOD_IS_OUT);
>>>  		if (ret) {
>>> -			log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
>>> -			    __func__);
>>> -		} else {
>>> -			dm_gpio_set_value(&reset_gpio, 1);
>>> -			mdelay(1);
>>> -			dm_gpio_set_value(&reset_gpio, 0);
>>> +			/* legacy reset */
>>> +			ret = gpio_request_by_name(dev, "gpio-reset", 0,
>>> +						   &reset_gpio, GPIOD_IS_OUT);
>>> +			if (ret) {
>>> +				log(LOGC_NONE, LOGL_NOTICE,
>>> +				    "%s: missing reset GPIO\n",  __func__);
>>> +				goto init;
>>> +			}
>>
>> And here it is clear that gpio-reset is used which should deprecated
>> that's why you should print message about it here.
> 
> yes, I can do that. makes sense
> 
>>
>>
>>>  		}
>>> +		dm_gpio_set_value(&reset_gpio, 1);
>>> +		mdelay(1);
>>> +		dm_gpio_set_value(&reset_gpio, 0);
>>>  	}
>>
>> What about this to remove that goto?
> 
> um, what is the problem with the goto (IMO tidier than yet another
> conditional); it is not as if this goto is making the code obscure.
> 
> with the change below you just removed previous functionality
> (ie indicating that there is no GPIO reset provided, hence why at
> first sight might look cleaner than a goto)

I tend to use goto unless there is no way around. But up2you.

M

Re: [PATCHv2] drivers: tpm2: update reset gpio semantics

[Jorge Ramirez-Ortiz, Foundries]

On 01/06/21, Michal Simek wrote:
> 
> 
> On 6/1/21 9:35 AM, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 01/06/21, Michal Simek wrote:
> >>
> >>
> >> On 6/1/21 8:09 AM, Jorge Ramirez-Ortiz wrote:
> >>> Use the more generic reset-gpios propery name.
> >>>
> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> >>> ---
> >>>   v2: kept gpio-reset as legacy
> >>>
> >>>  .../tpm2/tis-tpm2-spi.txt                     |  2 +-
> >>>  drivers/tpm/tpm2_tis_spi.c                    | 21 ++++++++++++-------
> >>>  2 files changed, 14 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> >>> index 3a2ee4bd17..bbcd12950f 100644
> >>> --- a/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> >>> +++ b/doc/device-tree-bindings/tpm2/tis-tpm2-spi.txt
> >>> @@ -6,7 +6,7 @@ Required properties:
> >>>  - reg			: SPI Chip select
> >>>  
> >>>  Optional properties:
> >>> -- gpio-reset		: Reset GPIO (if not connected to the SoC reset line)
> >>
> >> As I said you shouldn't remove this. Just extend description that it is
> >> deprecated and reset-gpios should be used instead.
> > 
> > I dont really agree with that. IMO we should remove the documentation
> > since it is obsolete after this commit and anyone reading it should
> > not care about the gpio-reset property.
> 
> Run this on linux kernel and you will see that normal style is to keep
> it there.
> git grep deprecated Documentation/devicetree/bindings

um, ok then (although IMO it only makes sense when we have the tools
to parse such documents and apply them at build time)...if you have to
read the docs you might just as well change the property.

will post later today (I am also adding an additional test to tpm2 to
read the rng)

> 
> > 
> >>
> >>> +- reset-gpios		: Reset GPIO (if not connected to the SoC reset line)
> >>>  - spi-max-frequency	: See spi-bus.txt
> >>>  
> >>>  Example:
> >>> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> >>> index 4b33ac8fd3..1f9f89f68f 100644
> >>> --- a/drivers/tpm/tpm2_tis_spi.c
> >>> +++ b/drivers/tpm/tpm2_tis_spi.c
> >>> @@ -589,18 +589,23 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> >>>  	if (CONFIG_IS_ENABLED(DM_GPIO)) {
> >>>  		struct gpio_desc reset_gpio;
> >>>  
> >>> -		ret = gpio_request_by_name(dev, "gpio-reset", 0,
> >>> +		ret = gpio_request_by_name(dev, "reset-gpios", 0,
> >>>  					   &reset_gpio, GPIOD_IS_OUT);
> >>>  		if (ret) {
> >>> -			log(LOGC_NONE, LOGL_NOTICE, "%s: missing reset GPIO\n",
> >>> -			    __func__);
> >>> -		} else {
> >>> -			dm_gpio_set_value(&reset_gpio, 1);
> >>> -			mdelay(1);
> >>> -			dm_gpio_set_value(&reset_gpio, 0);
> >>> +			/* legacy reset */
> >>> +			ret = gpio_request_by_name(dev, "gpio-reset", 0,
> >>> +						   &reset_gpio, GPIOD_IS_OUT);
> >>> +			if (ret) {
> >>> +				log(LOGC_NONE, LOGL_NOTICE,
> >>> +				    "%s: missing reset GPIO\n",  __func__);
> >>> +				goto init;
> >>> +			}
> >>
> >> And here it is clear that gpio-reset is used which should deprecated
> >> that's why you should print message about it here.
> > 
> > yes, I can do that. makes sense
> > 
> >>
> >>
> >>>  		}
> >>> +		dm_gpio_set_value(&reset_gpio, 1);
> >>> +		mdelay(1);
> >>> +		dm_gpio_set_value(&reset_gpio, 0);
> >>>  	}
> >>
> >> What about this to remove that goto?
> > 
> > um, what is the problem with the goto (IMO tidier than yet another
> > conditional); it is not as if this goto is making the code obscure.
> > 
> > with the change below you just removed previous functionality
> > (ie indicating that there is no GPIO reset provided, hence why at
> > first sight might look cleaner than a goto)
> 
> I tend to use goto unless there is no way around. But up2you.
> 
> M
>