diff mbox

[v2,05/12] tpm: Use read/write_bytes for drivers without more specialized methods

Message ID 1460577351-24632-6-git-send-email-christophe-h.ricard@st.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christophe Ricard April 13, 2016, 7:55 p.m. UTC
Some drivers might choose to implement only functions for transferring an
arbitrary number of bytes, without special handling of word or dword
transfers.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
---
 drivers/char/tpm/tpm_tis_core.h | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Comments

Jason Gunthorpe April 13, 2016, 8:42 p.m. UTC | #1
On Wed, Apr 13, 2016 at 09:55:44PM +0200, Christophe Ricard wrote:
> Some drivers might choose to implement only functions for transferring an
> arbitrary number of bytes, without special handling of word or dword
> transfers.

Hurm, better to provide common write/read16/32 helpers that a driver can
just dump into it's function pointers rather than using null,
simpler/faster.

Jason

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Christophe Ricard April 14, 2016, 9:46 p.m. UTC | #2
On 13/04/2016 22:42, Jason Gunthorpe wrote:
> On Wed, Apr 13, 2016 at 09:55:44PM +0200, Christophe Ricard wrote:
>> Some drivers might choose to implement only functions for transferring an
>> arbitrary number of bytes, without special handling of word or dword
>> transfers.
> Hurm, better to provide common write/read16/32 helpers that a driver can
> just dump into it's function pointers rather than using null,
> simpler/faster.
It is not really clear to me here. In short, Do you expect me to drop 
this patch and force drivers to
implement their own write/read16/32 helpers so that it will avoid a null 
check ?
> Jason


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Jason Gunthorpe April 14, 2016, 9:50 p.m. UTC | #3
On Thu, Apr 14, 2016 at 11:46:33PM +0200, Christophe Ricard wrote:
> 
> 
> On 13/04/2016 22:42, Jason Gunthorpe wrote:
> >On Wed, Apr 13, 2016 at 09:55:44PM +0200, Christophe Ricard wrote:
> >>Some drivers might choose to implement only functions for transferring an
> >>arbitrary number of bytes, without special handling of word or dword
> >>transfers.
> >Hurm, better to provide common write/read16/32 helpers that a driver can
> >just dump into it's function pointers rather than using null,
> >simpler/faster.
> It is not really clear to me here. In short, Do you expect me to drop this
> patch and force drivers to
> implement their own write/read16/32 helpers so that it will avoid a null
> check ?

No, just do something like

static const tpm_..ops spi_phy_ops = {
   .read_bytes = spi_read_bytes,
   .read16 = tpm_tis_common_read16,
   .read32 = tpm_tis_common_read32,
}

And have the core provide implementations of tpm_tis_common_readXX
that call read_bytes.

Jason

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
Christophe Ricard April 14, 2016, 9:51 p.m. UTC | #4
ok that's clearer.
Thanks for the idea :)

On 14/04/2016 23:50, Jason Gunthorpe wrote:
> On Thu, Apr 14, 2016 at 11:46:33PM +0200, Christophe Ricard wrote:
>>
>> On 13/04/2016 22:42, Jason Gunthorpe wrote:
>>> On Wed, Apr 13, 2016 at 09:55:44PM +0200, Christophe Ricard wrote:
>>>> Some drivers might choose to implement only functions for transferring an
>>>> arbitrary number of bytes, without special handling of word or dword
>>>> transfers.
>>> Hurm, better to provide common write/read16/32 helpers that a driver can
>>> just dump into it's function pointers rather than using null,
>>> simpler/faster.
>> It is not really clear to me here. In short, Do you expect me to drop this
>> patch and force drivers to
>> implement their own write/read16/32 helpers so that it will avoid a null
>> check ?
> No, just do something like
>
> static const tpm_..ops spi_phy_ops = {
>     .read_bytes = spi_read_bytes,
>     .read16 = tpm_tis_common_read16,
>     .read32 = tpm_tis_common_read32,
> }
>
> And have the core provide implementations of tpm_tis_common_readXX
> that call read_bytes.
>
> Jason


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index b0f1e3c..732f305 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -65,15 +65,31 @@  static inline int tpm_read8(struct tpm_chip *chip, u32 addr, u8 *result)
 static inline int tpm_read16(struct tpm_chip *chip, u32 addr, u16 *result)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int rc;
 
-	return priv->lowlevel->read16(chip, addr, result);
+	if (priv->lowlevel->read16)
+		return priv->lowlevel->read16(chip, addr, result);
+
+	rc = priv->lowlevel->read_bytes(chip, addr, sizeof(u16), (u8 *)result);
+	if (!rc)
+		*result = le16_to_cpu(*result);
+
+	return rc;
 }
 
 static inline u32 tpm_read32(struct tpm_chip *chip, u32 addr, u32 *result)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int rc;
+
+	if (priv->lowlevel->read32)
+		return priv->lowlevel->read32(chip, addr, result);
 
-	return priv->lowlevel->read32(chip, addr, result);
+	rc = priv->lowlevel->read_bytes(chip, addr, sizeof(u32), (u8 *)result);
+	if (!rc)
+		*result = le32_to_cpu(*result);
+
+	return rc;
 }
 
 static inline int tpm_write_bytes(struct tpm_chip *chip, u32 addr, u16 len,
@@ -95,7 +111,12 @@  static inline int tpm_write32(struct tpm_chip *chip, u32 addr, u32 value)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	return priv->lowlevel->write32(chip, addr, value);
+	if (priv->lowlevel->write32)
+		return priv->lowlevel->write32(chip, addr, value);
+
+	value = cpu_to_le32(value);
+	return priv->lowlevel->write_bytes(chip, addr, sizeof(u32),
+					   (u8 *)&value);
 }
 
 #endif