Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1730574 > unrolled thread
| Started by | Himanshi Jain <himshijain.hj@gmail.com> |
|---|---|
| First post | 2017-09-12 00:50 +0200 |
| Last post | 2017-09-12 20:20 +0200 |
| Articles | 8 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Himanshi Jain <himshijain.hj@gmail.com> - 2017-09-12 00:50 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Daniel Baluta <daniel.baluta@gmail.com> - 2017-09-12 10:00 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Julia Lawall <julia.lawall@lip6.fr> - 2017-09-12 10:00 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Julia Lawall <julia.lawall@lip6.fr> - 2017-09-12 20:10 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Lars-Peter Clausen <lars@metafoo.de> - 2017-09-12 20:20 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names himanshi <himshijain.hj@gmail.com> - 2017-09-12 20:40 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names himanshi <himshijain.hj@gmail.com> - 2017-09-12 20:10 +0200
Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names Lars-Peter Clausen <lars@metafoo.de> - 2017-09-12 20:20 +0200
| From | Himanshi Jain <himshijain.hj@gmail.com> |
|---|---|
| Date | 2017-09-12 00:50 +0200 |
| Subject | [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoFGF-1ai-1@gated-at.bofh.it> |
Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
string instead of implicit conversion to string using
preprocessors. Added double quotes around names in
existing usage of IIO_DEVICE_ATTR_NAMED.
Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
---
drivers/iio/adc/ad7793.c | 2 +-
drivers/staging/iio/adc/ad7192.c | 2 +-
drivers/staging/iio/adc/ad7280a.c | 4 ++--
include/linux/iio/sysfs.h | 6 +++++-
include/linux/sysfs.h | 7 +++++++
5 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index e6706a0..d74e324 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev,
}
static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
- in_voltage-voltage_scale_available, S_IRUGO,
+ "in_voltage-voltage_scale_available", S_IRUGO,
ad7793_show_scale_available, NULL, 0);
static struct attribute *ad7793_attributes[] = {
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index d11c6de..daff38c 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
}
static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
- in_voltage-voltage_scale_available,
+ "in_voltage-voltage_scale_available",
0444, ad7192_show_scale_available, NULL, 0);
static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index f85dde9..fd32e9a 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
}
static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
- in_voltage-voltage_thresh_low_value,
+ "in_voltage-voltage_thresh_low_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
AD7280A_CELL_UNDERVOLTAGE);
static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
- in_voltage-voltage_thresh_high_value,
+ "in_voltage-voltage_thresh_high_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index ce9426c..49c81a4 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -55,6 +55,10 @@ struct iio_const_attr {
{ .dev_attr = __ATTR(_name, _mode, _show, _store), \
.address = _addr }
+#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
+ { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \
+ .address = _addr }
+
#define IIO_ATTR_RO(_name, _addr) \
{ .dev_attr = __ATTR_RO(_name), \
.address = _addr }
@@ -85,7 +89,7 @@ struct iio_const_attr {
#define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
struct iio_dev_attr iio_dev_attr_##_vname \
- = IIO_ATTR(_name, _mode, _show, _store, _addr)
+ = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
#define IIO_CONST_ATTR(_name, _string) \
struct iio_const_attr iio_const_attr_##_name \
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index aa02c32..20321cf 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -104,6 +104,13 @@ struct attribute_group {
.store = _store, \
}
+#define __ATTR_NAMED(_name, _mode, _show, _store) { \
+ .attr = {.name = _name, \
+ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
+ .show = _show, \
+ .store = _store, \
+}
+
#define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
.attr = {.name = __stringify(_name), \
.mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
--
1.9.1
[toc] | [next] | [standalone]
| From | Daniel Baluta <daniel.baluta@gmail.com> |
|---|---|
| Date | 2017-09-12 10:00 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoOgW-6V5-5@gated-at.bofh.it> |
| In reply to | #1730574 |
Hi Himanshi,
On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
> string instead of implicit conversion to string using
> preprocessors. Added double quotes around names in
> existing usage of IIO_DEVICE_ATTR_NAMED.
Always use imperative mood in commit subject (Fix instead of Fixed).
The subject should contain a tag, which describes the subsytem/files affected.
I would split this patch into:
1) sysfs: iio: Introduce *_ATTR_NAMED
and explain here why do we need __ATTR_NAMED and IIO_ATTR_NAMED
2) iio: Use new IIO_DEVICE_ATTR_NAMED API
But of course, lets wait to see Lars and Jonathan's opinions.
thanks,
Daniel.
>
> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> ---
> drivers/iio/adc/ad7793.c | 2 +-
> drivers/staging/iio/adc/ad7192.c | 2 +-
> drivers/staging/iio/adc/ad7280a.c | 4 ++--
> include/linux/iio/sysfs.h | 6 +++++-
> include/linux/sysfs.h | 7 +++++++
> 5 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
> index e6706a0..d74e324 100644
> --- a/drivers/iio/adc/ad7793.c
> +++ b/drivers/iio/adc/ad7793.c
> @@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev,
> }
>
> static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
> - in_voltage-voltage_scale_available, S_IRUGO,
> + "in_voltage-voltage_scale_available", S_IRUGO,
> ad7793_show_scale_available, NULL, 0);
>
> static struct attribute *ad7793_attributes[] = {
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> index d11c6de..daff38c 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> }
>
> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> - in_voltage-voltage_scale_available,
> + "in_voltage-voltage_scale_available",
> 0444, ad7192_show_scale_available, NULL, 0);
>
> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index f85dde9..fd32e9a 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
> }
>
> static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> - in_voltage-voltage_thresh_low_value,
> + "in_voltage-voltage_thresh_low_value",
> 0644,
> ad7280_read_channel_config,
> ad7280_write_channel_config,
> AD7280A_CELL_UNDERVOLTAGE);
>
> static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
> - in_voltage-voltage_thresh_high_value,
> + "in_voltage-voltage_thresh_high_value",
> 0644,
> ad7280_read_channel_config,
> ad7280_write_channel_config,
> diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> index ce9426c..49c81a4 100644
> --- a/include/linux/iio/sysfs.h
> +++ b/include/linux/iio/sysfs.h
> @@ -55,6 +55,10 @@ struct iio_const_attr {
> { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> .address = _addr }
>
> +#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
> + { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \
> + .address = _addr }
> +
> #define IIO_ATTR_RO(_name, _addr) \
> { .dev_attr = __ATTR_RO(_name), \
> .address = _addr }
> @@ -85,7 +89,7 @@ struct iio_const_attr {
>
> #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
> struct iio_dev_attr iio_dev_attr_##_vname \
> - = IIO_ATTR(_name, _mode, _show, _store, _addr)
> + = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
>
> #define IIO_CONST_ATTR(_name, _string) \
> struct iio_const_attr iio_const_attr_##_name \
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index aa02c32..20321cf 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -104,6 +104,13 @@ struct attribute_group {
> .store = _store, \
> }
>
> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \
> + .attr = {.name = _name, \
> + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> + .show = _show, \
> + .store = _store, \
> +}
> +
> #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
> .attr = {.name = __stringify(_name), \
> .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170911224348.GA13259%40himanshi-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-09-12 10:00 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoOgX-6V5-15@gated-at.bofh.it> |
| In reply to | #1730723 |
On Tue, 12 Sep 2017, Daniel Baluta wrote:
> Hi Himanshi,
>
> On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> > Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
> > string instead of implicit conversion to string using
> > preprocessors. Added double quotes around names in
> > existing usage of IIO_DEVICE_ATTR_NAMED.
>
> Always use imperative mood in commit subject (Fix instead of Fixed).
I haven't had a chance to look at the patch in detail, but try also for
something more descriptive than "fix". Rewrite could be good in this
case.
julia
>
> The subject should contain a tag, which describes the subsytem/files affected.
>
> I would split this patch into:
>
> 1) sysfs: iio: Introduce *_ATTR_NAMED
>
> and explain here why do we need __ATTR_NAMED and IIO_ATTR_NAMED
>
> 2) iio: Use new IIO_DEVICE_ATTR_NAMED API
>
> But of course, lets wait to see Lars and Jonathan's opinions.
> thanks,
> Daniel.
>
> >
> > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> > ---
> > drivers/iio/adc/ad7793.c | 2 +-
> > drivers/staging/iio/adc/ad7192.c | 2 +-
> > drivers/staging/iio/adc/ad7280a.c | 4 ++--
> > include/linux/iio/sysfs.h | 6 +++++-
> > include/linux/sysfs.h | 7 +++++++
> > 5 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
> > index e6706a0..d74e324 100644
> > --- a/drivers/iio/adc/ad7793.c
> > +++ b/drivers/iio/adc/ad7793.c
> > @@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev,
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
> > - in_voltage-voltage_scale_available, S_IRUGO,
> > + "in_voltage-voltage_scale_available", S_IRUGO,
> > ad7793_show_scale_available, NULL, 0);
> >
> > static struct attribute *ad7793_attributes[] = {
> > diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> > index d11c6de..daff38c 100644
> > --- a/drivers/staging/iio/adc/ad7192.c
> > +++ b/drivers/staging/iio/adc/ad7192.c
> > @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> > - in_voltage-voltage_scale_available,
> > + "in_voltage-voltage_scale_available",
> > 0444, ad7192_show_scale_available, NULL, 0);
> >
> > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > index f85dde9..fd32e9a 100644
> > --- a/drivers/staging/iio/adc/ad7280a.c
> > +++ b/drivers/staging/iio/adc/ad7280a.c
> > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> > - in_voltage-voltage_thresh_low_value,
> > + "in_voltage-voltage_thresh_low_value",
> > 0644,
> > ad7280_read_channel_config,
> > ad7280_write_channel_config,
> > AD7280A_CELL_UNDERVOLTAGE);
> >
> > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
> > - in_voltage-voltage_thresh_high_value,
> > + "in_voltage-voltage_thresh_high_value",
> > 0644,
> > ad7280_read_channel_config,
> > ad7280_write_channel_config,
> > diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> > index ce9426c..49c81a4 100644
> > --- a/include/linux/iio/sysfs.h
> > +++ b/include/linux/iio/sysfs.h
> > @@ -55,6 +55,10 @@ struct iio_const_attr {
> > { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> > .address = _addr }
> >
> > +#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
> > + { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \
> > + .address = _addr }
> > +
> > #define IIO_ATTR_RO(_name, _addr) \
> > { .dev_attr = __ATTR_RO(_name), \
> > .address = _addr }
> > @@ -85,7 +89,7 @@ struct iio_const_attr {
> >
> > #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
> > struct iio_dev_attr iio_dev_attr_##_vname \
> > - = IIO_ATTR(_name, _mode, _show, _store, _addr)
> > + = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
> >
> > #define IIO_CONST_ATTR(_name, _string) \
> > struct iio_const_attr iio_const_attr_##_name \
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index aa02c32..20321cf 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -104,6 +104,13 @@ struct attribute_group {
> > .store = _store, \
> > }
> >
> > +#define __ATTR_NAMED(_name, _mode, _show, _store) { \
> > + .attr = {.name = _name, \
> > + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> > + .show = _show, \
> > + .store = _store, \
> > +}
> > +
> > #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
> > .attr = {.name = __stringify(_name), \
> > .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
> > --
> > 1.9.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170911224348.GA13259%40himanshi-Inspiron-5558.
> > For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAEnQRZDgXj85OTQAoEpij_3bSUnmcx1owNyeg10WCmiBGURURg%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-09-12 20:10 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoXNf-4I9-5@gated-at.bofh.it> |
| In reply to | #1730723 |
On Tue, 12 Sep 2017, himanshi wrote:
> Thanks for the review Daniel! I will change the imperative mood for the commit
> message once the other changes are finalised too and as suggested by Julia,
> would try to make the description specific than general.
>
> I tried to think of adding subsystem to the commit subject but could not
> conclude any because of the files involved.
> I like the idea of splitting the patch into 2 as you suggested but I
> have a doubt that adding the new MACROS to different sysfs files can be put into 1
> patch with the subsystem you mentioned but changing the existing
> IIO_DEVICE_ATTR_NAMED to use IIO_ATTR_NAMED (sysfs file again) would be included
> in the second patch if I am not wrong. So would it be fine to keep the
> subsystem as iio for the second patch?
Indeed, the kernel has to compile after every commit. Unless you change
the name of the macro, to allow the old and new versions to co-exist, it
seems hard to break up such a patch.
julia
>
> On Tue, Sep 12, 2017 at 10:53:26AM +0300, Daniel Baluta wrote:
> > Hi Himanshi,
> >
> > On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> > > Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
> > > string instead of implicit conversion to string using
> > > preprocessors. Added double quotes around names in
> > > existing usage of IIO_DEVICE_ATTR_NAMED.
> >
> > Always use imperative mood in commit subject (Fix instead of Fixed).
> >
> > The subject should contain a tag, which describes the subsytem/files affected.
> >
> > I would split this patch into:
> >
> > 1) sysfs: iio: Introduce *_ATTR_NAMED
> >
> > and explain here why do we need __ATTR_NAMED and IIO_ATTR_NAMED
> >
> > 2) iio: Use new IIO_DEVICE_ATTR_NAMED API
> >
> > But of course, lets wait to see Lars and Jonathan's opinions.
> > thanks,
> > Daniel.
> >
> > >
> > > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> > > ---
> > > drivers/iio/adc/ad7793.c | 2 +-
> > > drivers/staging/iio/adc/ad7192.c | 2 +-
> > > drivers/staging/iio/adc/ad7280a.c | 4 ++--
> > > include/linux/iio/sysfs.h | 6 +++++-
> > > include/linux/sysfs.h | 7 +++++++
> > > 5 files changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
> > > index e6706a0..d74e324 100644
> > > --- a/drivers/iio/adc/ad7793.c
> > > +++ b/drivers/iio/adc/ad7793.c
> > > @@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev,
> > > }
> > >
> > > static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
> > > - in_voltage-voltage_scale_available, S_IRUGO,
> > > + "in_voltage-voltage_scale_available", S_IRUGO,
> > > ad7793_show_scale_available, NULL, 0);
> > >
> > > static struct attribute *ad7793_attributes[] = {
> > > diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> > > index d11c6de..daff38c 100644
> > > --- a/drivers/staging/iio/adc/ad7192.c
> > > +++ b/drivers/staging/iio/adc/ad7192.c
> > > @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> > > }
> > >
> > > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> > > - in_voltage-voltage_scale_available,
> > > + "in_voltage-voltage_scale_available",
> > > 0444, ad7192_show_scale_available, NULL, 0);
> > >
> > > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > > index f85dde9..fd32e9a 100644
> > > --- a/drivers/staging/iio/adc/ad7280a.c
> > > +++ b/drivers/staging/iio/adc/ad7280a.c
> > > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
> > > }
> > >
> > > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> > > - in_voltage-voltage_thresh_low_value,
> > > + "in_voltage-voltage_thresh_low_value",
> > > 0644,
> > > ad7280_read_channel_config,
> > > ad7280_write_channel_config,
> > > AD7280A_CELL_UNDERVOLTAGE);
> > >
> > > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
> > > - in_voltage-voltage_thresh_high_value,
> > > + "in_voltage-voltage_thresh_high_value",
> > > 0644,
> > > ad7280_read_channel_config,
> > > ad7280_write_channel_config,
> > > diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> > > index ce9426c..49c81a4 100644
> > > --- a/include/linux/iio/sysfs.h
> > > +++ b/include/linux/iio/sysfs.h
> > > @@ -55,6 +55,10 @@ struct iio_const_attr {
> > > { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> > > .address = _addr }
> > >
> > > +#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
> > > + { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \
> > > + .address = _addr }
> > > +
> > > #define IIO_ATTR_RO(_name, _addr) \
> > > { .dev_attr = __ATTR_RO(_name), \
> > > .address = _addr }
> > > @@ -85,7 +89,7 @@ struct iio_const_attr {
> > >
> > > #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
> > > struct iio_dev_attr iio_dev_attr_##_vname \
> > > - = IIO_ATTR(_name, _mode, _show, _store, _addr)
> > > + = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
> > >
> > > #define IIO_CONST_ATTR(_name, _string) \
> > > struct iio_const_attr iio_const_attr_##_name \
> > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > > index aa02c32..20321cf 100644
> > > --- a/include/linux/sysfs.h
> > > +++ b/include/linux/sysfs.h
> > > @@ -104,6 +104,13 @@ struct attribute_group {
> > > .store = _store, \
> > > }
> > >
> > > +#define __ATTR_NAMED(_name, _mode, _show, _store) { \
> > > + .attr = {.name = _name, \
> > > + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> > > + .show = _show, \
> > > + .store = _store, \
> > > +}
> > > +
> > > #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
> > > .attr = {.name = __stringify(_name), \
> > > .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
> > > --
> > > 1.9.1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170911224348.GA13259%40himanshi-Inspiron-5558.
> > > For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170912180132.GB4607%40himanshi-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
>
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2017-09-12 20:20 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoXWW-4LY-13@gated-at.bofh.it> |
| In reply to | #1731131 |
On 09/12/2017 08:06 PM, Julia Lawall wrote: > > > On Tue, 12 Sep 2017, himanshi wrote: > >> Thanks for the review Daniel! I will change the imperative mood for the commit >> message once the other changes are finalised too and as suggested by Julia, >> would try to make the description specific than general. >> >> I tried to think of adding subsystem to the commit subject but could not >> conclude any because of the files involved. >> I like the idea of splitting the patch into 2 as you suggested but I >> have a doubt that adding the new MACROS to different sysfs files can be put into 1 >> patch with the subsystem you mentioned but changing the existing >> IIO_DEVICE_ATTR_NAMED to use IIO_ATTR_NAMED (sysfs file again) would be included >> in the second patch if I am not wrong. So would it be fine to keep the >> subsystem as iio for the second patch? > > Indeed, the kernel has to compile after every commit. Unless you change > the name of the macro, to allow the old and new versions to co-exist, it > seems hard to break up such a patch. We can still split things into two parts. One patch introducing __ATTR_NAMED in the device driver core and then another patch making use of that macro in the IIO subsystem.
[toc] | [prev] | [next] | [standalone]
| From | himanshi <himshijain.hj@gmail.com> |
|---|---|
| Date | 2017-09-12 20:40 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoYgi-4Sp-11@gated-at.bofh.it> |
| In reply to | #1731141 |
Okay. This way with each commit the version can compile and I will be able to define the subsystem appropriately. Thank you for the clarification Lars. On Tue, Sep 12, 2017 at 08:14:18PM +0200, Lars-Peter Clausen wrote: > On 09/12/2017 08:06 PM, Julia Lawall wrote: > > > > > > On Tue, 12 Sep 2017, himanshi wrote: > > > >> Thanks for the review Daniel! I will change the imperative mood for the commit > >> message once the other changes are finalised too and as suggested by Julia, > >> would try to make the description specific than general. > >> > >> I tried to think of adding subsystem to the commit subject but could not > >> conclude any because of the files involved. > >> I like the idea of splitting the patch into 2 as you suggested but I > >> have a doubt that adding the new MACROS to different sysfs files can be put into 1 > >> patch with the subsystem you mentioned but changing the existing > >> IIO_DEVICE_ATTR_NAMED to use IIO_ATTR_NAMED (sysfs file again) would be included > >> in the second patch if I am not wrong. So would it be fine to keep the > >> subsystem as iio for the second patch? > > > > Indeed, the kernel has to compile after every commit. Unless you change > > the name of the macro, to allow the old and new versions to co-exist, it > > seems hard to break up such a patch. > > We can still split things into two parts. One patch introducing __ATTR_NAMED > in the device driver core and then another patch making use of that macro in > the IIO subsystem.
[toc] | [prev] | [next] | [standalone]
| From | himanshi <himshijain.hj@gmail.com> |
|---|---|
| Date | 2017-09-12 20:10 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoXNf-4I9-7@gated-at.bofh.it> |
| In reply to | #1730723 |
Thanks for the review Daniel! I will change the imperative mood for the commit
message once the other changes are finalised too and as suggested by Julia,
would try to make the description specific than general.
I tried to think of adding subsystem to the commit subject but could not
conclude any because of the files involved.
I like the idea of splitting the patch into 2 as you suggested but I
have a doubt that adding the new MACROS to different sysfs files can be put into 1
patch with the subsystem you mentioned but changing the existing
IIO_DEVICE_ATTR_NAMED to use IIO_ATTR_NAMED (sysfs file again) would be included
in the second patch if I am not wrong. So would it be fine to keep the
subsystem as iio for the second patch?
On Tue, Sep 12, 2017 at 10:53:26AM +0300, Daniel Baluta wrote:
> Hi Himanshi,
>
> On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> > Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
> > string instead of implicit conversion to string using
> > preprocessors. Added double quotes around names in
> > existing usage of IIO_DEVICE_ATTR_NAMED.
>
> Always use imperative mood in commit subject (Fix instead of Fixed).
>
> The subject should contain a tag, which describes the subsytem/files affected.
>
> I would split this patch into:
>
> 1) sysfs: iio: Introduce *_ATTR_NAMED
>
> and explain here why do we need __ATTR_NAMED and IIO_ATTR_NAMED
>
> 2) iio: Use new IIO_DEVICE_ATTR_NAMED API
>
> But of course, lets wait to see Lars and Jonathan's opinions.
> thanks,
> Daniel.
>
> >
> > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> > ---
> > drivers/iio/adc/ad7793.c | 2 +-
> > drivers/staging/iio/adc/ad7192.c | 2 +-
> > drivers/staging/iio/adc/ad7280a.c | 4 ++--
> > include/linux/iio/sysfs.h | 6 +++++-
> > include/linux/sysfs.h | 7 +++++++
> > 5 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
> > index e6706a0..d74e324 100644
> > --- a/drivers/iio/adc/ad7793.c
> > +++ b/drivers/iio/adc/ad7793.c
> > @@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev,
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
> > - in_voltage-voltage_scale_available, S_IRUGO,
> > + "in_voltage-voltage_scale_available", S_IRUGO,
> > ad7793_show_scale_available, NULL, 0);
> >
> > static struct attribute *ad7793_attributes[] = {
> > diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> > index d11c6de..daff38c 100644
> > --- a/drivers/staging/iio/adc/ad7192.c
> > +++ b/drivers/staging/iio/adc/ad7192.c
> > @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> > - in_voltage-voltage_scale_available,
> > + "in_voltage-voltage_scale_available",
> > 0444, ad7192_show_scale_available, NULL, 0);
> >
> > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > index f85dde9..fd32e9a 100644
> > --- a/drivers/staging/iio/adc/ad7280a.c
> > +++ b/drivers/staging/iio/adc/ad7280a.c
> > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
> > }
> >
> > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> > - in_voltage-voltage_thresh_low_value,
> > + "in_voltage-voltage_thresh_low_value",
> > 0644,
> > ad7280_read_channel_config,
> > ad7280_write_channel_config,
> > AD7280A_CELL_UNDERVOLTAGE);
> >
> > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
> > - in_voltage-voltage_thresh_high_value,
> > + "in_voltage-voltage_thresh_high_value",
> > 0644,
> > ad7280_read_channel_config,
> > ad7280_write_channel_config,
> > diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> > index ce9426c..49c81a4 100644
> > --- a/include/linux/iio/sysfs.h
> > +++ b/include/linux/iio/sysfs.h
> > @@ -55,6 +55,10 @@ struct iio_const_attr {
> > { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> > .address = _addr }
> >
> > +#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
> > + { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \
> > + .address = _addr }
> > +
> > #define IIO_ATTR_RO(_name, _addr) \
> > { .dev_attr = __ATTR_RO(_name), \
> > .address = _addr }
> > @@ -85,7 +89,7 @@ struct iio_const_attr {
> >
> > #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
> > struct iio_dev_attr iio_dev_attr_##_vname \
> > - = IIO_ATTR(_name, _mode, _show, _store, _addr)
> > + = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
> >
> > #define IIO_CONST_ATTR(_name, _string) \
> > struct iio_const_attr iio_const_attr_##_name \
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index aa02c32..20321cf 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -104,6 +104,13 @@ struct attribute_group {
> > .store = _store, \
> > }
> >
> > +#define __ATTR_NAMED(_name, _mode, _show, _store) { \
> > + .attr = {.name = _name, \
> > + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> > + .show = _show, \
> > + .store = _store, \
> > +}
> > +
> > #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
> > .attr = {.name = __stringify(_name), \
> > .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
> > --
> > 1.9.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170911224348.GA13259%40himanshi-Inspiron-5558.
> > For more options, visit https://groups.google.com/d/optout.
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2017-09-12 20:20 +0200 |
| Subject | Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names |
| Message-ID | <uoXWW-4LY-17@gated-at.bofh.it> |
| In reply to | #1730723 |
On 09/12/2017 09:53 AM, Daniel Baluta wrote: > Hi Himanshi, > > On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain <himshijain.hj@gmail.com> wrote: >> Fixed IIO_DEVICE_ATTR_NAMED API to take name as a >> string instead of implicit conversion to string using >> preprocessors. Added double quotes around names in >> existing usage of IIO_DEVICE_ATTR_NAMED. > > Always use imperative mood in commit subject (Fix instead of Fixed). > > The subject should contain a tag, which describes the subsytem/files affected. > > I would split this patch into: > > 1) sysfs: iio: Introduce *_ATTR_NAMED > > and explain here why do we need __ATTR_NAMED and IIO_ATTR_NAMED > > 2) iio: Use new IIO_DEVICE_ATTR_NAMED API > > But of course, lets wait to see Lars and Jonathan's opinions. Fully agreed with what you wrote.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web