| Message ID | 20180830154518.29507-6-embed3d@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO-based thermal sensor driver for Allwinner H3 and A83T SoC | expand |
On Thu, 30 Aug 2018 17:44:53 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > We are moving the SUN4I_GPADC_CHANNEL define to the header file. Maxime has raised this point in other patches... Why? Obvious what but I have no idea why you are doing this. Thanks, Jonathan > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- > include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > index d95dd0fde2a6..666329940e1e 100644 > --- a/drivers/iio/adc/sun4i-gpadc-iio.c > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { > struct device *sensor_device; > }; > > -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > - .type = IIO_VOLTAGE, \ > - .indexed = 1, \ > - .channel = _channel, \ > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > - .datasheet_name = _name, \ > -} > - > static struct iio_map sun4i_gpadc_hwmon_maps[] = { > { > .adc_channel_label = "temp_adc", > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > index 139872c2e0fe..54c7c9375c1b 100644 > --- a/include/linux/mfd/sun4i-gpadc.h > +++ b/include/linux/mfd/sun4i-gpadc.h > @@ -90,6 +90,15 @@ > /* 10s delay before suspending the IP */ > #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > > +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = _name, \ > +} > + > struct sun4i_gpadc_dev { > struct device *dev; > struct regmap *regmap;
On 02.09.2018 22:01, Jonathan Cameron wrote: > On Thu, 30 Aug 2018 17:44:53 +0200 > Philipp Rossak <embed3d@gmail.com> wrote: > >> We are moving the SUN4I_GPADC_CHANNEL define to the header file. > Maxime has raised this point in other patches... > > Why? Obvious what but I have no idea why you are doing this. > > Thanks, > > Jonathan There are two reasons: 1. Personal taste: I like to have the #define stuff in the header file. 2. When I started the rework I had to get some better overview, so I moved it... Since those two reasons are no good reasons to submit a patch I will drop it and keep it in the *.c file. Philipp >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> --- >> drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- >> include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ >> 2 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> index d95dd0fde2a6..666329940e1e 100644 >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { >> struct device *sensor_device; >> }; >> >> -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ >> - .type = IIO_VOLTAGE, \ >> - .indexed = 1, \ >> - .channel = _channel, \ >> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >> - .datasheet_name = _name, \ >> -} >> - >> static struct iio_map sun4i_gpadc_hwmon_maps[] = { >> { >> .adc_channel_label = "temp_adc", >> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h >> index 139872c2e0fe..54c7c9375c1b 100644 >> --- a/include/linux/mfd/sun4i-gpadc.h >> +++ b/include/linux/mfd/sun4i-gpadc.h >> @@ -90,6 +90,15 @@ >> /* 10s delay before suspending the IP */ >> #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 >> >> +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ >> + .type = IIO_VOLTAGE, \ >> + .indexed = 1, \ >> + .channel = _channel, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >> + .datasheet_name = _name, \ >> +} >> + >> struct sun4i_gpadc_dev { >> struct device *dev; >> struct regmap *regmap; >
On Mon, 3 Sep 2018 16:24:32 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > On 02.09.2018 22:01, Jonathan Cameron wrote: > > On Thu, 30 Aug 2018 17:44:53 +0200 > > Philipp Rossak <embed3d@gmail.com> wrote: > > > >> We are moving the SUN4I_GPADC_CHANNEL define to the header file. > > Maxime has raised this point in other patches... > > > > Why? Obvious what but I have no idea why you are doing this. > > > > Thanks, > > > > Jonathan > There are two reasons: > 1. Personal taste: I like to have the #define stuff in the header file. > 2. When I started the rework I had to get some better overview, so I > moved it... > > Since those two reasons are no good reasons to submit a patch I will > drop it and keep it in the *.c file. Don't move it. For a 'utility' type define like this that is just about cutting down on code repetition it is nice to be able to see what it does near to where it is used. Also, as a general rule kernel style is to not put things in a header unless they are needed in multiple files. There are exceptions, but it is generally felt keeping things local to where they are used leads to easier to review code. Jonathan > > Philipp > > >> > >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> > >> --- > >> drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- > >> include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ > >> 2 files changed, 9 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c > >> index d95dd0fde2a6..666329940e1e 100644 > >> --- a/drivers/iio/adc/sun4i-gpadc-iio.c > >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c > >> @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { > >> struct device *sensor_device; > >> }; > >> > >> -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > >> - .type = IIO_VOLTAGE, \ > >> - .indexed = 1, \ > >> - .channel = _channel, \ > >> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > >> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > >> - .datasheet_name = _name, \ > >> -} > >> - > >> static struct iio_map sun4i_gpadc_hwmon_maps[] = { > >> { > >> .adc_channel_label = "temp_adc", > >> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h > >> index 139872c2e0fe..54c7c9375c1b 100644 > >> --- a/include/linux/mfd/sun4i-gpadc.h > >> +++ b/include/linux/mfd/sun4i-gpadc.h > >> @@ -90,6 +90,15 @@ > >> /* 10s delay before suspending the IP */ > >> #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > >> > >> +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ > >> + .type = IIO_VOLTAGE, \ > >> + .indexed = 1, \ > >> + .channel = _channel, \ > >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > >> + .datasheet_name = _name, \ > >> +} > >> + > >> struct sun4i_gpadc_dev { > >> struct device *dev; > >> struct regmap *regmap; > >
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c index d95dd0fde2a6..666329940e1e 100644 --- a/drivers/iio/adc/sun4i-gpadc-iio.c +++ b/drivers/iio/adc/sun4i-gpadc-iio.c @@ -109,15 +109,6 @@ struct sun4i_gpadc_iio { struct device *sensor_device; }; -#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .channel = _channel, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ - .datasheet_name = _name, \ -} - static struct iio_map sun4i_gpadc_hwmon_maps[] = { { .adc_channel_label = "temp_adc", diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h index 139872c2e0fe..54c7c9375c1b 100644 --- a/include/linux/mfd/sun4i-gpadc.h +++ b/include/linux/mfd/sun4i-gpadc.h @@ -90,6 +90,15 @@ /* 10s delay before suspending the IP */ #define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 +#define SUN4I_GPADC_ADC_CHANNEL(_channel, _name) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = _channel, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .datasheet_name = _name, \ +} + struct sun4i_gpadc_dev { struct device *dev; struct regmap *regmap;
We are moving the SUN4I_GPADC_CHANNEL define to the header file. Signed-off-by: Philipp Rossak <embed3d@gmail.com> --- drivers/iio/adc/sun4i-gpadc-iio.c | 9 --------- include/linux/mfd/sun4i-gpadc.h | 9 +++++++++ 2 files changed, 9 insertions(+), 9 deletions(-)