Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756831AbbLAUrz (ORCPT ); Tue, 1 Dec 2015 15:47:55 -0500 Received: from down.free-electrons.com ([37.187.137.238]:52328 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755995AbbLAUrx (ORCPT ); Tue, 1 Dec 2015 15:47:53 -0500 Date: Tue, 1 Dec 2015 21:47:00 +0100 From: Alexandre Belloni To: Joshua Clayton Cc: Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups Message-ID: <20151201204700.GZ22136@piout.net> References: <63d5284dfb9d04f1f0a33c02cd937e2ddd72fb9b.1446587705.git.stillcompiling@gmail.com> <20151124233101.GH3950@piout.net> <20151201122811.771db44e@jclayton-pc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151201122811.771db44e@jclayton-pc> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8187 Lines: 244 On 01/12/2015 at 12:28:11 -0800, Joshua Clayton wrote : > > On Wed, 25 Nov 2015 00:31:01 +0100 > Alexandre Belloni 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 > > > --- > > > 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, ®); > > > - 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, ®); > > > - 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/