Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751683Ab3J2SA2 (ORCPT ); Tue, 29 Oct 2013 14:00:28 -0400 Received: from ns.mm-sol.com ([212.124.72.66]:38914 "EHLO extserv.mm-sol.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751337Ab3J2SA0 (ORCPT ); Tue, 29 Oct 2013 14:00:26 -0400 Subject: Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI From: "Ivan T. Ivanov" To: Josh Cartwright Cc: Greg Kroah-Hartman , linux-arm-msm@vger.kernel.org, Gilad Avidov , linux-kernel@vger.kernel.org, Michael Bohan , Sagar Dharia , linux-arm-kernel@lists.infradead.org In-Reply-To: <20131029162615.GH20207@joshc.qualcomm.com> References: <149bbfe89e37376cc176c3aeb6c1fab9e4fd2b91.1382985169.git.joshc@codeaurora.org> <1383058923.2837.33.camel@iivanov-dev.int.mm-sol.com> <20131029162615.GH20207@joshc.qualcomm.com> Content-Type: text/plain; charset="UTF-8" Date: Tue, 29 Oct 2013 20:00:23 +0200 Message-ID: <1383069623.14905.7.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.32.3 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4708 Lines: 171 Hi, On Tue, 2013-10-29 at 11:26 -0500, Josh Cartwright wrote: > On Tue, Oct 29, 2013 at 05:02:03PM +0200, Ivan T. Ivanov wrote: > > On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote: > > > From: Kenneth Heitke > > > > > > System Power Management Interface (SPMI) is a specification > > > developed by the MIPI (Mobile Industry Process Interface) Alliance > > > optimized for the real time control of Power Management ICs (PMIC). > > > > > > SPMI is a two-wire serial interface that supports up to 4 master > > > devices and up to 16 logical slaves. > > > > > > The framework supports message APIs, multiple busses (1 controller > > > per bus) and multiple clients/slave devices per controller. > > > > > > Signed-off-by: Kenneth Heitke > > > Signed-off-by: Michael Bohan > > > Signed-off-by: Josh Cartwright > [..] > > > +static int spmi_drv_probe(struct device *dev) > > > +{ > > > + const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); > > > + int err = 0; > > > + > > > + if (sdrv->probe) > > > + err = sdrv->probe(to_spmi_device(dev)); > > > + > > > + return err; > > > +} > > > + > > > +static int spmi_drv_remove(struct device *dev) > > > +{ > > > + const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); > > > + int err = 0; > > > + > > > + if (sdrv->remove) > > > + err = sdrv->remove(to_spmi_device(dev)); > > > + > > > + return err; > > > +} > > > + > > > +static void spmi_drv_shutdown(struct device *dev) > > > +{ > > > + const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); > > > + > > > + if (sdrv->shutdown) > > > > If driver for device is not loaded this will cause kernel NULL > > pointer dereference. > > Indeed. I'll fix this. > > > > +static int of_spmi_register_devices(struct spmi_controller *ctrl) > > > +{ > > > + struct device_node *node; > > > + int err; > > > + > > > + dev_dbg(&ctrl->dev, "of_spmi_register_devices\n"); > > > + > > > + if (!ctrl->dev.of_node) > > > + return -ENODEV; > > > + > > > + dev_dbg(&ctrl->dev, "looping through children\n"); > > > + > > > + for_each_available_child_of_node(ctrl->dev.of_node, node) { > > > + struct spmi_device *sdev; > > > + u32 reg[2]; > > > + > > > + dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name); > > > + > > > + err = of_property_read_u32_array(node, "reg", reg, 2); > > > + if (err) { > > > + dev_err(&ctrl->dev, > > > + "node %s does not have 'reg' property\n", > > > + node->full_name); > > > + continue; > > > > Shouldn't this be a fatal error? > > Fatal in what way? It is fatal in the sense that this particular child > node is skipped, but other children can still be enumerated. Oh, I have missed this. > Are you > suggesting that we bail completely when we hit a wrongly-described > child? Please ignore my comment. > > > > + } > > > + > > > + if (reg[1] != SPMI_USID) { > > > + dev_err(&ctrl->dev, > > > + "node %s contains unsupported 'reg' entry\n", > > > + node->full_name); > > > + continue; > > > + } > > > + > > > + if (reg[0] > 0xF) { > > > + dev_err(&ctrl->dev, > > > + "invalid usid on node %s\n", > > > + node->full_name); > > > + continue; > > > + } > > > + > > > + dev_dbg(&ctrl->dev, "read usid %02x\n", reg[0]); > > > + > > > + sdev = spmi_device_alloc(ctrl); > > > + if (!sdev) > > > + continue; > > > + > > > + sdev->dev.of_node = node; > > > + sdev->usid = (u8) reg[0]; > > > + > > > + err = spmi_device_add(sdev); > > > + if (err) { > > > + dev_err(&sdev->dev, > > > + "failure adding device. status %d\n", err); > > > + spmi_device_put(sdev); > > > + continue; There is no need from this 'continue' here. > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +int spmi_controller_add(struct spmi_controller *ctrl) > > > +{ > > > + int ret; > > > + > > > + /* Can't register until after driver model init */ > > > + if (WARN_ON(!spmi_bus_type.p)) > > > + return -EAGAIN; > > > + > > > + ret = device_add(&ctrl->dev); > > > + if (ret) > > > + return ret; > > > + > > > + if (IS_ENABLED(CONFIG_OF)) > > > + of_spmi_register_devices(ctrl); > > > > Check for error code here? > > And do what with it? This was related to my previous comment, which is not valid. > Maybe instead, I should make > of_spmi_register_devices() return void. Sound reasonable to me and will be the same as i2c bus. Regards, Ivan > -- 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/