diff mbox series

[2/2] dt-bindings: tpm: Add st,st33ktpm2xi2c to TCG TIS binding

Message ID 20240413071621.12509-3-michael.haener@siemens.com (mailing list archive)
State New, archived
Headers show
Series Add ST33KTPM2XI2C chip to the TPM TIS I2C driver | expand

Commit Message

Michael Haener April 13, 2024, 7:15 a.m. UTC 
From: Michael Haener <michael.haener@siemens.com>

Add the ST chip st33ktpm2xi2c to the supported compatible strings of the
TPM TIS I2C schema. The Chip is compliant with the TCG PC Client TPM
Profile specification.

For reference, a datasheet is available at:
https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Michael Haener <michael.haener@siemens.com>
---
 Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
 1 file changed, 1 insertion(+)

Comments

Krzysztof Kozlowski April 13, 2024, 8:10 a.m. UTC | #1 
On 13/04/2024 09:15, M. Haener wrote:
> From: Michael Haener <michael.haener@siemens.com>
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


>  Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> index 3ab4434b7352..af7720dc4a12 100644
> --- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> +++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> @@ -32,6 +32,7 @@ properties:
>            - enum:
>                - infineon,slb9673
>                - nuvoton,npct75x
> +              - st,st33ktpm2xi2c

I got only one patch, but if these are compatible, why do you need
second patch? Plus binding come before users.

Please explain what are you doing in other patch.

Best regards,
Krzysztof
Krzysztof Kozlowski April 13, 2024, 8:38 a.m. UTC | #2 
On 13/04/2024 09:15, M. Haener wrote:
> From: Michael Haener <michael.haener@siemens.com>
> 
> Add the ST chip st33ktpm2xi2c to the supported compatible strings of the
> TPM TIS I2C schema. The Chip is compliant with the TCG PC Client TPM
> Profile specification.
> 
> For reference, a datasheet is available at:
> https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
> 
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Michael Haener <michael.haener@siemens.com>
> ---


Not tested...

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof
Lukas Wunner April 13, 2024, 10:18 a.m. UTC | #3 
On Sat, Apr 13, 2024 at 10:10:49AM +0200, Krzysztof Kozlowski wrote:
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.

To be fair, "dt-bindings: tpm: " is actually the only prefix used
so far for the file that's touched here:

$ git log --oneline Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml 
26c9d15 dt-bindings: tpm: Consolidate TCG TIS bindings

Personally I don't think we need to differentiate between spi/i2c/mmio
bindings in the prefix, so the prefix used by Michael seems fine.


> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Right, so maybe just:

dt-bindings: tpm: Add st,st33ktpm2xi2c

?


> I got only one patch, but if these are compatible, why do you need
> second patch? Plus binding come before users.

Right, the order of the patches needs to be reversed it seems.

Thanks,

Lukas
Krzysztof Kozlowski April 13, 2024, 10:23 a.m. UTC | #4 
On 13/04/2024 12:18, Lukas Wunner wrote:
> On Sat, Apr 13, 2024 at 10:10:49AM +0200, Krzysztof Kozlowski wrote:
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> 
> To be fair, "dt-bindings: tpm: " is actually the only prefix used
> so far for the file that's touched here:
> 
> $ git log --oneline Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml 
> 26c9d15 dt-bindings: tpm: Consolidate TCG TIS bindings

Command should be run on the directory, but anyway you are right. So
it's fine.

> 
> Personally I don't think we need to differentiate between spi/i2c/mmio
> bindings in the prefix, so the prefix used by Michael seems fine.
> 
> 
>> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
>> prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> Right, so maybe just:
> 
> dt-bindings: tpm: Add st,st33ktpm2xi2c
> 
> ?


> 
> 
>> I got only one patch, but if these are compatible, why do you need
>> second patch? Plus binding come before users.
> 
> Right, the order of the patches needs to be reversed it seems.

What is the second patch? Device is or is not compatible?

Best regards,
Krzysztof
Lukas Wunner April 13, 2024, 10:34 a.m. UTC | #5 
On Sat, Apr 13, 2024 at 12:23:47PM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 12:18, Lukas Wunner wrote:
> > On Sat, Apr 13, 2024 at 10:10:49AM +0200, Krzysztof Kozlowski wrote:
> > > I got only one patch, but if these are compatible, why do you need
> > > second patch? Plus binding come before users.
> > 
> > Right, the order of the patches needs to be reversed it seems.
> 
> What is the second patch? Device is or is not compatible?

The other patch just adds an entry to of_tis_i2c_match[] in the driver,
pretty unspectacular:

https://lore.kernel.org/all/20240413071621.12509-2-michael.haener@siemens.com/

Unfortunately it was only cc'ed to TPM driver maintainers.
Krzysztof Kozlowski April 13, 2024, 10:43 a.m. UTC | #6 
On 13/04/2024 12:34, Lukas Wunner wrote:
> On Sat, Apr 13, 2024 at 12:23:47PM +0200, Krzysztof Kozlowski wrote:
>> On 13/04/2024 12:18, Lukas Wunner wrote:
>>> On Sat, Apr 13, 2024 at 10:10:49AM +0200, Krzysztof Kozlowski wrote:
>>>> I got only one patch, but if these are compatible, why do you need
>>>> second patch? Plus binding come before users.
>>>
>>> Right, the order of the patches needs to be reversed it seems.
>>
>> What is the second patch? Device is or is not compatible?
> 
> The other patch just adds an entry to of_tis_i2c_match[] in the driver,
> pretty unspectacular:
> 
> https://lore.kernel.org/all/20240413071621.12509-2-michael.haener@siemens.com/
> 

Then why is it needed?

To re-iterate:
"Device is or is not compatible?"

Decide, one of the two patches is wrong.

Best regards,
Krzysztof
Lukas Wunner April 13, 2024, 10:51 a.m. UTC | #7 
On Sat, Apr 13, 2024 at 12:43:38PM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 12:34, Lukas Wunner wrote:
> > The other patch just adds an entry to of_tis_i2c_match[] in the driver,
> > pretty unspectacular:
> > 
> > https://lore.kernel.org/all/20240413071621.12509-2-michael.haener@siemens.com/
> 
> Then why is it needed?

The binding requires two entries in the compatible string used in the DT,
the chip name followed by the generic string:

        items:
          - enum:
              - infineon,slb9673
              - nuvoton,npct75x
          - const: tcg,tpm-tis-i2c

This allows us to deal with device-specific quirks, should they pop up
(e.g. special timing requirements, hardware bugs).  We don't know in
advance if they will be discovered, but if they are, it's cumbersome
to determine after the fact which products (and thus DTs) are affected.
So having the name of the actual chip used on the board has value.

Thanks,

Lukas
Krzysztof Kozlowski April 13, 2024, 10:53 a.m. UTC | #8 
On 13/04/2024 12:51, Lukas Wunner wrote:
> On Sat, Apr 13, 2024 at 12:43:38PM +0200, Krzysztof Kozlowski wrote:
>> On 13/04/2024 12:34, Lukas Wunner wrote:
>>> The other patch just adds an entry to of_tis_i2c_match[] in the driver,
>>> pretty unspectacular:
>>>
>>> https://lore.kernel.org/all/20240413071621.12509-2-michael.haener@siemens.com/
>>
>> Then why is it needed?
> 
> The binding requires two entries in the compatible string used in the DT,
> the chip name followed by the generic string:
> 
>         items:
>           - enum:
>               - infineon,slb9673
>               - nuvoton,npct75x
>           - const: tcg,tpm-tis-i2c
> 
> This allows us to deal with device-specific quirks, should they pop up
> (e.g. special timing requirements, hardware bugs).  We don't know in
> advance if they will be discovered, but if they are, it's cumbersome
> to determine after the fact which products (and thus DTs) are affected.
> So having the name of the actual chip used on the board has value.

So you say devices are compatible. Then the second patch is wrong.

I cannot respond to it, though... so NAK-here-for-second-patch.

Best regards,
Krzysztof
Lukas Wunner April 13, 2024, 10:56 a.m. UTC | #9 
On Sat, Apr 13, 2024 at 12:53:25PM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 12:51, Lukas Wunner wrote:
> > The binding requires two entries in the compatible string used in the DT,
> > the chip name followed by the generic string:
> > 
> >         items:
> >           - enum:
> >               - infineon,slb9673
> >               - nuvoton,npct75x
> >           - const: tcg,tpm-tis-i2c
> > 
> > This allows us to deal with device-specific quirks, should they pop up
> > (e.g. special timing requirements, hardware bugs).  We don't know in
> > advance if they will be discovered, but if they are, it's cumbersome
> > to determine after the fact which products (and thus DTs) are affected.
> > So having the name of the actual chip used on the board has value.
> 
> So you say devices are compatible. Then the second patch is wrong.
> 
> I cannot respond to it, though... so NAK-here-for-second-patch.

I disagree.  It's ugly to have inconsistencies between the DT bindings
and the driver.  So I think patch [1/2] in this series is fine.

Thanks,

Lukas
Krzysztof Kozlowski April 13, 2024, 11:01 a.m. UTC | #10 
On 13/04/2024 12:56, Lukas Wunner wrote:
> On Sat, Apr 13, 2024 at 12:53:25PM +0200, Krzysztof Kozlowski wrote:
>> On 13/04/2024 12:51, Lukas Wunner wrote:
>>> The binding requires two entries in the compatible string used in the DT,
>>> the chip name followed by the generic string:
>>>
>>>         items:
>>>           - enum:
>>>               - infineon,slb9673
>>>               - nuvoton,npct75x
>>>           - const: tcg,tpm-tis-i2c
>>>
>>> This allows us to deal with device-specific quirks, should they pop up
>>> (e.g. special timing requirements, hardware bugs).  We don't know in
>>> advance if they will be discovered, but if they are, it's cumbersome
>>> to determine after the fact which products (and thus DTs) are affected.
>>> So having the name of the actual chip used on the board has value.
>>
>> So you say devices are compatible. Then the second patch is wrong.
>>
>> I cannot respond to it, though... so NAK-here-for-second-patch.
> 
> I disagree.  It's ugly to have inconsistencies between the DT bindings
> and the driver.  So I think patch [1/2] in this series is fine.

You are mixing different things. This patchset creates inconsistency.
You even refer here to bindings while we discuss the driver...

Why this one driver is different than all other Linux drivers? Why do
you keep pushing here entirely different behavior?

The devices are compatible, so growing match table is both redundant and
confusing. That's everywhere. TPM is not different.

Best regards,
Krzysztof
Michael Haener April 13, 2024, 8:26 p.m. UTC | #11 
On Sat, 2024-04-13 at 10:38 +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 09:15, M. Haener wrote:
> > From: Michael Haener <michael.haener@siemens.com>
> >
> > Add the ST chip st33ktpm2xi2c to the supported compatible strings
> > of the
> > TPM TIS I2C schema. The Chip is compliant with the TCG PC Client
> > TPM
> > Profile specification.
> >
> > For reference, a datasheet is available at:
> > https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
> >
> > Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > Signed-off-by: Michael Haener <michael.haener@siemens.com>
> > ---
>
>
> Not tested...

I was only able to verify and test the conformity of the ST chip
st33ktpm2xi2c with kernel 6.1, so I left out the test-by tag.
Unfortunately, there is no newer kernel for my embedded hardware.

>
> Please use scripts/get_maintainers.pl to get a list of necessary
> people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on
> some
> ancient tree (don't, instead use mainline), work on fork of kernel
> (don't, instead use mainline) or you ignore some maintainers (really
> don't). Just use b4 and everything should be fine, although remember
> about `b4 prep --auto-to-cc` if you added new patches to the
> patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling.

I called the script scripts/get_maintainer.pl on the latest kernel
version for each of the two patches and added the output list to the
individual patches accordingly. And only for the cover-letter I linked
the two lists together.
I understand now that I should have sent the whole series to both
lists.

>
> Please kindly resend and include all necessary To/Cc entries.
>
> Best regards,
> Krzysztof
>

Best regards,
Michael
Krzysztof Kozlowski April 13, 2024, 9:13 p.m. UTC | #12 
On 13/04/2024 22:26, Haener, Michael wrote:
> On Sat, 2024-04-13 at 10:38 +0200, Krzysztof Kozlowski wrote:
>> On 13/04/2024 09:15, M. Haener wrote:
>>> From: Michael Haener <michael.haener@siemens.com>
>>>
>>> Add the ST chip st33ktpm2xi2c to the supported compatible strings
>>> of the
>>> TPM TIS I2C schema. The Chip is compliant with the TCG PC Client
>>> TPM
>>> Profile specification.
>>>
>>> For reference, a datasheet is available at:
>>> https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
>>>
>>> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>> Signed-off-by: Michael Haener <michael.haener@siemens.com>
>>> ---
>>
>>
>> Not tested...
> 
> I was only able to verify and test the conformity of the ST chip
> st33ktpm2xi2c with kernel 6.1, so I left out the test-by tag.

I don't mean your tag. Your SoB means you tested it, but I meant you did
not send the binding for testing via automation.

> Unfortunately, there is no newer kernel for my embedded hardware.
> 
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary
>> people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>> people, so fix your workflow. Tools might also fail if you work on
>> some
>> ancient tree (don't, instead use mainline), work on fork of kernel
>> (don't, instead use mainline) or you ignore some maintainers (really
>> don't). Just use b4 and everything should be fine, although remember
>> about `b4 prep --auto-to-cc` if you added new patches to the
>> patchset.
>>
>> You missed at least devicetree list (maybe more), so this won't be
>> tested by automated tooling.
> 
> I called the script scripts/get_maintainer.pl on the latest kernel
> version for each of the two patches and added the output list to the
> individual patches accordingly. And only for the cover-letter I linked
> the two lists together.
> I understand now that I should have sent the whole series to both
> lists.
> 

No, that's not the case. You did not Cc output of get_maintainer.pl.
Read *AGAIN* my message..

Best regards,
Krzysztof
Jarkko Sakkinen April 13, 2024, 9:47 p.m. UTC | #13 
On Sat Apr 13, 2024 at 10:15 AM EEST, M. Haener wrote:
> From: Michael Haener <michael.haener@siemens.com>
>
> Add the ST chip st33ktpm2xi2c to the supported compatible strings of the
> TPM TIS I2C schema. The Chip is compliant with the TCG PC Client TPM
> Profile specification.
>
> For reference, a datasheet is available at:
> https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
>
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Michael Haener <michael.haener@siemens.com>
> ---
>  Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> index 3ab4434b7352..af7720dc4a12 100644
> --- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> +++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> @@ -32,6 +32,7 @@ properties:
>            - enum:
>                - infineon,slb9673
>                - nuvoton,npct75x
> +              - st,st33ktpm2xi2c
>            - const: tcg,tpm-tis-i2c
>  
>        - description: TPM 1.2 and 2.0 chips with vendor-specific I²C interface

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

I'm ready to pick these unless Rob has anything to complain (hold
to next week).

BR, Jarkko
Krzysztof Kozlowski April 13, 2024, 9:48 p.m. UTC | #14 
On 13/04/2024 23:47, Jarkko Sakkinen wrote:
> On Sat Apr 13, 2024 at 10:15 AM EEST, M. Haener wrote:
>> From: Michael Haener <michael.haener@siemens.com>
>>
>> Add the ST chip st33ktpm2xi2c to the supported compatible strings of the
>> TPM TIS I2C schema. The Chip is compliant with the TCG PC Client TPM
>> Profile specification.
>>
>> For reference, a datasheet is available at:
>> https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
>>
>> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>> Signed-off-by: Michael Haener <michael.haener@siemens.com>
>> ---
>>  Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> index 3ab4434b7352..af7720dc4a12 100644
>> --- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> +++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> @@ -32,6 +32,7 @@ properties:
>>            - enum:
>>                - infineon,slb9673
>>                - nuvoton,npct75x
>> +              - st,st33ktpm2xi2c
>>            - const: tcg,tpm-tis-i2c
>>  
>>        - description: TPM 1.2 and 2.0 chips with vendor-specific I²C interface
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I'm ready to pick these unless Rob has anything to complain (hold
> to next week).

I complained and NAKed second patch.

Best regards,
Krzysztof
Jarkko Sakkinen April 13, 2024, 9:49 p.m. UTC | #15 
On Sat Apr 13, 2024 at 11:10 AM EEST, Krzysztof Kozlowski wrote:
> On 13/04/2024 09:15, M. Haener wrote:
> > From: Michael Haener <michael.haener@siemens.com>
> > 
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
>
> >  Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> > index 3ab4434b7352..af7720dc4a12 100644
> > --- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
> > @@ -32,6 +32,7 @@ properties:
> >            - enum:
> >                - infineon,slb9673
> >                - nuvoton,npct75x
> > +              - st,st33ktpm2xi2c
>
> I got only one patch, but if these are compatible, why do you need
> second patch? Plus binding come before users.
>
> Please explain what are you doing in other patch.
>
> Best regards,
> Krzysztof

Thanks, I agree with the remarks. So holding for update.

BR, Jarkko
Krzysztof Kozlowski April 13, 2024, 9:50 p.m. UTC | #16 
On 13/04/2024 23:47, Jarkko Sakkinen wrote:
> On Sat Apr 13, 2024 at 10:15 AM EEST, M. Haener wrote:
>> From: Michael Haener <michael.haener@siemens.com>
>>
>> Add the ST chip st33ktpm2xi2c to the supported compatible strings of the
>> TPM TIS I2C schema. The Chip is compliant with the TCG PC Client TPM
>> Profile specification.
>>
>> For reference, a datasheet is available at:
>> https://www.st.com/resource/en/data_brief/st33ktpm2xi2c.pdf
>>
>> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>> Signed-off-by: Michael Haener <michael.haener@siemens.com>
>> ---
>>  Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> index 3ab4434b7352..af7720dc4a12 100644
>> --- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> +++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
>> @@ -32,6 +32,7 @@ properties:
>>            - enum:
>>                - infineon,slb9673
>>                - nuvoton,npct75x
>> +              - st,st33ktpm2xi2c
>>            - const: tcg,tpm-tis-i2c
>>  
>>        - description: TPM 1.2 and 2.0 chips with vendor-specific I²C interface
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I'm ready to pick these unless Rob has anything to complain (hold
> to next week).

Plus, even the first patch was not tested... I already wrote it, so
let's be more specific:

NAK, not tested, even though testing is trivial.

I don't understand why opting-out from testing, even for trivial patches
like this. Automation, expressed by get_maintainers.pl, is there for a
reason.

Best regards,
Krzysztof
Michael Haener April 14, 2024, 5:42 a.m. UTC | #17 
On Sat, 2024-04-13 at 13:01 +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 12:56, Lukas Wunner wrote:
> > On Sat, Apr 13, 2024 at 12:53:25PM +0200, Krzysztof Kozlowski
> > wrote:
> > > On 13/04/2024 12:51, Lukas Wunner wrote:
> > > > The binding requires two entries in the compatible string used
> > > > in the DT,
> > > > the chip name followed by the generic string:
> > > > 
> > > >         items:
> > > >           - enum:
> > > >               - infineon,slb9673
> > > >               - nuvoton,npct75x
> > > >           - const: tcg,tpm-tis-i2c
> > > > 
> > > > This allows us to deal with device-specific quirks, should they
> > > > pop up
> > > > (e.g. special timing requirements, hardware bugs).  We don't
> > > > know in
> > > > advance if they will be discovered, but if they are, it's
> > > > cumbersome
> > > > to determine after the fact which products (and thus DTs) are
> > > > affected.
> > > > So having the name of the actual chip used on the board has
> > > > value.
> > > 
> > > So you say devices are compatible. Then the second patch is
> > > wrong.
> > > 
> > > I cannot respond to it, though... so NAK-here-for-second-patch.
> > 
> > I disagree.  It's ugly to have inconsistencies between the DT
> > bindings
> > and the driver.  So I think patch [1/2] in this series is fine.
> 
> You are mixing different things. This patchset creates inconsistency.
> You even refer here to bindings while we discuss the driver...
> 
> Why this one driver is different than all other Linux drivers? Why do
> you keep pushing here entirely different behavior?
> 
> The devices are compatible, so growing match table is both redundant
> and
> confusing. That's everywhere. TPM is not different.
> 
> Best regards,
> Krzysztof
> 

Sorry for the mistakes made (kernel noob).
I made the patch for the driver analogous to the i2c-tpm of
infineon,slb9673 and nuvoton,npct75x. In the discussion I realize that
the compatibility is a duplication and should not be extended.

I now adjust the following in the series:
- correct cc receiver list
- delete driver patch with compatibility extension
- change commit title to "dt-bindings: tpm: Add st,st33ktpm2xi2c"

Best regards,
Michael
Krzysztof Kozlowski April 14, 2024, 5:52 a.m. UTC | #18 
On 14/04/2024 07:42, Haener, Michael wrote:
> On Sat, 2024-04-13 at 13:01 +0200, Krzysztof Kozlowski wrote:
>> On 13/04/2024 12:56, Lukas Wunner wrote:
>>> On Sat, Apr 13, 2024 at 12:53:25PM +0200, Krzysztof Kozlowski
>>> wrote:
>>>> On 13/04/2024 12:51, Lukas Wunner wrote:
>>>>> The binding requires two entries in the compatible string used
>>>>> in the DT,
>>>>> the chip name followed by the generic string:
>>>>>
>>>>>         items:
>>>>>           - enum:
>>>>>               - infineon,slb9673
>>>>>               - nuvoton,npct75x
>>>>>           - const: tcg,tpm-tis-i2c
>>>>>
>>>>> This allows us to deal with device-specific quirks, should they
>>>>> pop up
>>>>> (e.g. special timing requirements, hardware bugs).  We don't
>>>>> know in
>>>>> advance if they will be discovered, but if they are, it's
>>>>> cumbersome
>>>>> to determine after the fact which products (and thus DTs) are
>>>>> affected.
>>>>> So having the name of the actual chip used on the board has
>>>>> value.
>>>>
>>>> So you say devices are compatible. Then the second patch is
>>>> wrong.
>>>>
>>>> I cannot respond to it, though... so NAK-here-for-second-patch.
>>>
>>> I disagree.  It's ugly to have inconsistencies between the DT
>>> bindings
>>> and the driver.  So I think patch [1/2] in this series is fine.
>>
>> You are mixing different things. This patchset creates inconsistency.
>> You even refer here to bindings while we discuss the driver...
>>
>> Why this one driver is different than all other Linux drivers? Why do
>> you keep pushing here entirely different behavior?
>>
>> The devices are compatible, so growing match table is both redundant
>> and
>> confusing. That's everywhere. TPM is not different.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Sorry for the mistakes made (kernel noob).
> I made the patch for the driver analogous to the i2c-tpm of
> infineon,slb9673 and nuvoton,npct75x. In the discussion I realize that
> the compatibility is a duplication and should not be extended.
> 
> I now adjust the following in the series:
> - correct cc receiver list
> - delete driver patch with compatibility extension
> - change commit title to "dt-bindings: tpm: Add st,st33ktpm2xi2c"

Thanks.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
index 3ab4434b7352..af7720dc4a12 100644
--- a/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
+++ b/Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.yaml
@@ -32,6 +32,7 @@  properties:
           - enum:
               - infineon,slb9673
               - nuvoton,npct75x
+              - st,st33ktpm2xi2c
           - const: tcg,tpm-tis-i2c
 
       - description: TPM 1.2 and 2.0 chips with vendor-specific I²C interface