From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754821Ab3IMTs3 (ORCPT ); Fri, 13 Sep 2013 15:48:29 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:33028 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752534Ab3IMTs0 (ORCPT ); Fri, 13 Sep 2013 15:48:26 -0400 Date: Fri, 13 Sep 2013 12:48:22 -0700 From: Guenter Roeck To: =?iso-8859-1?Q?S=F6ren?= Brinkmann Cc: Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , Rob Landley , Mike Turquette , Grant Likely , devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Hyun Kwon Subject: Re: [PATCH] clk: si570: Add a driver for SI570 oscillators Message-ID: <20130913194822.GA12117@roeck-us.net> References: <1379033737-30015-1-git-send-email-soren.brinkmann@xilinx.com> <1379033737-30015-2-git-send-email-soren.brinkmann@xilinx.com> <20130913170005.GB5681@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 13, 2013 at 10:26:04AM -0700, Sören Brinkmann wrote: > On Fri, Sep 13, 2013 at 10:00:05AM -0700, Guenter Roeck wrote: > > On Thu, Sep 12, 2013 at 05:55:37PM -0700, Soren Brinkmann wrote: > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > The devices generate low-jitter clock signals and are reprogrammable via > > > an I2C interface. > > > > > > Cc: Guenter Roeck > > > Signed-off-by: Soren Brinkmann > > > --- [ ... ] > > > + /* Applying a new frequency can take up to 10ms */ > > > + usleep_range(10000, 10001); > > > > One microsecond range doesn't really buy anything besides forcing the compiler > > to generate extra code. Why not just use 10000 ? > That's due to checkpatch. I originally had 'msleep(10)' and checkpatch > suggested to use 'usleep_range()'. Then I put > 'usleep_range(10000, 10000)' and checkpatch suggested to not use the > same values for MIN and MAX, so I added the 1 to the MAX value. > If it is considered to be safe and okay to put in MIN=MAX=10000, I'll > change it accordingly. > Guess what checkpatch means is to put in something resonable, such as maybe (10000, 12000). (10000, 10001) just defeats checkpatch which doesn't really provide any value. So either provide a reasonable and acceptable range or use a single value. [ ... ] > > > + match = of_match_node(clk_si570_of_match, client->dev.of_node); > > > + if (!match) > > > + return -EINVAL; > > > > Seems unusual. Is this really needed ? It precludes the driver from being used > > in a non-devicetree environment, for example. I would guess that there is a match > > if client->dev.of_node is set. Otherwise, this code would be needed in every > > driver supporting devicetree, and I don't recall seeing that. > > > > > + ddata = match->data; > > > + > > You should be able to get this information (ie the pointer to si570_device_data) > > from id->driver_data. That would be more consistent with other i2c devices. > I think I copied this approach from the other clk-si... driver. I'll > do some research on your suggestion and change it. Could you point me to > an example for your proposal? > drivers/hwmon/lm90.c or drivers/hwmon/max6697.c. > > > + > > > + if (of_property_read_u32(client->dev.of_node, "factory-fout", > > > + &factory_fout)) { > > > + dev_warn(&client->dev, > > > + "DTS does not contain factory-fout, using default\n"); > > > > Is that really worth a warning ? > My understanding is, that the default output frequency is part-specific > and required to calculate the frequency of the internal crystal. Hence, > if you do not provide the correct default output frequency for your part, all > frequencies generated by this driver will be off, unless the here used > default matches your part. Please correct me if I'm wrong, otherwise, I > think it's worth a warning. > Maybe the property should be mandatory ? Thanks, Guenter