On 2019-05-16, at 22:45:33 +0200, Greg KH wrote: > 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(). I made the change to DEVICE_ATTR_RO in a later patch. I'll merge that into this one. > Also, the scnprintf() nonsense can go away, that should just be a simple > sprintf(). Will do. Thanks for the review. J.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel