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

[Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>]

On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
> On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
> <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>
>>
>> On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
>>>
>>> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>>>
>>>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>>
>>>> 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-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html