[PATCH v1 0/1] Add more compatibility strings to tpm-tis-i2c

[PATCH v1 0/1] Add more compatibility strings to tpm-tis-i2c

[Ninad Palsule]

The new IBM system1 bmc machine uses Nuvoton TPM chip. I had this commit
as part of the device tree for new machine but reviewer suggested send
the driver commit as separate patch.

The patchset for IBM system1 bmc machine device tree is as follows:
https://lore.kernel.org/linux-kernel/20231212164004.1683589-1-ninad@linux.ibm.com/

Joel Stanley (1):
  tpm: tis-i2c: Add more compatible strings

 drivers/char/tpm/tpm_tis_i2c.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.39.2

[PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

From: Joel Stanley <joel@jms.id.au>

The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.

https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/

Add a compatible string for it, and the generic compatible.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/char/tpm/tpm_tis_i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index a897402cc36a..9511c0d50185 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
 #ifdef CONFIG_OF
 static const struct of_device_id of_tis_i2c_match[] = {
 	{ .compatible = "infineon,slb9673", },
+	{ .compatible = "nuvoton,npct75x", },
+	{ .compatible = "tcg,tpm-tis-i2c", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
-- 
2.39.2

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Thu, Dec 14, 2023 at 08:49:53AM -0600, Ninad Palsule wrote:
> From: Joel Stanley <joel@jms.id.au>
> 
> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> 
> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> 
> Add a compatible string for it, and the generic compatible.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>

I don't understand why you broke this series up and dropped patches.
NAK, these compatibles are not documented.

Cheers,
Conor.

> ---
>  drivers/char/tpm/tpm_tis_i2c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> index a897402cc36a..9511c0d50185 100644
> --- a/drivers/char/tpm/tpm_tis_i2c.c
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>  #ifdef CONFIG_OF
>  static const struct of_device_id of_tis_i2c_match[] = {
>  	{ .compatible = "infineon,slb9673", },
> +	{ .compatible = "nuvoton,npct75x", },
> +	{ .compatible = "tcg,tpm-tis-i2c", },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

On 1/9/24 11:11, Conor Dooley wrote:
> On Thu, Dec 14, 2023 at 08:49:53AM -0600, Ninad Palsule wrote:
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>
>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>
>> Add a compatible string for it, and the generic compatible.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> I don't understand why you broke this series up and dropped patches.
> NAK, these compatibles are not documented.
>
The original series has three patches:

1) Adding compatibility string which I am adding in this series.

2) Adding schema for the TIS I2c devices which is already covered by 
Lukas's patch (already merged in linux-next) 
https://lore.kernel.org/all/3f56f0a2bb90697a23e83583a21684b75dc7eea2.1701093036.git.lukas@wunner.de/

3) Removing "Infineon,slb9673" from trivial-devices.yaml which is not 
done as it is already added in the TPM specific file. I will add it in 
my patch. Good catch!

Thanks for the review.

Regards,

Ninad

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

On 1/9/24 11:11, Conor Dooley wrote:
> On Thu, Dec 14, 2023 at 08:49:53AM -0600, Ninad Palsule wrote:
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>
>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>
>> Add a compatible string for it, and the generic compatible.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> I don't understand why you broke this series up and dropped patches.
> NAK, these compatibles are not documented.

I have reconfirmed and other two patches are already dropped and merged 
by Lukas:

https://lore.kernel.org/all/3f56f0a2bb90697a23e83583a21684b75dc7eea2.1701093036.git.lukas@wunner.de/

This is the only one pending from that series. Please let me know if I 
need to do anything else for this patch series.

Regards,

Ninad

>
>

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On Thu, Jan 11, 2024 at 10:43:08AM -0600, Ninad Palsule wrote:
> Hello Conor,
> 
> On 1/9/24 11:11, Conor Dooley wrote:
> > On Thu, Dec 14, 2023 at 08:49:53AM -0600, Ninad Palsule wrote:
> > > From: Joel Stanley <joel@jms.id.au>
> > > 
> > > The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> > > 
> > > https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> > > 
> > > Add a compatible string for it, and the generic compatible.
> > > 
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
> > > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > I don't understand why you broke this series up and dropped patches.
> > NAK, these compatibles are not documented.
> > 
> The original series has three patches:
> 
> 1) Adding compatibility string which I am adding in this series.
> 
> 2) Adding schema for the TIS I2c devices which is already covered by Lukas's
> patch (already merged in linux-next) https://lore.kernel.org/all/3f56f0a2bb90697a23e83583a21684b75dc7eea2.1701093036.git.lukas@wunner.de/
> 
> 3) Removing "Infineon,slb9673" from trivial-devices.yaml which is not done
> as it is already added in the TPM specific file. I will add it in my patch.
> Good catch!

Dropping this should be a standalone patch (with a Fixes tag I suppose).

Looking at what got merged:
      - description: Generic TPM 2.0 chips conforming to TCG PTP interface
        items:
          - enum:
              - infineon,slb9673
              - nuvoton,npct75x
          - const: tcg,tpm-tis-i2c

There's no need to add "nuvoton,npct75x" to this driver, since a
fallback to tcg,tpm-tis-i2c is required by the binding. Adding the
generic compatible however makes sense.

If there's a good reason to add it (like existing QEMU releases that do
not have the generic compatible, but claim to have the npct75x) then
please note why we should make an exception in your commit message.

You need not carry the NAK, the motivation behind patch is fine.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

On 1/12/24 11:24, Conor Dooley wrote:
> On Thu, Jan 11, 2024 at 10:43:08AM -0600, Ninad Palsule wrote:
>> Hello Conor,
>>
>> On 1/9/24 11:11, Conor Dooley wrote:
>>> On Thu, Dec 14, 2023 at 08:49:53AM -0600, Ninad Palsule wrote:
>>>> From: Joel Stanley <joel@jms.id.au>
>>>>
>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>
>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>>>
>>>> Add a compatible string for it, and the generic compatible.
>>>>
>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>> I don't understand why you broke this series up and dropped patches.
>>> NAK, these compatibles are not documented.
>>>
>> The original series has three patches:
>>
>> 1) Adding compatibility string which I am adding in this series.
>>
>> 2) Adding schema for the TIS I2c devices which is already covered by Lukas's
>> patch (already merged in linux-next) https://lore.kernel.org/all/3f56f0a2bb90697a23e83583a21684b75dc7eea2.1701093036.git.lukas@wunner.de/
>>
>> 3) Removing "Infineon,slb9673" from trivial-devices.yaml which is not done
>> as it is already added in the TPM specific file. I will add it in my patch.
>> Good catch!
> Dropping this should be a standalone patch (with a Fixes tag I suppose).
>
> Looking at what got merged:
>        - description: Generic TPM 2.0 chips conforming to TCG PTP interface
>          items:
>            - enum:
>                - infineon,slb9673
>                - nuvoton,npct75x
>            - const: tcg,tpm-tis-i2c
>
> There's no need to add "nuvoton,npct75x" to this driver, since a
> fallback to tcg,tpm-tis-i2c is required by the binding. Adding the
> generic compatible however makes sense.
>
> If there's a good reason to add it (like existing QEMU releases that do
> not have the generic compatible, but claim to have the npct75x) then
> please note why we should make an exception in your commit message.
>
> You need not carry the NAK, the motivation behind patch is fine.

Make sense. As there is no specific code for npct75x in the driver, I 
will remove it.

Thanks for the review.

Regards,

Ninad

Re: [PATCH v1 1/1] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

Can you please send ack for my last patch if you don't have further 
review comments?

https://lore.kernel.org/linux-kernel/20240116181754.3905754-1-ninad@linux.ibm.com/

Thanks & Regards,

Ninad Palsule