| Message ID | 1460577351-24632-6-git-send-email-christophe-h.ricard@st.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 --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