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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
