From: Sakari Ailus <sakari.ailus@iki.fi>
To: Pavel Machek <pavel@ucw.cz>
Cc: "Pali Rohár" <pali.rohar@gmail.com>,
ivo.g.dimitrov.75@gmail.com, sre@kernel.org,
linux-media@vger.kernel.org, galak@codeaurora.org,
mchehab@osg.samsung.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor
Date: Mon, 19 Dec 2016 00:01:06 +0200 [thread overview]
Message-ID: <20161218220105.GS16630@valkosipuli.retiisi.org.uk> (raw)
In-Reply-To: <20161214201202.GB28424@amd>
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
next prev parent reply other threads:[~2016-12-18 22:01 UTC|newest]
Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-23 20:03 [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek
2016-10-23 20:19 ` Sakari Ailus
2016-10-23 20:33 ` Pavel Machek
2016-10-31 22:54 ` Sakari Ailus
2016-11-01 6:36 ` Ivaylo Dimitrov
2016-11-01 20:11 ` Sakari Ailus
2016-11-01 22:14 ` Ivaylo Dimitrov
2016-11-02 8:15 ` Pavel Machek
2016-11-02 8:16 ` Ivaylo Dimitrov
2016-11-01 15:39 ` Pavel Machek
2016-11-01 20:08 ` Sakari Ailus
2016-11-03 8:14 ` Pavel Machek
2016-11-03 22:48 ` Sebastian Reichel
2016-11-03 23:05 ` Sakari Ailus
2016-11-03 23:40 ` Ivaylo Dimitrov
2016-11-04 0:05 ` Sebastian Reichel
2016-11-14 21:58 ` Sakari Ailus
2016-11-15 0:53 ` Sebastian Reichel
2016-11-15 10:54 ` Pavel Machek
2016-11-15 22:55 ` Sakari Ailus
2016-10-23 20:40 ` Pavel Machek
2016-10-31 22:58 ` Sakari Ailus
2016-11-02 0:45 ` Laurent Pinchart
2016-10-23 20:47 ` Pavel Machek
2016-12-13 21:05 ` Pavel Machek
2016-12-18 21:56 ` Sakari Ailus
2016-11-19 23:29 ` Sakari Ailus
2016-11-20 10:02 ` Pavel Machek
2016-11-20 15:20 ` Pavel Machek
2016-11-20 15:21 ` Pavel Machek
2016-11-20 15:31 ` Pavel Machek
2016-12-14 12:24 ` [PATCH v5] " Pavel Machek
2016-12-14 13:03 ` Pali Rohár
2016-12-14 15:52 ` Ivaylo Dimitrov
2016-12-14 20:12 ` Pavel Machek
2016-12-14 22:07 ` Pali Rohár
2016-12-14 22:35 ` Pavel Machek
2016-12-18 22:01 ` Sakari Ailus [this message]
2016-12-20 12:37 ` Pavel Machek
2016-12-20 14:01 ` Sakari Ailus
2016-12-20 22:42 ` Pavel Machek
2016-12-21 13:42 ` Sakari Ailus
2016-12-21 22:42 ` Pavel Machek
2016-12-21 23:29 ` Sakari Ailus
2016-12-22 9:34 ` Pavel Machek
2016-12-22 10:01 ` [PATCH v6] " Pavel Machek
2016-12-22 13:39 ` [RFC/PATCH] media: Add video bus switch Pavel Machek
2016-12-22 14:32 ` Sebastian Reichel
2016-12-22 20:53 ` Pavel Machek
2016-12-22 23:11 ` Sebastian Reichel
2016-12-22 22:42 ` Pavel Machek
2016-12-22 23:40 ` Sebastian Reichel
2016-12-23 11:42 ` Pavel Machek
2016-12-23 18:53 ` Ivaylo Dimitrov
2016-12-23 20:56 ` Pavel Machek
2016-12-24 14:26 ` Pavel Machek
2016-12-24 14:43 ` Pavel Machek
2016-12-24 15:20 ` [PATCH] " Pavel Machek
2016-12-24 18:35 ` kbuild test robot
2017-01-12 11:17 ` Pavel Machek
2017-02-03 22:25 ` Sakari Ailus
2017-02-05 22:16 ` Pavel Machek
2017-02-05 22:44 ` Sakari Ailus
2017-02-03 12:35 ` [PATCH] devicetree: " Pavel Machek
2017-02-03 13:07 ` Sakari Ailus
2017-02-03 21:06 ` Pavel Machek
2017-02-03 21:34 ` Sakari Ailus
2017-02-04 21:56 ` Pavel Machek
2017-02-04 22:33 ` Sakari Ailus
2017-02-05 21:12 ` Pavel Machek
2017-02-05 23:40 ` Sebastian Reichel
2017-02-06 9:37 ` [PATCH] media: add operation to get configuration of "the other side" of the link Pavel Machek
2017-12-19 15:43 ` Sakari Ailus
2017-12-20 17:54 ` [PATCH] devicetree: Add video bus switch Laurent Pinchart
2017-12-21 9:05 ` Sakari Ailus
2017-12-21 16:36 ` Ivaylo Dimitrov
2017-12-22 9:24 ` Pavel Machek
2017-02-03 13:32 ` Pali Rohár
2017-02-03 21:07 ` Pavel Machek
2017-02-04 1:04 ` Sebastian Reichel
2017-02-08 21:36 ` Rob Herring
2017-02-08 22:30 ` Pavel Machek
2017-02-09 23:02 ` Rob Herring
2017-02-09 23:03 ` Rob Herring
2017-02-10 19:54 ` Pavel Machek
2017-02-10 22:17 ` Sakari Ailus
2017-02-13 9:54 ` Pavel Machek
2017-02-13 10:20 ` Sakari Ailus
2017-03-02 8:54 ` Pavel Machek
2017-02-08 22:34 ` Pavel Machek
2017-02-09 22:58 ` Rob Herring
2017-02-10 21:17 ` Pavel Machek
2016-12-27 9:26 ` [PATCH v6] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus
2016-12-27 20:45 ` Pavel Machek
2016-12-27 20:59 ` [PATCH] mark myself as mainainer for camera on N900 Pavel Machek
2016-12-27 23:57 ` Sebastian Reichel
2017-01-25 13:48 ` Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161218220105.GS16630@valkosipuli.retiisi.org.uk \
--to=sakari.ailus@iki.fi \
--cc=galak@codeaurora.org \
--cc=ivo.g.dimitrov.75@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@osg.samsung.com \
--cc=pali.rohar@gmail.com \
--cc=pavel@ucw.cz \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).