Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 16:42 GMT+01:00 Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>:
> Hello Cedric,
>
> On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
>> >
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> >
>> > You could get rid of this, when caching the value of CR1. Would save two
>> > register reads here. This doesn't work for all registers, but it should
>> > be possible to apply for most of them, maybe enough to get rid of the
>> > clr_bits and set_bits function.
>>
>> I agree at many places I could save registers read by not using
>> clr_bits and set_bits function when the registers in question has been
>> already read.
>> But it is not enough to get rid of the clr_bits and set_bits function.
>> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
>> register is never read before so set_bits function is useful.
>
> I didn't double check the manual, but I would expect that CR1 isn't
> modified by hardware. So you can cache the result in the driver data
> structure and do the necessary modifications with that one.

CR1 is modified by hardware to clear some bits set by software during
the communication.

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-KÃnig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Best regards,

Cedric