Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542172 > unrolled thread

Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-12-14 21:20 +0100
    Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Pali Rohár <pali.rohar@gmail.com> - 2016-12-14 23:10 +0100
      Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-12-14 23:40 +0100
    Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus <sakari.ailus@iki.fi> - 2016-12-18 23:10 +0100
      Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-12-20 13:40 +0100
        Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus <sakari.ailus@iki.fi> - 2016-12-20 15:10 +0100
          Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-12-20 23:50 +0100

#1542172 — Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor

[Multipart message — attachments visible in raw view] — view raw

Hi!

> On Wednesday 14 December 2016 13:24:51 Pavel Machek wrote:
> >  
> > Add driver for et8ek8 sensor, found in Nokia N900 main camera. Can be
> > used for taking photos in 2.5MP resolution with fcam-dev.
> > 
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> > From v4 I did cleanups to coding style and removed various oddities.
> > 
> > Exposure value is now in native units, which simplifies the code.
> > 
> > The patch to add device tree bindings was already acked by device tree
> > people.

> > +	default:
> > +		WARN_ONCE(1, ET8EK8_NAME ": %s: invalid message length.\n",
> > +			  __func__);
> 
> dev_warn_once()
...
> > +	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> > +		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> 
> Maybe replace it with dev_warn_once() too? That condition in WARN_ONCE
> does not look nice...
...
> > +			if (WARN(next->type != ET8EK8_REG_8BIT &&
> > +				 next->type != ET8EK8_REG_16BIT,
> > +				 "Invalid type = %d", next->type)) {
> dev_warn()
>
> > +	WARN_ON(sensor->power_count < 0);
> 
> Rather some dev_warn()? Do we need stack trace here?

I don't see what is wrong with WARN(). These are not expected to
trigger, if they do we'll fix it. If you feel strongly about this,
feel free to suggest a patch.

> > +static int et8ek8_i2c_reglist_find_write(struct i2c_client *client,
> > +					 struct et8ek8_meta_reglist *meta,
> > +					 u16 type)
> > +{
> > +	struct et8ek8_reglist *reglist;
> > +
> > +	reglist = et8ek8_reglist_find_type(meta, type);
> > +	if (!reglist)
> > +		return -EINVAL;
> > +
> > +	return et8ek8_i2c_write_regs(client, reglist->regs);
> > +}
> > +
> > +static struct et8ek8_reglist **et8ek8_reglist_first(
> > +		struct et8ek8_meta_reglist *meta)
> > +{
> > +	return &meta->reglist[0].ptr;
> > +}
> 
> Above code looks like re-implementation of linked-list. Does not kernel
> already provide some?

Its actually array of pointers as far as I can tell. I don't think any
helpers would be useful here.

> > +	new = et8ek8_gain_table[gain];
> > +
> > +	/* FIXME: optimise I2C writes! */
> 
> Is this FIMXE still valid?

Probably. Lets optimize it after merge.

> > +	if (sensor->power_count) {
> > +		WARN_ON(1);
> 
> Such warning is probably not useful...

It should not happen, AFAICT. That's why I'd like to know if it does.

> > +#include "et8ek8_reg.h"
> > +
> > +/*
> > + * Stingray sensor mode settings for Scooby
> > + */
> 
> Are settings for this sensor Stingray enough?

Seems to work well enough for me. If more modes are needed, we can add
them later.

> It was me who copied these sensors settings to kernel driver. And I
> chose only Stingray as this is what was needed for my N900 for
> testing... Btw, you could add somewhere my and Ivo's Signed-off and
> copyright state as we both modified et8ek8.c code...

Normally, people add copyrights when they modify the code. If you want
to do it now, please send me a patch. (With those warn_ons too, if you
care, but I think the code is fine as is).

I got code from Dmitry, so it has his signed-off.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1542234

On Wednesday 14 December 2016 21:12:02 Pavel Machek wrote:
> Hi!
> 
> > On Wednesday 14 December 2016 13:24:51 Pavel Machek wrote:
> > >  
> > > Add driver for et8ek8 sensor, found in Nokia N900 main camera. Can be
> > > used for taking photos in 2.5MP resolution with fcam-dev.
> > > 
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > > From v4 I did cleanups to coding style and removed various oddities.
> > > 
> > > Exposure value is now in native units, which simplifies the code.
> > > 
> > > The patch to add device tree bindings was already acked by device tree
> > > people.
> 
> > > +	default:
> > > +		WARN_ONCE(1, ET8EK8_NAME ": %s: invalid message length.\n",
> > > +			  __func__);
> > 
> > dev_warn_once()
> ...
> > > +	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> > > +		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> > 
> > Maybe replace it with dev_warn_once() too? That condition in WARN_ONCE
> > does not look nice...
> ...
> > > +			if (WARN(next->type != ET8EK8_REG_8BIT &&
> > > +				 next->type != ET8EK8_REG_16BIT,
> > > +				 "Invalid type = %d", next->type)) {
> > dev_warn()
> >
> > > +	WARN_ON(sensor->power_count < 0);
> > 
> > Rather some dev_warn()? Do we need stack trace here?
> 
> I don't see what is wrong with WARN(). These are not expected to
> trigger, if they do we'll fix it. If you feel strongly about this,
> feel free to suggest a patch.

One thing is consistency with other parts of code... On all other places
is used dev_warn and on above 4 places WARN. dev_warn automatically adds
device name for easy debugging...

Another thing is that above WARNs do not write why it is warning. It
just write that some condition is not truth...

> > > +static int et8ek8_i2c_reglist_find_write(struct i2c_client *client,
> > > +					 struct et8ek8_meta_reglist *meta,
> > > +					 u16 type)
> > > +{
> > > +	struct et8ek8_reglist *reglist;
> > > +
> > > +	reglist = et8ek8_reglist_find_type(meta, type);
> > > +	if (!reglist)
> > > +		return -EINVAL;
> > > +
> > > +	return et8ek8_i2c_write_regs(client, reglist->regs);
> > > +}
> > > +
> > > +static struct et8ek8_reglist **et8ek8_reglist_first(
> > > +		struct et8ek8_meta_reglist *meta)
> > > +{
> > > +	return &meta->reglist[0].ptr;
> > > +}
> > 
> > Above code looks like re-implementation of linked-list. Does not kernel
> > already provide some?
> 
> Its actually array of pointers as far as I can tell. I don't think any
> helpers would be useful here.

Ok.

> > > +	new = et8ek8_gain_table[gain];
> > > +
> > > +	/* FIXME: optimise I2C writes! */
> > 
> > Is this FIMXE still valid?
> 
> Probably. Lets optimize it after merge.
> 
> > > +	if (sensor->power_count) {
> > > +		WARN_ON(1);
> > 
> > Such warning is probably not useful...
> 
> It should not happen, AFAICT. That's why I'd like to know if it does.

I mean: warning should have better description, what happened. Such
warning for somebody who does not see this code is not useful...

> > > +#include "et8ek8_reg.h"
> > > +
> > > +/*
> > > + * Stingray sensor mode settings for Scooby
> > > + */
> > 
> > Are settings for this sensor Stingray enough?
> 
> Seems to work well enough for me. If more modes are needed, we can add
> them later.

Ok.

> > It was me who copied these sensors settings to kernel driver. And I
> > chose only Stingray as this is what was needed for my N900 for
> > testing... Btw, you could add somewhere my and Ivo's Signed-off and
> > copyright state as we both modified et8ek8.c code...
> 
> Normally, people add copyrights when they modify the code. If you want
> to do it now, please send me a patch. (With those warn_ons too, if you
> care, but I think the code is fine as is).

I think sending patch in unified diff format for such change is
overkill. Just place to header it.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1542272

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > Rather some dev_warn()? Do we need stack trace here?
> > 
> > I don't see what is wrong with WARN(). These are not expected to
> > trigger, if they do we'll fix it. If you feel strongly about this,
> > feel free to suggest a patch.
> 
> One thing is consistency with other parts of code... On all other places
> is used dev_warn and on above 4 places WARN. dev_warn automatically adds
> device name for easy debugging...
> 
> Another thing is that above WARNs do not write why it is warning. It
> just write that some condition is not truth...

As I said, I believe it is fine as is.

> > > It was me who copied these sensors settings to kernel driver. And I
> > > chose only Stingray as this is what was needed for my N900 for
> > > testing... Btw, you could add somewhere my and Ivo's Signed-off and
> > > copyright state as we both modified et8ek8.c code...
> > 
> > Normally, people add copyrights when they modify the code. If you want
> > to do it now, please send me a patch. (With those warn_ons too, if you
> > care, but I think the code is fine as is).
> 
> I think sending patch in unified diff format for such change is
> overkill. Just place to header it.

Then the change does not happen. Sorry, I do not know what you
modified and when, and if it is copyrightable.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544210

Hi Pavel,

On Wed, Dec 14, 2016 at 09:12:02PM +0100, Pavel Machek wrote:
> Hi!
> 
> > On Wednesday 14 December 2016 13:24:51 Pavel Machek wrote:
> > >  
> > > Add driver for et8ek8 sensor, found in Nokia N900 main camera. Can be
> > > used for taking photos in 2.5MP resolution with fcam-dev.
> > > 
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > > From v4 I did cleanups to coding style and removed various oddities.
> > > 
> > > Exposure value is now in native units, which simplifies the code.
> > > 
> > > The patch to add device tree bindings was already acked by device tree
> > > people.
> 
> > > +	default:
> > > +		WARN_ONCE(1, ET8EK8_NAME ": %s: invalid message length.\n",
> > > +			  __func__);
> > 
> > dev_warn_once()
> ...
> > > +	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> > > +		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> > 
> > Maybe replace it with dev_warn_once() too? That condition in WARN_ONCE
> > does not look nice...
> ...
> > > +			if (WARN(next->type != ET8EK8_REG_8BIT &&
> > > +				 next->type != ET8EK8_REG_16BIT,
> > > +				 "Invalid type = %d", next->type)) {
> > dev_warn()
> >
> > > +	WARN_ON(sensor->power_count < 0);
> > 
> > Rather some dev_warn()? Do we need stack trace here?
> 
> I don't see what is wrong with WARN(). These are not expected to
> trigger, if they do we'll fix it. If you feel strongly about this,
> feel free to suggest a patch.

I think WARN() is good. It's a driver bug and it deserves to be notified.

> 
> > > +static int et8ek8_i2c_reglist_find_write(struct i2c_client *client,
> > > +					 struct et8ek8_meta_reglist *meta,
> > > +					 u16 type)
> > > +{
> > > +	struct et8ek8_reglist *reglist;
> > > +
> > > +	reglist = et8ek8_reglist_find_type(meta, type);
> > > +	if (!reglist)
> > > +		return -EINVAL;
> > > +
> > > +	return et8ek8_i2c_write_regs(client, reglist->regs);
> > > +}
> > > +
> > > +static struct et8ek8_reglist **et8ek8_reglist_first(
> > > +		struct et8ek8_meta_reglist *meta)
> > > +{
> > > +	return &meta->reglist[0].ptr;
> > > +}
> > 
> > Above code looks like re-implementation of linked-list. Does not kernel
> > already provide some?
> 
> Its actually array of pointers as far as I can tell. I don't think any
> helpers would be useful here.
> 
> > > +	new = et8ek8_gain_table[gain];
> > > +
> > > +	/* FIXME: optimise I2C writes! */
> > 
> > Is this FIMXE still valid?
> 
> Probably. Lets optimize it after merge.

I guess it's been like this since 2008 or so. I guess the comment could be
simply removed, it's not a real problem.

> 
> > > +	if (sensor->power_count) {
> > > +		WARN_ON(1);
> > 
> > Such warning is probably not useful...
> 
> It should not happen, AFAICT. That's why I'd like to know if it does.
> 
> > > +#include "et8ek8_reg.h"
> > > +
> > > +/*
> > > + * Stingray sensor mode settings for Scooby
> > > + */
> > 
> > Are settings for this sensor Stingray enough?
> 
> Seems to work well enough for me. If more modes are needed, we can add
> them later.

AFAIR the module is called Stingray.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1545079

[Multipart message — attachments visible in raw view] — view raw

Hi!

> I think WARN() is good. It's a driver bug and it deserves to be notified.
...
> I guess it's been like this since 2008 or so. I guess the comment could be
> simply removed, it's not a real problem.
...
> AFAIR the module is called Stingray.

Ok, so it seems we are pretty good? Can you take the patch now? Device
tree documentation is in

Subject: [PATCH v6] media: et8ek8: add device tree binding documentation

and we have

Acked-by: Rob Herring <robh@kernel.org>

on that.

Thanks and best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1545143

Hi Pavel,

On Tue, Dec 20, 2016 at 01:37:56PM +0100, Pavel Machek wrote:
> Hi!
> 
> > I think WARN() is good. It's a driver bug and it deserves to be notified.
> ...
> > I guess it's been like this since 2008 or so. I guess the comment could be
> > simply removed, it's not a real problem.
> ...
> > AFAIR the module is called Stingray.
> 
> Ok, so it seems we are pretty good? Can you take the patch now? Device

Did you see this:

<URL:http://www.spinics.net/lists/linux-media/msg109426.html>

> tree documentation is in
> 
> Subject: [PATCH v6] media: et8ek8: add device tree binding documentation
> 
> and we have
> 
> Acked-by: Rob Herring <robh@kernel.org>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1545522

[Multipart message — attachments visible in raw view] — view raw

Hi!

> On Tue, Dec 20, 2016 at 01:37:56PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > I think WARN() is good. It's a driver bug and it deserves to be notified.
> > ...
> > > I guess it's been like this since 2008 or so. I guess the comment could be
> > > simply removed, it's not a real problem.
> > ...
> > > AFAIR the module is called Stingray.
> > 
> > Ok, so it seems we are pretty good? Can you take the patch now? Device
> 
> Did you see this:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg109426.html>

Yes, I did. I did add the WARN_ON() we discussed, and we are now using
the native units. Adjusting the userspace was "fun", but I agree that
native interface has some advantages, so lets keep it that way. I
truly believe we are ready now :-). 

> > tree documentation is in
> > 
> > Subject: [PATCH v6] media: et8ek8: add device tree binding documentation
> > 
> > and we have
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web