diff mbox

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

Message ID a6a5a21236c37cfc63329e54248d941bf6d49e3c.camel@perches.com (mailing list archive)
State Superseded
Headers show

Commit Message

Joe Perches June 20, 2018, 6:25 p.m. UTC 
(adding Julia Lawall and cocci mailing list)

On Wed, 2018-06-20 at 09:48 -0700, Guenter Roeck wrote:
[]
> > +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)

While running the cocci script below:

HANDLING: drivers/net/ethernet/intel/igb/e1000_nvm.c

---

Anyway, here's the cocci script:

$ cat same_typecast.cocci
@@
type T;
T foo;
@@

-	(T *)&foo
+	&foo

@@
type T;
T foo;
@@

-	(T)foo
+	foo

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Guenter Roeck June 20, 2018, 7:38 p.m. UTC | #1 
On Wed, Jun 20, 2018 at 11:25:08AM -0700, Joe Perches wrote:
> (adding Julia Lawall and cocci mailing list)
> 
> On Wed, 2018-06-20 at 09:48 -0700, Guenter Roeck wrote:
> []
> > > +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)
> 
> While running the cocci script below:
> 
> 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
> 
> ---
> 
> Anyway, here's the cocci script:
> 
> $ cat same_typecast.cocci
> @@
> type T;
> T foo;
> @@
> 
> -	(T *)&foo
> +	&foo
> 
> @@
> type T;
> T foo;
> @@
> 
> -	(T)foo
> +	foo
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall June 21, 2018, 1:17 p.m. UTC | #2 
On Wed, 20 Jun 2018, Joe Perches wrote:

> (adding Julia Lawall and cocci mailing list)
>
> On Wed, 2018-06-20 at 09:48 -0700, Guenter Roeck wrote:
> []
> > > +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

>
> While running the cocci script below:
>
> 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
>
> ---
>
> Anyway, here's the cocci script:
>
> $ cat same_typecast.cocci
> @@
> type T;
> T foo;
> @@
>
> -	(T *)&foo
> +	&foo
>
> @@
> type T;
> T foo;
> @@
>
> -	(T)foo
> +	foo
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches June 21, 2018, 2:48 p.m. UTC | #3 
On Thu, 2018-06-21 at 15:17 +0200, Julia Lawall wrote:
> On Wed, 20 Jun 2018, Joe Perches wrote:
> > 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.

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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