On Thu, May 16, 2019 at 09:04:02PM +0100, Jeremy Sowden wrote:
> Define separate simple show functions for each attribute instead of
> having a one big one containing a chain of conditionals.
There's nothing wrong with a change of contitionals, if you do it right
:)
> Signed-off-by: Jeremy Sowden <jeremy@xxxxxxxxxx>
> ---
> drivers/staging/kpc2000/kpc2000/cell_probe.c | 138 ++++++++++++-------
> 1 file changed, 92 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index 6a2ebdf20113..101eb23caaac 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -145,55 +145,102 @@ struct kpc_uio_device {
> u16 core_num;
> };
>
> -static ssize_t show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> {
> - struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> -
> - #define ATTR_NAME_CMP(v) (strcmp(v, attr->attr.name) == 0)
> - if ATTR_NAME_CMP("offset"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.offset);
> - } else if ATTR_NAME_CMP("size"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.length);
> - } else if ATTR_NAME_CMP("type"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.type);
> - }
> - else if ATTR_NAME_CMP("s2c_dma"){
> - if (kudev->cte.s2c_dma_present){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.s2c_dma_channel_num);
> - } else {
> - return scnprintf(buf, PAGE_SIZE, "not present\n");
> - }
> - } else if ATTR_NAME_CMP("c2s_dma"){
> - if (kudev->cte.c2s_dma_present){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.c2s_dma_channel_num);
> - } else {
> - return scnprintf(buf, PAGE_SIZE, "not present\n");
> - }
> - }
> - else if ATTR_NAME_CMP("irq_count"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.irq_count);
> - } else if ATTR_NAME_CMP("irq_base_num"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.irq_base_num);
> - } else if ATTR_NAME_CMP("core_num"){
> - return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->core_num);
> - } else {
> - return 0;
> - }
> - #undef ATTR_NAME_CMP
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.offset);
> +}
> +
> +static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.length);
> }
>
> +static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.type);
> +}
> +
> +static ssize_t s2c_dma_ch_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return 0;
> +}
> +
> +static ssize_t c2s_dma_ch_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return 0;
> +}
> +
> +static ssize_t s2c_dma_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
>
> -DEVICE_ATTR(offset, 0444, show_attr, NULL);
> -DEVICE_ATTR(size, 0444, show_attr, NULL);
> -DEVICE_ATTR(type, 0444, show_attr, NULL);
> -DEVICE_ATTR(s2c_dma_ch, 0444, show_attr, NULL);
> -DEVICE_ATTR(c2s_dma_ch, 0444, show_attr, NULL);
> -DEVICE_ATTR(s2c_dma, 0444, show_attr, NULL);
> -DEVICE_ATTR(c2s_dma, 0444, show_attr, NULL);
> -DEVICE_ATTR(irq_count, 0444, show_attr, NULL);
> -DEVICE_ATTR(irq_base_num, 0444, show_attr, NULL);
> -DEVICE_ATTR(core_num, 0444, show_attr, NULL);
> -struct attribute * kpc_uio_class_attrs[] = {
> + if (!kudev->cte.s2c_dma_present)
> + return scnprintf(buf, PAGE_SIZE, "not present\n");
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n",
> + kudev->cte.s2c_dma_channel_num);
> +}
> +
> +static ssize_t c2s_dma_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + if (!kudev->cte.c2s_dma_present)
> + return scnprintf(buf, PAGE_SIZE, "not present\n");
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n",
> + kudev->cte.c2s_dma_channel_num);
> +}
> +
> +static ssize_t irq_count_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.irq_count);
> +}
> +
> +static ssize_t irq_base_num_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->cte.irq_base_num);
> +}
> +
> +static ssize_t core_num_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct kpc_uio_device *kudev = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", kudev->core_num);
> +}
> +
> +DEVICE_ATTR(offset, 0444, offset_show, NULL);
> +DEVICE_ATTR(size, 0444, size_show, NULL);
> +DEVICE_ATTR(type, 0444, type_show, NULL);
> +DEVICE_ATTR(s2c_dma_ch, 0444, s2c_dma_ch_show, NULL);
> +DEVICE_ATTR(c2s_dma_ch, 0444, c2s_dma_ch_show, NULL);
> +DEVICE_ATTR(s2c_dma, 0444, s2c_dma_show, NULL);
> +DEVICE_ATTR(c2s_dma, 0444, c2s_dma_show, NULL);
> +DEVICE_ATTR(irq_count, 0444, irq_count_show, NULL);
> +DEVICE_ATTR(irq_base_num, 0444, irq_base_num_show, NULL);
> +DEVICE_ATTR(core_num, 0444, core_num_show, NULL);
If you are going to break things up like this, which is fine, you too
have to do it "right". And by that, I mean you need to use
DEVICE_ATTR_RO().
Also, the scnprintf() nonsense can go away, that should just be a simple
sprintf().
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
