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

Marcel Holtmann
2005-06-02T23:59:55Z

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