[PATCHv2] drivers: tpm2: update reset gpio semantics

Jorge Ramirez-Ortiz, Foundries jorge at foundries.io
Thu Jun 3 11:04:22 CEST 2021


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 at 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
> 


More information about the U-Boot mailing list