[Cocci] [PATCH v2 2/2] hwmon: npcm750: add NPCM7xx PWM and Fan driver

[Guenter Roeck]

(adding Julia Lawall and cocci mailing list)
[]

+static inline void npcm7xx_fan_start_capture(struct npcm7xx_pwm_fan_data *data,
+ u8 fan, u8 cmp)
+{
+ u8 fan_id = 0;
+ u8 reg_mode = 0;
+ u8 reg_int = 0;
+ unsigned long flags;
+
+ fan_id = NPCM7XX_FAN_INPUT(fan, cmp);
+
+ /* to check whether any fan tach is enable */
+ if (data->npcm7xx_fan[fan_id].FanStFlag != FAN_DISABLE) {
+ /* reset status */
+ spin_lock_irqsave(&data->npcm7xx_fan_lock[fan], flags);
+
+ data->npcm7xx_fan[fan_id].FanStFlag = FAN_INIT;
+ reg_int = ioread8(NPCM7XX_FAN_REG_TIEN(data->fan_base, fan));
+
+ if (cmp == NPCM7XX_FAN_CMPA) {
+ /* enable interrupt */
+ iowrite8((u8) (reg_int | (NPCM7XX_FAN_TIEN_TAIEN |
+ NPCM7XX_FAN_TIEN_TEIEN)),

Is the (u8) typecast really necessary ? Seems unlikely.

The cast is not really necessary here as there would
be an implicit cast already.
Some might complain about loss of type safety and
"make W=123" would probably emit something here.

I spent (wasted) some time browsing through the kernel.
Similar typecasts are only used if there is a real type change.
A warning here would not make sense unless NPCM7XX_FAN_TIEN_TAIEN
or NPCM7XX_FAN_TIEN_TEIEN would be outside the u8 range, and then
there would be one anyway.

So, no, I am not going to accept those typecasts. They just make the code
more difficult to read. For example, the code here could have been
simplified to something like

reg_int = ioread8(NPCM7XX_FAN_REG_TIEN(data->fan_base, fan));
reg_mode = ioread8(NPCM7XX_FAN_REG_TCKC(data->fan_base, fan));
if (cmp == NPCM7XX_FAN_CMPA) {
reg_int |= NPCM7XX_FAN_TIEN_TAIEN | NPCM7XX_FAN_TIEN_TEIEN;
reg_mode |= NPCM7XX_FAN_TCKC_CLK1_APB;
} else {
reg_int |= NPCM7XX_FAN_TIEN_TBIEN | NPCM7XX_FAN_TIEN_TFIEN;
reg_mode |= NPCM7XX_FAN_TCKC_CLK2_APB;
}
iowrite8(reg_int, NPCM7XX_FAN_REG_TIEN(data->fan_base, fan);
iowrite8(reg_mode, NPCM7XX_FAN_REG_TCKC(data->fan_base, fan);

This, in turn, leads to the question if it is really not necessary
to _clear_ those mask bits in the same context.

Guenter

But casts to the same type are not necessary.
A possible coccinelle script to find casts to the
same type is below, but there are some false positives
for things like __force and __user casts
Also, spatch (1.0.4) seems to have a defect for this
when the type is used in operations that change a
smaller type to int or unsigned int.
i.e.: (offset is u16, but offset * 2 is int)
HANDLING: drivers/net/ethernet/intel/igb/e1000_nvm.c
diff =
diff -u -p a/drivers/net/drivers/net/ethernet/intel/igb/e1000_nvm.c b/drivers/net/ethernet/intel/igb/e1000_nvm.c
--- a/drivers/net/ethernet/intel/igb/e1000_nvm.c
+++ b/drivers/net/ethernet/intel/igb/e1000_nvm.c
@@ -335,7 +335,7 @@ s32 igb_read_nvm_spi(struct e1000_hw *hw
/* Send the READ command (opcode + addr) */
igb_shift_out_eec_bits(hw, read_opcode, nvm->opcode_bits);
- igb_shift_out_eec_bits(hw, (u16)(offset*2), nvm->address_bits);
+ igb_shift_out_eec_bits(hw, (offset * 2), nvm->address_bits);
/* Read the data. SPI NVMs increment the address with each byte
* read and will roll over if reading beyond the end. This allows
---
$ cat same_typecast.cocci
@@
type T;
T foo;
@@
- (T *)&foo
+ &foo
@@
type T;
T foo;
@@
- (T)foo
+ foo

[Julia Lawall]

(adding Julia Lawall and cocci mailing list)
[]

+static inline void npcm7xx_fan_start_capture(struct npcm7xx_pwm_fan_data *data,
+ u8 fan, u8 cmp)
+{
+ u8 fan_id = 0;
+ u8 reg_mode = 0;
+ u8 reg_int = 0;
+ unsigned long flags;
+
+ fan_id = NPCM7XX_FAN_INPUT(fan, cmp);
+
+ /* to check whether any fan tach is enable */
+ if (data->npcm7xx_fan[fan_id].FanStFlag != FAN_DISABLE) {
+ /* reset status */
+ spin_lock_irqsave(&data->npcm7xx_fan_lock[fan], flags);
+
+ data->npcm7xx_fan[fan_id].FanStFlag = FAN_INIT;
+ reg_int = ioread8(NPCM7XX_FAN_REG_TIEN(data->fan_base, fan));
+
+ if (cmp == NPCM7XX_FAN_CMPA) {
+ /* enable interrupt */
+ iowrite8((u8) (reg_int | (NPCM7XX_FAN_TIEN_TAIEN |
+ NPCM7XX_FAN_TIEN_TEIEN)),

Is the (u8) typecast really necessary ? Seems unlikely.

The cast is not really necessary here as there would
be an implicit cast already.
Some might complain about loss of type safety and
"make W=123" would probably emit something here.
But casts to the same type are not necessary.
A possible coccinelle script to find casts to the
same type is below, but there are some false positives
for things like __force and __user casts
Also, spatch (1.0.4) seems to have a defect for this
when the type is used in operations that change a
smaller type to int or unsigned int.
i.e.: (offset is u16, but offset * 2 is int)

Ah. The rule is that the result type is always the larger one?
Unfortunately, Coccinelle doesn't know the size of any type. I could add
some special cases, but that may be more confusing than helpful.

julia

HANDLING: drivers/net/ethernet/intel/igb/e1000_nvm.c
diff =
diff -u -p a/drivers/net/drivers/net/ethernet/intel/igb/e1000_nvm.c b/drivers/net/ethernet/intel/igb/e1000_nvm.c
--- a/drivers/net/ethernet/intel/igb/e1000_nvm.c
+++ b/drivers/net/ethernet/intel/igb/e1000_nvm.c
@@ -335,7 +335,7 @@ s32 igb_read_nvm_spi(struct e1000_hw *hw
/* Send the READ command (opcode + addr) */
igb_shift_out_eec_bits(hw, read_opcode, nvm->opcode_bits);
- igb_shift_out_eec_bits(hw, (u16)(offset*2), nvm->address_bits);
+ igb_shift_out_eec_bits(hw, (offset * 2), nvm->address_bits);
/* Read the data. SPI NVMs increment the address with each byte
* read and will roll over if reading beyond the end. This allows
---
$ cat same_typecast.cocci
@@
type T;
T foo;
@@
- (T *)&foo
+ &foo
@@
type T;
T foo;
@@
- (T)foo
+ foo

[Joe Perches]

Post by Julia Lawall

Also, spatch (1.0.4) seems to have a defect for this
when the type is used in operations that change a
smaller type to int or unsigned int.
i.e.: (offset is u16, but offset * 2 is int)

Ah. The rule is that the result type is always the larger one?

Yes, but not quite, no.

The c90 rules are called "integer promotions" and are
detailed in section 6.3 Conversions

Basically, if any type is smaller than int, all operations
are done as int if possible, or unsigned int if necessary.
If any type is larger than int, then the larger type is used.

If you don't have the c90 standard, this one is close enough.
http://c0x.coding-guidelines.com/6.3.html
(use the next button several times to read the whole section)

Also, section 6.5 details "expressions" where the operands
of things like bit operations use integer promotions.

Post by Julia Lawall
Unfortunately, Coccinelle doesn't know the size of any type. I could add
some special cases, but that may be more confusing than helpful.

Maybe, but when I saw the suggested removal, I was surprised.