Hi Jonathan,
On Tue, 24 Jul 2018 22:01:17 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Mon, 23 Jul 2018 19:58:16 +0200
> Tomas Novotny <tomas@xxxxxxxxxx> wrote:
>
> > Hi Jonathan,
> >
> > On Sat, 21 Jul 2018 18:25:26 +0100
> > Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > > On Tue, 17 Jul 2018 18:46:51 +0200
> > > Tomas Novotny <tomas@xxxxxxxxxx> wrote:
> > >
> > > > This is the second iteration of vcnl4200 support. The first one was posted
> > > > on February, sorry for the long delay. I've implemented all notes from
> > > > reviews (thanks to Peter and Jonathan) and nothing more.
> > > >
> > > > Changes in v2:
> > > > - reading light and proximity values for vcnl4200 is blocking (if you
> > > > request new value too early)
> > > > - patches 2 and 3 were added; original patch 2 is now patch 4
> > > > - vcnl4010 id is handled (patch 2)
> > > > - warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
> > > > - minor stuff (add parenthesis, switch instead of if, rename sensors to
> > > > channels, fix return)
> > > >
> > > > Please note that I'm not sure if it is still good idea to have same driver
> > > > for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
> > > > bigger. Just see below for differences and add the blocking read which is
> > > > added in v2 for vcnl4200.
> > >
> > > Definitely marginal. My view is it's your choice as the person doing
> > > the work! Which would you prefer?
> >
> > Well, I was thinking about different driver in the first place. But the reason
> > was quite selfish (and not the technical one), as I'm interested only in
> > vcnl4200. But I've seen general attitude to combine similar drivers so I've
> > tried that way and I'm fine with the result. The only ugly part are some chip
> > specific things in a shared data structure.
> >
> > > I'm pretty happy with these patches if you want to go this way, but
> > > given I assume you might add the support for other features, if you
> > > think that is going to get even worse, then now is the time to make
> > > the decision to not have a unified driver.
> >
> > I will probably add some settings, but this should be handled easily. I don't
> > see anything problematic in the future (well, maybe interrupts or
> > thresholds?), as both variants are more or less simple light and proximity
> > chips.
> >
> > > Sadly it might be a case of doing a 'hatchet' job on the code to
> > > make a separate driver and see what it looks like. If it's really
> > > short and clean (which it probably is) then go with separate drivers.
> >
> > The separate vcnl4200 would be indeed short and clean. The structure and
> > size would be very similar to current vcnl4000.
> >
> > Anyway, I don't have enough experience to judge what is better. I'm ok to do
> > any variant.
>
> Let's go with what you have here on the basis it's marginal but you have
> done the work for this option already which makes this the better option!
ok, thanks for all your comments. I will prepare v3 then.
Tomas
> Jonathan
>
> >
> > Thanks for the review,
> >
> > Tomas
> >
> > > Jonathan
> > >
> > > >
> > > > Cover letter from v1:
> > > >
> > > > VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
> > > > adding support for that sensor to vcnl4000 driver, which currently supports
> > > > VCNL4000/10/20.
> > > >
> > > > The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
> > > > - integrated proximity and ambient light sensor
> > > > - SMBus compatible I2C interface
> > > > - Vishay VCNL4xxx series...
> > > >
> > > > Different things are:
> > > > - totally different register map
> > > > - 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
> > > > on VCNL4000. 16-bit value is in one register on VCNL4200.
> > > > - VCNL4000 has flags when the measurement is finished
> > > >
> > > > The first patch generalizes the driver to support differencies. The second
> > > > patch adds the support for VCNL4200.
> > > >
> > > > It is tested on VCNL4020 and VCNL4200.
> > > >
> > > > Tomas Novotny (4):
> > > > iio: vcnl4000: make the driver extendable
> > > > iio: vcnl4000: add VCNL4010 device id
> > > > iio: vcnl4000: warn on incorrectly specified device id
> > > > iio: vcnl4000: add support for VCNL4200
> > > >
> > > > drivers/iio/light/Kconfig | 5 +-
> > > > drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
> > > > 2 files changed, 196 insertions(+), 28 deletions(-)
> > > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
