Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752726AbbDBUau (ORCPT ); Thu, 2 Apr 2015 16:30:50 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:48393 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752679AbbDBUar (ORCPT ); Thu, 2 Apr 2015 16:30:47 -0400 Date: Thu, 2 Apr 2015 22:30:44 +0200 From: Pavel Machek To: Sakari Ailus Cc: Andrew Morton , pali.rohar@gmail.com, sre@debian.org, sre@ring0.de, kernel list , linux-arm-kernel , linux-omap@vger.kernel.org, tony@atomide.com, khilman@kernel.org, aaro.koskinen@iki.fi, ivo.g.dimitrov.75@gmail.com, patrikbachan@gmail.com, galak@codeaurora.org, bcousson@baylibre.com, m.chehab@samsung.com, devicetree@vger.kernel.org Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 Message-ID: <20150402203043.GB29963@amd> References: <20150402143846.GA11687@amd> <20150402161453.GH20756@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150402161453.GH20756@valkosipuli.retiisi.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3444 Lines: 114 Hi! > Hi Pawel, > > My apologies for the very late reply. > > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote: > > > > > > We are moving to device tree support on OMAP3, but that currently > > breaks ADP1653 driver. This adds device tree support, plus required > > documentation. > > > > Signed-off-by: Pavel Machek > > > > --- > > > > I'm not sure if it is device tree or media framework, either everyone > > waits for someone else, or noone really cares. > > Neither. Some people are unfortuantely very busy with many other things as > well. :-P Well.. Being busy is ok. Nitpicking is also ok. But both at the same time... is not good. > > Andrew, can you just merge it? > > > > Please apply, > > Please wait just a while. > > I think we can merge this eventually through the linux-media tree, but > please first see the comments below. > > > +Required properties of the flash LED child node: > > + > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > > The documentation says that the maximum value is used if these values are > not specified. I think I'd make these optional. I'd rather not: when you make a typo in dts, it would supply maximum available current, potentially damaging the LED. You will not be able to tell brightness difference with naked eye... > > __adp1653_set_power(struct adp1653_flash *flash, int on) > > { > > - int ret; > > + int ret = 0; > > + > > + if (flash->platform_data->power) { > > + ret = flash->platform_data->power(&flash->subdev, on); > > The power() callback should be dropped. It's controlling a GPIO. But that > can be done later on. The alternative is a patch before this one. I'd prefer to do it later; we want to keep functionality on N900 without DTS, too. > > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > > if (flash == NULL) > > return -ENOMEM; > > > > flash->platform_data = client->dev.platform_data; > > + if (!flash->platform_data) { > > I'd check whether dev->of_node is non-NULL instead. Ok. > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client, > > goto free_and_quit; > > > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > > - > > I rather liked the newline here. Please don't remove it. :-) Ok. > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, adp1653_id_table); > > > > -static struct dev_pm_ops adp1653_pm_ops = { > > +static const struct dev_pm_ops adp1653_pm_ops = { > > .suspend = adp1653_suspend, > > .resume = adp1653_resume, > > }; > > > > > > A corresponding change to the N900 dts would be very nice. Corresponding change to the dts will come in separate patch. Or do you have n900 for testing? > I think you're missing change to adp1653_i2c_driver.driver.of_match_table. It actually worked for me, which means device tree somehow does it magic. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/