Re: [PATCH V2] w1: ds2408: add magic sequence to disable P0 test mode

[Andrew Morton <akpm@linux-foundation.org>]

On Tue,  7 May 2013 10:00:48 -0400 Jean-Francois Dagenais <jeff.dagenais@gmail.com> wrote:

> V2: use the new bus_mutex
> 
> extract from http://datasheets.maximintegrated.com/en/ds/DS2408.pdf:
> 
> Power-up timing
> The DS2408 is sensitive to the power-on slew rate and can inadvertently
> power up with a test mode feature enabled. When this occurs, the P0 port
> does not respond to the Channel Access Write command.
> For most reliable operation, it is recommended to disable the test mode
> after every power-on reset using the Disable Test Mode sequence shown
> below. The 64-bit ROM code must be transmitted in the same bit sequence
> as with the Match ROM command, i.e., least significant bit first. This
> precaution is recommended in parasite power mode (VCC pin connected to
> GND) as well as with VCC power.
> 
> Disable Test Mode:
> RST,PD,96h,<64-bit DS2408 ROM Code>,3Ch,RST,PD
> 
> ...
>
> --- a/drivers/w1/slaves/w1_ds2408.c
> +++ b/drivers/w1/slaves/w1_ds2408.c
> @@ -301,7 +301,32 @@ error:
>  	return -EIO;
>  }
>  
> +/**

/** is used to introduce kerneldoc comments, but this isn't a kerneldoc
comment.  Just use /* here.

> + * This is a special sequence we must do to ensure the P0 output is not stuck
> + * in test mode. This is described in rev 2 of the ds2408's datasheet
> + * (http://datasheets.maximintegrated.com/en/ds/DS2408.pdf) under
> + * "APPLICATION INFORMATION/Power-up timing".
> + */
> +static int w1_f29_disable_test_mode(struct w1_slave *sl)
> +{
> +	int res;
> +	u8 magic[10] = {0x96, };
> +	u64 rn = le64_to_cpu(*((u64*)&sl->reg_num));
> +	memcpy(&magic[1], &rn, 8);
> +	magic[9] = 0x3C;

(please put a blank line between end-of-locals and start-of-code)

The casting looks decidedly dodgy.  I guess it won't cause an unalignned
exception due to reg_num's alignment, but it appears to defeat the
endianness handling in the definotion of `struct w1_reg_num'.

Are you sure this will work OK with all architectures and both
__LITTLE_ENDIAN_BITFIELD and __BIG_ENDIAN_BITFIELD?

> +	mutex_lock(&sl->master->bus_mutex);
>  
> +	res = w1_reset_bus(sl->master);
> +	if (res)
> +		goto out;
> +	w1_write_block(sl->master, magic, ARRAY_SIZE(magic));
> +
> +	res = w1_reset_bus(sl->master);
> +out:
> +	mutex_unlock(&sl->master->bus_mutex);
> +	return res;
> +}
>  
>  static struct bin_attribute w1_f29_sysfs_bin_files[] = {
>  	{
> @@ -362,6 +387,10 @@ static int w1_f29_add_slave(struct w1_slave *sl)
>  	int err = 0;
>  	int i;
>  
> +	err = w1_f29_disable_test_mode(sl);
> +	if (err)
> +		return err;
> +
>  	for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i)
>  		err = sysfs_create_bin_file(
>  			&sl->dev.kobj,