| Message ID | 20220311184925.99270-7-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/7] dt-bindings: iio: adc: ti,ads1015: Add missing ADS1115 compatible string | expand |
On Fri, Mar 11, 2022 at 8:50 PM Marek Vasut <marex@denx.de> wrote: > > Add compile-time static_assert wrapper to verify that shifted realbits > fit into storagebits. The macro is implemented in a more generic way so > it can be used to verify other values if required. Thanks! I think we may leave it to maintainers to decide if it is worth adding or not.
On 3/12/22 13:19, Andy Shevchenko wrote: > On Fri, Mar 11, 2022 at 8:50 PM Marek Vasut <marex@denx.de> wrote: >> >> Add compile-time static_assert wrapper to verify that shifted realbits >> fit into storagebits. The macro is implemented in a more generic way so >> it can be used to verify other values if required. > > Thanks! I think we may leave it to maintainers to decide if it is > worth adding or not. Right, that's why I placed it as 7/7, since the macro is ... not pretty.
On Sat, 12 Mar 2022 22:28:22 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/12/22 13:19, Andy Shevchenko wrote: > > On Fri, Mar 11, 2022 at 8:50 PM Marek Vasut <marex@denx.de> wrote: > >> > >> Add compile-time static_assert wrapper to verify that shifted realbits > >> fit into storagebits. The macro is implemented in a more generic way so > >> it can be used to verify other values if required. > > > > Thanks! I think we may leave it to maintainers to decide if it is > > worth adding or not. > > Right, that's why I placed it as 7/7, since the macro is ... not pretty. It's ugly but that's all wrapped up in the macro so I'll take it and see what blows up :) Longer term, maybe we'd should add a general runtime check in the IIO core? Not quite so nice as catching at compile time but would catch all such issues the moment anyone actually tries out a driver with whatever device they've just added support for. I'll let this sit a little longer for additional review before picking it up (missed this cycle anyway so lots of time). A few other comments on this driver whilst we are here on things we should cleanup at somepoint. 1) Move over to the read_avail callback rather than having the attribute groups. It may well be slightly more code but then makes them available to in kernel users. Slowly moving all drivers with _available for standard ABI over to the callback is on the todo list but it will take a while... 2) Possibly introduced a static const array of struct ad1015_chip_info { all the per device stuff currently handled in code in probe }; Then simply access that directly using the the device_match_data. Jonathan
On 3/20/22 14:10, Jonathan Cameron wrote: > On Sat, 12 Mar 2022 22:28:22 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 3/12/22 13:19, Andy Shevchenko wrote: >>> On Fri, Mar 11, 2022 at 8:50 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> Add compile-time static_assert wrapper to verify that shifted realbits >>>> fit into storagebits. The macro is implemented in a more generic way so >>>> it can be used to verify other values if required. >>> >>> Thanks! I think we may leave it to maintainers to decide if it is >>> worth adding or not. >> >> Right, that's why I placed it as 7/7, since the macro is ... not pretty. > > It's ugly but that's all wrapped up in the macro so I'll take it and > see what blows up :) > > Longer term, maybe we'd should add a general runtime check in the IIO core? I wonder whether something like that could be made completely generic, but, let's do that in the next step. > Not quite so nice as catching at compile time but would catch all such issues > the moment anyone actually tries out a driver with whatever device they've > just added support for. > > I'll let this sit a little longer for additional review before picking it up > (missed this cycle anyway so lots of time). > > > A few other comments on this driver whilst we are here on things we should > cleanup at somepoint. > 1) Move over to the read_avail callback rather than having the attribute > groups. It may well be slightly more code but then makes them available > to in kernel users. Slowly moving all drivers with _available for standard > ABI over to the callback is on the todo list but it will take a while... > 2) Possibly introduced a static const array of > struct ad1015_chip_info { > all the per device stuff currently handled in code in probe > }; > Then simply access that directly using the the device_match_data. I found a small bug in the tla2024 integration, the attrs assignment is wrong, so I'll send 1 and 2 above alongside V3 shortly.
diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c index dd36fe0a45cba..60b5ccbd49a1c 100644 --- a/drivers/iio/adc/ti-ads1015.c +++ b/drivers/iio/adc/ti-ads1015.c @@ -135,6 +135,28 @@ static const struct iio_event_spec ads1015_events[] = { }, }; +/* + * Compile-time check whether _fitbits can accommodate up to _testbits + * bits. Returns _fitbits on success, fails to compile otherwise. + * + * The test works such that it multiplies constant _fitbits by constant + * double-negation of size of a non-empty structure, i.e. it multiplies + * constant _fitbits by constant 1 in each successful compilation case. + * The non-empty structure may contain C11 _Static_assert(), make use of + * this and place the kernel variant of static assert in there, so that + * it performs the compile-time check for _testbits <= _fitbits. Note + * that it is not possible to directly use static_assert in compound + * statements, hence this convoluted construct. + */ +#define FIT_CHECK(_testbits, _fitbits) \ + ( \ + (_fitbits) * \ + !!sizeof(struct { \ + static_assert((_testbits) <= (_fitbits)); \ + int pad; \ + }) \ + ) + #define ADS1015_V_CHAN(_chan, _addr, _realbits, _shift, _event_spec, _num_event_specs) { \ .type = IIO_VOLTAGE, \ .indexed = 1, \ @@ -147,7 +169,7 @@ static const struct iio_event_spec ads1015_events[] = { .scan_type = { \ .sign = 's', \ .realbits = (_realbits), \ - .storagebits = 16, \ + .storagebits = FIT_CHECK((_realbits) + (_shift), 16), \ .shift = (_shift), \ .endianness = IIO_CPU, \ }, \ @@ -170,7 +192,7 @@ static const struct iio_event_spec ads1015_events[] = { .scan_type = { \ .sign = 's', \ .realbits = (_realbits), \ - .storagebits = 16, \ + .storagebits = FIT_CHECK((_realbits) + (_shift), 16), \ .shift = (_shift), \ .endianness = IIO_CPU, \ }, \
Add compile-time static_assert wrapper to verify that shifted realbits fit into storagebits. The macro is implemented in a more generic way so it can be used to verify other values if required. 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> --- V2: New patch --- drivers/iio/adc/ti-ads1015.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)