Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
On Wednesday 19 June 2013 02:52 AM, Sylwester Nawrocki wrote: Hi Kishon, I've noticed there is a little inconsistency between the code and documentation. On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote: +3. Creating the PHY + +The PHY driver should create the PHY in order for other peripheral controllers +to make use of it. The PHY framework provides 2 APIs to create the PHY. + +struct phy *phy_create(struct device *dev, int id, const struct phy_ops *ops, +void *priv); +struct phy *devm_phy_create(struct device *dev, int id, +const struct phy_ops *ops, void *priv); The 'label' argument is missing in those function prototypes. It seems the labels must be unique, I guess this needs to be documented. And do we allow NULL labels ? If so, then there is probably a check for NULL label needed in phy_lookup() and if not, then phy_create() should fail when NULL label is passed ? Yeah. Label is used only for non-dt case so NULL should be allowed while phy_create(). Thanks Kishon ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On Wednesday 19 June 2013 09:19 PM, Sylwester Nawrocki wrote:
Hi,
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, "no device provided for PHY\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(&phy->dev);
+
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->dev.of_node = dev->of_node;
+ phy->id = id;
+ phy->ops = ops;
+ phy->label = label;
Perhaps we could make it:
phy->label = kstrdup(label, GFP_KERNEL);
and free the label in phy_destroy() ?
yeah.. Fixed.
That way the users would't need to keep the label around. It might
be useful when PHY provider generates the labels dynamically. I guess
it is OK for PHY providers to hard code the labels and having the PHY
user drivers passed labels in platform data ?
yeah. But the only concern here is there is no way to enforce the
labels are passed in platform data. The PHY user driver can also
hard code the labels while getting the reference to the PHY and we can
catch such cases only by review.
+ dev_set_drvdata(&phy->dev, priv);
+
+ ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(&phy->dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(&phy->dev);
+
+ return phy;
+
+err1:
+ put_device(&phy->dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(phy_create);
+/**
+ * phy_destroy() - destroy the phy
+ * @phy: the phy to be destroyed
+ *
+ * Called to destroy the phy.
+ */
+void phy_destroy(struct phy *phy)
+{
+ pm_runtime_disable(&phy->dev);
+ device_unregister(&phy->dev);
Isn't kfree(phy); missing here ?
Not actually. Its done in phy_release (class's dev_release method)
Thanks
Kishon
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
> +/**
> + * phy_create() - create a new phy
> + * @dev: device that is creating the new phy
> + * @id: id of the phy
> + * @ops: function pointers for performing phy operations
> + * @label: label given to the phy
> + * @priv: private data from phy driver
> + *
> + * Called to create a phy using phy framework.
> + */
> +struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
> + const char *label, void *priv)
> +{
> + int ret;
> + struct phy *phy;
> +
> + if (!dev) {
> + dev_WARN(dev, "no device provided for PHY\n");
> + ret = -EINVAL;
> + goto err0;
> + }
> +
> + phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> + if (!phy) {
> + ret = -ENOMEM;
> + goto err0;
> + }
> +
> + device_initialize(&phy->dev);
> +
> + phy->dev.class = phy_class;
> + phy->dev.parent = dev;
> + phy->dev.of_node = dev->of_node;
> + phy->id = id;
> + phy->ops = ops;
> + phy->label = label;
Perhaps we could make it:
phy->label = kstrdup(label, GFP_KERNEL);
and free the label in phy_destroy() ?
That way the users would't need to keep the label around. It might
be useful when PHY provider generates the labels dynamically. I guess
it is OK for PHY providers to hard code the labels and having the PHY
user drivers passed labels in platform data ?
> + dev_set_drvdata(&phy->dev, priv);
> +
> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
> + if (ret)
> + goto err1;
> +
> + ret = device_add(&phy->dev);
> + if (ret)
> + goto err1;
> +
> + if (pm_runtime_enabled(dev))
> + pm_runtime_enable(&phy->dev);
> +
> + return phy;
> +
> +err1:
> + put_device(&phy->dev);
> + kfree(phy);
> +
> +err0:
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(phy_create);
> +/**
> + * phy_destroy() - destroy the phy
> + * @phy: the phy to be destroyed
> + *
> + * Called to destroy the phy.
> + */
> +void phy_destroy(struct phy *phy)
> +{
> + pm_runtime_disable(&phy->dev);
> + device_unregister(&phy->dev);
Isn't kfree(phy); missing here ?
> +}
> +EXPORT_SYMBOL_GPL(phy_destroy);
Thanks,
Sylwester
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi Kishon,
I've noticed there is a little inconsistency between the code and
documentation.
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+3. Creating the PHY
+
+The PHY driver should create the PHY in order for other peripheral controllers
+to make use of it. The PHY framework provides 2 APIs to create the PHY.
+
+struct phy *phy_create(struct device *dev, int id, const struct phy_ops *ops,
+ void *priv);
+struct phy *devm_phy_create(struct device *dev, int id,
+ const struct phy_ops *ops, void *priv);
The 'label' argument is missing in those function prototypes. It seems the
labels must be unique, I guess this needs to be documented. And do we allow
NULL labels ? If so, then there is probably a check for NULL label needed
in phy_lookup() and if not, then phy_create() should fail when NULL label
is passed ?
+The PHY drivers can use one of the above 2 APIs to create the PHY by passing
+the device pointer, id, phy ops and a driver data.
+phy_ops is a set of function pointers for performing PHY operations such as
+init, exit, power_on and power_off.
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, "no device provided for PHY\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(&phy->dev);
+
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->dev.of_node = dev->of_node;
+ phy->id = id;
+ phy->ops = ops;
+ phy->label = label;
+
+ dev_set_drvdata(&phy->dev, priv);
+
+ ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(&phy->dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(&phy->dev);
+
+ return phy;
+
+err1:
+ put_device(&phy->dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
Thanks,
Sylwester
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On Thu, Jun 13, 2013 at 02:13:51PM +0530, Kishon Vijay Abraham I wrote:
> +struct phy_provider *of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args))
I would rename this to __of_phy_provider_register() and in a header
have:
#define of_phy_provider_register(dev, xlate)\
__of_phy_provider_register((dev), THIS_MODULE, (xlate))
then your users don't need to remember always passing THIS_MODULE
argument.
> +struct phy_provider *devm_of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args))
likewise.
> +/**
> + * phy_release() - release the phy
> + * @dev: the dev member within phy
> + *
> + * When the last reference to the device is removed, it is called
> + * from the embedded kobject as release method.
> + */
> +static void phy_release(struct device *dev)
> +{
> + struct phy *phy;
> +
> + phy = container_of(dev, struct phy, dev);
I would have a:
#define to_phy(dev) (container_of((dev), struct phy dev))
somewhere in a header, just for the sake of brevity :-p
> + dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
make it dev_vdbg() instead.
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> new file mode 100644
> index 000..df0c98a
> --- /dev/null
> +++ b/include/linux/phy/phy.h
[ ... ]
> +static inline int phy_init(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + if (!pm_runtime_enabled(&phy->dev))
> + goto no_pm_runtime;
> +
> + ret = pm_runtime_get_sync(&phy->dev);
> + if (ret < 0) {
> + dev_err(&phy->dev, "get sync failed: %d\n", ret);
> + return ret;
> + }
> +
> +no_pm_runtime:
you can avoid this goto if you code as below, note that I'm also solving
a possible unbalanced PM runtime call which would prevent actual
*runtime* suspend of the PHY:
ret = phy_pm_runtime_get_sync(phy);
if (ret < 0 && ret != -ENOTSUPP)
return ret;
if (phy->ops->init) {
ret = phy->ops->init(phy);
if (ret < 0) {
dev_err(&phy->dev, "failed to initialize --> %d\n",
ret);
goto out;
}
}
out:
/*
* In order to avoid unbalanced PM runtime calls, let's make
* sure to disable what we might have enabled when entering this
* function.
*/
phy_pm_runtime_put(phy);
return ret;
> +}
> +
> +static inline int phy_exit(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + if (phy->ops->exit)
> + ret = phy->ops->exit(phy);
> +
> + if (!pm_runtime_enabled(&phy->dev))
> + goto no_pm_runtime;
> +
> + ret = pm_runtime_put_sync(&phy->dev);
move your pm runtime wrappers before these calls and you can use them.
--
balbi
signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
