From mboxrd@z Thu Jan 1 00:00:00 1970 From: okaya@codeaurora.org Subject: Re: [PATCH V6 2/3] dma: add Qualcomm Technologies HIDMA management driver Date: Sun, 22 Nov 2015 19:52:52 -0000 Message-ID: <565bdce834ba1cf4a73029b5c027856e.squirrel@www.codeaurora.org> References: <1448210257-4768-1-git-send-email-okaya@codeaurora.org> <1448210257-4768-3-git-send-email-okaya@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:36081 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752012AbbKVTwy (ORCPT ); Sun, 22 Nov 2015 14:52:54 -0500 In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Andy Shevchenko Cc: Sinan Kaya , dmaengine , Timur Tabi , cov@codeaurora.org, jcm@redhat.com, Andy Gross , Arnd Bergmann , linux-arm-msm@vger.kernel.org, linux-arm Mailing List , "linux-kernel@vger.kernel.org" > On Sun, Nov 22, 2015 at 6:37 PM, Sinan Kaya wr= ote: >> 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=E2=80=A6 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 fo= r people making quick changes to code while ignoring the operator precede= nce 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 a= lready 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 =3D 0; i < mgmtdev->dma_channels; i++) { >> + if (mgmtdev->priority[i] > 1) { >> + dev_err(&mgmtdev->pdev->dev, "priority can b= e 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] =3D=3D 0) >> + mgmtdev->weight[i] =3D 1; >> + } >> + >> + pm_runtime_get_sync(&mgmtdev->pdev->dev); >> + val =3D readl(mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET); >> + val &=3D ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS); > >> + val |=3D (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_P= OS); > > Ditto. ok > >> + val &=3D ~(MAX_BUS_REQ_LEN_MASK); > > Ditto. ok > >> + val |=3D (mgmtdev->max_read_request); > > Ditto. ok > >> + writel(val, mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET); >> + >> + val =3D readl(mgmtdev->virtaddr + MAX_XACTIONS_OFFSET); >> + val &=3D ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS); > >> + val |=3D (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_PO= S); >> + val &=3D ~(MAX_RD_XACTIONS_MASK); >> + val |=3D (mgmtdev->max_rd_xactions); > > Ditto for all 3 above. alright > >> + writel(val, mgmtdev->virtaddr + MAX_XACTIONS_OFFSET); >> + >> + mgmtdev->hw_version =3D readl(mgmtdev->virtaddr + HW_VERSION= _OFFSET); >> + mgmtdev->hw_version_major =3D (mgmtdev->hw_version >> 28) & = 0xF; >> + mgmtdev->hw_version_minor =3D (mgmtdev->hw_version >> 16) & = 0xF; >> + >> + for (i =3D 0; i < mgmtdev->dma_channels; i++) { >> + val =3D readl(mgmtdev->virtaddr + QOS_N_OFFSET + (4 = * i)); > > Ditto. > >> + val &=3D ~(1 << PRIORITY_BIT_POS); >> + val |=3D ((mgmtdev->priority[i] & 0x1) << PRIORITY_B= IT_POS); >> + val &=3D ~(WEIGHT_MASK << WRR_BIT_POS); >> + val |=3D ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_= BIT_POS); >> + writel(val, mgmtdev->virtaddr + QOS_N_OFFSET + (4 * = i)); > > Ditto. > >> + } >> + >> + val =3D readl(mgmtdev->virtaddr + CHRESET_TIMEOUT_OFFSET); >> + val &=3D ~CHRESET_TIMEOUUT_MASK; > > Look here, you use it like it intuitively feels. > >> + val |=3D (mgmtdev->chreset_timeout_cycles & CHRESET_TIMEOUUT= _MASK); > > But here=E2=80=A6 > >> + 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_TIM= EOUT); >> + 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 =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + virtaddr =3D devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(virtaddr)) { >> + rc =3D -ENOMEM; >> + goto out; >> + } >> + >> + irq =3D platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(&pdev->dev, "irq resources not found\n"); >> + rc =3D irq; >> + goto out; >> + } >> + >> + mgmtdev =3D devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_K= ERNEL); >> + if (!mgmtdev) { >> + rc =3D -ENOMEM; >> + goto out; >> + } >> + >> + mgmtdev->pdev =3D pdev; >> + mgmtdev->addrsize =3D resource_size(res); >> + mgmtdev->virtaddr =3D virtaddr; >> + >> + rc =3D 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 =3D 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 =3D 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 =3D 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 =3D device_property_read_u32(&pdev->dev, "max-write-trans= actions", >> + &mgmtdev->max_wr_xactions); >> + if (rc) { >> + dev_err(&pdev->dev, "max-write-transactions missing\= n"); >> + goto out; >> + } >> + >> + rc =3D device_property_read_u32(&pdev->dev, "max-read-transa= ctions", >> + &mgmtdev->max_rd_xactions); >> + if (rc) { >> + dev_err(&pdev->dev, "max-read-transactions missing\n= "); >> + goto out; >> + } >> + >> + mgmtdev->priority =3D devm_kcalloc(&pdev->dev, >> + mgmtdev->dma_channels, sizeof(*mgmtdev->priority), >> GFP_KERNEL); >> + if (!mgmtdev->priority) { >> + rc =3D -ENOMEM; >> + goto out; >> + } >> + >> + mgmtdev->weight =3D devm_kcalloc(&pdev->dev, >> + mgmtdev->dma_channels, sizeof(*mgmtdev->weight), >> GFP_KERNEL); >> + if (!mgmtdev->weight) { >> + rc =3D -ENOMEM; >> + goto out; >> + } >> + >> + rc =3D device_property_read_u32_array(&pdev->dev, "channel-p= riority", >> + mgmtdev->priority, mgmtdev->dma_chan= nels); >> + if (rc) { >> + dev_err(&pdev->dev, "channel-priority missing\n"); >> + goto out; >> + } >> + >> + rc =3D device_property_read_u32_array(&pdev->dev, "channel-w= eight", >> + mgmtdev->weight, mgmtdev->dma_channe= ls); >> + if (rc) { >> + dev_err(&pdev->dev, "channel-weight missing\n"); >> + goto out; >> + } >> + >> + rc =3D hidma_mgmt_setup(mgmtdev); >> + if (rc) { >> + dev_err(&pdev->dev, "setup failed\n"); >> + goto out; >> + } >> + >> + /* start the HW */ >> + val =3D readl(mgmtdev->virtaddr + CFG_OFFSET); >> + val |=3D 1; >> + writel(val, mgmtdev->virtaddr + CFG_OFFSET); >> + >> + rc =3D 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_mino= r, >> + &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[] =3D { >> + {"QCOM8060"}, >> + {}, >> +}; >> +#endif >> + >> +static const struct of_device_id hidma_mgmt_match[] =3D { >> + { .compatible =3D "qcom,hidma-mgmt-1.0", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, hidma_mgmt_match); >> + >> +static struct platform_driver hidma_mgmt_driver =3D { >> + .probe =3D hidma_mgmt_probe, >> + .driver =3D { >> + .name =3D "hidma-mgmt", >> + .of_match_table =3D hidma_mgmt_match, >> + .acpi_match_table =3D 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 mo= dify >> + * it under the terms of the GNU General Public License version 2 a= nd >> + * 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 mo= dify >> + * it under the terms of the GNU General Public License version 2 a= nd >> + * 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 >> +#include >> + >> +#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 =3D mdev->name; \ >> + mdev->name =3D val; \ >> + rc =3D hidma_mgmt_setup(mdev); \ >> + if (rc) \ >> + mdev->name =3D 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 =3D mdev->priority[i]; >> + mdev->priority[i] =3D val; >> + rc =3D hidma_mgmt_setup(mdev); >> + if (rc) >> + mdev->priority[i] =3D 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 =3D mdev->weight[i]; >> + mdev->weight[i] =3D val; >> + rc =3D hidma_mgmt_setup(mdev); >> + if (rc) >> + mdev->weight[i] =3D tmp; >> + return rc; >> +} >> + >> +static struct fileinfo files[] =3D { > > 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_attrib= ute >> *attr, >> + char *buf) >> +{ >> + struct platform_device *pdev =3D to_platform_device(dev); >> + struct hidma_mgmt_dev *mdev =3D platform_get_drvdata(pdev); >> + unsigned int i; >> + I'll initialize buf as buf[0] =3D 0; here. >> + for (i =3D 0; i < ARRAY_SIZE(files); i++) { >> + if (strcmp(attr->attr.name, files[i].name) =3D=3D 0)= { >> + sprintf(buf, "%d\n", files[i].get(mdev)); >> + goto done; >> + } >> + } >> + >> + for (i =3D 0; i < mdev->dma_channels; i++) { >> + char name[30]; >> + >> + sprintf(name, "channel%d_priority", i); >> + if (strcmp(attr->attr.name, name) =3D=3D 0) { >> + sprintf(buf, "%d\n", mdev->priority[i]); >> + goto done; >> + } >> + >> + sprintf(name, "channel%d_weight", i); >> + if (strcmp(attr->attr.name, name) =3D=3D 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 compil= er? > >> +} >> + >> +static ssize_t set_values(struct device *dev, struct device_attribu= te >> *attr, >> + const char *buf, size_t count) >> +{ >> + struct platform_device *pdev =3D to_platform_device(dev); >> + struct hidma_mgmt_dev *mdev =3D platform_get_drvdata(pdev); >> + unsigned long tmp; >> + unsigned int i; >> + int rc; >> + >> + rc =3D kstrtoul(buf, 0, &tmp); >> + if (rc) >> + return rc; >> + >> + for (i =3D 0; i < ARRAY_SIZE(files); i++) { >> + if (strcmp(attr->attr.name, files[i].name) =3D=3D 0)= { >> + rc =3D files[i].set(mdev, tmp); >> + if (rc) >> + return rc; >> + >> + goto done; >> + } >> + } >> + >> + for (i =3D 0; i < mdev->dma_channels; i++) { >> + char name[30]; >> + >> + sprintf(name, "channel%d_priority", i); >> + if (strcmp(attr->attr.name, name) =3D=3D 0) { >> + rc =3D set_priority(mdev, i, tmp); >> + if (rc) >> + return rc; >> + goto done; >> + } >> + >> + sprintf(name, "channel%d_weight", i); >> + if (strcmp(attr->attr.name, name) =3D=3D 0) { >> + rc =3D set_weight(mdev, i, tmp); >> + if (rc) >> + return rc; >> + } >> + } >> +done: >> + return count; >> +} >> + >> +static int create_sysfs_entry(struct hidma_mgmt_dev *dev, char *nam= e, int >> mode) >> +{ >> + struct device_attribute *attrs; >> + char *name_copy; >> + >> + attrs =3D devm_kmalloc(&dev->pdev->dev, >> + sizeof(struct device_attribute), GFP_KERNEL)= ; >> + if (!attrs) >> + return -ENOMEM; >> + >> + name_copy =3D devm_kstrdup(&dev->pdev->dev, name, GFP_KERNEL= ); >> + if (!name_copy) >> + return -ENOMEM; >> + >> + attrs->attr.name =3D name_copy; >> + attrs->attr.mode =3D mode; >> + attrs->show =3D show_values; >> + attrs->store =3D 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 =3D 0; i < ARRAY_SIZE(files); i++) { >> + rc =3D create_sysfs_entry(dev, files[i].name, files[= i].mode); >> + if (rc) >> + return rc; >> + } >> + > > Can it be like > > /sys/=E2=80=A6/DEVICExx/ > channelYY/ > attr1 > attr2 > =E2=80=A6 > > ? I'll work on it. I didn't know that you are allowed to create subdirect= ories 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 =3D 0; i < dev->dma_channels; i++) { >> + char name[30]; >> + >> + sprintf(name, "channel%d_priority", i); >> + rc =3D create_sysfs_entry(dev, name, (S_IRUGO|S_IWUG= O)); >> + if (rc) >> + return rc; >> + >> + sprintf(name, "channel%d_weight", i); >> + rc =3D create_sysfs_entry(dev, name, (S_IRUGO|S_IWUG= O)); >> + if (rc) >> + return rc; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(hidma_mgmt_init_sys); > > -- > With Best Regards, > Andy Shevchenko >