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

["Jorge Ramirez-Ortiz, Foundries" <jorge@foundries.io>]

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