Re: [PATCH v2 5/6] tpm: add driver for cr50 on SPI

[Alexander Steffen]

On 17.07.2019 21:57, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-17 05:00:06)
> > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > From: Andrey Pronin <apronin@xxxxxxxxxxxx>
> > >
> > > +static unsigned short rng_quality = 1022;
> > > +module_param(rng_quality, ushort, 0644);
> > > +MODULE_PARM_DESC(rng_quality,
> > > +              "Estimation of true entropy, in bits per 1024 bits.");
> >
> > What is the purpose of this parameter? None of the other TPM drivers
> > have it.
>
> I think the idea is to let users override the quality if they decide
> that they don't want to use the default value specified in the driver.

But isn't this something that applies to all TPMs, not only cr50? So 
shouldn't this parameter be added to one of the global modules (tpm? 
tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis, 
tpm_tis_spi, ...) need this parameter to provide a consistent interface 
for the user?

> > This copies a lot of code from tpm_tis_spi, but then slightly changes
> > some things, without really explaining why.
>
> The commit text has some explanations. Here's the copy/paste from above:
>
> > >    - need to ensure a certain delay between spi transactions, or else
> > >      the chip may miss some part of the next transaction;
> > >    - if there is no spi activity for some time, it may go to sleep,
> > >      and needs to be waken up before sending further commands;
> > >    - access to vendor-specific registers.
>
> Do you want me to describe something further?
>
> > For example, struct
> > cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
> > single iobuf, that is allocated via devm_kmalloc instead of being part
> > of the struct. Maybe the difference matters, maybe not, who knows?
>
> Ok. Are you asking if this is a full-duplex SPI device?

No, this was meant as an example for the previous question. As far as I 
understood it, cr50 is basically compliant to the spec implemented by 
tpm_tis_spi, but needs special handling in some cases. Therefore, I'd 
expect a driver for cr50 to look exactly like tpm_tis_spi except for the 
special bits here and there. The way buffers are allocated within the 
driver is probably not something that should differ because of the TPM chip.

Alexander