Re: [PATCH 02/11] staging: kpc2000: add separate show functions for kpc_uio_class device attributes.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux