Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Nicolas Ferre]

On 29/05/2018 at 16:34, Nicolas Ferre wrote:

On 29/05/2018 at 16:28, Radu Pirea wrote:



On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.


[..]


+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of
cs-gpios and to read their values one by one?


As Alexandre said, we can make this driver OF specific.

What could be interesting is to use the gpio descriptors API and not the
older one. This would allow us to have far more control over the gpio
that we use in our drivers (Ludovic is converting our drivers to only
use gpiod structures).


Oh, but we already said that we cannot.
Disregard my comment then, sorry for the noise.


Regards,
Nicolas






+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}

[..]




--
Nicolas Ferre

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Nicolas Ferre]

On 29/05/2018 at 16:34, Nicolas Ferre wrote:

On 29/05/2018 at 16:28, Radu Pirea wrote:



On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.


[..]


+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of
cs-gpios and to read their values one by one?


As Alexandre said, we can make this driver OF specific.

What could be interesting is to use the gpio descriptors API and not the
older one. This would allow us to have far more control over the gpio
that we use in our drivers (Ludovic is converting our drivers to only
use gpiod structures).


Oh, but we already said that we cannot.
Disregard my comment then, sorry for the noise.


Regards,
Nicolas






+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}

[..]




--
Nicolas Ferre

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Nicolas Ferre]

On 29/05/2018 at 16:28, Radu Pirea wrote:



On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.


[..]


+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of
cs-gpios and to read their values one by one?


As Alexandre said, we can make this driver OF specific.

What could be interesting is to use the gpio descriptors API and not the 
older one. This would allow us to have far more control over the gpio 
that we use in our drivers (Ludovic is converting our drivers to only 
use gpiod structures).


Regards,
  Nicolas






+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}

[..]

--
Nicolas Ferre

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Nicolas Ferre]

On 29/05/2018 at 16:28, Radu Pirea wrote:



On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.


[..]


+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of
cs-gpios and to read their values one by one?


As Alexandre said, we can make this driver OF specific.

What could be interesting is to use the gpio descriptors API and not the 
older one. This would allow us to have far more control over the gpio 
that we use in our drivers (Ludovic is converting our drivers to only 
use gpiod structures).


Regards,
  Nicolas






+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}

[..]

--
Nicolas Ferre

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Radu Pirea]

On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.

The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.



+#include 


What is the use of it?


I need of_gpio.h for of_gpio_named_count, of_get_named_gpio and 
devm_gpio_request_one(found in gpio.h)





+#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
US_MR_CLKO | \
+   US_MR_WRDBT)


Don't split lines like this, it's hard to read.

#define FOO \
  (BAR1 | BAR2)


I'll fix it.



I think I already told this to someone recently, maybe to you.


+/* Register access macros */
+#define spi_readl(port, reg) \
+   readl_relaxed((port)->regs + US_##reg)
+#define spi_writel(port, reg, value) \
+   writel_relaxed((value), (port)->regs + US_##reg)
+
+#define spi_readb(port, reg) \
+   readb_relaxed((port)->regs + US_##reg)
+#define spi_writeb(port, reg, value) \
+   writeb_relaxed((value), (port)->regs + US_##reg)


Names are too generic. You better to use the same prefix as for the
rest, i.e. at91_spi_


Good ideea. I will change the names.




+   /*used in interrupt to protect data reading*/


Comment style.

You need to read some existing code, perhaps, to see how it's done.


Ok. I will add the comment.




+static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
+{
+   unsigned int len = aus->current_transfer->len;
+   unsigned int remaining = aus->current_tx_remaining_bytes;
+   const u8  *tx_buf = aus->current_transfer->tx_buf;
+



+   if (remaining)
+   if (at91_usart_spi_tx_ready(aus)) {


if (x) {
  if (y) {
...
  }
}

is equivalent to if (x && y) {}.

Though, considering your intention here, I would rather go with better
pattern, i.e.

if (!remaining)
  return;


Thank for suggestion. I will change.




+   spi_writeb(aus, THR, tx_buf[len - remaining]);
+   aus->current_tx_remaining_bytes--;
+   }
+}
+
+static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
+{



+   if (remaining) {
+   rx_buf[len - remaining] = spi_readb(aus, RHR);
+   aus->current_rx_remaining_bytes--;
+   }


Ditto.


+}




+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of 
cs-gpios and to read their values one by one?





+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}
+
+static int at91_usart_spi_probe(struct platform_device *pdev)
+{



+   regs = platform_get_resource(to_platform_device(pdev->dev.parent),
+IORESOURCE_MEM, 0);
+   if (!regs)
+   return -EINVAL;


This looks weird. Supply resource to _this_ device in your MFD code.


I know weird, but is the safest way to pass the resource and the of_node.




+   dev_info(>dev,
+"Atmel USART SPI Controller version 0x%x at 0x%08lx (irq 
%d)\n",
+spi_readl(aus, VERSION),
+(unsigned long)regs->start, irq);


I think I already told you, don't use explicit casting when print.
If it wasn't you, do you homework then. But above is no go. >

+   return 0;



+static struct platform_driver at91_usart_spi_driver = {
+   .driver = {
+   .name = "at91_usart_spi",



+   .of_match_table = of_match_ptr(at91_usart_spi_dt_ids),


Drop of_match_ptr(). It's not needed.


+   },
+   .probe = at91_usart_spi_probe,



+   .remove = at91_usart_spi_remove, };


Already told ya, split lines correctly.

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Radu Pirea]

On 05/28/2018 11:21 AM, Andy Shevchenko wrote:

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.

The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.



+#include 


What is the use of it?


I need of_gpio.h for of_gpio_named_count, of_get_named_gpio and 
devm_gpio_request_one(found in gpio.h)





+#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
US_MR_CLKO | \
+   US_MR_WRDBT)


Don't split lines like this, it's hard to read.

#define FOO \
  (BAR1 | BAR2)


I'll fix it.



I think I already told this to someone recently, maybe to you.


+/* Register access macros */
+#define spi_readl(port, reg) \
+   readl_relaxed((port)->regs + US_##reg)
+#define spi_writel(port, reg, value) \
+   writel_relaxed((value), (port)->regs + US_##reg)
+
+#define spi_readb(port, reg) \
+   readb_relaxed((port)->regs + US_##reg)
+#define spi_writeb(port, reg, value) \
+   writeb_relaxed((value), (port)->regs + US_##reg)


Names are too generic. You better to use the same prefix as for the
rest, i.e. at91_spi_


Good ideea. I will change the names.




+   /*used in interrupt to protect data reading*/


Comment style.

You need to read some existing code, perhaps, to see how it's done.


Ok. I will add the comment.




+static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
+{
+   unsigned int len = aus->current_transfer->len;
+   unsigned int remaining = aus->current_tx_remaining_bytes;
+   const u8  *tx_buf = aus->current_transfer->tx_buf;
+



+   if (remaining)
+   if (at91_usart_spi_tx_ready(aus)) {


if (x) {
  if (y) {
...
  }
}

is equivalent to if (x && y) {}.

Though, considering your intention here, I would rather go with better
pattern, i.e.

if (!remaining)
  return;


Thank for suggestion. I will change.




+   spi_writeb(aus, THR, tx_buf[len - remaining]);
+   aus->current_tx_remaining_bytes--;
+   }
+}
+
+static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
+{



+   if (remaining) {
+   rx_buf[len - remaining] = spi_readb(aus, RHR);
+   aus->current_rx_remaining_bytes--;
+   }


Ditto.


+}




+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{



+   struct device_node  *np = pdev->dev.parent->of_node;


Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.


Ok. What do you suggest to use instead of OF API to get the count of 
cs-gpios and to read their values one by one?





+   int i;



+   int ret = 0;
+   int nb = 0;


What happened to indentation?

Redundnant assignment for both.


+   if (!np)
+   return -EINVAL;
+
+   nb = of_gpio_named_count(np, "cs-gpios");
+   for (i = 0; i < nb; i++) {
+   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+   if (cs_gpio < 0)
+   return cs_gpio;
+
+   if (gpio_is_valid(cs_gpio)) {
+   ret = devm_gpio_request_one(>dev, cs_gpio,
+   GPIOF_DIR_OUT,
+   dev_name(>dev));
+   if (ret)
+   return ret;
+   }
+   }
+
+   return 0;
+}
+
+static int at91_usart_spi_probe(struct platform_device *pdev)
+{



+   regs = platform_get_resource(to_platform_device(pdev->dev.parent),
+IORESOURCE_MEM, 0);
+   if (!regs)
+   return -EINVAL;


This looks weird. Supply resource to _this_ device in your MFD code.


I know weird, but is the safest way to pass the resource and the of_node.




+   dev_info(>dev,
+"Atmel USART SPI Controller version 0x%x at 0x%08lx (irq 
%d)\n",
+spi_readl(aus, VERSION),
+(unsigned long)regs->start, irq);


I think I already told you, don't use explicit casting when print.
If it wasn't you, do you homework then. But above is no go. >

+   return 0;



+static struct platform_driver at91_usart_spi_driver = {
+   .driver = {
+   .name = "at91_usart_spi",



+   .of_match_table = of_match_ptr(at91_usart_spi_dt_ids),


Drop of_match_ptr(). It's not needed.


+   },
+   .probe = at91_usart_spi_probe,



+   .remove = at91_usart_spi_remove, };


Already told ya, split lines correctly.

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Andy Shevchenko]

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:
> This is the driver for at91-usart in spi mode. The USART IP can be configured
> to work in many modes and one of them is SPI.
>
> The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
> enc28j60 ethernet controller as slave.

> +#include 

What is the use of it?

> +#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
> US_MR_CLKO | \
> +   US_MR_WRDBT)

Don't split lines like this, it's hard to read.

#define FOO \
 (BAR1 | BAR2)

I think I already told this to someone recently, maybe to you.

> +/* Register access macros */
> +#define spi_readl(port, reg) \
> +   readl_relaxed((port)->regs + US_##reg)
> +#define spi_writel(port, reg, value) \
> +   writel_relaxed((value), (port)->regs + US_##reg)
> +
> +#define spi_readb(port, reg) \
> +   readb_relaxed((port)->regs + US_##reg)
> +#define spi_writeb(port, reg, value) \
> +   writeb_relaxed((value), (port)->regs + US_##reg)

Names are too generic. You better to use the same prefix as for the
rest, i.e. at91_spi_

> +   /*used in interrupt to protect data reading*/

Comment style.

You need to read some existing code, perhaps, to see how it's done.

> +static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
> +{
> +   unsigned int len = aus->current_transfer->len;
> +   unsigned int remaining = aus->current_tx_remaining_bytes;
> +   const u8  *tx_buf = aus->current_transfer->tx_buf;
> +

> +   if (remaining)
> +   if (at91_usart_spi_tx_ready(aus)) {

if (x) {
 if (y) {
...
 }
}

is equivalent to if (x && y) {}.

Though, considering your intention here, I would rather go with better
pattern, i.e.

if (!remaining)
 return;

> +   spi_writeb(aus, THR, tx_buf[len - remaining]);
> +   aus->current_tx_remaining_bytes--;
> +   }
> +}
> +
> +static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
> +{

> +   if (remaining) {
> +   rx_buf[len - remaining] = spi_readb(aus, RHR);
> +   aus->current_rx_remaining_bytes--;
> +   }

Ditto.

> +}


> +static int at91_usart_gpio_setup(struct platform_device *pdev)
> +{

> +   struct device_node  *np = pdev->dev.parent->of_node;

Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.

> +   int i;

> +   int ret = 0;
> +   int nb = 0;

What happened to indentation?

Redundnant assignment for both.

> +   if (!np)
> +   return -EINVAL;
> +
> +   nb = of_gpio_named_count(np, "cs-gpios");
> +   for (i = 0; i < nb; i++) {
> +   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
> +
> +   if (cs_gpio < 0)
> +   return cs_gpio;
> +
> +   if (gpio_is_valid(cs_gpio)) {
> +   ret = devm_gpio_request_one(>dev, cs_gpio,
> +   GPIOF_DIR_OUT,
> +   dev_name(>dev));
> +   if (ret)
> +   return ret;
> +   }
> +   }
> +
> +   return 0;
> +}
> +
> +static int at91_usart_spi_probe(struct platform_device *pdev)
> +{

> +   regs = platform_get_resource(to_platform_device(pdev->dev.parent),
> +IORESOURCE_MEM, 0);
> +   if (!regs)
> +   return -EINVAL;

This looks weird. Supply resource to _this_ device in your MFD code.

> +   dev_info(>dev,
> +"Atmel USART SPI Controller version 0x%x at 0x%08lx (irq 
> %d)\n",
> +spi_readl(aus, VERSION),
> +(unsigned long)regs->start, irq);

I think I already told you, don't use explicit casting when print.
If it wasn't you, do you homework then. But above is no go.

> +   return 0;

> +static struct platform_driver at91_usart_spi_driver = {
> +   .driver = {
> +   .name = "at91_usart_spi",

> +   .of_match_table = of_match_ptr(at91_usart_spi_dt_ids),

Drop of_match_ptr(). It's not needed.

> +   },
> +   .probe = at91_usart_spi_probe,

> +   .remove = at91_usart_spi_remove, };

Already told ya, split lines correctly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Andy Shevchenko]

On Fri, May 25, 2018 at 8:19 PM, Radu Pirea  wrote:
> This is the driver for at91-usart in spi mode. The USART IP can be configured
> to work in many modes and one of them is SPI.
>
> The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
> enc28j60 ethernet controller as slave.

> +#include 

What is the use of it?

> +#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
> US_MR_CLKO | \
> +   US_MR_WRDBT)

Don't split lines like this, it's hard to read.

#define FOO \
 (BAR1 | BAR2)

I think I already told this to someone recently, maybe to you.

> +/* Register access macros */
> +#define spi_readl(port, reg) \
> +   readl_relaxed((port)->regs + US_##reg)
> +#define spi_writel(port, reg, value) \
> +   writel_relaxed((value), (port)->regs + US_##reg)
> +
> +#define spi_readb(port, reg) \
> +   readb_relaxed((port)->regs + US_##reg)
> +#define spi_writeb(port, reg, value) \
> +   writeb_relaxed((value), (port)->regs + US_##reg)

Names are too generic. You better to use the same prefix as for the
rest, i.e. at91_spi_

> +   /*used in interrupt to protect data reading*/

Comment style.

You need to read some existing code, perhaps, to see how it's done.

> +static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
> +{
> +   unsigned int len = aus->current_transfer->len;
> +   unsigned int remaining = aus->current_tx_remaining_bytes;
> +   const u8  *tx_buf = aus->current_transfer->tx_buf;
> +

> +   if (remaining)
> +   if (at91_usart_spi_tx_ready(aus)) {

if (x) {
 if (y) {
...
 }
}

is equivalent to if (x && y) {}.

Though, considering your intention here, I would rather go with better
pattern, i.e.

if (!remaining)
 return;

> +   spi_writeb(aus, THR, tx_buf[len - remaining]);
> +   aus->current_tx_remaining_bytes--;
> +   }
> +}
> +
> +static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
> +{

> +   if (remaining) {
> +   rx_buf[len - remaining] = spi_readb(aus, RHR);
> +   aus->current_rx_remaining_bytes--;
> +   }

Ditto.

> +}


> +static int at91_usart_gpio_setup(struct platform_device *pdev)
> +{

> +   struct device_node  *np = pdev->dev.parent->of_node;

Your driver is not OF specific as far as I can see. Drop all these
device_node stuff and change API calls respectively.

> +   int i;

> +   int ret = 0;
> +   int nb = 0;

What happened to indentation?

Redundnant assignment for both.

> +   if (!np)
> +   return -EINVAL;
> +
> +   nb = of_gpio_named_count(np, "cs-gpios");
> +   for (i = 0; i < nb; i++) {
> +   int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
> +
> +   if (cs_gpio < 0)
> +   return cs_gpio;
> +
> +   if (gpio_is_valid(cs_gpio)) {
> +   ret = devm_gpio_request_one(>dev, cs_gpio,
> +   GPIOF_DIR_OUT,
> +   dev_name(>dev));
> +   if (ret)
> +   return ret;
> +   }
> +   }
> +
> +   return 0;
> +}
> +
> +static int at91_usart_spi_probe(struct platform_device *pdev)
> +{

> +   regs = platform_get_resource(to_platform_device(pdev->dev.parent),
> +IORESOURCE_MEM, 0);
> +   if (!regs)
> +   return -EINVAL;

This looks weird. Supply resource to _this_ device in your MFD code.

> +   dev_info(>dev,
> +"Atmel USART SPI Controller version 0x%x at 0x%08lx (irq 
> %d)\n",
> +spi_readl(aus, VERSION),
> +(unsigned long)regs->start, irq);

I think I already told you, don't use explicit casting when print.
If it wasn't you, do you homework then. But above is no go.

> +   return 0;

> +static struct platform_driver at91_usart_spi_driver = {
> +   .driver = {
> +   .name = "at91_usart_spi",

> +   .of_match_table = of_match_ptr(at91_usart_spi_dt_ids),

Drop of_match_ptr(). It's not needed.

> +   },
> +   .probe = at91_usart_spi_probe,

> +   .remove = at91_usart_spi_remove, };

Already told ya, split lines correctly.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Radu Pirea]

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.

The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.

Signed-off-by: Radu Pirea 
---
 drivers/spi/Kconfig  |   9 +
 drivers/spi/Makefile |   1 +
 drivers/spi/spi-at91-usart.c | 431 +++
 3 files changed, 441 insertions(+)
 create mode 100644 drivers/spi/spi-at91-usart.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6fb0347a24f2..1a002a32d7aa 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -77,6 +77,15 @@ config SPI_ATMEL
  This selects a driver for the Atmel SPI Controller, present on
  many AT91 (ARM) chips.
 
+config SPI_AT91_USART
+   tristate "Atmel USART Controller SPI driver"
+   depends on HAS_DMA
+   depends on (ARCH_AT91 || COMPILE_TEST)
+   select MFD_AT91_USART
+   help
+ This selects a driver for the AT91 USART Controller as SPI Master,
+ present on AT91 and SAMA5 SoC series.
+
 config SPI_AU1550
tristate "Au1550/Au1200/Au1300 SPI Controller"
depends on MIPS_ALCHEMY
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 34c5f2832ddf..fb6cb42f4eaa 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)   += 
spi-loopback-test.o
 obj-$(CONFIG_SPI_ALTERA)   += spi-altera.o
 obj-$(CONFIG_SPI_ARMADA_3700)  += spi-armada-3700.o
 obj-$(CONFIG_SPI_ATMEL)+= spi-atmel.o
+obj-$(CONFIG_SPI_AT91_USART)   += spi-at91-usart.o
 obj-$(CONFIG_SPI_ATH79)+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)   += spi-au1550.o
 obj-$(CONFIG_SPI_AXI_SPI_ENGINE)   += spi-axi-spi-engine.o
diff --git a/drivers/spi/spi-at91-usart.c b/drivers/spi/spi-at91-usart.c
new file mode 100644
index ..3ca0fab6691e
--- /dev/null
+++ b/drivers/spi/spi-at91-usart.c
@@ -0,0 +1,431 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AT91 USART Controllers as SPI
+ *
+ * Copyright (C) 2018 Microchip Technology Inc.
+ * Author: Radu Pirea 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define US_CR  0x00
+#define US_MR  0x04
+#define US_IER 0x08
+#define US_IDR 0x0C
+#define US_CSR 0x14
+#define US_RHR 0x18
+#define US_THR 0x1C
+#define US_BRGR0x20
+#define US_VERSION 0xFC
+
+#define US_CR_RSTRXBIT(2)
+#define US_CR_RSTTXBIT(3)
+#define US_CR_RXEN BIT(4)
+#define US_CR_RXDISBIT(5)
+#define US_CR_TXEN BIT(6)
+#define US_CR_TXDISBIT(7)
+
+#define US_MR_SPI_MASTER   0x0E
+#define US_MR_CHRL GENMASK(7, 6)
+#define US_MR_CPHA BIT(8)
+#define US_MR_CPOL BIT(16)
+#define US_MR_CLKO BIT(18)
+#define US_MR_WRDBTBIT(20)
+#define US_MR_LOOP BIT(15)
+
+#define US_IR_RXRDYBIT(0)
+#define US_IR_TXRDYBIT(1)
+#define US_IR_OVRE BIT(5)
+
+#define US_BRGR_SIZE   BIT(16)
+
+#define US_MIN_CLK_DIV 0x06
+#define US_MAX_CLK_DIV BIT(16)
+
+#define US_RESET   (US_CR_RSTRX | US_CR_RSTTX)
+#define US_DISABLE (US_CR_RXDIS | US_CR_TXDIS)
+#define US_ENABLE  (US_CR_RXEN | US_CR_TXEN)
+#define US_OVRE_RXRDY_IRQS (US_IR_OVRE | US_IR_RXRDY)
+
+#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
US_MR_CLKO | \
+   US_MR_WRDBT)
+
+/* Register access macros */
+#define spi_readl(port, reg) \
+   readl_relaxed((port)->regs + US_##reg)
+#define spi_writel(port, reg, value) \
+   writel_relaxed((value), (port)->regs + US_##reg)
+
+#define spi_readb(port, reg) \
+   readb_relaxed((port)->regs + US_##reg)
+#define spi_writeb(port, reg, value) \
+   writeb_relaxed((value), (port)->regs + US_##reg)
+
+struct at91_usart_spi {
+   struct spi_transfer *current_transfer;
+   void __iomem*regs;
+   struct device   *dev;
+   struct clk  *clk;
+
+   /*used in interrupt to protect data reading*/
+   spinlock_t  lock;
+
+   int irq;
+   unsigned intcurrent_tx_remaining_bytes;
+   unsigned intcurrent_rx_remaining_bytes;
+   int done_status;
+
+   u32 spi_clk;
+   u32 status;
+
+   boolxfer_failed;
+};
+
+static inline u32 at91_usart_spi_tx_ready(struct at91_usart_spi *aus)
+{
+   return aus->status & US_IR_TXRDY;

[PATCH v4 5/6] spi: at91-usart: add driver for at91-usart as spi

[Radu Pirea]

This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.

The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.

Signed-off-by: Radu Pirea 
---
 drivers/spi/Kconfig  |   9 +
 drivers/spi/Makefile |   1 +
 drivers/spi/spi-at91-usart.c | 431 +++
 3 files changed, 441 insertions(+)
 create mode 100644 drivers/spi/spi-at91-usart.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6fb0347a24f2..1a002a32d7aa 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -77,6 +77,15 @@ config SPI_ATMEL
  This selects a driver for the Atmel SPI Controller, present on
  many AT91 (ARM) chips.
 
+config SPI_AT91_USART
+   tristate "Atmel USART Controller SPI driver"
+   depends on HAS_DMA
+   depends on (ARCH_AT91 || COMPILE_TEST)
+   select MFD_AT91_USART
+   help
+ This selects a driver for the AT91 USART Controller as SPI Master,
+ present on AT91 and SAMA5 SoC series.
+
 config SPI_AU1550
tristate "Au1550/Au1200/Au1300 SPI Controller"
depends on MIPS_ALCHEMY
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 34c5f2832ddf..fb6cb42f4eaa 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)   += 
spi-loopback-test.o
 obj-$(CONFIG_SPI_ALTERA)   += spi-altera.o
 obj-$(CONFIG_SPI_ARMADA_3700)  += spi-armada-3700.o
 obj-$(CONFIG_SPI_ATMEL)+= spi-atmel.o
+obj-$(CONFIG_SPI_AT91_USART)   += spi-at91-usart.o
 obj-$(CONFIG_SPI_ATH79)+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)   += spi-au1550.o
 obj-$(CONFIG_SPI_AXI_SPI_ENGINE)   += spi-axi-spi-engine.o
diff --git a/drivers/spi/spi-at91-usart.c b/drivers/spi/spi-at91-usart.c
new file mode 100644
index ..3ca0fab6691e
--- /dev/null
+++ b/drivers/spi/spi-at91-usart.c
@@ -0,0 +1,431 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AT91 USART Controllers as SPI
+ *
+ * Copyright (C) 2018 Microchip Technology Inc.
+ * Author: Radu Pirea 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define US_CR  0x00
+#define US_MR  0x04
+#define US_IER 0x08
+#define US_IDR 0x0C
+#define US_CSR 0x14
+#define US_RHR 0x18
+#define US_THR 0x1C
+#define US_BRGR0x20
+#define US_VERSION 0xFC
+
+#define US_CR_RSTRXBIT(2)
+#define US_CR_RSTTXBIT(3)
+#define US_CR_RXEN BIT(4)
+#define US_CR_RXDISBIT(5)
+#define US_CR_TXEN BIT(6)
+#define US_CR_TXDISBIT(7)
+
+#define US_MR_SPI_MASTER   0x0E
+#define US_MR_CHRL GENMASK(7, 6)
+#define US_MR_CPHA BIT(8)
+#define US_MR_CPOL BIT(16)
+#define US_MR_CLKO BIT(18)
+#define US_MR_WRDBTBIT(20)
+#define US_MR_LOOP BIT(15)
+
+#define US_IR_RXRDYBIT(0)
+#define US_IR_TXRDYBIT(1)
+#define US_IR_OVRE BIT(5)
+
+#define US_BRGR_SIZE   BIT(16)
+
+#define US_MIN_CLK_DIV 0x06
+#define US_MAX_CLK_DIV BIT(16)
+
+#define US_RESET   (US_CR_RSTRX | US_CR_RSTTX)
+#define US_DISABLE (US_CR_RXDIS | US_CR_TXDIS)
+#define US_ENABLE  (US_CR_RXEN | US_CR_TXEN)
+#define US_OVRE_RXRDY_IRQS (US_IR_OVRE | US_IR_RXRDY)
+
+#define US_INIT(US_MR_SPI_MASTER | US_MR_CHRL | 
US_MR_CLKO | \
+   US_MR_WRDBT)
+
+/* Register access macros */
+#define spi_readl(port, reg) \
+   readl_relaxed((port)->regs + US_##reg)
+#define spi_writel(port, reg, value) \
+   writel_relaxed((value), (port)->regs + US_##reg)
+
+#define spi_readb(port, reg) \
+   readb_relaxed((port)->regs + US_##reg)
+#define spi_writeb(port, reg, value) \
+   writeb_relaxed((value), (port)->regs + US_##reg)
+
+struct at91_usart_spi {
+   struct spi_transfer *current_transfer;
+   void __iomem*regs;
+   struct device   *dev;
+   struct clk  *clk;
+
+   /*used in interrupt to protect data reading*/
+   spinlock_t  lock;
+
+   int irq;
+   unsigned intcurrent_tx_remaining_bytes;
+   unsigned intcurrent_rx_remaining_bytes;
+   int done_status;
+
+   u32 spi_clk;
+   u32 status;
+
+   boolxfer_failed;
+};
+
+static inline u32 at91_usart_spi_tx_ready(struct at91_usart_spi *aus)
+{
+   return aus->status & US_IR_TXRDY;
+}
+
+static inline u32