Re: [PATCH v3 01/13] spi: sunxi: set maximum and minimum speed of SPI master

Re: [PATCH v3 01/13] spi: sunxi: set maximum and minimum speed of SPI master

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

On Mon, Jun 13, 2016 at 05:46:49PM -0000, Michal Suchanek wrote:
> The speed limits are unset in the sun4i and sun6i SPI drivers.
> 
> The maximum speed of SPI master is used when maximum speed of SPI slave
> is not specified. Also the __spi_validate function should check that
> transfer speeds do not exceed the master limits.
> 
> The user manual for A10 and A31 specifies maximum
> speed of the SPI clock as 100MHz and minimum as 3kHz.
> 
> Setting the SPI clock to out-of-spec values can lock up the SoC.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> --
> v2:
> new patch
> v3:
> fix constant style
> ---
>  drivers/spi/spi-sun4i.c | 2 ++
>  drivers/spi/spi-sun6i.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 1ddd9e2..4213508 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -387,6 +387,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
>  	}
>  
>  	sspi->master = master;
> +	master->max_speed_hz = 100 * 1000 * 1000;
> +	master->min_speed_hz =          3 * 1000;
>  	master->set_cs = sun4i_spi_set_cs;
>  	master->transfer_one = sun4i_spi_transfer_one;
>  	master->num_chipselect = 4;
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 42e2c4b..fe70695 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -386,6 +386,8 @@ static int sun6i_spi_probe(struct platform_device *pdev)
>  	}
>  
>  	sspi->master = master;
> +	master->max_speed_hz = 100 * 1000 * 1000;
> +	master->min_speed_hz =          3 * 1000;
>  	master->set_cs = sun6i_spi_set_cs;
>  	master->transfer_one = sun6i_spi_transfer_one;
>  	master->num_chipselect = 4;

I really don't get why you want to do that kind of padding, when no
one does in the rest of the driver, or the rest of the kernel.

Once properly changed,
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 02/13] spi: sunxi: fix transfer timeout

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Mon, Jun 13, 2016 at 05:46:49PM -0000, Michal Suchanek wrote:
> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
> actual time the transfer is supposed to take and multiply by 2 for good
> measure.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 03/13] spi: sun4i: fix FIFO limit

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

On Mon, Jun 13, 2016 at 05:46:49PM -0000, Michal Suchanek wrote:
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout.
> 
> Always leave room for one byte in the FIFO.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 04/13] spi: sunxi: expose maximum transfer size limit

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Mon, Jun 13, 2016 at 05:46:50PM -0000, Michal Suchanek wrote:
> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> support so report the limitation. Same for sun6i.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 00/13] sunxi spi fixes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
> Hello,
> 
> This is update of the sunxi spi patches that should give full-featured SPI
> driver.
> 
> First three patches fix issues with the current driver and can be of use for
> stable kernels so adding cc for those.
> 
> I merged the sun4i and sun6i driver because there several issues that need to
> be fixed in both separately and they are even out of sync wrt some fixes.
> I guess some of the merge patches can be squashed.
> 
> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
> tell if that side was broken by this patchset - especially the last patch that
> adds DMA was afaik never tested on sun6i.

So, you didn't run that code through checkpatch and you rewrite the
whole thing entirely without even testing it... Awesome.

For the record, I'm still very much opposed to such a merge.

The first fixes are very welcome though, can and should go in.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 00/13] sunxi spi fixes

[Michal Suchanek]

Hello,

On 13 June 2016 at 21:57, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
>> Hello,
>>
>> This is update of the sunxi spi patches that should give full-featured SPI
>> driver.
>>
>> First three patches fix issues with the current driver and can be of use for
>> stable kernels so adding cc for those.
>>
>> I merged the sun4i and sun6i driver because there several issues that need to
>> be fixed in both separately and they are even out of sync wrt some fixes.
>> I guess some of the merge patches can be squashed.
>>
>> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
>> tell if that side was broken by this patchset - especially the last patch that
>> adds DMA was afaik never tested on sun6i.
>
> So, you didn't run that code through checkpatch and you rewrite the
> whole thing entirely without even testing it... Awesome.

Aside from the DMA part this is not a rewrite.

And I did run the code through checkpatch. It still gives some
warnings about BUG_ON and overly long lines, sure. II don't think
those are that serious or that fixing them would improve the code.

Thanks

Michal

Re: [PATCH v3 00/13] sunxi spi fixes

[Michal Suchanek]

Hello,

On 13 June 2016 at 21:57, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
>> Hello,
>>
>> This is update of the sunxi spi patches that should give full-featured SPI
>> driver.
>>
>> First three patches fix issues with the current driver and can be of use for
>> stable kernels so adding cc for those.
>>
>> I merged the sun4i and sun6i driver because there several issues that need to
>> be fixed in both separately and they are even out of sync wrt some fixes.
>> I guess some of the merge patches can be squashed.
>>
>> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
>> tell if that side was broken by this patchset - especially the last patch that
>> adds DMA was afaik never tested on sun6i.
>

>
> For the record, I'm still very much opposed to such a merge.
>

What is the reason against the merge? I did not find the original discussion.

I tried to rename everything in the drivers from sun4i and sun6i to
sunxi to look at a clean diff and found about 5 differences 2 of which
look like a bug.

Thanks

Michal

Re: [PATCH v3 00/13] sunxi spi fixes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On Fri, Jun 17, 2016 at 12:34:44PM +0200, Michal Suchanek wrote:
> Hello,
> 
> On 13 June 2016 at 21:57, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
> >> Hello,
> >>
> >> This is update of the sunxi spi patches that should give full-featured SPI
> >> driver.
> >>
> >> First three patches fix issues with the current driver and can be of use for
> >> stable kernels so adding cc for those.
> >>
> >> I merged the sun4i and sun6i driver because there several issues that need to
> >> be fixed in both separately and they are even out of sync wrt some fixes.
> >> I guess some of the merge patches can be squashed.
> >>
> >> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
> >> tell if that side was broken by this patchset - especially the last patch that
> >> adds DMA was afaik never tested on sun6i.
> >
> >
> > For the record, I'm still very much opposed to such a merge.
> 
> What is the reason against the merge? I did not find the original
> discussion.

I really prefer some code that is concise and clear but a little
duplicated over some code that shares every possible lines of code but
is a giant mess impossible to understand.

I just came across the reg_field stuff in regmap that would allow to
partially address that problem though, there's still the bit indices
issue to overcome though.

> I tried to rename everything in the drivers from sun4i and sun6i to
> sunxi to look at a clean diff and found about 5 differences 2 of which
> look like a bug.

It's hard to tell without testing.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 00/13] sunxi spi fixes

[Michal Suchanek]

Hello,

On 25 July 2016 at 09:32, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Jun 17, 2016 at 12:34:44PM +0200, Michal Suchanek wrote:
>> Hello,
>>
>> On 13 June 2016 at 21:57, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
>> >> Hello,
>> >>
>> >> This is update of the sunxi spi patches that should give full-featured SPI
>> >> driver.
>> >>
>> >> First three patches fix issues with the current driver and can be of use for
>> >> stable kernels so adding cc for those.
>> >>
>> >> I merged the sun4i and sun6i driver because there several issues that need to
>> >> be fixed in both separately and they are even out of sync wrt some fixes.
>> >> I guess some of the merge patches can be squashed.
>> >>
>> >> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
>> >> tell if that side was broken by this patchset - especially the last patch that
>> >> adds DMA was afaik never tested on sun6i.
>> >
>> >
>> > For the record, I'm still very much opposed to such a merge.
>>
>> What is the reason against the merge? I did not find the original
>> discussion.
>
> I really prefer some code that is concise and clear but a little
> duplicated over some code that shares every possible lines of code but
> is a giant mess impossible to understand.

Yes, it's been tried. In the case of this driver there is more duplication
than differences. Also bitrot due to different variants receiving different
updates and fixes crept in already.

Adding the remap layer certainly does not make the driver easier to
understand but it's not becoming giant mess either.

>
> I just came across the reg_field stuff in regmap that would allow to
> partially address that problem though, there's still the bit indices
> issue to overcome though.
>
>> I tried to rename everything in the drivers from sun4i and sun6i to
>> sunxi to look at a clean diff and found about 5 differences 2 of which
>> look like a bug.
>
> It's hard to tell without testing.

I tested the merged (and separate) driver on a H3 board and found a
type bug in the remap layer.

I also found I can transfer at most 68 bytes without DMA although
the manuals claim 128 byte FIFO. DMA transfers weren't very reliable
which may or may not be a wiring issue. I have seen similar issues
intermittently with SPI on A10s.

Thanks

Michal

Re: [PATCH v3 00/13] sunxi spi fixes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

On Mon, Jul 25, 2016 at 10:03:14AM +0200, Michal Suchanek wrote:
> Hello,
> 
> On 25 July 2016 at 09:32, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Fri, Jun 17, 2016 at 12:34:44PM +0200, Michal Suchanek wrote:
> >> Hello,
> >>
> >> On 13 June 2016 at 21:57, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
> >> >> Hello,
> >> >>
> >> >> This is update of the sunxi spi patches that should give full-featured SPI
> >> >> driver.
> >> >>
> >> >> First three patches fix issues with the current driver and can be of use for
> >> >> stable kernels so adding cc for those.
> >> >>
> >> >> I merged the sun4i and sun6i driver because there several issues that need to
> >> >> be fixed in both separately and they are even out of sync wrt some fixes.
> >> >> I guess some of the merge patches can be squashed.
> >> >>
> >> >> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
> >> >> tell if that side was broken by this patchset - especially the last patch that
> >> >> adds DMA was afaik never tested on sun6i.
> >> >
> >> >
> >> > For the record, I'm still very much opposed to such a merge.
> >>
> >> What is the reason against the merge? I did not find the original
> >> discussion.
> >
> > I really prefer some code that is concise and clear but a little
> > duplicated over some code that shares every possible lines of code but
> > is a giant mess impossible to understand.
> 
> Yes, it's been tried. In the case of this driver there is more duplication
> than differences. Also bitrot due to different variants receiving different
> updates and fixes crept in already.
> 
> Adding the remap layer certainly does not make the driver easier to
> understand but it's not becoming giant mess either.

Well, when you say that you're not quite fond of using your structure
all over the place because "it makes your eyes bleed", I wouldn't call
that a good sales pitch.

But again, reg_field seems like a good solution for that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 00/13] sunxi spi fixes

[Michal Suchanek]

On 29 July 2016 at 22:22, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Mon, Jul 25, 2016 at 10:03:14AM +0200, Michal Suchanek wrote:
>> Hello,
>>
>> On 25 July 2016 at 09:32, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Fri, Jun 17, 2016 at 12:34:44PM +0200, Michal Suchanek wrote:
>> >> Hello,
>> >>
>> >> On 13 June 2016 at 21:57, Maxime Ripard
>> >> <maxime.ripard@free-electrons.com> wrote:
>> >> > On Mon, Jun 13, 2016 at 05:46:48PM -0000, Michal Suchanek wrote:
>> >> >> Hello,
>> >> >>
>> >> >> This is update of the sunxi spi patches that should give full-featured SPI
>> >> >> driver.
>> >> >>
>> >> >> First three patches fix issues with the current driver and can be of use for
>> >> >> stable kernels so adding cc for those.
>> >> >>
>> >> >> I merged the sun4i and sun6i driver because there several issues that need to
>> >> >> be fixed in both separately and they are even out of sync wrt some fixes.
>> >> >> I guess some of the merge patches can be squashed.
>> >> >>
>> >> >> I tested this with A10s Olinuxino Micro. I have no sun6i device so I cannot
>> >> >> tell if that side was broken by this patchset - especially the last patch that
>> >> >> adds DMA was afaik never tested on sun6i.
>> >> >
>> >> >
>> >> > For the record, I'm still very much opposed to such a merge.
>> >>
>> >> What is the reason against the merge? I did not find the original
>> >> discussion.
>> >
>> > I really prefer some code that is concise and clear but a little
>> > duplicated over some code that shares every possible lines of code but
>> > is a giant mess impossible to understand.
>>
>> Yes, it's been tried. In the case of this driver there is more duplication
>> than differences. Also bitrot due to different variants receiving different
>> updates and fixes crept in already.
>>
>> Adding the remap layer certainly does not make the driver easier to
>> understand but it's not becoming giant mess either.
>
> Well, when you say that you're not quite fond of using your structure
> all over the place because "it makes your eyes bleed", I wouldn't call
> that a good sales pitch.

That's why put dereferencing it inside the read function. It reduces the noise
throughout the code and make it easier to add checks or change the remap
implementation.

>
> But again, reg_field seems like a good solution for that.

I will look at reg_field.

I saw some drivers using remap layers that are way more heavyweight than
looking up an integer in a table so I wanted to avoid that.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 07/13] spi: sunxi: rename constants to match between sun4i and sun6i

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> SUNXI_CTL_ -> SUNXI_TFR_CTL_
> SUNXI_TFR_CTL_LMTF -> SUNXI_TFR_CTL_FBS

I don't know these abbreviations, are they both referring to the same thing?

> SUNXI_TFR_CTL_CS_ACTIVE_LOW -> SUNXI_TFR_CTL_SPOL

It looks like you're making the constant name less descriptive here.
Is the old version (CS_ACTIVE_LOW) incorrect?

> and some SUNXI_???_CTL_ -> SUNXI_CTL_
> for constants migrated to different registers between sun4i and sun6i
>
> No functional change.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/spi/spi-sun4i.c | 68 ++++++++++++++++++++++++-------------------------
>  drivers/spi/spi-sun6i.c | 14 +++++-----
>  2 files changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 155d720..b7f8de1 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -28,21 +28,21 @@
>
>  #define SUNXI_TXDATA_REG               0x04
>
> -#define SUNXI_CTL_REG                  0x08
> +#define SUNXI_TFR_CTL_REG              0x08
>  #define SUNXI_CTL_ENABLE               BIT(0)
>  #define SUNXI_CTL_MASTER               BIT(1)
> -#define SUNXI_CTL_CPHA                 BIT(2)
> -#define SUNXI_CTL_CPOL                 BIT(3)
> -#define SUNXI_CTL_CS_ACTIVE_LOW                BIT(4)
> -#define SUNXI_CTL_LMTF                 BIT(6)
> +#define SUNXI_TFR_CTL_CPHA             BIT(2)
> +#define SUNXI_TFR_CTL_CPOL             BIT(3)
> +#define SUNXI_TFR_CTL_SPOL             BIT(4)
> +#define SUNXI_TFR_CTL_FBS              BIT(6)
>  #define SUNXI_CTL_TF_RST               BIT(8)
>  #define SUNXI_CTL_RF_RST               BIT(9)
> -#define SUNXI_CTL_XCH                  BIT(10)
> -#define SUNXI_CTL_CS_MASK              0x3000
> -#define SUNXI_CTL_CS(cs)               (((cs) << 12) & SUNXI_CTL_CS_MASK)
> -#define SUNXI_CTL_DHB                  BIT(15)
> -#define SUNXI_CTL_CS_MANUAL            BIT(16)
> -#define SUNXI_CTL_CS_LEVEL             BIT(17)
> +#define SUNXI_TFR_CTL_XCH              BIT(10)
> +#define SUNXI_TFR_CTL_CS_MASK          0x3000
> +#define SUNXI_TFR_CTL_CS(cs)           (((cs) << 12) & SUNXI_TFR_CTL_CS_MASK)
> +#define SUNXI_TFR_CTL_DHB              BIT(15)
> +#define SUNXI_TFR_CTL_CS_MANUAL                BIT(16)
> +#define SUNXI_TFR_CTL_CS_LEVEL         BIT(17)
>  #define SUNXI_CTL_TP                   BIT(18)
>
>  #define SUNXI_INT_CTL_REG              0x0c
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index a27bf8f..f26b52a 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -26,9 +26,9 @@
>  #define SUNXI_FIFO_DEPTH               128
>
>  #define SUNXI_GBL_CTL_REG              0x04
> -#define SUNXI_GBL_CTL_BUS_ENABLE       BIT(0)
> -#define SUNXI_GBL_CTL_MASTER           BIT(1)
> -#define SUNXI_GBL_CTL_TP               BIT(7)
> +#define SUNXI_CTL_ENABLE               BIT(0)
> +#define SUNXI_CTL_MASTER               BIT(1)
> +#define SUNXI_CTL_TP                   BIT(7)

If these are bit definitions for the GBL register, why throw that
information away?

>  #define SUNXI_GBL_CTL_RST              BIT(31)
>
>  #define SUNXI_TFR_CTL_REG              0x08
> @@ -50,8 +50,8 @@
>  #define SUNXI_INT_STA_REG              0x14
>
>  #define SUNXI_FIFO_CTL_REG             0x18
> -#define SUNXI_FIFO_CTL_RF_RST          BIT(15)
> -#define SUNXI_FIFO_CTL_TF_RST          BIT(31)
> +#define SUNXI_CTL_RF_RST               BIT(15)
> +#define SUNXI_CTL_TF_RST               BIT(31)

Same here with FIFO.

>
>  #define SUNXI_FIFO_STA_REG             0x1c
>  #define SUNXI_FIFO_STA_RF_CNT_MASK     0x7f

My gut feeling on this is that we have a lot of cases of a definition
of a register offset, then definitions of the bits in that register
with that register encoded into the constant's name. You appear to be
throwing a lot of that information away which makes me worry.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 07/13] spi: sunxi: rename constants to match between sun4i and sun6i

[Michal Suchanek]

Hello,

On 14 June 2016 at 01:31, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>> SUNXI_CTL_ -> SUNXI_TFR_CTL_
>> SUNXI_TFR_CTL_LMTF -> SUNXI_TFR_CTL_FBS
>
> I don't know these abbreviations, are they both referring to the same thing?
>
>> SUNXI_TFR_CTL_CS_ACTIVE_LOW -> SUNXI_TFR_CTL_SPOL
>
> It looks like you're making the constant name less descriptive here.
> Is the old version (CS_ACTIVE_LOW) incorrect?
>
>> and some SUNXI_???_CTL_ -> SUNXI_CTL_
>> for constants migrated to different registers between sun4i and sun6i
>>
>> No functional change.
>>
>>  #define SUNXI_INT_CTL_REG              0x0c
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index a27bf8f..f26b52a 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -26,9 +26,9 @@
>>  #define SUNXI_FIFO_DEPTH               128
>>
>>  #define SUNXI_GBL_CTL_REG              0x04
>> -#define SUNXI_GBL_CTL_BUS_ENABLE       BIT(0)
>> -#define SUNXI_GBL_CTL_MASTER           BIT(1)
>> -#define SUNXI_GBL_CTL_TP               BIT(7)
>> +#define SUNXI_CTL_ENABLE               BIT(0)
>> +#define SUNXI_CTL_MASTER               BIT(1)
>> +#define SUNXI_CTL_TP                   BIT(7)
>
> If these are bit definitions for the GBL register, why throw that
> information away?

Those bits are on the TFR register in the earlier IP so it makes
perfect sense to me this way.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> The drivers are very similar and share multiple flaws which needed
> separate fixes for both drivers.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/spi/Kconfig     |   8 +-
>  drivers/spi/Makefile    |   1 -
>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>  4 files changed, 143 insertions(+), 620 deletions(-)
>  delete mode 100644 drivers/spi/spi-sun6i.c
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 0b8e6c6..c76f8e4 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>
>         /* Reset FIFOs */
> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
> +       if (sspi->type == SPI_SUN4I)
> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
> +       else
> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));

If we're already doing different stuff for each generation of the IP,
why not just use the register offsets and bit definitions directly?

>
>         /*
>          * Setup the transfer control register: Chip Select,
> @@ -427,7 +473,19 @@ static int sunxi_spi_runtime_resume(struct device *dev)
>                 goto err;
>         }
>
> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
> +       if (sspi->rstc) {
> +               ret = reset_control_deassert(sspi->rstc);
> +               if (ret) {
> +                       dev_err(dev, "Couldn't deassert the device from reset\n");
> +                       goto err2;
> +               }
> +       }
> +
> +       if (sspi->type == SPI_SUN4I)
> +               reg = SUNXI_TFR_CTL_REG;
> +       else
> +               reg = SUNXI_GBL_CTL_REG;
> +       sunxi_spi_write(sspi, reg,
>                         sspi_bits(sspi, SUNXI_CTL_ENABLE) |
>                         sspi_bits(sspi, SUNXI_CTL_MASTER) |
>                         sspi_bits(sspi, SUNXI_CTL_TP));

Same here.

> @@ -491,10 +558,37 @@ static int sunxi_spi_probe(struct platform_device *pdev)
>         }
>
>         sspi->master = master;
> -       sspi->fifo_depth = SUN4I_FIFO_DEPTH;
> -       sspi->type = SPI_SUN4I;
> -       sspi->regmap = &sun4i_regmap;
> -       sspi->bitmap = &sun4i_bitmap;
> +       if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
> +               sspi->fifo_depth = SUN4I_FIFO_DEPTH;
> +               sspi->type = SPI_SUN4I;
> +               sspi->regmap = &sun4i_regmap;
> +               sspi->bitmap = &sun4i_bitmap;
> +       } else if (of_device_is_compatible(pdev->dev.of_node,
> +                                          SUN6I_COMPATIBLE)) {
> +               sspi->fifo_depth = SUN6I_FIFO_DEPTH;
> +               sspi->type = SPI_SUN6I;
> +               sspi->regmap = &sun6i_regmap;
> +               sspi->bitmap = &sun6i_bitmap;

Can you store data in the match table instead of doing this?

> +       } else {
> +               const char *str = NULL;
> +               int i = 1;
> +
> +               of_property_read_string(pdev->dev.of_node, "compatible", &str);
> +               dev_err(&pdev->dev, "Unknown device compatible %s", str);
> +               /* is there no sane way to print a string array property ? */
> +               if (of_property_count_strings(pdev->dev.of_node, "compatible")
> +                   > 1) {
> +                       while (!of_property_read_string_index(pdev->dev.of_node,
> +                                                             "compatible", i,
> +                                                             &str)) {
> +                               pr_err(", %s", str);
> +                               i++;
> +                       }
> +               }
> +               ret = -EINVAL;
> +               goto err_free_master;
> +       }
> +
>         master->max_speed_hz = 100 * 1000 * 1000;
>         master->min_speed_hz =          3 * 1000;
>         master->set_cs = sunxi_spi_set_cs;

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Michal Suchanek]

Hello,

On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>> The drivers are very similar and share multiple flaws which needed
>> separate fixes for both drivers.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> ---
>>  drivers/spi/Kconfig     |   8 +-
>>  drivers/spi/Makefile    |   1 -
>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 0b8e6c6..c76f8e4 100644
>> --- a/drivers/spi/spi-sun4i.c
>> +++ b/drivers/spi/spi-sun4i.c
>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>
>>         /* Reset FIFOs */
>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>> +       if (sspi->type == SPI_SUN4I)
>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>> +       else
>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>
> If we're already doing different stuff for each generation of the IP,
> why not just use the register offsets and bit definitions directly?

Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
makes my eyes bleed and you cannot use the check that you are
accessing a register that actually exists.

>> @@ -491,10 +558,37 @@ static int sunxi_spi_probe(struct platform_device *pdev)
>>         }
>>
>>         sspi->master = master;
>> -       sspi->fifo_depth = SUN4I_FIFO_DEPTH;
>> -       sspi->type = SPI_SUN4I;
>> -       sspi->regmap = &sun4i_regmap;
>> -       sspi->bitmap = &sun4i_bitmap;
>> +       if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
>> +               sspi->fifo_depth = SUN4I_FIFO_DEPTH;
>> +               sspi->type = SPI_SUN4I;
>> +               sspi->regmap = &sun4i_regmap;
>> +               sspi->bitmap = &sun4i_bitmap;
>> +       } else if (of_device_is_compatible(pdev->dev.of_node,
>> +                                          SUN6I_COMPATIBLE)) {
>> +               sspi->fifo_depth = SUN6I_FIFO_DEPTH;
>> +               sspi->type = SPI_SUN6I;
>> +               sspi->regmap = &sun6i_regmap;
>> +               sspi->bitmap = &sun6i_bitmap;
>
> Can you store data in the match table instead of doing this?

That might be nicer. Will look into this.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> Hello,
>
> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> The drivers are very similar and share multiple flaws which needed
>>> separate fixes for both drivers.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>> ---
>>>  drivers/spi/Kconfig     |   8 +-
>>>  drivers/spi/Makefile    |   1 -
>>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>>
>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>> index 0b8e6c6..c76f8e4 100644
>>> --- a/drivers/spi/spi-sun4i.c
>>> +++ b/drivers/spi/spi-sun4i.c
>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>>
>>>         /* Reset FIFOs */
>>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>> +       if (sspi->type == SPI_SUN4I)
>>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>> +       else
>>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>
>> If we're already doing different stuff for each generation of the IP,
>> why not just use the register offsets and bit definitions directly?
>
> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
> makes my eyes bleed and you cannot use the check that you are
> accessing a register that actually exists.

I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the
indirection you added and using them directly, i.e.

#define SUN4I_TFR_CTL_RF_RST BIT(x)
#define SUN4I_TFR_CTL_TF_RST BIT(x)
#define SUN6I_FIFO_CTL_RF_RST BIT(x)
#define SUN6I_FIFO_CTL_TF_RST BIT(x)

then

if (sspi->type == SPI_SUN4I)
    sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg |
SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST);
else
    sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg |
SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);

I.e. the bits that need setting are in different registers, so you
have to have an if statement and separate calls. Therefore there's no
real benefit from the indirection you've introduced here, unless
you're expecting the SUN8I variant to use different bits in one of
those two registers.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Michal Suchanek]

On 14 June 2016 at 06:47, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>> Hello,
>>
>> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>> The drivers are very similar and share multiple flaws which needed
>>>> separate fixes for both drivers.
>>>>
>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>> ---
>>>>  drivers/spi/Kconfig     |   8 +-
>>>>  drivers/spi/Makefile    |   1 -
>>>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>>>
>>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>>> index 0b8e6c6..c76f8e4 100644
>>>> --- a/drivers/spi/spi-sun4i.c
>>>> +++ b/drivers/spi/spi-sun4i.c
>>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>>>
>>>>         /* Reset FIFOs */
>>>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>> +       if (sspi->type == SPI_SUN4I)
>>>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>> +       else
>>>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>>>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>
>>> If we're already doing different stuff for each generation of the IP,
>>> why not just use the register offsets and bit definitions directly?
>>
>> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
>> makes my eyes bleed and you cannot use the check that you are
>> accessing a register that actually exists.
>
> I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the
> indirection you added and using them directly, i.e.
>
> #define SUN4I_TFR_CTL_RF_RST BIT(x)
> #define SUN4I_TFR_CTL_TF_RST BIT(x)
> #define SUN6I_FIFO_CTL_RF_RST BIT(x)
> #define SUN6I_FIFO_CTL_TF_RST BIT(x)
>
> then
>
> if (sspi->type == SPI_SUN4I)
>     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg |
> SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST);
> else
>     sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg |
> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>
> I.e. the bits that need setting are in different registers, so you
> have to have an if statement and separate calls. Therefore there's no
> real benefit from the indirection you've introduced here, unless
> you're expecting the SUN8I variant to use different bits in one of
> those two registers.
>

That looks nice for this particular case.

Still you will have to remember that these bits are specified directly
while other bits on the register are mapped which is not so nice.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 14 June 2016 at 06:47, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> Hello,
>>>
>>> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
>>>> Hi Michal,
>>>>
>>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>> The drivers are very similar and share multiple flaws which needed
>>>>> separate fixes for both drivers.
>>>>>
>>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>>> ---
>>>>>  drivers/spi/Kconfig     |   8 +-
>>>>>  drivers/spi/Makefile    |   1 -
>>>>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>>>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>>>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>>>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>>>>
>>>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>>>> index 0b8e6c6..c76f8e4 100644
>>>>> --- a/drivers/spi/spi-sun4i.c
>>>>> +++ b/drivers/spi/spi-sun4i.c
>>>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>>>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>>>>
>>>>>         /* Reset FIFOs */
>>>>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>> +       if (sspi->type == SPI_SUN4I)
>>>>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>> +       else
>>>>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>>>>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>
>>>> If we're already doing different stuff for each generation of the IP,
>>>> why not just use the register offsets and bit definitions directly?
>>>
>>> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
>>> makes my eyes bleed and you cannot use the check that you are
>>> accessing a register that actually exists.
>>
>> I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the
>> indirection you added and using them directly, i.e.
>>
>> #define SUN4I_TFR_CTL_RF_RST BIT(x)
>> #define SUN4I_TFR_CTL_TF_RST BIT(x)
>> #define SUN6I_FIFO_CTL_RF_RST BIT(x)
>> #define SUN6I_FIFO_CTL_TF_RST BIT(x)
>>
>> then
>>
>> if (sspi->type == SPI_SUN4I)
>>     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg |
>> SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST);
>> else
>>     sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg |
>> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>>
>> I.e. the bits that need setting are in different registers, so you
>> have to have an if statement and separate calls. Therefore there's no
>> real benefit from the indirection you've introduced here, unless
>> you're expecting the SUN8I variant to use different bits in one of
>> those two registers.
>>
>
> That looks nice for this particular case.

There was another case I pointed out in this driver that could
potentially benefit from this.

> Still you will have to remember that these bits are specified directly
> while other bits on the register are mapped which is not so nice.

True. I'm not sure which path is best in this case. To my eye, your
indirection scheme seems like the "heavy" solution, however I have no
idea what form a "lighter" solution would take.

Other drivers I've seen which have tackled similar problems have used
a large struct to hold all the variant-specific stuff, whether that's
function pointers, register offsets, constants, etc. (so this code
could theoretically be re-written as sunxi_spi_write(sspi,
sspi->fifo_reg, reg | sspi->fifo_reset_arg) or sspi->reset_fifo())
however the driver I saw doing this (rtl8xxxu) was handling much more
significant differences between device variants than you are.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Michal Suchanek]

Hello,

On 14 June 2016 at 07:45, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>> On 14 June 2016 at 06:47, Julian Calaby <julian.calaby@gmail.com> wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>> Hello,
>>>>
>>>> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
>>>>> Hi Michal,
>>>>>
>>>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>>> The drivers are very similar and share multiple flaws which needed
>>>>>> separate fixes for both drivers.
>>>>>>
>>>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>>>> ---
>>>>>>  drivers/spi/Kconfig     |   8 +-
>>>>>>  drivers/spi/Makefile    |   1 -
>>>>>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>>>>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>>>>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>>>>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>>>>>
>>>>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>>>>> index 0b8e6c6..c76f8e4 100644
>>>>>> --- a/drivers/spi/spi-sun4i.c
>>>>>> +++ b/drivers/spi/spi-sun4i.c
>>>>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>>>>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>>>>>
>>>>>>         /* Reset FIFOs */
>>>>>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>> +       if (sspi->type == SPI_SUN4I)
>>>>>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>> +       else
>>>>>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>
>>>>> If we're already doing different stuff for each generation of the IP,
>>>>> why not just use the register offsets and bit definitions directly?
>>>>
>>>> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
>>>> makes my eyes bleed and you cannot use the check that you are
>>>> accessing a register that actually exists.
>>>
>>> I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the
>>> indirection you added and using them directly, i.e.
>>>
>>> #define SUN4I_TFR_CTL_RF_RST BIT(x)
>>> #define SUN4I_TFR_CTL_TF_RST BIT(x)
>>> #define SUN6I_FIFO_CTL_RF_RST BIT(x)
>>> #define SUN6I_FIFO_CTL_TF_RST BIT(x)
>>>
>>> then
>>>
>>> if (sspi->type == SPI_SUN4I)
>>>     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg |
>>> SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST);
>>> else
>>>     sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg |
>>> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>>>
>>> I.e. the bits that need setting are in different registers, so you
>>> have to have an if statement and separate calls. Therefore there's no
>>> real benefit from the indirection you've introduced here, unless
>>> you're expecting the SUN8I variant to use different bits in one of
>>> those two registers.
>>>
>>
>> That looks nice for this particular case.
>
> There was another case I pointed out in this driver that could
> potentially benefit from this.
>
>> Still you will have to remember that these bits are specified directly
>> while other bits on the register are mapped which is not so nice.
>
> True. I'm not sure which path is best in this case. To my eye, your
> indirection scheme seems like the "heavy" solution, however I have no
> idea what form a "lighter" solution would take.
>
> Other drivers I've seen which have tackled similar problems have used
> a large struct to hold all the variant-specific stuff, whether that's
> function pointers, register offsets, constants, etc. (so this code
> could theoretically be re-written as sunxi_spi_write(sspi,
> sspi->fifo_reg, reg | sspi->fifo_reset_arg)

This won't do. There is fifo_reg and tfr_reg on both IPs but some bits
from one migrated to the other.

> or sspi->reset_fifo())

This would work but would require more thorough rewrite and heavier
adaptation layer.

As it is where different register is used in different IP revision it
is specified explicitly in the code while register names to numbers
are mapped in read() and write(). Value bits are mapped by explicitly
calling a function on the name.

This gives nice 1:1 mapping to the old code and allows checking that
both the register and the bit value in question exist on the IP.

Matching the value to register relies on the driver code, however.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 4:35 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> Hello,
>
> On 14 June 2016 at 07:45, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> On 14 June 2016 at 06:47, Julian Calaby <julian.calaby@gmail.com> wrote:
>>>> Hi Michal,
>>>>
>>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>> Hello,
>>>>>
>>>>> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@gmail.com> wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>>>>>> The drivers are very similar and share multiple flaws which needed
>>>>>>> separate fixes for both drivers.
>>>>>>>
>>>>>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/spi/Kconfig     |   8 +-
>>>>>>>  drivers/spi/Makefile    |   1 -
>>>>>>>  drivers/spi/spi-sun4i.c | 156 +++++++++++--
>>>>>>>  drivers/spi/spi-sun6i.c | 598 ------------------------------------------------
>>>>>>>  4 files changed, 143 insertions(+), 620 deletions(-)
>>>>>>>  delete mode 100644 drivers/spi/spi-sun6i.c
>>>>>>>
>>>>>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>>>>>> index 0b8e6c6..c76f8e4 100644
>>>>>>> --- a/drivers/spi/spi-sun4i.c
>>>>>>> +++ b/drivers/spi/spi-sun4i.c
>>>>>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master,
>>>>>>>         reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
>>>>>>>
>>>>>>>         /* Reset FIFOs */
>>>>>>> -       sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>>>> -                       reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>>> -                       sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>>> +       if (sspi->type == SPI_SUN4I)
>>>>>>> +               sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
>>>>>>> +                               reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>>> +       else
>>>>>>> +               sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
>>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_RF_RST) |
>>>>>>> +                               sspi_bits(sspi, SUNXI_CTL_TF_RST));
>>>>>>
>>>>>> If we're already doing different stuff for each generation of the IP,
>>>>>> why not just use the register offsets and bit definitions directly?
>>>>>
>>>>> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place
>>>>> makes my eyes bleed and you cannot use the check that you are
>>>>> accessing a register that actually exists.
>>>>
>>>> I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the
>>>> indirection you added and using them directly, i.e.
>>>>
>>>> #define SUN4I_TFR_CTL_RF_RST BIT(x)
>>>> #define SUN4I_TFR_CTL_TF_RST BIT(x)
>>>> #define SUN6I_FIFO_CTL_RF_RST BIT(x)
>>>> #define SUN6I_FIFO_CTL_TF_RST BIT(x)
>>>>
>>>> then
>>>>
>>>> if (sspi->type == SPI_SUN4I)
>>>>     sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg |
>>>> SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST);
>>>> else
>>>>     sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg |
>>>> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>>>>
>>>> I.e. the bits that need setting are in different registers, so you
>>>> have to have an if statement and separate calls. Therefore there's no
>>>> real benefit from the indirection you've introduced here, unless
>>>> you're expecting the SUN8I variant to use different bits in one of
>>>> those two registers.
>>>>
>>>
>>> That looks nice for this particular case.
>>
>> There was another case I pointed out in this driver that could
>> potentially benefit from this.
>>
>>> Still you will have to remember that these bits are specified directly
>>> while other bits on the register are mapped which is not so nice.
>>
>> True. I'm not sure which path is best in this case. To my eye, your
>> indirection scheme seems like the "heavy" solution, however I have no
>> idea what form a "lighter" solution would take.
>>
>> Other drivers I've seen which have tackled similar problems have used
>> a large struct to hold all the variant-specific stuff, whether that's
>> function pointers, register offsets, constants, etc. (so this code
>> could theoretically be re-written as sunxi_spi_write(sspi,
>> sspi->fifo_reg, reg | sspi->fifo_reset_arg)
>
> This won't do. There is fifo_reg and tfr_reg on both IPs but some bits
> from one migrated to the other.
>
>> or sspi->reset_fifo())
>
> This would work but would require more thorough rewrite and heavier
> adaptation layer.
>
> As it is where different register is used in different IP revision it
> is specified explicitly in the code while register names to numbers
> are mapped in read() and write(). Value bits are mapped by explicitly
> calling a function on the name.
>
> This gives nice 1:1 mapping to the old code and allows checking that
> both the register and the bit value in question exist on the IP.

And your method is almost beautiful from that perspective, however I
still feel that it's too "heavy". That said, neither of my suggestions
are much better. I provided them in the hope that they might be
illuminating.

> Matching the value to register relies on the driver code, however.

Of course.

I'm not sure what the state-of-the-art for dealing with variants of
devices where the manufacturer keeps scrambling the registers is,
however I feel we can do better, though I'm not sure how.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 11/13] dt: spi: sun4i: merge sun4i and sun6i binding doc

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  .../devicetree/bindings/spi/spi-sun4i.txt          | 21 ++++++++++---------
>  .../devicetree/bindings/spi/spi-sun6i.txt          | 24 ----------------------
>  2 files changed, 11 insertions(+), 34 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/spi/spi-sun6i.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-sun4i.txt b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
> index de827f5..329e543 100644
> --- a/Documentation/devicetree/bindings/spi/spi-sun4i.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
> @@ -1,7 +1,8 @@
> -Allwinner A10 SPI controller
> +Allwinner A10/A31 SPI controller
>
>  Required properties:
> -- compatible: Should be "allwinner,sun4-a10-spi".
> +- compatible: Should be one of "allwinner,sun4i-a10-spi" and
> +                               "allwinner,sun6i-a31-spi"
>  - reg: Should contain register location and length.
>  - interrupts: Should contain interrupt.
>  - clocks: phandle to the clocks feeding the SPI controller. Two are
> @@ -9,16 +10,16 @@ Required properties:
>    - "ahb": the gated AHB parent clock
>    - "mod": the parent module clock
>  - clock-names: Must contain the clock names described just above
> +- resets: (sun6i only) phandle to the reset controller asserting
> +         this device in reset
>
>  Example:
>
> -spi1: spi@01c06000 {
> -       compatible = "allwinner,sun4i-a10-spi";
> -       reg = <0x01c06000 0x1000>;
> -       interrupts = <11>;
> -       clocks = <&ahb_gates 21>, <&spi1_clk>;
> +spi1: spi@01c69000 {
> +       compatible = "allwinner,sun6i-a31-spi";
> +       reg = <0x01c69000 0x1000>;
> +       interrupts = <0 66 4>;
> +       clocks = <&ahb1_gates 21>, <&spi1_clk>;
>         clock-names = "ahb", "mod";
> -       status = "disabled";
> -       #address-cells = <1>;
> -       #size-cells = <0>;
> +       resets = <&ahb1_rst 21>;

Why not have an example of each type?

>  };

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v3 11/13] dt: spi: sun4i: merge sun4i and sun6i binding doc

[Michal Suchanek]

On 14 June 2016 at 01:45, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> ---
>>  .../devicetree/bindings/spi/spi-sun4i.txt          | 21 ++++++++++---------
>>  .../devicetree/bindings/spi/spi-sun6i.txt          | 24 ----------------------
>>  2 files changed, 11 insertions(+), 34 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/spi/spi-sun6i.txt
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-sun4i.txt b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>> index de827f5..329e543 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>> @@ -1,7 +1,8 @@
>> -Allwinner A10 SPI controller
>> +Allwinner A10/A31 SPI controller
>>
>>  Required properties:
>> -- compatible: Should be "allwinner,sun4-a10-spi".
>> +- compatible: Should be one of "allwinner,sun4i-a10-spi" and
>> +                               "allwinner,sun6i-a31-spi"
>>  - reg: Should contain register location and length.
>>  - interrupts: Should contain interrupt.
>>  - clocks: phandle to the clocks feeding the SPI controller. Two are
>> @@ -9,16 +10,16 @@ Required properties:
>>    - "ahb": the gated AHB parent clock
>>    - "mod": the parent module clock
>>  - clock-names: Must contain the clock names described just above
>> +- resets: (sun6i only) phandle to the reset controller asserting
>> +         this device in reset
>>
>>  Example:
>>
>> -spi1: spi@01c06000 {
>> -       compatible = "allwinner,sun4i-a10-spi";
>> -       reg = <0x01c06000 0x1000>;
>> -       interrupts = <11>;
>> -       clocks = <&ahb_gates 21>, <&spi1_clk>;
>> +spi1: spi@01c69000 {
>> +       compatible = "allwinner,sun6i-a31-spi";
>> +       reg = <0x01c69000 0x1000>;
>> +       interrupts = <0 66 4>;
>> +       clocks = <&ahb1_gates 21>, <&spi1_clk>;
>>         clock-names = "ahb", "mod";
>> -       status = "disabled";
>> -       #address-cells = <1>;
>> -       #size-cells = <0>;
>> +       resets = <&ahb1_rst 21>;
>
> Why not have an example of each type?

How many binding docs have examples of all types?

There are actual DTs using these so you can look at those as well.

This driver covers 3 types of bindings which look different in the DT:

sun4i IP with some Chinese interrupt controller, sun4i IP with GIC,
and sun6i IP with GIC.

Thanks

Michal

Re: [linux-sunxi] [PATCH v3 11/13] dt: spi: sun4i: merge sun4i and sun6i binding doc

[Julian Calaby]

Hi Michal,

On Tue, Jun 14, 2016 at 2:40 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 14 June 2016 at 01:45, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>> ---
>>>  .../devicetree/bindings/spi/spi-sun4i.txt          | 21 ++++++++++---------
>>>  .../devicetree/bindings/spi/spi-sun6i.txt          | 24 ----------------------
>>>  2 files changed, 11 insertions(+), 34 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/spi/spi-sun6i.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-sun4i.txt b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>>> index de827f5..329e543 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>>> +++ b/Documentation/devicetree/bindings/spi/spi-sun4i.txt
>>> @@ -1,7 +1,8 @@
>>> -Allwinner A10 SPI controller
>>> +Allwinner A10/A31 SPI controller
>>>
>>>  Required properties:
>>> -- compatible: Should be "allwinner,sun4-a10-spi".
>>> +- compatible: Should be one of "allwinner,sun4i-a10-spi" and
>>> +                               "allwinner,sun6i-a31-spi"
>>>  - reg: Should contain register location and length.
>>>  - interrupts: Should contain interrupt.
>>>  - clocks: phandle to the clocks feeding the SPI controller. Two are
>>> @@ -9,16 +10,16 @@ Required properties:
>>>    - "ahb": the gated AHB parent clock
>>>    - "mod": the parent module clock
>>>  - clock-names: Must contain the clock names described just above
>>> +- resets: (sun6i only) phandle to the reset controller asserting
>>> +         this device in reset
>>>
>>>  Example:
>>>
>>> -spi1: spi@01c06000 {
>>> -       compatible = "allwinner,sun4i-a10-spi";
>>> -       reg = <0x01c06000 0x1000>;
>>> -       interrupts = <11>;
>>> -       clocks = <&ahb_gates 21>, <&spi1_clk>;
>>> +spi1: spi@01c69000 {
>>> +       compatible = "allwinner,sun6i-a31-spi";
>>> +       reg = <0x01c69000 0x1000>;
>>> +       interrupts = <0 66 4>;
>>> +       clocks = <&ahb1_gates 21>, <&spi1_clk>;
>>>         clock-names = "ahb", "mod";
>>> -       status = "disabled";
>>> -       #address-cells = <1>;
>>> -       #size-cells = <0>;
>>> +       resets = <&ahb1_rst 21>;
>>
>> Why not have an example of each type?
>
> How many binding docs have examples of all types?

I'm pretty sure that there's a few. This was only a suggestion, so if
it's not to your taste, ignore it.

> There are actual DTs using these so you can look at those as well.
>
> This driver covers 3 types of bindings which look different in the DT:
>
> sun4i IP with some Chinese interrupt controller, sun4i IP with GIC,
> and sun6i IP with GIC.

Fair point.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/