Re: [PATCH V6 2/3] dma: add Qualcomm Technologies HIDMA management driver

[okaya]

> On Sun, Nov 22, 2015 at 6:37 PM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
>> The Qualcomm Technologies HIDMA device has been designed
>> to support virtualization technology. The driver has been
>> divided into two to follow the hardware design.
>>
>> 1. HIDMA Management driver
>> 2. HIDMA Channel driver
>>
>> Each HIDMA HW consists of multiple channels. These channels
>> share some set of common parameters. These parameters are
>> initialized by the management driver during power up.
>> Same management driver is used for monitoring the execution
>> of the channels. Management driver can change the performance
>> behavior dynamically such as bandwidth allocation and
>> prioritization.
>>
>> The management driver is executed in hypervisor context and
>> is the main management entity for all channels provided by
>> the device.
>
>> +What:          /sys/devices/platform/hidma-mgmt*/channel*_weight
>> +               /sys/devices/platform/QCOM8060:*/channel*__weight
>
> Extra _ ?

Done.

>
>
>> +Each HIDMA HW consists of multiple channels. These channels
>> +share some set of common parameters. These parameters are
>> +initialized by the management driver during power up.
>> +Same management driver is used for monitoring the execution
>> +of the channels. Management driver can change the performance
>> +behavior dynamically such as bandwidth allocation and
>> +prioritization.
>> +
>> +All channel devices get probed in the hypervisor
>> +context during power up. They show up as DMA engine
>> +DMA channels. Then, before starting the virtualization; each
>> +channel device is unbound from the hypervisor by VFIO
>> +and assign to the guest machine for control.
>> +
>> +This management driver will  be used by the system
>> +admin to monitor/reset the execution state of the DMA
>> +channels. This will be the management interface.
>
> Hmmâ?¦ Can lines be longer up to 76-78 characters?
>
>> +#define MAX_WR_XACTIONS_MASK           0x1F
>> +#define MAX_RD_XACTIONS_MASK           0x1F
>> +#define WEIGHT_MASK                    0x7F
>> +#define MAX_BUS_REQ_LEN_MASK           0xFFFF
>> +#define CHRESET_TIMEOUUT_MASK          0xFFFFF
>
> GENMASK?

done

>
>> +int hidma_mgmt_setup(struct hidma_mgmt_dev *mgmtdev)
>> +{
>> +       unsigned int i;
>> +       u32 val;
>> +
>> +       if (!is_power_of_2(mgmtdev->max_write_request) ||
>> +               (mgmtdev->max_write_request < 128) ||
>
> Someone likes parens.

yes, I do. I don't trust compilers and also don't like to open holes for
people making quick changes to code while ignoring the operator precedence for
maintenance reasons.

> I might agree with these cases, but below in assignments and combined
> operations the parens are indeed redundant.
>

OK. I think we are already in personal style of code zone now. I have already
broken another review because you don't like for loops but rather have a while
loop.

I'll leave ifs and change the assignments only. I'll need your reviewed-by
once you are happy.

>> +               (mgmtdev->max_write_request > 1024)) {
>> +               dev_err(&mgmtdev->pdev->dev, "invalid write request %d\n",
>> +                       mgmtdev->max_write_request);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!is_power_of_2(mgmtdev->max_read_request) ||
>> +               (mgmtdev->max_read_request < 128) ||
>> +               (mgmtdev->max_read_request > 1024)) {
>
> Ditto.
>
>> +               dev_err(&mgmtdev->pdev->dev, "invalid read request %d\n",
>> +                       mgmtdev->max_read_request);
>> +               return  -EINVAL;
>> +       }
>> +
>> +       if (mgmtdev->max_wr_xactions > MAX_WR_XACTIONS_MASK) {
>> +               dev_err(&mgmtdev->pdev->dev,
>> +                       "max_wr_xactions cannot be bigger than %d\n",
>> +                       MAX_WR_XACTIONS_MASK);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (mgmtdev->max_rd_xactions > MAX_RD_XACTIONS_MASK) {
>> +               dev_err(&mgmtdev->pdev->dev,
>> +                       "max_rd_xactions cannot be bigger than %d\n",
>> +                       MAX_RD_XACTIONS_MASK);
>> +               return -EINVAL;
>> +       }
>> +
>> +       for (i = 0; i < mgmtdev->dma_channels; i++) {
>> +               if (mgmtdev->priority[i] > 1) {
>> +                       dev_err(&mgmtdev->pdev->dev, "priority can be 0 or
>> 1\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               if (mgmtdev->weight[i] > MAX_CHANNEL_WEIGHT) {
>> +                       dev_err(&mgmtdev->pdev->dev,
>> +                               "max value of weight can be %d.\n",
>> +                               MAX_CHANNEL_WEIGHT);
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* weight needs to be at least one */
>> +               if (mgmtdev->weight[i] == 0)
>> +                       mgmtdev->weight[i] = 1;
>> +       }
>> +
>> +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
>> +       val = readl(mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET);
>> +       val &= ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS);
>
>> +       val |= (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS);
>
> Ditto.

ok

>
>> +       val &= ~(MAX_BUS_REQ_LEN_MASK);
>
> Ditto.

ok

>
>> +       val |= (mgmtdev->max_read_request);
>
> Ditto.
ok
>
>> +       writel(val, mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET);
>> +
>> +       val = readl(mgmtdev->virtaddr + MAX_XACTIONS_OFFSET);
>> +       val &= ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS);
>
>> +       val |= (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS);
>> +       val &= ~(MAX_RD_XACTIONS_MASK);
>> +       val |= (mgmtdev->max_rd_xactions);
>
> Ditto for all 3 above.

alright
>
>> +       writel(val, mgmtdev->virtaddr + MAX_XACTIONS_OFFSET);
>> +
>> +       mgmtdev->hw_version = readl(mgmtdev->virtaddr + HW_VERSION_OFFSET);
>> +       mgmtdev->hw_version_major = (mgmtdev->hw_version >> 28) & 0xF;
>> +       mgmtdev->hw_version_minor = (mgmtdev->hw_version >> 16) & 0xF;
>> +
>> +       for (i = 0; i < mgmtdev->dma_channels; i++) {
>> +               val = readl(mgmtdev->virtaddr + QOS_N_OFFSET + (4 * i));
>
> Ditto.
>
>> +               val &= ~(1 << PRIORITY_BIT_POS);
>> +               val |= ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS);
>> +               val &= ~(WEIGHT_MASK << WRR_BIT_POS);
>> +               val |= ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS);
>> +               writel(val, mgmtdev->virtaddr + QOS_N_OFFSET + (4 * i));
>
> Ditto.
>
>> +       }
>> +
>> +       val = readl(mgmtdev->virtaddr + CHRESET_TIMEOUT_OFFSET);
>> +       val &= ~CHRESET_TIMEOUUT_MASK;
>
> Look here, you use it like it intuitively feels.
>
>> +       val |= (mgmtdev->chreset_timeout_cycles & CHRESET_TIMEOUUT_MASK);
>
> But hereâ?¦
>
>> +       writel(val, mgmtdev->virtaddr + CHRESET_TIMEOUT_OFFSET);
>> +
>> +       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
>> +       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(hidma_mgmt_setup);
>> +
>> +static int hidma_mgmt_probe(struct platform_device *pdev)
>> +{
>> +       struct hidma_mgmt_dev *mgmtdev;
>> +       struct resource *res;
>> +       void __iomem *virtaddr;
>> +       int irq;
>> +       int rc;
>> +       u32 val;
>> +
>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
>> +       pm_runtime_use_autosuspend(&pdev->dev);
>> +       pm_runtime_set_active(&pdev->dev);
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_get_sync(&pdev->dev);
>
> +empty line

added a new line for you. I don't know why.

>
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       virtaddr = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(virtaddr)) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(&pdev->dev, "irq resources not found\n");
>> +               rc = irq;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL);
>> +       if (!mgmtdev) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->pdev = pdev;
>> +       mgmtdev->addrsize = resource_size(res);
>> +       mgmtdev->virtaddr = virtaddr;
>> +
>> +       rc = device_property_read_u32(&pdev->dev, "dma-channels",
>> +                               &mgmtdev->dma_channels);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "number of channels missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32(&pdev->dev,
>> +                               "channel-reset-timeout-cycles",
>> +                               &mgmtdev->chreset_timeout_cycles);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "channel reset timeout missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32(&pdev->dev, "max-write-burst-bytes",
>> +                               &mgmtdev->max_write_request);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "max-write-burst-bytes missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32(&pdev->dev, "max-read-burst-bytes",
>> +                               &mgmtdev->max_read_request);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "max-read-burst-bytes missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32(&pdev->dev, "max-write-transactions",
>> +                               &mgmtdev->max_wr_xactions);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "max-write-transactions missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32(&pdev->dev, "max-read-transactions",
>> +                               &mgmtdev->max_rd_xactions);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "max-read-transactions missing\n");
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->priority = devm_kcalloc(&pdev->dev,
>> +               mgmtdev->dma_channels, sizeof(*mgmtdev->priority),
>> GFP_KERNEL);
>> +       if (!mgmtdev->priority) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->weight = devm_kcalloc(&pdev->dev,
>> +               mgmtdev->dma_channels, sizeof(*mgmtdev->weight),
>> GFP_KERNEL);
>> +       if (!mgmtdev->weight) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32_array(&pdev->dev, "channel-priority",
>> +                               mgmtdev->priority, mgmtdev->dma_channels);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "channel-priority missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = device_property_read_u32_array(&pdev->dev, "channel-weight",
>> +                               mgmtdev->weight, mgmtdev->dma_channels);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "channel-weight missing\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = hidma_mgmt_setup(mgmtdev);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "setup failed\n");
>> +               goto out;
>> +       }
>> +
>> +       /* start the HW */
>> +       val = readl(mgmtdev->virtaddr + CFG_OFFSET);
>> +       val |= 1;
>> +       writel(val, mgmtdev->virtaddr + CFG_OFFSET);
>> +
>> +       rc = hidma_mgmt_init_sys(mgmtdev);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "sysfs setup failed\n");
>> +               goto out;
>> +       }
>> +
>> +       dev_info(&pdev->dev,
>> +                "HW rev: %d.%d @ %pa with %d physical channels\n",
>> +                mgmtdev->hw_version_major, mgmtdev->hw_version_minor,
>> +                &res->start, mgmtdev->dma_channels);
>> +
>> +       platform_set_drvdata(pdev, mgmtdev);
>> +       pm_runtime_mark_last_busy(&pdev->dev);
>> +       pm_runtime_put_autosuspend(&pdev->dev);
>> +       return 0;
>
>> +out:
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_sync_suspend(&pdev->dev);
>
> I'm not sure the sequence is logically correct, though it might work.
>
reversed

>> +       return rc;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static const struct acpi_device_id hidma_mgmt_acpi_ids[] = {
>> +       {"QCOM8060"},
>> +       {},
>> +};
>> +#endif
>> +
>> +static const struct of_device_id hidma_mgmt_match[] = {
>> +       { .compatible = "qcom,hidma-mgmt-1.0", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, hidma_mgmt_match);
>> +
>> +static struct platform_driver hidma_mgmt_driver = {
>> +       .probe = hidma_mgmt_probe,
>> +       .driver = {
>> +               .name = "hidma-mgmt",
>> +               .of_match_table = hidma_mgmt_match,
>> +               .acpi_match_table = ACPI_PTR(hidma_mgmt_acpi_ids),
>> +       },
>> +};
>> +module_platform_driver(hidma_mgmt_driver);
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/dma/qcom/hidma_mgmt.h b/drivers/dma/qcom/hidma_mgmt.h
>> new file mode 100644
>> index 0000000..04340ff
>> --- /dev/null
>> +++ b/drivers/dma/qcom/hidma_mgmt.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Qualcomm Technologies HIDMA Management common header
>> + *
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +struct hidma_mgmt_dev {
>> +       u8 hw_version_major;
>> +       u8 hw_version_minor;
>> +
>> +       u32 max_wr_xactions;
>> +       u32 max_rd_xactions;
>> +       u32 max_write_request;
>> +       u32 max_read_request;
>> +       u32 dma_channels;
>> +       u32 chreset_timeout_cycles;
>> +       u32 hw_version;
>> +       u32 *priority;
>> +       u32 *weight;
>> +
>> +       /* Hardware device constants */
>> +       void __iomem *virtaddr;
>> +       resource_size_t addrsize;
>> +
>> +       struct platform_device *pdev;
>> +};
>> +
>> +int hidma_mgmt_init_sys(struct hidma_mgmt_dev *dev);
>> +int hidma_mgmt_setup(struct hidma_mgmt_dev *mgmtdev);
>> diff --git a/drivers/dma/qcom/hidma_mgmt_sys.c
>> b/drivers/dma/qcom/hidma_mgmt_sys.c
>> new file mode 100644
>> index 0000000..70d324e
>> --- /dev/null
>> +++ b/drivers/dma/qcom/hidma_mgmt_sys.c
>> @@ -0,0 +1,231 @@
>> +/*
>> + * Qualcomm Technologies HIDMA Management SYS interface
>> + *
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/sysfs.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "hidma_mgmt.h"
>> +
>> +struct fileinfo {
>> +       char *name;
>> +       int mode;
>> +       int (*get)(struct hidma_mgmt_dev *mdev);
>> +       int (*set)(struct hidma_mgmt_dev *mdev, u64 val);
>> +};
>> +
>> +#define IMPLEMENT_GETSET(name)                                 \
>> +static int get_##name(struct hidma_mgmt_dev *mdev)             \
>> +{                                                              \
>> +       return mdev->name;                                      \
>> +}                                                              \
>> +static int set_##name(struct hidma_mgmt_dev *mdev, u64 val)    \
>> +{                                                              \
>> +       u64 tmp;                                                \
>> +       int rc;                                                 \
>> +                                                               \
>> +       tmp = mdev->name;                                       \
>> +       mdev->name = val;                                       \
>> +       rc = hidma_mgmt_setup(mdev);                            \
>> +       if (rc)                                                 \
>> +               mdev->name = tmp;                               \
>> +       return rc;                                              \
>> +}
>> +
>> +#define DECLARE_ATTRIBUTE(name, mode)                          \
>> +       {#name, mode, get_##name, set_##name}
>> +
>> +IMPLEMENT_GETSET(hw_version_major)
>> +IMPLEMENT_GETSET(hw_version_minor)
>> +IMPLEMENT_GETSET(max_wr_xactions)
>> +IMPLEMENT_GETSET(max_rd_xactions)
>> +IMPLEMENT_GETSET(max_write_request)
>> +IMPLEMENT_GETSET(max_read_request)
>> +IMPLEMENT_GETSET(dma_channels)
>> +IMPLEMENT_GETSET(chreset_timeout_cycles)
>> +
>> +static int set_priority(struct hidma_mgmt_dev *mdev, unsigned int i, u64
>> val)
>> +{
>> +       u64 tmp;
>> +       int rc;
>> +
>> +       if (i > mdev->dma_channels)
>> +               return -EINVAL;
>> +
>> +       tmp = mdev->priority[i];
>> +       mdev->priority[i] = val;
>> +       rc = hidma_mgmt_setup(mdev);
>> +       if (rc)
>> +               mdev->priority[i] = tmp;
>> +       return rc;
>> +}
>> +
>> +static int set_weight(struct hidma_mgmt_dev *mdev, unsigned int i, u64 val)
>> +{
>> +       u64 tmp;
>> +       int rc;
>> +
>> +       if (i > mdev->dma_channels)
>> +               return -EINVAL;
>> +
>> +       tmp = mdev->weight[i];
>> +       mdev->weight[i] = val;
>> +       rc = hidma_mgmt_setup(mdev);
>> +       if (rc)
>> +               mdev->weight[i] = tmp;
>> +       return rc;
>> +}
>> +
>> +static struct fileinfo files[] = {
>
> Perhaps hidma_mgmt_files ?

done

>
>> +       DECLARE_ATTRIBUTE(hw_version_major, S_IRUGO),
>> +       DECLARE_ATTRIBUTE(hw_version_minor, S_IRUGO),
>> +       DECLARE_ATTRIBUTE(dma_channels, S_IRUGO),
>> +       DECLARE_ATTRIBUTE(chreset_timeout_cycles, S_IRUGO),
>> +       DECLARE_ATTRIBUTE(max_wr_xactions, (S_IRUGO|S_IWUGO)),
>> +       DECLARE_ATTRIBUTE(max_rd_xactions, (S_IRUGO|S_IWUGO)),
>> +       DECLARE_ATTRIBUTE(max_write_request, (S_IRUGO|S_IWUGO)),
>> +       DECLARE_ATTRIBUTE(max_read_request, (S_IRUGO|S_IWUGO)),
>> +};
>> +
>> +static ssize_t show_values(struct device *dev, struct device_attribute
>> *attr,
>> +                               char *buf)
>> +{
>> +       struct platform_device *pdev = to_platform_device(dev);
>> +       struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev);
>> +       unsigned int i;
>> +

I'll initialize buf as

buf[0] = 0;

here.

>> +       for (i = 0; i < ARRAY_SIZE(files); i++) {
>> +               if (strcmp(attr->attr.name, files[i].name) == 0) {
>> +                       sprintf(buf, "%d\n", files[i].get(mdev));
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +       for (i = 0; i < mdev->dma_channels; i++) {
>> +               char name[30];
>> +
>> +               sprintf(name, "channel%d_priority", i);
>> +               if (strcmp(attr->attr.name, name) == 0) {
>> +                       sprintf(buf, "%d\n", mdev->priority[i]);
>> +                       goto done;
>> +               }
>> +
>> +               sprintf(name, "channel%d_weight", i);
>> +               if (strcmp(attr->attr.name, name) == 0) {
>> +                       sprintf(buf, "%d\n", mdev->weight[i]);
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +done:
>> +       return strlen(buf);
>
> buf might be uninitialized here. Have you got any warning from compiler?
>
>> +}
>> +
>> +static ssize_t set_values(struct device *dev, struct device_attribute
>> *attr,
>> +                               const char *buf, size_t count)
>> +{
>> +       struct platform_device *pdev = to_platform_device(dev);
>> +       struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev);
>> +       unsigned long tmp;
>> +       unsigned int i;
>> +       int rc;
>> +
>> +       rc = kstrtoul(buf, 0, &tmp);
>> +       if (rc)
>> +               return rc;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(files); i++) {
>> +               if (strcmp(attr->attr.name, files[i].name) == 0) {
>> +                       rc = files[i].set(mdev, tmp);
>> +                       if (rc)
>> +                               return rc;
>> +
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +       for (i = 0; i < mdev->dma_channels; i++) {
>> +               char name[30];
>> +
>> +               sprintf(name, "channel%d_priority", i);
>> +               if (strcmp(attr->attr.name, name) == 0) {
>> +                       rc = set_priority(mdev, i, tmp);
>> +                       if (rc)
>> +                               return rc;
>> +                       goto done;
>> +               }
>> +
>> +               sprintf(name, "channel%d_weight", i);
>> +               if (strcmp(attr->attr.name, name) == 0) {
>> +                       rc = set_weight(mdev, i, tmp);
>> +                       if (rc)
>> +                               return rc;
>> +               }
>> +       }
>> +done:
>> +       return count;
>> +}
>> +
>> +static int create_sysfs_entry(struct hidma_mgmt_dev *dev, char *name, int
>> mode)
>> +{
>> +       struct device_attribute *attrs;
>> +       char *name_copy;
>> +
>> +       attrs = devm_kmalloc(&dev->pdev->dev,
>> +                       sizeof(struct device_attribute), GFP_KERNEL);
>> +       if (!attrs)
>> +               return -ENOMEM;
>> +
>> +       name_copy = devm_kstrdup(&dev->pdev->dev, name, GFP_KERNEL);
>> +       if (!name_copy)
>> +               return -ENOMEM;
>> +
>> +       attrs->attr.name = name_copy;
>> +       attrs->attr.mode = mode;
>> +       attrs->show      = show_values;
>> +       attrs->store     = set_values;
>> +       sysfs_attr_init(&attrs->attr);
>> +
>> +       return device_create_file(&dev->pdev->dev, attrs);
>> +}
>> +
>> +
>
> Extra empty line.
>
Removed

>> +int hidma_mgmt_init_sys(struct hidma_mgmt_dev *dev)
>> +{
>> +       unsigned int i;
>> +       int rc;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(files); i++) {
>> +               rc = create_sysfs_entry(dev, files[i].name, files[i].mode);
>> +               if (rc)
>> +                       return rc;
>> +       }
>> +
>
> Can it be like
>
> /sys/â?¦/DEVICExx/
>  channelYY/
>  attr1
>  attr2
>  â?¦
>
> ?

I'll work on it. I didn't know that you are allowed to create subdirectories
in sysfs. I was just creating attributes to keep it simple. But, your
suggestion is cleaner.

>
> I think it will be easier to handle in code and from user. (Similar
> way DMAEngine API does for slave DMA devices)

Now, the good stuff. Can you clarify your comment? I didn't understand it.

>
>> +       for (i = 0; i < dev->dma_channels; i++) {
>> +               char name[30];
>> +
>> +               sprintf(name, "channel%d_priority", i);
>> +               rc = create_sysfs_entry(dev, name, (S_IRUGO|S_IWUGO));
>> +               if (rc)
>> +                       return rc;
>> +
>> +               sprintf(name, "channel%d_weight", i);
>> +               rc = create_sysfs_entry(dev, name, (S_IRUGO|S_IWUGO));
>> +               if (rc)
>> +                       return rc;
>> +       }
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(hidma_mgmt_init_sys);
>
> --
> With Best Regards,
> Andy Shevchenko
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/