| Message ID | 20220310003402.490478-4-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] dt-bindings: iio: adc: ti,ads1015: Add missing ADS1115 compatible string | expand |
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: > These macros differ only in the number of valid bits of each ADC sample > and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, > ADS1115 is 16bit ADC shifted by 0. No functional change. > Signed-off-by: Marek Vasut <marex@denx.de> ... > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Daniel Baluta <daniel.baluta@nxp.com> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> You may consider using --cc parameter in `git send-email` to avoid this noise in the commit messages. ... > - .realbits = 12, \ > + .realbits = (_realbits), \ > .storagebits = 16, \ This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits? ... > - .realbits = 16, \ > + .realbits = (_realbits), \ > .storagebits = 16, \ Ditto. I see two options: 1) add static assert to make sure realbits <= storagebits; 2) make it also configurable.
On 3/10/22 15:25, Andy Shevchenko wrote: > On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: >> These macros differ only in the number of valid bits of each ADC sample >> and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, >> ADS1115 is 16bit ADC shifted by 0. No functional change. > >> Signed-off-by: Marek Vasut <marex@denx.de> > > ... > >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> >> Cc: Daniel Baluta <daniel.baluta@nxp.com> >> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > You may consider using --cc parameter in `git send-email` to avoid this noise > in the commit messages. This is deliberate so I can keep track of who to CC on which patch. > ... > >> - .realbits = 12, \ >> + .realbits = (_realbits), \ >> .storagebits = 16, \ > > This seems inconsistent a bit. What if the next chip wants to have more than > 16 bits in realbits? When such a chip exists, this can be parametrized as well. > ... > >> - .realbits = 16, \ >> + .realbits = (_realbits), \ >> .storagebits = 16, \ > > Ditto. > > I see two options: > 1) add static assert to make sure realbits <= storagebits; Does static_assert work in array of structures (I don't think it does) ? > 2) make it also configurable. That would be unnecessary duplication, this patch is trying to DEduplicate the driver code, not REduplicate it differently.
On Fri, Mar 11, 2022 at 1:55 AM Marek Vasut <marex@denx.de> wrote: > On 3/10/22 15:25, Andy Shevchenko wrote: > > On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: ... > >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Cc: Daniel Baluta <daniel.baluta@nxp.com> > >> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > You may consider using --cc parameter in `git send-email` to avoid this noise > > in the commit messages. > > This is deliberate so I can keep track of who to CC on which patch. You may add the Link tag to lore (which `b4` tool can do automatically), so you can always access the email from the archives and track this down. No need to have this in each of the commit messages. ... > >> - .realbits = 12, \ > >> + .realbits = (_realbits), \ > >> .storagebits = 16, \ > > > > This seems inconsistent a bit. What if the next chip wants to have more than > > 16 bits in realbits? > > When such a chip exists, this can be parametrized as well. Yes, My point is that it's error prone. ... > > I see two options: > > 1) add static assert to make sure realbits <= storagebits; > > Does static_assert work in array of structures (I don't think it does) ? You can check, but IIRC some of the macros have it. Don't remember the details, though. > > 2) make it also configurable. > > That would be unnecessary duplication, this patch is trying to > DEduplicate the driver code, not REduplicate it differently.
On 3/11/22 12:29, Andy Shevchenko wrote: > On Fri, Mar 11, 2022 at 1:55 AM Marek Vasut <marex@denx.de> wrote: >> On 3/10/22 15:25, Andy Shevchenko wrote: >>> On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: > > ... > >>>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> >>>> Cc: Daniel Baluta <daniel.baluta@nxp.com> >>>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> >>> You may consider using --cc parameter in `git send-email` to avoid this noise >>> in the commit messages. >> >> This is deliberate so I can keep track of who to CC on which patch. > > You may add the Link tag to lore (which `b4` tool can do > automatically), so you can always access the email from the archives > and track this down. No need to have this in each of the commit > messages. This is used by git-send-email to put the right people on Cc, not by Lore. > ... > >>>> - .realbits = 12, \ >>>> + .realbits = (_realbits), \ >>>> .storagebits = 16, \ >>> >>> This seems inconsistent a bit. What if the next chip wants to have more than >>> 16 bits in realbits? >> >> When such a chip exists, this can be parametrized as well. > > Yes, My point is that it's error prone. Won't IIO core warn if realbits > storagebits ? > ... > >>> I see two options: >>> 1) add static assert to make sure realbits <= storagebits; >> >> Does static_assert work in array of structures (I don't think it does) ? > > You can check, but IIRC some of the macros have it. Don't remember the > details, though. I already checked before replying, hence my question, as I didn't find a way to make it work.
On Fri, Mar 11, 2022 at 12:33:49PM +0100, Marek Vasut wrote: > On 3/11/22 12:29, Andy Shevchenko wrote: > > On Fri, Mar 11, 2022 at 1:55 AM Marek Vasut <marex@denx.de> wrote: > > > On 3/10/22 15:25, Andy Shevchenko wrote: > > > > On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: ... > > > > > - .realbits = 12, \ > > > > > + .realbits = (_realbits), \ > > > > > .storagebits = 16, \ > > > > > > > > This seems inconsistent a bit. What if the next chip wants to have more than > > > > 16 bits in realbits? > > > > > > When such a chip exists, this can be parametrized as well. > > > > Yes, My point is that it's error prone. > > Won't IIO core warn if realbits > storagebits ? If it's the case, then it's very good! ... > > > > I see two options: > > > > 1) add static assert to make sure realbits <= storagebits; > > > > > > Does static_assert work in array of structures (I don't think it does) ? > > > > You can check, but IIRC some of the macros have it. Don't remember the > > details, though. > > I already checked before replying, hence my question, as I didn't find a way > to make it work. It seems that current use cases have it either in functions or in the expressions as ({...}). I dunno if the result of ({...}) can be a data structure or compound literal.
On 3/11/22 18:03, Andy Shevchenko wrote: > On Fri, Mar 11, 2022 at 12:33:49PM +0100, Marek Vasut wrote: >> On 3/11/22 12:29, Andy Shevchenko wrote: >>> On Fri, Mar 11, 2022 at 1:55 AM Marek Vasut <marex@denx.de> wrote: >>>> On 3/10/22 15:25, Andy Shevchenko wrote: >>>>> On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: > > ... > >>>>>> - .realbits = 12, \ >>>>>> + .realbits = (_realbits), \ >>>>>> .storagebits = 16, \ >>>>> >>>>> This seems inconsistent a bit. What if the next chip wants to have more than >>>>> 16 bits in realbits? >>>> >>>> When such a chip exists, this can be parametrized as well. >>> >>> Yes, My point is that it's error prone. >> >> Won't IIO core warn if realbits > storagebits ? > > If it's the case, then it's very good! No, apparently it won't . > ... > >>>>> I see two options: >>>>> 1) add static assert to make sure realbits <= storagebits; >>>> >>>> Does static_assert work in array of structures (I don't think it does) ? >>> >>> You can check, but IIRC some of the macros have it. Don't remember the >>> details, though. >> >> I already checked before replying, hence my question, as I didn't find a way >> to make it work. > > It seems that current use cases have it either in functions or in > the expressions as ({...}). I dunno if the result of ({...}) can be > a data structure or compound literal. I added a patch to v2, but ugh, it isn't nice: [PATCH v2 7/7] iio: adc: ti-ads1015: Add static assert to test if shifted realbits fit into storagebits
On Fri, 11 Mar 2022 19:51:56 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/11/22 18:03, Andy Shevchenko wrote: > > On Fri, Mar 11, 2022 at 12:33:49PM +0100, Marek Vasut wrote: > >> On 3/11/22 12:29, Andy Shevchenko wrote: > >>> On Fri, Mar 11, 2022 at 1:55 AM Marek Vasut <marex@denx.de> wrote: > >>>> On 3/10/22 15:25, Andy Shevchenko wrote: > >>>>> On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote: > > > > ... > > > >>>>>> - .realbits = 12, \ > >>>>>> + .realbits = (_realbits), \ > >>>>>> .storagebits = 16, \ > >>>>> > >>>>> This seems inconsistent a bit. What if the next chip wants to have more than > >>>>> 16 bits in realbits? > >>>> > >>>> When such a chip exists, this can be parametrized as well. > >>> > >>> Yes, My point is that it's error prone. > >> > >> Won't IIO core warn if realbits > storagebits ? > > > > If it's the case, then it's very good! > > No, apparently it won't . Easy to add I think and a good idea. Though can only be a runtime check obviously. Put a verification check in iio_buffer_add_channel_sys() which is registering the _type attr used to get access to this info from user space. Jonathan > > > ... > > > >>>>> I see two options: > >>>>> 1) add static assert to make sure realbits <= storagebits; > >>>> > >>>> Does static_assert work in array of structures (I don't think it does) ? > >>> > >>> You can check, but IIRC some of the macros have it. Don't remember the > >>> details, though. > >> > >> I already checked before replying, hence my question, as I didn't find a way > >> to make it work. > > > > It seems that current use cases have it either in functions or in > > the expressions as ({...}). I dunno if the result of ({...}) can be > > a data structure or compound literal. > > I added a patch to v2, but ugh, it isn't nice: > > [PATCH v2 7/7] iio: adc: ti-ads1015: Add static assert to test if > shifted realbits fit into storagebits
diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c index 85932b9dc166a..fc3381ff34710 100644 --- a/drivers/iio/adc/ti-ads1015.c +++ b/drivers/iio/adc/ti-ads1015.c @@ -134,7 +134,7 @@ static const struct iio_event_spec ads1015_events[] = { }, }; -#define ADS1015_V_CHAN(_chan, _addr) { \ +#define ADS1015_V_CHAN(_chan, _addr, _realbits, _shift) { \ .type = IIO_VOLTAGE, \ .indexed = 1, \ .address = _addr, \ @@ -145,9 +145,9 @@ static const struct iio_event_spec ads1015_events[] = { .scan_index = _addr, \ .scan_type = { \ .sign = 's', \ - .realbits = 12, \ + .realbits = (_realbits), \ .storagebits = 16, \ - .shift = 4, \ + .shift = (_shift), \ .endianness = IIO_CPU, \ }, \ .event_spec = ads1015_events, \ @@ -155,7 +155,7 @@ static const struct iio_event_spec ads1015_events[] = { .datasheet_name = "AIN"#_chan, \ } -#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr) { \ +#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr, _realbits, _shift) { \ .type = IIO_VOLTAGE, \ .differential = 1, \ .indexed = 1, \ @@ -168,51 +168,9 @@ static const struct iio_event_spec ads1015_events[] = { .scan_index = _addr, \ .scan_type = { \ .sign = 's', \ - .realbits = 12, \ - .storagebits = 16, \ - .shift = 4, \ - .endianness = IIO_CPU, \ - }, \ - .event_spec = ads1015_events, \ - .num_event_specs = ARRAY_SIZE(ads1015_events), \ - .datasheet_name = "AIN"#_chan"-AIN"#_chan2, \ -} - -#define ADS1115_V_CHAN(_chan, _addr) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .address = _addr, \ - .channel = _chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_SAMP_FREQ), \ - .scan_index = _addr, \ - .scan_type = { \ - .sign = 's', \ - .realbits = 16, \ - .storagebits = 16, \ - .endianness = IIO_CPU, \ - }, \ - .event_spec = ads1015_events, \ - .num_event_specs = ARRAY_SIZE(ads1015_events), \ - .datasheet_name = "AIN"#_chan, \ -} - -#define ADS1115_V_DIFF_CHAN(_chan, _chan2, _addr) { \ - .type = IIO_VOLTAGE, \ - .differential = 1, \ - .indexed = 1, \ - .address = _addr, \ - .channel = _chan, \ - .channel2 = _chan2, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_SAMP_FREQ), \ - .scan_index = _addr, \ - .scan_type = { \ - .sign = 's', \ - .realbits = 16, \ + .realbits = (_realbits), \ .storagebits = 16, \ + .shift = (_shift), \ .endianness = IIO_CPU, \ }, \ .event_spec = ads1015_events, \ @@ -290,26 +248,26 @@ static const struct regmap_config ads1015_regmap_config = { }; static const struct iio_chan_spec ads1015_channels[] = { - ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1), - ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3), - ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3), - ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3), - ADS1015_V_CHAN(0, ADS1015_AIN0), - ADS1015_V_CHAN(1, ADS1015_AIN1), - ADS1015_V_CHAN(2, ADS1015_AIN2), - ADS1015_V_CHAN(3, ADS1015_AIN3), + ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4), + ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 12, 4), + ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 12, 4), + ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 12, 4), + ADS1015_V_CHAN(0, ADS1015_AIN0, 12, 4), + ADS1015_V_CHAN(1, ADS1015_AIN1, 12, 4), + ADS1015_V_CHAN(2, ADS1015_AIN2, 12, 4), + ADS1015_V_CHAN(3, ADS1015_AIN3, 12, 4), IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP), }; static const struct iio_chan_spec ads1115_channels[] = { - ADS1115_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1), - ADS1115_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3), - ADS1115_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3), - ADS1115_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3), - ADS1115_V_CHAN(0, ADS1015_AIN0), - ADS1115_V_CHAN(1, ADS1015_AIN1), - ADS1115_V_CHAN(2, ADS1015_AIN2), - ADS1115_V_CHAN(3, ADS1015_AIN3), + ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 16, 0), + ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 16, 0), + ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 16, 0), + ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 16, 0), + ADS1015_V_CHAN(0, ADS1015_AIN0, 16, 0), + ADS1015_V_CHAN(1, ADS1015_AIN1, 16, 0), + ADS1015_V_CHAN(2, ADS1015_AIN2, 16, 0), + ADS1015_V_CHAN(3, ADS1015_AIN3, 16, 0), IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP), };
These macros differ only in the number of valid bits of each ADC sample and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, ADS1115 is 16bit ADC shifted by 0. No functional change. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Baluta <daniel.baluta@nxp.com> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- drivers/iio/adc/ti-ads1015.c | 86 +++++++++--------------------------- 1 file changed, 22 insertions(+), 64 deletions(-)