| Message ID | 20210601060927.30894-1-jorge@foundries.io |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [PATCHv2] drivers: tpm2: update reset gpio semantics | expand |
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
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
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
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 >
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);
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(-)