On Fri, May 25, 2012 at 1:55 PM, Eduardo Valentin
[off-list ref] wrote:

This patch introduces a MFD core device driver for
OMAP system control module.

The control module allows software control of
various static modes supported by the device. It is
composed of two control submodules: general control
module and device (padconfiguration) control
module.

In this patch, the children defined are for:
. USB-phy pin control
. Bangap temperature sensor

Device driver is probed with postcore_initcall.
However, as some of the APIs exposed by this driver
may be needed in very early init phase, an early init
class is also available: "early_omap_control".

Signed-off-by: J Keerthy <j-keerthy@ti.com>
Signed-off-by: Kishon Vijay Abraham I <redacted>
Signed-off-by: Eduardo Valentin <redacted>
---

[..]

Thanks for getting rid of OMAP CONFIG here.

I might have missed in the last scan, but can you let
function return the register value.

I am guessing, you did this for error case handling. You might
want to stick to read API semantic and just have WARN_ON()
to take care of error case.

+/**
+ * omap_control_writel: Write a single omap control module register.
+ *
+ * @dev: device to read from.
+ * @val: value to write.
+ * @reg: register to write to.
+ *
+ * returns 0 on success or -EINVAL in case struct device is invalid.
+ */
+int omap_control_writel(struct device *dev, u32 val, u32 reg)
+{
+ ? ? ? struct omap_control *omap_control = dev_get_drvdata(dev);
+ ? ? ? unsigned long flags;
+
+ ? ? ? if (!omap_control)
+ ? ? ? ? ? ? ? return -EINVAL;
+
+ ? ? ? spin_lock_irqsave(&omap_control->reg_lock, flags);
+ ? ? ? writel(val, omap_control->base + reg);
+ ? ? ? spin_unlock_irqrestore(&omap_control->reg_lock, flags);
+
+ ? ? ? return 0;
+}
+EXPORT_SYMBOL_GPL(omap_control_writel);
+
+/**
+ * omap_control_get: returns the control module device pinter
+ *
+ * The modules which has to use control module API's to read or write should
+ * call this API to get the control module device pointer.
+ */
+struct device *omap_control_get(void)
+{
+ ? ? ? unsigned long flags;
+
+ ? ? ? if (!omap_control_module)
+ ? ? ? ? ? ? ? return ERR_PTR(-ENODEV);
+
+ ? ? ? spin_lock_irqsave(&omap_control_module->reg_lock, flags);
+ ? ? ? omap_control_module->use_count++;
+ ? ? ? spin_unlock_irqrestore(&omap_control_module->reg_lock, flags);
+
+ ? ? ? return omap_control_module->dev;
+}
+EXPORT_SYMBOL_GPL(omap_control_get);
+
+/**
+ * omap_control_put: returns the control module device pinter
+ *
+ * The modules which has to use control module API's to read or write should
+ * call this API to get the control module device pointer.
+ */
+void omap_control_put(struct device *dev)
+{
+ ? ? ? struct omap_control *omap_control = dev_get_drvdata(dev);
+ ? ? ? unsigned long flags;
+
+ ? ? ? if (!omap_control)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? spin_lock_irqsave(&omap_control->reg_lock, flags);
+ ? ? ? omap_control->use_count--;
+ ? ? ? spin_unlock_irqrestore(&omap_control_module->reg_lock, flags);
+}
+EXPORT_SYMBOL_GPL(omap_control_put);
+
+static const struct of_device_id of_omap_control_match[] = {
+ ? ? ? { .compatible = "ti,omap3-control", },
+ ? ? ? { .compatible = "ti,omap4-control", },
+ ? ? ? { .compatible = "ti,omap5-control", },
+ ? ? ? { },
+};
+
+static int __devinit omap_control_probe(struct platform_device *pdev)
+{
+ ? ? ? struct resource *res;
+ ? ? ? void __iomem *base;
+ ? ? ? struct device *dev = &pdev->dev;
+ ? ? ? struct device_node *np = dev->of_node;
+ ? ? ? struct omap_control *omap_control;
+
+ ? ? ? omap_control = devm_kzalloc(dev, sizeof(*omap_control), GFP_KERNEL);
+ ? ? ? if (!omap_control) {
+ ? ? ? ? ? ? ? dev_err(dev, "not enough memory for omap_control\n");
+ ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? }
+
+ ? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ ? ? ? if (!res) {
+ ? ? ? ? ? ? ? dev_err(dev, "missing memory base resource\n");
+ ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? }
+
+ ? ? ? base = devm_request_and_ioremap(dev, res);
+ ? ? ? if (!base) {
+ ? ? ? ? ? ? ? dev_err(dev, "ioremap failed\n");
+ ? ? ? ? ? ? ? return -EADDRNOTAVAIL;
+ ? ? ? }
+
+ ? ? ? omap_control->base = base;
+ ? ? ? omap_control->dev = dev;
+ ? ? ? spin_lock_init(&omap_control->reg_lock);
+
+ ? ? ? platform_set_drvdata(pdev, omap_control);
+ ? ? ? omap_control_module = omap_control;
+
+ ? ? ? return of_platform_populate(np, of_omap_control_match, NULL, dev);
+}
+

Will the probe get called on multiple devices and race ?

+static int __devexit omap_control_remove(struct platform_device *pdev)
+{
+ ? ? ? struct omap_control *omap_control = platform_get_drvdata(pdev);
+
+ ? ? ? spin_lock(&omap_control->reg_lock);
+ ? ? ? if (omap_control->use_count > 0) {
+ ? ? ? ? ? ? ? spin_unlock(&omap_control->reg_lock);
+ ? ? ? ? ? ? ? dev_err(&pdev->dev, "device removed while still being used\n");
+ ? ? ? ? ? ? ? return -EBUSY;
+ ? ? ? }
+ ? ? ? spin_unlock(&omap_control->reg_lock);
+

Do you really need above lock where you are just doing the
register read. smp_rmb(), should be enough, I guess.

Regards
Santosh