[rtc-linux] Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

On 01/12/2015 at 12:28:11 -0800, Joshua Clayton wrote :
>
> On Wed, 25 Nov 2015 00:31:01 +0100
> Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> 
> > Hi,
> > 
> > This is okay but I'm not actually sure about the usefulness of that
> > interface. The driver is exactly here to abstract access to the
> > registers. Limit it to registers that are not used by the driver?
> > Also, It would probably better live in debugfs rather than in sysfs.
> > 
> 
> I agree completely... can I do that? Can I just move them to debugfs or
> remove them?
> I ask because these sysfs register files are in mainline already, and
> I know removing user facing kernel features is usually frowned on.
> 

My guess is that they haven't been documented anyway. If that is the
case, I wouldn't mind removing them. My thinking is that using them in a
production system is totally insane.

> > On 04/11/2015 at 07:36:39 -0800, Joshua Clayton wrote :
> > > Switch from manually creating all the sysfs attributes to
> > > defining them mostly in the canonical way.
> > > We are adding them to an spi driver, so we must still add and remove
> > > the group manually, but everythig else is done by The Book(tm) .
> > > 
> > > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > > ---
> > >  drivers/rtc/rtc-pcf2123.c | 118
> > > +++++++++++++++++++++------------------------- 1 file changed, 55
> > > insertions(+), 63 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> > > index 6701e6d..d494638 100644
> > > --- a/drivers/rtc/rtc-pcf2123.c
> > > +++ b/drivers/rtc/rtc-pcf2123.c
> > > @@ -84,16 +84,6 @@
> > >  
> > >  static struct spi_driver pcf2123_driver;
> > >  
> > > -struct pcf2123_sysfs_reg {
> > > -	struct device_attribute attr;
> > > -	char name[2];
> > > -};
> > > -
> > > -struct pcf2123_plat_data {
> > > -	struct rtc_device *rtc;
> > > -	struct pcf2123_sysfs_reg regs[16];
> > > -};
> > > -
> > >  /*
> > >   * Causes a 30 nanosecond delay to ensure that the PCF2123 chip
> > > select
> > >   * is released properly after an SPI write.  This function should
> > > be @@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device
> > > *dev, u8 reg, u8 val) static ssize_t pcf2123_show(struct device
> > > *dev, struct device_attribute *attr, char *buffer)
> > >  {
> > > -	struct pcf2123_sysfs_reg *r;
> > >  	u8 rxbuf[1];
> > >  	unsigned long reg;
> > >  	int ret;
> > >  
> > > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > > -
> > > -	ret = kstrtoul(r->name, 16, &reg);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > +	reg = hex_to_bin(attr->attr.name[0]);
> > >  	ret = pcf2123_read(dev, reg, rxbuf, 1);
> > >  	if (ret < 0)
> > >  		return -EIO;
> > > @@ -166,25 +150,19 @@ static ssize_t pcf2123_show(struct device
> > > *dev, struct device_attribute *attr, 
> > >  static ssize_t pcf2123_store(struct device *dev, struct
> > > device_attribute *attr, const char *buffer, size_t count) {
> > > -	struct pcf2123_sysfs_reg *r;
> > >  	unsigned long reg;
> > >  	unsigned long val;
> > > -
> > >  	int ret;
> > >  
> > > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > > -
> > > -	ret = kstrtoul(r->name, 16, &reg);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	ret = kstrtoul(buffer, 0, &val);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	reg = hex_to_bin(attr->attr.name[0]);
> > >  	pcf2123_write_reg(dev, reg, val);
> > >  	if (ret < 0)
> > >  		return -EIO;
> > > +
> > >  	return count;
> > >  }
> > >  
> > > @@ -326,17 +304,56 @@ static const struct rtc_class_ops
> > > pcf2123_rtc_ops = { .set_time	= pcf2123_rtc_set_time,
> > >  };
> > >  
> > > +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(2, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(3,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(7, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(8,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(c, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(d,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); + +static struct attribute *pcf2123_attrs[] = {
> > > +	&dev_attr_0.attr,
> > > +	&dev_attr_1.attr,
> > > +	&dev_attr_2.attr,
> > > +	&dev_attr_3.attr,
> > > +	&dev_attr_4.attr,
> > > +	&dev_attr_5.attr,
> > > +	&dev_attr_6.attr,
> > > +	&dev_attr_7.attr,
> > > +	&dev_attr_8.attr,
> > > +	&dev_attr_9.attr,
> > > +	&dev_attr_a.attr,
> > > +	&dev_attr_b.attr,
> > > +	&dev_attr_c.attr,
> > > +	&dev_attr_d.attr,
> > > +	&dev_attr_e.attr,
> > > +	&dev_attr_f.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group pcf2123_group = {
> > > +	.attrs = pcf2123_attrs,
> > > +};
> > > +
> > > +static const struct attribute_group *pcf2123_groups[] = {
> > > +	&pcf2123_group,
> > > +	NULL
> > > +};
> > > +
> > >  static int pcf2123_probe(struct spi_device *spi)
> > >  {
> > >  	struct rtc_device *rtc;
> > > -	struct pcf2123_plat_data *pdata;
> > > -	int ret, i;
> > > -
> > > -	pdata = devm_kzalloc(&spi->dev, sizeof(struct
> > > pcf2123_plat_data),
> > > -				GFP_KERNEL);
> > > -	if (!pdata)
> > > -		return -ENOMEM;
> > > -	spi->dev.platform_data = pdata;
> > > +	int ret;
> > >  
> > >  	if (!pcf2123_time_valid(&spi->dev)) {
> > >  		ret = pcf2123_reset(&spi->dev);
> > > @@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device
> > > *spi) goto kfree_exit;
> > >  	}
> > >  
> > > -	pdata->rtc = rtc;
> > > -
> > > -	for (i = 0; i < 16; i++) {
> > > -		sysfs_attr_init(&pdata->regs[i].attr.attr);
> > > -		sprintf(pdata->regs[i].name, "%1x", i);
> > > -		pdata->regs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> > > -		pdata->regs[i].attr.attr.name =
> > > pdata->regs[i].name;
> > > -		pdata->regs[i].attr.show = pcf2123_show;
> > > -		pdata->regs[i].attr.store = pcf2123_store;
> > > -		ret = device_create_file(&spi->dev,
> > > &pdata->regs[i].attr);
> > > -		if (ret) {
> > > -			dev_err(&spi->dev, "Unable to create sysfs
> > > %s\n",
> > > -				pdata->regs[i].name);
> > > -			goto sysfs_exit;
> > > -		}
> > > -	}
> > > +	ret = sysfs_create_groups(&spi->dev.kobj, pcf2123_groups);
> > > +	if (ret)
> > > +		goto sysfs_exit;
> > >  
> > >  	return 0;
> > > -
> > >  sysfs_exit:
> > > -	for (i--; i >= 0; i--)
> > > -		device_remove_file(&spi->dev,
> > > &pdata->regs[i].attr); -
> > > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> > >  kfree_exit:
> > >  	spi->dev.platform_data = NULL;
> > >  	return ret;
> > > @@ -390,16 +391,7 @@ kfree_exit:
> > >  
> > >  static int pcf2123_remove(struct spi_device *spi)
> > >  {
> > > -	struct pcf2123_plat_data *pdata =
> > > dev_get_platdata(&spi->dev);
> > > -	int i;
> > > -
> > > -	if (pdata) {
> > > -		for (i = 0; i < 16; i++)
> > > -			if (pdata->regs[i].name[0])
> > > -				device_remove_file(&spi->dev,
> > > -
> > > &pdata->regs[i].attr);
> > > -	}
> > > -
> > > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.5.0
> > > 
> > 
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.