[PATCH] spi: spi-sh: add IORESOURCE_MEM_TYPE_MASK decoding for access size

[PATCH] spi: spi-sh: add IORESOURCE_MEM_TYPE_MASK decoding for access size

[Shimoda, Yoshihiro]

This SPI controller's access size is 32, or 8-bit. The previous driver
supported 32-bit only. So, this patch adds IORESOURCE_MEM_TYPE_MASK
decoding, an then, the driver can handle the SPI controller of 8-bit.
This patch also changes the readl/writel to ioread*/iowrite*.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/spi/spi-sh.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-sh.c b/drivers/spi/spi-sh.c
index 70c8af9..79442c3 100644
--- a/drivers/spi/spi-sh.c
+++ b/drivers/spi/spi-sh.c
@@ -92,17 +92,26 @@ struct spi_sh_data {
 	unsigned long cr1;
 	wait_queue_head_t wait;
 	spinlock_t lock;
+	int width;
 };

 static void spi_sh_write(struct spi_sh_data *ss, unsigned long data,
 			     unsigned long offset)
 {
-	writel(data, ss->addr + offset);
+	if (ss->width = 8)
+		iowrite8(data, ss->addr + (offset >> 2));
+	else if (ss->width = 32)
+		iowrite32(data, ss->addr + offset);
 }

 static unsigned long spi_sh_read(struct spi_sh_data *ss, unsigned long offset)
 {
-	return readl(ss->addr + offset);
+	if (ss->width = 8)
+		return ioread8(ss->addr + (offset >> 2));
+	else if (ss->width = 32)
+		return ioread32(ss->addr + offset);
+	else
+		return 0;
 }

 static void spi_sh_set_bit(struct spi_sh_data *ss, unsigned long val,
@@ -464,6 +473,18 @@ static int __devinit spi_sh_probe(struct platform_device *pdev)
 	ss = spi_master_get_devdata(master);
 	dev_set_drvdata(&pdev->dev, ss);

+	switch (res->flags & IORESOURCE_MEM_TYPE_MASK) {
+	case IORESOURCE_MEM_8BIT:
+		ss->width = 8;
+		break;
+	case IORESOURCE_MEM_32BIT:
+		ss->width = 32;
+		break;
+	default:
+		dev_err(&pdev->dev, "No support width\n");
+		ret = -ENODEV;
+		goto error1;
+	}
 	ss->irq = irq;
 	ss->master = master;
 	ss->addr = ioremap(res->start, resource_size(res));
-- 
1.7.1

Re: [PATCH] spi: spi-sh: add IORESOURCE_MEM_TYPE_MASK decoding for access size

[Paul Mundt]

On Thu, Jan 26, 2012 at 05:43:57PM +0900, Shimoda, Yoshihiro wrote:
> This SPI controller's access size is 32, or 8-bit. The previous driver
> supported 32-bit only. So, this patch adds IORESOURCE_MEM_TYPE_MASK
> decoding, an then, the driver can handle the SPI controller of 8-bit.
> This patch also changes the readl/writel to ioread*/iowrite*.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/spi/spi-sh.c |   25 +++++++++++++++++++++++--
>  1 files changed, 23 insertions(+), 2 deletions(-)
> 
..
>  static void spi_sh_set_bit(struct spi_sh_data *ss, unsigned long val,
> @@ -464,6 +473,18 @@ static int __devinit spi_sh_probe(struct platform_device *pdev)
>  	ss = spi_master_get_devdata(master);
>  	dev_set_drvdata(&pdev->dev, ss);
> 
> +	switch (res->flags & IORESOURCE_MEM_TYPE_MASK) {
> +	case IORESOURCE_MEM_8BIT:
> +		ss->width = 8;
> +		break;
> +	case IORESOURCE_MEM_32BIT:
> +		ss->width = 32;
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "No support width\n");
> +		ret = -ENODEV;
> +		goto error1;

If the default up to this point has been 32-bit only then it makes sense
for 32 to still remain the default. The 8-bit user is presumably a new
one and therefore has no existing platform data configuration to worry
about, while this change would require existing users to be updated for
the new 32-bit flag to behave the same way they have up until now. 

If you wish to do this incrementally then you can of course convert all
of the existing platforms to the new mechanism for 32-bit as well and
then simply error out as above for the undefined width case, but I still
think it makes more sense to have a usable default.

Re: [PATCH] spi: spi-sh: add IORESOURCE_MEM_TYPE_MASK decoding for access size

[Shimoda, Yoshihiro]

2012/01/26 19:22, Paul Mundt wrote:
> On Thu, Jan 26, 2012 at 05:43:57PM +0900, Shimoda, Yoshihiro wrote:
[ snip ]
>>  static void spi_sh_set_bit(struct spi_sh_data *ss, unsigned long val,
>> @@ -464,6 +473,18 @@ static int __devinit spi_sh_probe(struct platform_device *pdev)
>>  	ss = spi_master_get_devdata(master);
>>  	dev_set_drvdata(&pdev->dev, ss);
>>
>> +	switch (res->flags & IORESOURCE_MEM_TYPE_MASK) {
>> +	case IORESOURCE_MEM_8BIT:
>> +		ss->width = 8;
>> +		break;
>> +	case IORESOURCE_MEM_32BIT:
>> +		ss->width = 32;
>> +		break;
>> +	default:
>> +		dev_err(&pdev->dev, "No support width\n");
>> +		ret = -ENODEV;
>> +		goto error1;
> 
> If the default up to this point has been 32-bit only then it makes sense
> for 32 to still remain the default. The 8-bit user is presumably a new
> one and therefore has no existing platform data configuration to worry
> about, while this change would require existing users to be updated for
> the new 32-bit flag to behave the same way they have up until now. 

Thank you for your comment.
Unfortunately, the value of IORESOURCE_MEM_8BIT is (0<<3), so existing users
will not enter to the default.

> If you wish to do this incrementally then you can of course convert all
> of the existing platforms to the new mechanism for 32-bit as well and
> then simply error out as above for the undefined width case, but I still
> think it makes more sense to have a usable default.

I think so. But, I think I cannot write such a code using IORESOURCE_MEM_*BIT
by the above reason.
At the moment, existing platform which uses this driver is one only. And,
I already sent a patch to modify the resource.
http://marc.info/?l=linux-sh&m\x132756762518679&w=2

Best regards,
Yoshihiro Shimoda

Re: [PATCH] spi: spi-sh: add IORESOURCE_MEM_TYPE_MASK decoding for access size

[Paul Mundt]

On Fri, Jan 27, 2012 at 10:14:49AM +0900, Shimoda, Yoshihiro wrote:
> 2012/01/26 19:22, Paul Mundt wrote:
> > On Thu, Jan 26, 2012 at 05:43:57PM +0900, Shimoda, Yoshihiro wrote:
> [ snip ]
> >>  static void spi_sh_set_bit(struct spi_sh_data *ss, unsigned long val,
> >> @@ -464,6 +473,18 @@ static int __devinit spi_sh_probe(struct platform_device *pdev)
> >>  	ss = spi_master_get_devdata(master);
> >>  	dev_set_drvdata(&pdev->dev, ss);
> >>
> >> +	switch (res->flags & IORESOURCE_MEM_TYPE_MASK) {
> >> +	case IORESOURCE_MEM_8BIT:
> >> +		ss->width = 8;
> >> +		break;
> >> +	case IORESOURCE_MEM_32BIT:
> >> +		ss->width = 32;
> >> +		break;
> >> +	default:
> >> +		dev_err(&pdev->dev, "No support width\n");
> >> +		ret = -ENODEV;
> >> +		goto error1;
> > 
> > If the default up to this point has been 32-bit only then it makes sense
> > for 32 to still remain the default. The 8-bit user is presumably a new
> > one and therefore has no existing platform data configuration to worry
> > about, while this change would require existing users to be updated for
> > the new 32-bit flag to behave the same way they have up until now. 
> 
> Thank you for your comment.
> Unfortunately, the value of IORESOURCE_MEM_8BIT is (0<<3), so existing users
> will not enter to the default.
> 
Ah, ok, I missed that. In that case I suppose your original patch makes
the most sense. We should be able to roll in the platform update patch
early anyways.