[PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[Varka Bhadram]

Hai Alex,

I followed the process that you mailed earlier, thnks for that.

I am expecting the mail from Alan about the changes.

Thank you,
Varka Bhadram.


Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/net/ieee802154/mrf24j40.c |  115 ++++++++++++++++++++++++++++---------
 1 file changed, 89 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 246befa..4a9a1ee 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -609,10 +609,95 @@ out:
 	return IRQ_HANDLED;
 }
 
+static int mrf24j40_hw_init(struct mrf24j40 *devrec)
+{
+	int ret;
+	u8 val;
+
+	/* Initialize the device.
+		From datasheet section 3.2: Initialization. */
+	ret = write_short_reg(devrec, REG_SOFTRST, 0x07);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_PACON2, 0x98);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_TXSTBL, 0x95);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON0, 0x03);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON1, 0x01);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON2, 0x80);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON6, 0x90);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON7, 0x80);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_RFCON8, 0x10);
+	if (ret)
+		goto err_ret;
+
+	ret = write_long_reg(devrec, REG_SLPCON1, 0x21);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_BBREG2, 0x80);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_CCAEDTH, 0x60);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_BBREG6, 0x40);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_RFCTL, 0x04);
+	if (ret)
+		goto err_ret;
+
+	ret = write_short_reg(devrec, REG_RFCTL, 0x0);
+	if (ret)
+		goto err_ret;
+
+	udelay(192);
+
+	/* Set RX Mode. RXMCR<1:0>: 0x0 normal, 0x1 promisc, 0x2 error */
+	ret = read_short_reg(devrec, REG_RXMCR, &val);
+	if (ret)
+		goto err_ret;
+
+	val &= ~0x3; /* Clear RX mode (normal) */
+
+	ret = write_short_reg(devrec, REG_RXMCR, val);
+	if (ret)
+		goto err_ret;
+
+	return 0;
+
+err_ret:
+	return ret;
+}
+
 static int mrf24j40_probe(struct spi_device *spi)
 {
 	int ret = -ENOMEM;
-	u8 val;
 	struct mrf24j40 *devrec;
 
 	printk(KERN_INFO "mrf24j40: probe(). IRQ: %d\n", spi->irq);
@@ -649,31 +734,9 @@ static int mrf24j40_probe(struct spi_device *spi)
 	if (ret)
 		goto err_register_device;
 
-	/* Initialize the device.
-		From datasheet section 3.2: Initialization. */
-	write_short_reg(devrec, REG_SOFTRST, 0x07);
-	write_short_reg(devrec, REG_PACON2, 0x98);
-	write_short_reg(devrec, REG_TXSTBL, 0x95);
-	write_long_reg(devrec, REG_RFCON0, 0x03);
-	write_long_reg(devrec, REG_RFCON1, 0x01);
-	write_long_reg(devrec, REG_RFCON2, 0x80);
-	write_long_reg(devrec, REG_RFCON6, 0x90);
-	write_long_reg(devrec, REG_RFCON7, 0x80);
-	write_long_reg(devrec, REG_RFCON8, 0x10);
-	write_long_reg(devrec, REG_SLPCON1, 0x21);
-	write_short_reg(devrec, REG_BBREG2, 0x80);
-	write_short_reg(devrec, REG_CCAEDTH, 0x60);
-	write_short_reg(devrec, REG_BBREG6, 0x40);
-	write_short_reg(devrec, REG_RFCTL, 0x04);
-	write_short_reg(devrec, REG_RFCTL, 0x0);
-	udelay(192);
-
-	/* Set RX Mode. RXMCR<1:0>: 0x0 normal, 0x1 promisc, 0x2 error */
-	ret = read_short_reg(devrec, REG_RXMCR, &val);
+	ret = mrf24j40_hw_init(devrec);
 	if (ret)
-		goto err_read_reg;
-	val &= ~0x3; /* Clear RX mode (normal) */
-	write_short_reg(devrec, REG_RXMCR, val);
+		goto err_hw_init;
 
 	ret = request_threaded_irq(spi->irq,
 				   NULL,
@@ -690,7 +753,7 @@ static int mrf24j40_probe(struct spi_device *spi)
 	return 0;
 
 err_irq:
-err_read_reg:
+err_hw_init:
 	ieee802154_unregister_device(devrec->dev);
 err_register_device:
 	ieee802154_free_device(devrec->dev);
-- 
1.7.9.5

Re: [PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[Alan Ott]

On 04/23/2014 09:11 AM, Varka Bhadram wrote:
> I followed the process that you mailed earlier, thnks for that.
>
> I am expecting the mail from Alan about the changes.

Hi Varka,

Is there a specific problem you're seeing? Typically in the kernel we 
expect the SPI controller to succeed for a couple reasons:
1. It's part of the basic, core functionality of a system. Checking for 
errors on SPI transfers is analogous to making sure RAM you wrote 
actually got written.
2. Most of the time an SPI failure is not something we can detect 
anyway. (disconnect one of the lines and see what you get).
3. The code to check for it just adds a lot of bloat without much 
measurable benefit.

I've read the above in the comments in other drivers, but I can't 
remember exactly where right now. There are plenty of examples in the 
kernel of SPI being done this way, as it seems to be accepted practice 
in the kernel.

If there is a specific issue that you're seeing, then let's talk about 
it, otherwise I'm going to NAK this change.

Alan.

>
>
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>   drivers/net/ieee802154/mrf24j40.c |  115 ++++++++++++++++++++++++++++---------
>   1 file changed, 89 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
> index 246befa..4a9a1ee 100644
> --- a/drivers/net/ieee802154/mrf24j40.c
> +++ b/drivers/net/ieee802154/mrf24j40.c
> @@ -609,10 +609,95 @@ out:
>   	return IRQ_HANDLED;
>   }
>   
> +static int mrf24j40_hw_init(struct mrf24j40 *devrec)
> +{
> +	int ret;
> +	u8 val;
> +
> +	/* Initialize the device.
> +		From datasheet section 3.2: Initialization. */
> +	ret = write_short_reg(devrec, REG_SOFTRST, 0x07);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_PACON2, 0x98);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_TXSTBL, 0x95);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON0, 0x03);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON1, 0x01);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON2, 0x80);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON6, 0x90);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON7, 0x80);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_RFCON8, 0x10);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_long_reg(devrec, REG_SLPCON1, 0x21);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_BBREG2, 0x80);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_CCAEDTH, 0x60);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_BBREG6, 0x40);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_RFCTL, 0x04);
> +	if (ret)
> +		goto err_ret;
> +
> +	ret = write_short_reg(devrec, REG_RFCTL, 0x0);
> +	if (ret)
> +		goto err_ret;
> +
> +	udelay(192);
> +
> +	/* Set RX Mode. RXMCR<1:0>: 0x0 normal, 0x1 promisc, 0x2 error */
> +	ret = read_short_reg(devrec, REG_RXMCR, &val);
> +	if (ret)
> +		goto err_ret;
> +
> +	val &= ~0x3; /* Clear RX mode (normal) */
> +
> +	ret = write_short_reg(devrec, REG_RXMCR, val);
> +	if (ret)
> +		goto err_ret;
> +
> +	return 0;
> +
> +err_ret:
> +	return ret;
> +}
> +
>   static int mrf24j40_probe(struct spi_device *spi)
>   {
>   	int ret = -ENOMEM;
> -	u8 val;
>   	struct mrf24j40 *devrec;
>   
>   	printk(KERN_INFO "mrf24j40: probe(). IRQ: %d\n", spi->irq);
> @@ -649,31 +734,9 @@ static int mrf24j40_probe(struct spi_device *spi)
>   	if (ret)
>   		goto err_register_device;
>   
> -	/* Initialize the device.
> -		From datasheet section 3.2: Initialization. */
> -	write_short_reg(devrec, REG_SOFTRST, 0x07);
> -	write_short_reg(devrec, REG_PACON2, 0x98);
> -	write_short_reg(devrec, REG_TXSTBL, 0x95);
> -	write_long_reg(devrec, REG_RFCON0, 0x03);
> -	write_long_reg(devrec, REG_RFCON1, 0x01);
> -	write_long_reg(devrec, REG_RFCON2, 0x80);
> -	write_long_reg(devrec, REG_RFCON6, 0x90);
> -	write_long_reg(devrec, REG_RFCON7, 0x80);
> -	write_long_reg(devrec, REG_RFCON8, 0x10);
> -	write_long_reg(devrec, REG_SLPCON1, 0x21);
> -	write_short_reg(devrec, REG_BBREG2, 0x80);
> -	write_short_reg(devrec, REG_CCAEDTH, 0x60);
> -	write_short_reg(devrec, REG_BBREG6, 0x40);
> -	write_short_reg(devrec, REG_RFCTL, 0x04);
> -	write_short_reg(devrec, REG_RFCTL, 0x0);
> -	udelay(192);
> -
> -	/* Set RX Mode. RXMCR<1:0>: 0x0 normal, 0x1 promisc, 0x2 error */
> -	ret = read_short_reg(devrec, REG_RXMCR, &val);
> +	ret = mrf24j40_hw_init(devrec);
>   	if (ret)
> -		goto err_read_reg;
> -	val &= ~0x3; /* Clear RX mode (normal) */
> -	write_short_reg(devrec, REG_RXMCR, val);
> +		goto err_hw_init;
>   
>   	ret = request_threaded_irq(spi->irq,
>   				   NULL,
> @@ -690,7 +753,7 @@ static int mrf24j40_probe(struct spi_device *spi)
>   	return 0;
>   
>   err_irq:
> -err_read_reg:
> +err_hw_init:
>   	ieee802154_unregister_device(devrec->dev);
>   err_register_device:
>   	ieee802154_free_device(devrec->dev);

Re: [PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[Alexander Aring]

Hi Alan,

On Wed, Apr 23, 2014 at 09:51:33AM -0400, Alan Ott wrote:
> On 04/23/2014 09:11 AM, Varka Bhadram wrote:
> >I followed the process that you mailed earlier, thnks for that.
> >
> >I am expecting the mail from Alan about the changes.
> 
> Hi Varka,
> 
> Is there a specific problem you're seeing? Typically in the kernel we expect
> the SPI controller to succeed for a couple reasons:
> 1. It's part of the basic, core functionality of a system. Checking for
> errors on SPI transfers is analogous to making sure RAM you wrote actually
> got written.
> 2. Most of the time an SPI failure is not something we can detect anyway.
> (disconnect one of the lines and see what you get).
> 3. The code to check for it just adds a lot of bloat without much measurable
> benefit.
> 
> I've read the above in the comments in other drivers, but I can't remember
> exactly where right now. There are plenty of examples in the kernel of SPI
> being done this way, as it seems to be accepted practice in the kernel.
> 
> If there is a specific issue that you're seeing, then let's talk about it,
> otherwise I'm going to NAK this change.
> 

if somebody hasn't a right spi configuration the probe function should
fail. Assumed that spi_sync will return a errno then.

- Alex

Re: [PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[Varka Bhadram]

Hi Alan,

My view is same as Alex view. If spi_sync() is not able to write and get 
the data we will get
an error number return.

The driver is allocating so many kernel resources even if you get the 
errno return also.
So this patch will return from driver probe() if spi_sync() returns err.

Thanks and Regards,
Varka Bhadram

On 04/23/2014 07:30 PM, Alexander Aring wrote:
> Hi Alan,
>
> On Wed, Apr 23, 2014 at 09:51:33AM -0400, Alan Ott wrote:
>> On 04/23/2014 09:11 AM, Varka Bhadram wrote:
>>> I followed the process that you mailed earlier, thnks for that.
>>>
>>> I am expecting the mail from Alan about the changes.
>> Hi Varka,
>>
>> Is there a specific problem you're seeing? Typically in the kernel we expect
>> the SPI controller to succeed for a couple reasons:
>> 1. It's part of the basic, core functionality of a system. Checking for
>> errors on SPI transfers is analogous to making sure RAM you wrote actually
>> got written.
>> 2. Most of the time an SPI failure is not something we can detect anyway.
>> (disconnect one of the lines and see what you get).
>> 3. The code to check for it just adds a lot of bloat without much measurable
>> benefit.
>>
>> I've read the above in the comments in other drivers, but I can't remember
>> exactly where right now. There are plenty of examples in the kernel of SPI
>> being done this way, as it seems to be accepted practice in the kernel.
>>
>> If there is a specific issue that you're seeing, then let's talk about it,
>> otherwise I'm going to NAK this change.
>>
> if somebody hasn't a right spi configuration the probe function should
> fail. Assumed that spi_sync will return a errno then.
>
> - Alex


-------------------------------------------------------------------------------------------------------------------------------

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
-------------------------------------------------------------------------------------------------------------------------------


------------------------------------------------------------------------------
Start Your Social Network Today - Download eXo Platform
Build your Enterprise Intranet with eXo Platform Software
Java Based Open Source Intranet - Social, Extensible, Cloud Ready
Get Started Now And Turn Your Intranet Into A Collaboration Platform
http://p.sf.net/sfu/ExoPlatform

Re: [Linux-zigbee-devel] [PATCH] mrf24j40: seperate mrf24j40 h/w init and add checkings

[Werner Almesberger]

Alan Ott wrote:
> 3. The code to check for it just adds a lot of bloat without much 
> measurable benefit.

As a very general note, if - in any C program, not just the kernel
- you have too many error checks for comfort, you may want to
consider keeping a cumulative error status (e.g., in this case,
struct mrf24j40), and just check that. Similar to ferror in stdio.

Something like


static int my_check_and_clear_rc(struct foo *foo)
{
	int rc;

	rc = foo->rc;
	foo->rc = 0;
	return rc;
}


static int my_operation(struct foo *foo, int arg)
{
	int rc;

	/* don't make it worse - optional */
	if (foo->rc)
		return foo->rc;

	rc = really_do_my_operation(foo, arg);
	if (rc < 0 && !foo->rc)
		foo->rc = rc;

	return foo->rc ? foo->rc : rc;
}


Then the phalanx of tedious checks shrinks to

	/* make sure foo->rc is initialized to 0 */

	my_operation(foo, 1);
	my_operation(foo, 2);
	...
	my_operation(foo, 1000);

	rc = my_check_and_clear_rc(foo);
	if (rc) {
		complain("something terribly wrong");
		...
	}
	...

You can easily extend this to also record line numbers, file
names, and such, if necessary. Add atomic/locking as needed.

- Werner