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

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

2016-06-14 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 4:35 PM, Michal Suchanek  wrote:
> Hello,
>
> On 14 June 2016 at 07:45, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
>>> On 14 June 2016 at 06:47, Julian Calaby  wrote:
 Hi Michal,

 On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  
 wrote:
> Hello,
>
> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
>> wrote:
>>> The drivers are very similar and share multiple flaws which needed
>>> separate fixes for both drivers.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>  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

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

2016-06-14 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 4:35 PM, Michal Suchanek  wrote:
> Hello,
>
> On 14 June 2016 at 07:45, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
>>> On 14 June 2016 at 06:47, Julian Calaby  wrote:
 Hi Michal,

 On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  
 wrote:
> Hello,
>
> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
>> wrote:
>>> The drivers are very similar and share multiple flaws which needed
>>> separate fixes for both drivers.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>  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

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

2016-06-14 Thread Michal Suchanek

Hello,

On 14 June 2016 at 07:45, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
>> On 14 June 2016 at 06:47, Julian Calaby  wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
 Hello,

 On 14 June 2016 at 01:43, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
> wrote:
>> The drivers are very similar and share multiple flaws which needed
>> separate fixes for both drivers.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>  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

2016-06-14 Thread Michal Suchanek

Hello,

On 14 June 2016 at 07:45, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
>> On 14 June 2016 at 06:47, Julian Calaby  wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
 Hello,

 On 14 June 2016 at 01:43, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
> wrote:
>> The drivers are very similar and share multiple flaws which needed
>> separate fixes for both drivers.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>  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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
> On 14 June 2016 at 06:47, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
>>> Hello,
>>>
>>> On 14 June 2016 at 01:43, Julian Calaby  wrote:
 Hi Michal,

 On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
 wrote:
> The drivers are very similar and share multiple flaws which needed
> separate fixes for both drivers.
>
> Signed-off-by: Michal Suchanek 
> ---
>  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.cal...@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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek  wrote:
> On 14 June 2016 at 06:47, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
>>> Hello,
>>>
>>> On 14 June 2016 at 01:43, Julian Calaby  wrote:
 Hi Michal,

 On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  
 wrote:
> The drivers are very similar and share multiple flaws which needed
> separate fixes for both drivers.
>
> Signed-off-by: Michal Suchanek 
> ---
>  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.cal...@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

2016-06-13 Thread Michal Suchanek

On 14 June 2016 at 06:47, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
>> Hello,
>>
>> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
 The drivers are very similar and share multiple flaws which needed
 separate fixes for both drivers.

 Signed-off-by: Michal Suchanek 
 ---
  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

2016-06-13 Thread Michal Suchanek

On 14 June 2016 at 06:47, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
>> Hello,
>>
>> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>>> Hi Michal,
>>>
>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
 The drivers are very similar and share multiple flaws which needed
 separate fixes for both drivers.

 Signed-off-by: Michal Suchanek 
 ---
  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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
> Hello,
>
> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
>>> The drivers are very similar and share multiple flaws which needed
>>> separate fixes for both drivers.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>  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.cal...@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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek  wrote:
> Hello,
>
> On 14 June 2016 at 01:43, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
>>> The drivers are very similar and share multiple flaws which needed
>>> separate fixes for both drivers.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>  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.cal...@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

2016-06-13 Thread Michal Suchanek

Hello,

On 14 June 2016 at 01:43, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
>> The drivers are very similar and share multiple flaws which needed
>> separate fixes for both drivers.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>  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 = _regmap;
>> -   sspi->bitmap = _bitmap;
>> +   if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
>> +   sspi->fifo_depth = SUN4I_FIFO_DEPTH;
>> +   sspi->type = SPI_SUN4I;
>> +   sspi->regmap = _regmap;
>> +   sspi->bitmap = _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 = _regmap;
>> +   sspi->bitmap = _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

2016-06-13 Thread Michal Suchanek

Hello,

On 14 June 2016 at 01:43, Julian Calaby  wrote:
> Hi Michal,
>
> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
>> The drivers are very similar and share multiple flaws which needed
>> separate fixes for both drivers.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>  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 = _regmap;
>> -   sspi->bitmap = _bitmap;
>> +   if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
>> +   sspi->fifo_depth = SUN4I_FIFO_DEPTH;
>> +   sspi->type = SPI_SUN4I;
>> +   sspi->regmap = _regmap;
>> +   sspi->bitmap = _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 = _regmap;
>> +   sspi->bitmap = _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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
> The drivers are very similar and share multiple flaws which needed
> separate fixes for both drivers.
>
> Signed-off-by: Michal Suchanek 
> ---
>  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 = _regmap;
> -   sspi->bitmap = _bitmap;
> +   if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
> +   sspi->fifo_depth = SUN4I_FIFO_DEPTH;
> +   sspi->type = SPI_SUN4I;
> +   sspi->regmap = _regmap;
> +   sspi->bitmap = _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 = _regmap;
> +   sspi->bitmap = _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", 
> );
> +   dev_err(>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,
> + )) {
> +   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.cal...@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

2016-06-13 Thread Julian Calaby

Hi Michal,

On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek  wrote:
> The drivers are very similar and share multiple flaws which needed
> separate fixes for both drivers.
>
> Signed-off-by: Michal Suchanek 
> ---
>  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 = _regmap;
> -   sspi->bitmap = _bitmap;
> +   if (of_device_is_compatible(pdev->dev.of_node, SUN4I_COMPATIBLE)) {
> +   sspi->fifo_depth = SUN4I_FIFO_DEPTH;
> +   sspi->type = SPI_SUN4I;
> +   sspi->regmap = _regmap;
> +   sspi->bitmap = _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 = _regmap;
> +   sspi->bitmap = _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", 
> );
> +   dev_err(>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,
> + )) {
> +   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.cal...@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

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

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

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

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

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

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

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

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

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

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

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

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

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

14 matches

Site Navigation

Mail list logo

Footer information