Re: [RFC/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
<tthayer@xxxxxxxxxxxxxxxxxxxxx> wrote:


On 03/07/2015 01:52 PM, Andy Shevchenko wrote:

On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@xxxxxxxxxxxxxxxxxxxxx> wrote:

From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>

Altera's Arria10 SoC interconnect requires a 32 bit write for APB
peripherals. The current spi-dw driver uses 16bit accesses in
some locations. Use function pointers to support 32 bit accesses
but retain legacy 16 bit access.

--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device
*pdev)

          num_cs = 4;

-       if (pdev->dev.of_node)
+       if (pdev->dev.of_node) {
                  of_property_read_u32(pdev->dev.of_node, "num-cs",
&num_cs);
+               if (of_property_read_bool(pdev->dev.of_node,
"32bit_access")) {

"32bit-access"


Thanks. I will make the change.

+                       dws->read_w = dw_readw32;
+                       dws->write_w = dw_writew32;


Can we use just  readw/writew (w/o underscores) as names for the
accessors?


I tried this initially and got a namespace conflict with the readw & write2
macros.
macro writew passed 3 arguments, but takes just 2
macro readw passed 2 arguments, but takes just 1

Might be I wasn't clear enough. I meant

dws->readw = …
dws->writew = …


Yes, we agree. These are exactly what I was using when I saw the error.

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -141,6 +141,8 @@ struct dw_spi {
   #ifdef CONFIG_DEBUG_FS
          struct dentry *debugfs;
   #endif
+       u16 (*read_w)(struct dw_spi *dws, u32 offset);
+       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);

readw
writew

As I can see there are no such field names in struct dw_spi.


Yes. This seems safe but I still see the macro conflict which is strange to me. Like you, I initially used the dws->readw and dws->writew because I didn't see any conflicting field names.

Are readw & writew reserved variable names?

   };

   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
@@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32
offset, u16 val)
          __raw_writew(val, dws->regs + offset);
   }

+static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
+{
+       return (u16)__raw_readl(dws->regs + offset)

Maybe return dw_readl(dws, offset);

+}
+
+static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
+{
+       __raw_writel((u32)val, dws->regs + offset);

dw_writel(dws, offset, val);


Yes, I'll make these two changes - at least the write change. Based on feedback from your suggestion below, the dw_readw32 may be removed.

+}
+

So, does simple
dws->readw = dw_readl;
dws->writew = dw_writel;

work for you?


Yes, but I get the macro conflict shown above and "assignment from
incompatible pointer type" warnings. If I use the dws->read_w and
dws->write_w names, I get the incompatible pointer type warnings but it
works.

Thanks for reviewing.

Okay, I guess there are two variants:
a) replace u16 by u32 in existing functions;
b) introduce new ones like you did.

If no other opinions I think we better go b), but see above comments.

And one more thing. If dw_readw() works for maybe we leave them as is for now?


Yes, I just need the 32 bit write. I was trying to remain consistent but I agree that only changing only writes would minimize the changes.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux