On Wed, 2012-09-26 at 20:31 +0530, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of API's for the PHY drivers to
> create/destroy a PHY and API's
Just some trivial notes.
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
[]
> @@ -0,0 +1,445 @@
[]
> +static void devm_phy_release(struct device *dev, void *res)
> +{
> + struct phy *phy = *(struct phy **)res;
That's a bit twisted isn't it?
Perhaps
struct phy *phy = res;
[]
> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
> +{
> + return res == match_data;
static bool devm_phy_match(etc...)
[]
> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, u8
> index)
> +{
> + struct phy *phy = NULL, **ptr;
> + struct device_node *node;
> +
> + if (!dev->of_node) {
> + dev_dbg(dev, "device does not have a device node entry\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + node = of_parse_phandle(dev->of_node, phandle, index);
> + if (!node) {
> + dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
> + dev->of_node->full_name);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
Is this the right size?
Because ptr is **, perhaps sizeof(struct phy) is clearer.
> + if (!ptr) {
> + dev_dbg(dev, "failed to allocate memory for devres\n");
alloc failures generally don't need specific OOM messages
as the general alloc OOM dumps stack.
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + mutex_lock(&phy_list_mutex);
> +
> + phy = of_phy_lookup(dev, node);
> + if (IS_ERR(phy) || !phy->dev.class ||
> + !try_module_get(phy->dev.class->owner)) {
I think it's tasteful coding style to align like:
if (IS_ERR(phy) || !phy->dev.class ||
!try_module_get(phy->dev.class->owner)) {
[]
> +struct phy *phy_create(struct device *dev, struct phy_descriptor *desc)
> +{
> + int ret;
> + struct phy *phy;
> + struct phy_bind *phy_bind;
> + const char *devname = NULL;
> +
> + if (!dev || !desc) {
> + dev_err(dev, "no descriptor/device provided for PHY\n");
> + ret = -EINVAL;
> + goto err0;
> + }
> +
> + if (!desc->ops) {
> + dev_err(dev, "no PHY ops provided\n");
> + ret = -EINVAL;
> + goto err0;
> + }
> +
> + phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> + if (!phy) {
> + dev_err(dev, "no memory for PHY\n");
No OOM message required...
[]
> +static int __init phy_core_init(void)
> +{
> + phy_class = class_create(THIS_MODULE, "phy");
> + if (IS_ERR(phy_class)) {
> + pr_err("failed to create phy class --> %ld\n",
> + PTR_ERR(phy_class));
You might add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any include so that pr_<levels> are prefixed.
cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
