From patchwork Sun Nov 20 15:20:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Machek X-Patchwork-Id: 9438517 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C099E606DB for ; Sun, 20 Nov 2016 15:21:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B46C2288CA for ; Sun, 20 Nov 2016 15:21:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A90AF28A01; Sun, 20 Nov 2016 15:21:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_TVD_MIME_EPI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2800D288CA for ; Sun, 20 Nov 2016 15:21:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753080AbcKTPUn (ORCPT ); Sun, 20 Nov 2016 10:20:43 -0500 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:57707 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752778AbcKTPUn (ORCPT ); Sun, 20 Nov 2016 10:20:43 -0500 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id D24A8823C3; Sun, 20 Nov 2016 16:20:39 +0100 (CET) Date: Sun, 20 Nov 2016 16:20:36 +0100 From: Pavel Machek To: Sakari Ailus Cc: ivo.g.dimitrov.75@gmail.com, sre@kernel.org, pali.rohar@gmail.com, linux-media@vger.kernel.org, galak@codeaurora.org, mchehab@osg.samsung.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor Message-ID: <20161120152034.GA5189@amd> References: <20161023200355.GA5391@amd> <20161119232943.GF13965@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161119232943.GF13965@valkosipuli.retiisi.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi! > > + u32 min, max; > > + > > + v4l2_ctrl_handler_init(&sensor->ctrl_handler, 4); > > + > > + /* V4L2_CID_GAIN */ > > + v4l2_ctrl_new_std(&sensor->ctrl_handler, &et8ek8_ctrl_ops, > > + V4L2_CID_GAIN, 0, ARRAY_SIZE(et8ek8_gain_table) - 1, > > + 1, 0); > > + > > + /* V4L2_CID_EXPOSURE */ > > + min = et8ek8_exposure_rows_to_us(sensor, 1); > > + max = et8ek8_exposure_rows_to_us(sensor, > > + sensor->current_reglist->mode.max_exp); > > Haven't I suggested to use lines instead? I vaguely remember doing so... > this would remove quite some code from the driver. Do you have some more hints? I'll try to figure it out... > > +#ifdef CONFIG_PM > > + > > +static int et8ek8_suspend(struct device *dev) > > static int __maybe_unused ... > > Please check the smiapp patches I just sent to the list. The smiapp driver > had similar issues. Ok, I guess I figured it out from other code (no network at the moment). > > + if (sensor->power_count) { > > + gpiod_set_value(sensor->reset, 0); > > + clk_disable_unprepare(sensor->ext_clk); > > + sensor->power_count = 0; > > + } > > + > > You're missing v4l2_async_unregister_subdev() here. Added. > > + v4l2_device_unregister_subdev(&sensor->subdev); > > + device_remove_file(&client->dev, &dev_attr_priv_mem); > > + v4l2_ctrl_handler_free(&sensor->ctrl_handler); > > + media_entity_cleanup(&sensor->subdev.entity); > > + > > + return 0; > > +} > > +MODULE_DEVICE_TABLE(i2c, et8ek8_id_table); > > + > > +static const struct dev_pm_ops et8ek8_pm_ops = { > > + .suspend = et8ek8_suspend, > > + .resume = et8ek8_resume, > > How about using SET_SYSTEM_SLEEP_PM_OPS() here? Ok, I guess that saves few lines. > > +module_i2c_driver(et8ek8_i2c_driver); > > + > > +MODULE_AUTHOR("Sakari Ailus "); > > You should put your name here as well. :-) > > It's been a long time I even tried to use it. :-i Me? Ok, I can list myself there, but I don't really know much about that driver. > > + * Contact: Sakari Ailus > > + * Tuukka Toivonen > > Tuukka's e-mail is wrong here (the correct address is elsewhere in the > patch). Fixed. Ok, these cleanups are here. Pavel diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c index eb131b2..eb8c1b4 100644 --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c @@ -5,6 +5,7 @@ * * Contact: Sakari Ailus * Tuukka Toivonen + * Pavel Machek * * Based on code from Toni Leinonen . * @@ -1435,9 +1436,7 @@ static const struct v4l2_subdev_internal_ops et8ek8_internal_ops = { /* -------------------------------------------------------------------------- * I2C driver */ -#ifdef CONFIG_PM - -static int et8ek8_suspend(struct device *dev) +static int __maybe_unused et8ek8_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); @@ -1449,7 +1448,7 @@ static int et8ek8_suspend(struct device *dev) return __et8ek8_set_power(sensor, false); } -static int et8ek8_resume(struct device *dev) +static int __maybe_unused et8ek8_resume(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); @@ -1461,13 +1460,6 @@ static int et8ek8_resume(struct device *dev) return __et8ek8_set_power(sensor, true); } -#else - -#define et8ek8_suspend NULL -#define et8ek8_resume NULL - -#endif /* CONFIG_PM */ - static int et8ek8_probe(struct i2c_client *client, const struct i2c_device_id *devid) { @@ -1542,6 +1534,7 @@ static int __exit et8ek8_remove(struct i2c_client *client) v4l2_device_unregister_subdev(&sensor->subdev); device_remove_file(&client->dev, &dev_attr_priv_mem); v4l2_ctrl_handler_free(&sensor->ctrl_handler); + v4l2_async_unregister_subdev(&sensor->subdev); media_entity_cleanup(&sensor->subdev.entity); return 0; @@ -1559,8 +1552,7 @@ static const struct i2c_device_id et8ek8_id_table[] = { MODULE_DEVICE_TABLE(i2c, et8ek8_id_table); static const struct dev_pm_ops et8ek8_pm_ops = { - .suspend = et8ek8_suspend, - .resume = et8ek8_resume, + SET_SYSTEM_SLEEP_PM_OPS(et8ek8_suspend, et8ek8_resume) }; static struct i2c_driver et8ek8_i2c_driver = { @@ -1576,6 +1568,6 @@ static struct i2c_driver et8ek8_i2c_driver = { module_i2c_driver(et8ek8_i2c_driver); -MODULE_AUTHOR("Sakari Ailus "); +MODULE_AUTHOR("Sakari Ailus , Pavel Machek