Messages in this thread | | | Date | Sat, 22 Feb 2020 13:34:57 +0100 | From | Wolfram Sang <> | Subject | Re: [PATCH 1/5] i2c: i2c-stm32f7: disable/restore Fast Mode Plus bits in low power modes |
| |
Hi Alain,
thanks for the patch. A few comments:
> @@ -303,6 +303,8 @@ struct stm32f7_i2c_msg { > * @dma: dma data > * @use_dma: boolean to know if dma is used in the current transfer > * @regmap: holds SYSCFG phandle for Fast Mode Plus bits > + * @regmap_reg: register address for setting Fast Mode Plus bits > + * @regmap_mask: mask for Fast Mode Plus bits in set register > * @wakeup_src: boolean to know if the device is a wakeup source > */ > struct stm32f7_i2c_dev { > @@ -326,6 +328,8 @@ struct stm32f7_i2c_dev { > struct stm32_i2c_dma *dma; > bool use_dma; > struct regmap *regmap; > + u32 regmap_reg; > + u32 regmap_mask;
Is this really a descriptive naming? From looking at the code, 'syscfg_reg' or 'fmp_reg' sound more suitable to me?
> +{ > + if (i2c_dev->speed != STM32_I2C_SPEED_FAST_PLUS || > + IS_ERR_OR_NULL(i2c_dev->regmap)) { > + /* Optional */ > + return 0; > + }
No brackets needed here.
> - ret = of_property_read_u32_index(np, "st,syscfg-fmp", 2, &mask); > + ret = of_property_read_u32_index(np, "st,syscfg-fmp", 2, > + &i2c_dev->regmap_mask); > if (ret) > return ret; > > - return regmap_update_bits(i2c_dev->regmap, reg, mask, mask); > + return 0;
Can be shortened now to return of_property_read_u32_index(...);
> + ret = stm32f7_i2c_write_fm_plus_bits(i2c_dev, 1);
The type of the last parameter is bool, so using 'true/false' instead of '1/0' is a tad more readable, I think.
Regards,
Wolfram
[unhandled content-type:application/pgp-signature] | |