Re: [patch 2.6.12-rc3] dell_rbu: Resubmitting patch for new Dell BIOS update driver

[Marcel Holtmann <marcel@holtmann.org>]

Hi Abhay,

> Resubmitting after cleaning up spaces/tabs etc...

and now starting with the coding style nitpicking ;)

> +	/* check if we have any packets */
> +	if (0 == rbu_data.num_packets) {

Make it "if (!rbu_data.num_packets) {".

> +	while(--packet_count) {
> +		ptemp_list = ptemp_list->next;
> +	}

Don't forget the space between "while" and "(" and the "{"/"}" are not
needed.

> +	ppacket = list_entry(ptemp_list,struct packet_data, list);

We always put a space after ",".

> +	if ((rbu_data.packet_write_count + length) > rbu_data.packetsize) {

Make it "(rbu_data.packet_write_count + length > rbu_data.packetsize)".

> +	/* copy the incoming data in to the new buffer */
> +	memcpy((ppacket->data + rbu_data.packet_write_count),
> +			data, length);

Make it "memcpy(ppacket->data + rbu_data.packet_write_count, ".

> +	if ((rbu_data.packet_write_count + length) == rbu_data.packetsize) {
> +		/*
> +		 this was the last data chunk in the packet
> +		 so reinitialize the packet data counter to zero
> +		*/
> +		rbu_data.packet_write_count = 0;
> +	} else
> +		rbu_data.packet_write_count += length;

Why not:

	rbu_data.packet_write_count += length;
	if (rbu_data.packet_write_count == rbu_data.packetsize)
		rbu_data.packet_write_count = 0;

> +	/* try allocating a new buffer to fit the request */
> +	pbuf =(unsigned char *)__get_free_pages(GFP_KERNEL, *ordernum);

Forgot a space after "=" and after "...char*)".

> +	if (pbuf != NULL) {

Make it "if (!pbuf)",

> +		/* check if the image is with in limits */
> +		img_buf_phys_addr = (unsigned long)virt_to_phys(pbuf);

Put a space after "...long)".

> +		if ((limit != 0) && ((img_buf_phys_addr + size) > limit)) {

Make it "if (!limit && img_bug_phys_addr + size > limit)"

> +			free_pages ((unsigned long)pbuf, *ordernum);

Space.

> +			 Try allocating a new buffer from the 
> +			 GFP_DMA range as it is with in 16MB range.
> +			*/
> +			pbuf =(unsigned char *)__get_free_pages(GFP_DMA,

Space.

> +			if (pbuf == NULL)

Use "(!pbuf)".

> +	if (rbu_data.packetsize == 0 ) {

Use "if (!rbu_data.packetsize)".

> +	if(newpacket == NULL) {

Use "if (!newpacket)".

> +	if(newpacket->data == NULL) {

Use "if (!newpacket->data)"

> +	if (rbu_data.packet_write_count == 0) {
> +		if ((rc = create_packet(length)) != 0 )
> +			return rc;
> +	}

Use this:

	if (!rbu_data.packet_write_count)
		if (!(rc = create_packet(length)))
			return rc;

> +	if ((rc = fill_last_packet(data, length)) != 0)

Use "if (!(rc = fill_last_packet(data, length)))".

> +		free_pages((unsigned long)newpacket->data, newpacket->ordernum);

Space.

> +	if (rbu_data.image_update_buffer == NULL)

Use "(!rbu_data.image_update_buffer)".

> +	free_pages((unsigned long)rbu_data.image_update_buffer,

Space.

> +		if ((size != 0) && (rbu_data.image_update_buffer == NULL)) {

Use "if (!size && !rbu_data.image_update_buffer)"

> +	image_update_buffer = (unsigned char *)get_free_pages_limited(size,

Space.

> +	if (image_update_buffer != NULL) {

Use "if (!image_update_buffer)".

> +		memset(rbu_data.image_update_buffer,0,

Space.

> +	sscanf(buf, "%d",&size);	

Spaces.

> +	if (size != 0)

Use "if (!size)".

> +	if (type == MONOLITHIC )

Space.

> +	if ( type == MONOLITHIC )

Spaces.

> +		size = sprintf(buf, "%lu\n",  rbu_data.bios_image_size);

Extra space not needed.

> +	if (type == MONOLITHIC ) 

Space.

> +	if (strlen(buf) >= 256 ) {

Space.

> +	if (type == MONOLITHIC ) {

Space.

> +	if (type == MONOLITHIC ) {

Space.

> +		if (fw_entry->size == 0 )

use "if (!fw_entry->size)".

> +				if (rc == 0) {

Use "if (!rc)".

> +		if ( rbu_data.packetsize != fw_entry->size )

Spaces.

> +		if ( rc == 0 )

Spaces.

> +static decl_subsys(dell_rbu,&ktype_dell_rbu,NULL);

Spaces.

> +        memset(rbu_dev, 0, sizeof (*rbu_dev));

No tab.

> +	if (type == MONOLITHIC)

Space.

> +		if (type == MONOLITHIC ) {

Space.

> +	if (rbu_dev != NULL) {

Use "if (!rbu_dev)".

> +static int __init dcdrbu_init(void)

What stand "dcd" for? Why not only "rbu_init"?

> +	if (rbu_download_mono == NULL) {	

Use "if (!rbu_download)".

> +	rbu_download_packet=  create_rbu_download_entry (PACKETIZED);

Wrong spaces.

> +	if (rbu_download_packet == NULL) {	

Use "if (!rbu_download_packet)".

> +	strncpy(rbu_device.bus_id,"firmware", BUS_ID_SIZE);

Space.

> +static __exit void dcdrbu_exit( void)

Why "dcd"?

> +config DELL_RBU
> +        tristate "BIOS update support for DELL systems via sysfs"
> +        default n
> +	select FW_LOADER

Space vs tab clash.

Regards

Marcel