[U-Boot] [PATCH v2 2/2] xilinx_xiic: Fix transfer initialisation

[Melin Tomas]

On 6/26/19 4:36 PM, Marek Vasut wrote:
> On 6/26/19 3:19 PM, Melin Tomas wrote:
>> On 6/26/19 3:48 PM, Marek Vasut wrote:
>>> On 6/26/19 2:45 PM, Melin Tomas wrote:
>>>> On 6/26/19 3:26 PM, Marek Vasut wrote:
>>>>> On 6/26/19 2:19 PM, Melin Tomas wrote:
>>>>>> On 6/26/19 2:49 PM, Marek Vasut wrote:
>>>>>>> On 6/26/19 1:25 PM, Melin Tomas wrote:
>>>>>>>> On 6/26/19 1:47 PM, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>> On 6/26/19 12:39 PM, Melin Tomas wrote:
>>>>>>>>>
>>>>>>>>>> As such, it's probably a good idea to keep the same delay values here as
>>>>>>>>>> in the original driver unless good reason to use something else.
>>>>>>>>>>
>>>>>>>>>> As what goes for the original reasoning for 3ms, the commit history does
>>>>>>>>>> not mention that so I cannot comment.
>>>>>>>>> So would you be so kind and research this ?
>>>>>>>> Based on a short study of other i2c bus drivers it seems most have bus
>>>>>>>> busy timeout checks.
>>>>>>>>
>>>>>>>> The timeout values seems to differ, ranging from milliseconds to seconds.
>>>>>>> Yep
>>>>>>>
>>>>>>>> So probably it's just a number, after all it's just a check to know if
>>>>>>>> we are good to go.
>>>>>>> And is the number large enough ?
>>>>>> As mentioned, good approach is probably using value known to work
>>>>>> instead of
>>>>>>
>>>>>> guessing a new number.
>>>>> So why did kernel pick that specific number ? Surely there was some
>>>>> reasoning, they didn't just pull it out of /dev/random .
>>>> Yes, history does not tell.
>>>>
>>>> I do see that this driver uses timeout of 1000ms for bus busy when
>>>> probing, perhaps you can enlighten how that number was concluded? If
>>>> that could give some clues about this.
>>> I don't know.
>> But you are author of that line?
> +	ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET,
> +			     XIIC_SR_BUS_BUSY_MASK, false, 3, true);
> +
>
> comes from 2/2 ?

No, I was referring to the already existing wait_for_bit_8 of bus busy

https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/i2c/xilinx_xiic.c#L294

>>> You're the patch author, it's your responsibility to know why you're
>>> adding/changing the code you're adding/changing.
>> yes, and the reasoning is:
>>
>> * the value has been deemed good in original driver. If it would be bad,
>> probably it would have been changed during course of time
>>
>> * the value has been tested for this driver as well with success
> So shouldn't there be some upper bound on the bus busy time , demanded
> either by the i2c bus spec or the xiic core spec ?

In case some of the devices on the bus misbehaves, bus could potentially 
stay

busy until device reset or power-cycle.


thanks,

Tomas