* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 7:53 ` [Outreachy kernel] " Daniel Baluta
@ 2017-09-12 7:55 ` Julia Lawall
2017-09-12 18:01 ` himanshi
2017-09-12 18:12 ` Lars-Peter Clausen
2 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2017-09-12 7:55 UTC (permalink / raw)
To: Daniel Baluta
Cc: Himanshi Jain, outreachy-kernel, Lars-Peter Clausen, Hennerich,
Michael, Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
Greg Kroah-Hartman, linux-iio, driverdev,
Linux Kernel Mailing List, nick.desaulniers
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 7:53 ` [Outreachy kernel] " Daniel Baluta
2017-09-12 7:55 ` Julia Lawall
@ 2017-09-12 18:01 ` himanshi
2017-09-12 18:06 ` Julia Lawall
2017-09-12 18:12 ` Lars-Peter Clausen
2 siblings, 1 reply; 8+ messages in thread
From: himanshi @ 2017-09-12 18:01 UTC (permalink / raw)
To: Daniel Baluta
Cc: outreachy-kernel, Lars-Peter Clausen, Hennerich, Michael,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
Greg Kroah-Hartman, linux-iio, driverdev,
Linux Kernel Mailing List, nick.desaulniers
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 18:01 ` himanshi
@ 2017-09-12 18:06 ` Julia Lawall
2017-09-12 18:14 ` Lars-Peter Clausen
0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2017-09-12 18:06 UTC (permalink / raw)
To: himanshi
Cc: Daniel Baluta, outreachy-kernel, Lars-Peter Clausen, Hennerich,
Michael, Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
Greg Kroah-Hartman, linux-iio, driverdev,
Linux Kernel Mailing List, nick.desaulniers
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 18:06 ` Julia Lawall
@ 2017-09-12 18:14 ` Lars-Peter Clausen
2017-09-12 18:31 ` himanshi
0 siblings, 1 reply; 8+ messages in thread
From: Lars-Peter Clausen @ 2017-09-12 18:14 UTC (permalink / raw)
To: Julia Lawall, himanshi
Cc: Daniel Baluta, outreachy-kernel, Hennerich, Michael,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
Greg Kroah-Hartman, linux-iio, driverdev,
Linux Kernel Mailing List, nick.desaulniers
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 18:14 ` Lars-Peter Clausen
@ 2017-09-12 18:31 ` himanshi
0 siblings, 0 replies; 8+ messages in thread
From: himanshi @ 2017-09-12 18:31 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Julia Lawall, Daniel Baluta, outreachy-kernel, Hennerich,
Michael, Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
Greg Kroah-Hartman, linux-iio, driverdev,
Linux Kernel Mailing List, nick.desaulniers
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names
2017-09-12 7:53 ` [Outreachy kernel] " Daniel Baluta
2017-09-12 7:55 ` Julia Lawall
2017-09-12 18:01 ` himanshi
@ 2017-09-12 18:12 ` Lars-Peter Clausen
2 siblings, 0 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2017-09-12 18:12 UTC (permalink / raw)
To: Daniel Baluta, Himanshi Jain
Cc: outreachy-kernel, Hennerich, Michael, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Greg Kroah-Hartman, linux-iio,
driverdev, Linux Kernel Mailing List, nick.desaulniers
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.
^ permalink raw reply [flat|nested] 8+ messages in thread