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

[Michal Simek <michal.simek@xilinx.com>]

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