From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH] clk: si570: Add a driver for SI570 oscillators Date: Fri, 13 Sep 2013 12:48:22 -0700 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-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-doc-owner@vger.kernel.org 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 List-Id: devicetree@vger.kernel.org On Fri, Sep 13, 2013 at 10:26:04AM -0700, S=F6ren 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 oscillato= rs. > > > The devices generate low-jitter clock signals and are reprogramma= ble via > > > an I2C interface. > > >=20 > > > Cc: Guenter Roeck > > > Signed-off-by: Soren Brinkmann > > > --- [ ... ] > > > + /* Applying a new frequency can take up to 10ms */ > > > + usleep_range(10000, 10001); > >=20 > > One microsecond range doesn't really buy anything besides forcing t= he compiler > > to generate extra code. Why not just use 10000 ? > That's due to checkpatch. I originally had 'msleep(10)' and checkpatc= h > 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=3DMAX=3D10000, = I'll > change it accordingly. >=20 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 =3D of_match_node(clk_si570_of_match, client->dev.of_node= ); > > > + if (!match) > > > + return -EINVAL; > >=20 > > Seems unusual. Is this really needed ? It precludes the driver from= being used > > in a non-devicetree environment, for example. I would guess that th= ere 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. > >=20 > > > + ddata =3D 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? >=20 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"); > >=20 > > Is that really worth a warning ? > My understanding is, that the default output frequency is part-specif= ic > and required to calculate the frequency of the internal crystal. Henc= e, > if you do not provide the correct default output frequency for your p= art, all > frequencies generated by this driver will be off, unless the here use= d > default matches your part. Please correct me if I'm wrong, otherwise,= I > think it's worth a warning. >=20 Maybe the property should be mandatory ? Thanks, Guenter