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

On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
[off-list ref] wrote:

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

quoted

On Sat, Mar 7, 2015 at 1:46 AM,  [off-list ref] wrote:

quoted

From: Thor Thayer <redacted>

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.

quoted

quoted

--- 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"

quoted

quoted

+                       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 = …

quoted

quoted

--- 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.

quoted

quoted

  };

  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);

quoted

quoted

+}
+
+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);

quoted

quoted

+}
+

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?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html