Re: [PATCH v6 2/2] iio: proximity: vl53l0x: add interrupt support

[Jonathan Cameron]

On 30 September 2018 00:49:43 BST, Rob Herring <robh@xxxxxxxxxx> wrote:
>On Sat, Sep 29, 2018 at 6:10 AM Jonathan Cameron <jic23@xxxxxxxxxx>
>wrote:
>>
>> On Fri, 28 Sep 2018 18:52:13 -0500
>> Rob Herring <robh@xxxxxxxxxx> wrote:
>>
>> > On Fri, Sep 28, 2018 at 4:36 AM Song Qiang
><songqiang1304521@xxxxxxxxx> wrote:
>> > >
>> > > On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
>> > > > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron
>wrote:
>> > > > > On Tue, 18 Sep 2018 16:24:22 +0800
>> > > > > Song Qiang <songqiang1304521@xxxxxxxxx> wrote:
>> > > > >
>> > > > > > The first version of this driver issues a measuring request
>and polling
>> > > > > > for a status register in the device for measuring
>completes.
>> > > > > > vl53l0x support configuring GPIO1 on it to generate
>interrupt to
>> > > > > > indicate that new measurement is ready. This patch adds
>support for
>> > > > > > using this mechanisim to reduce cpu cost.
>> > > > > >
>> > > > > > Signed-off-by: Song Qiang <songqiang1304521@xxxxxxxxx>
>> > > > > Hi Song.
>> > > > >
>> > > > > Looks correct in principal but a few things to tidy up before
>> > > > > this is ready to apply.
>> > > > >
>> > > > > Also we have an unrelated change in here to check the devices
>ID.
>> > > > > That should be in it's own patch.
>> > > > >
>> > > > > Thanks,
>> > > > >
>> > > > > Jonathan
>> > > > > > ---
>> > > > > >  .../bindings/iio/proximity/vl53l0x.txt        |  14 +-
>> > > >
>> > > > This should have been split with the complete binding in one
>patch
>> > > > rather than piecemeal driver feature by feature.
>> > > >
>> > >
>> > > Hi Rob,
>>
>> Hi Rob, Song,
>>
>> > >
>> > > A few days ago when I was submitting this driver, I didn't do it
>very
>> > > well, the function of this driver is limited. I added interrupt
>support
>> > > the next day after you acked my first patch. I thought it's not
>polite
>> > > to add something after someone acked that patch, so I send the
>interrupt
>> > > support as a second patch. The first patch is merged to togreg
>now, but
>> > > this doesn't. I don't know when can I add new functions to the
>code that
>> > > just merged to togreg branch, could you offer some suggestions?
>> >
>> > You just needed to state why you didn't add a ack. But really, just
>> > don't send things except as RFC until they are "done".
>>
>> The RFC bit actually disagree on.  This driver could be considered
>'done'
>> with just patch 1.  The driver worked, it was useful. When the early
>> versions of that patch came out Song had no idea how much work it
>would
>> be to add interrupt support.  As it turns out it was simple - it
>often isn't :(
>
>I meant specifically in the context of this getting revised within a
>number of days. I agree that drivers don't have to be complete, but
>bindings should be as complete as possible. You can't foresee
>everything, but I don't think that applies in this case.
>
>> > What to do next depends on Jonathan and whether he wants a
>follow-up
>> > patch or he will drop the first one.
>>
>> Yeah. I should have picked up on the binding in the second patch and
>merged
>> it.  I'd seen the first patch a few times before so was happy with it
>and
>> applied before actually looking at the second.
>>
>> If they had come in separate series in my view the partial binding
>then
>> extend with 'optional' elements is fine.  The number of times I've
>discovered
>> issues with documentation of hardware that would have made any
>binding written
>> from the docs wrong is non trivial. So in my view it is always a
>gamble to
>> write bindings until you have tested they work.  I have not problem
>with
>> people who are confident and want to add them from the start though.
>
>Well, if they were broken is some way, I don't think backwards
>compatibility matters and we can still fix things. I'm not talking
>about complex cases here. It is pretty trivial to determine whether a
>device has an interrupt or not.
>
>>
>> Obviously this only works for optional elements.
>>
>> So follow up patch for 'optional' stuff is fine by me.  The only real
>> issue to my mind here is that they were in the same series, so we
>should
>> have seen a separate precursor patch giving the binding for all of
>it.
>
>Certainly, that can't be avoided sometimes and is fine. It's really
>the time frame for this particular case and reviewer bandwidth.

Agreed. The timing wasn't ideal (in this limited sense)  Song got this done much faster than I normally expect
when someone says, I'll do this later! 

Thanks for clarifying.

Jonathan
>
>Rob

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.