| Message ID | 20220320181428.168109-10-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/10] dt-bindings: iio: adc: ti,ads1015: Add missing ADS1115 compatible string | expand |
On Sun, Mar 20, 2022 at 8:14 PM Marek Vasut <marex@denx.de> wrote: > > Replace sysfs attributes with read_avail() callback. This also permits > removal of ads1115_info, since the scale attribute tables are now part > of chip data. ... > +static const int ads1015_fullscale_range[] = { > 6144, 4096, 2048, 1024, 512, 256, 256, 256 Keep a comma at the end. Also applies to the rest of the modified data structures below. > };
On 3/21/22 10:27, Andy Shevchenko wrote: > On Sun, Mar 20, 2022 at 8:14 PM Marek Vasut <marex@denx.de> wrote: >> >> Replace sysfs attributes with read_avail() callback. This also permits >> removal of ads1115_info, since the scale attribute tables are now part >> of chip data. > > ... > >> +static const int ads1015_fullscale_range[] = { >> 6144, 4096, 2048, 1024, 512, 256, 256, 256 > > Keep a comma at the end. > Also applies to the rest of the modified data structures below. Why ? We don't expect these arrays to grow new values, if there is ever some new ADC, it will likely have its own array.
On Mon, Mar 21, 2022 at 03:44:24PM +0100, Marek Vasut wrote: > On 3/21/22 10:27, Andy Shevchenko wrote: > > On Sun, Mar 20, 2022 at 8:14 PM Marek Vasut <marex@denx.de> wrote: ... > > > +static const int ads1015_fullscale_range[] = { > > > 6144, 4096, 2048, 1024, 512, 256, 256, 256 > > > > Keep a comma at the end. > > Also applies to the rest of the modified data structures below. > > Why ? We don't expect these arrays to grow new values, if there is ever some > new ADC, it will likely have its own array. Just for the sake of a common style. But it's not critical, so I leave it to the maintainers.
On 3/21/22 17:19, Andy Shevchenko wrote: > On Mon, Mar 21, 2022 at 03:44:24PM +0100, Marek Vasut wrote: >> On 3/21/22 10:27, Andy Shevchenko wrote: >>> On Sun, Mar 20, 2022 at 8:14 PM Marek Vasut <marex@denx.de> wrote: > > ... > >>>> +static const int ads1015_fullscale_range[] = { >>>> 6144, 4096, 2048, 1024, 512, 256, 256, 256 >>> >>> Keep a comma at the end. >>> Also applies to the rest of the modified data structures below. >> >> Why ? We don't expect these arrays to grow new values, if there is ever some >> new ADC, it will likely have its own array. > > Just for the sake of a common style. But it's not critical, > so I leave it to the maintainers. The common style of the other arrays in that driver is without a trailing comma, hence the way it is right now is the common style. I do however understand the rationale why a trailing comma in structures makes sense (you don't have to add it in some subsequent patch, which does only cause churn and unrelated line changes), but I don't think that applies in this particular case.
On Mon, 21 Mar 2022 20:46:04 +0100 Marek Vasut <marex@denx.de> wrote: > On 3/21/22 17:19, Andy Shevchenko wrote: > > On Mon, Mar 21, 2022 at 03:44:24PM +0100, Marek Vasut wrote: > >> On 3/21/22 10:27, Andy Shevchenko wrote: > >>> On Sun, Mar 20, 2022 at 8:14 PM Marek Vasut <marex@denx.de> wrote: > > > > ... > > > >>>> +static const int ads1015_fullscale_range[] = { > >>>> 6144, 4096, 2048, 1024, 512, 256, 256, 256 > >>> > >>> Keep a comma at the end. > >>> Also applies to the rest of the modified data structures below. > >> > >> Why ? We don't expect these arrays to grow new values, if there is ever some > >> new ADC, it will likely have its own array. > > > > Just for the sake of a common style. But it's not critical, > > so I leave it to the maintainers. > > The common style of the other arrays in that driver is without a > trailing comma, hence the way it is right now is the common style. > > I do however understand the rationale why a trailing comma in structures > makes sense (you don't have to add it in some subsequent patch, which > does only cause churn and unrelated line changes), but I don't think > that applies in this particular case. Agreed. I tend to let this go with local style in a driver for these cases and generally let either go in new drivers as long as they are consistent. Jonathan
On Sun, 20 Mar 2022 19:14:28 +0100 Marek Vasut <marex@denx.de> wrote: > Replace sysfs attributes with read_avail() callback. This also permits > removal of ads1115_info, since the scale attribute tables are now part > of chip data. > > 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> > --- > V3: New patch > --- > drivers/iio/adc/ti-ads1015.c | 102 +++++++++++++++++++---------------- > 1 file changed, 57 insertions(+), 45 deletions(-) > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c > index 66431d1445d9b..b1257f65d7431 100644 > --- a/drivers/iio/adc/ti-ads1015.c > +++ b/drivers/iio/adc/ti-ads1015.c > @@ -81,6 +81,9 @@ struct ads1015_chip_data { > int num_channels; > const struct iio_info *info; > const unsigned int *data_rate; Should probably change this to signed. > + const unsigned int data_rate_len; > + const unsigned int *scale; Why unsigned int given we use it as an array of signed ints? > + const unsigned int scale_len; > bool has_comparator; > }; > > @@ -108,10 +111,18 @@ static const unsigned int ads1115_data_rate[] = { > * Translation from PGA bits to full-scale positive and negative input voltage > * range in mV > */ > -static int ads1015_fullscale_range[] = { > +static const int ads1015_fullscale_range[] = { Technically unrelated but good fix and not hurting patch readability significantly so perhaps not worth a separate patch. > 6144, 4096, 2048, 1024, 512, 256, 256, 256 > }; > > +static const int ads1015_scale[] = { /* 12bit ADC */ > + 256, 11, 512, 11, 1024, 11, 2048, 11, 4096, 11, 6144, 11 I wonder if it's worth either using a 2D array and casting a dimension away, or perhaps just formatting these pair wise so we can see what is going on more obviously? I don't feel strongly about this so up to you. > +}; > + > +static const int ads1115_scale[] = { /* 16bit ADC */ > + 256, 15, 512, 15, 1024, 15, 2048, 15, 4096, 15, 6144, 15 > +};
On 3/22/22 22:15, Jonathan Cameron wrote: [...] >> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c >> index 66431d1445d9b..b1257f65d7431 100644 >> --- a/drivers/iio/adc/ti-ads1015.c >> +++ b/drivers/iio/adc/ti-ads1015.c >> @@ -81,6 +81,9 @@ struct ads1015_chip_data { >> int num_channels; >> const struct iio_info *info; >> const unsigned int *data_rate; > Should probably change this to signed. The data_rate values are unsigned ... why ? >> + const unsigned int data_rate_len; >> + const unsigned int *scale; > > Why unsigned int given we use it as an array of signed ints? Scale is also unsigned ... why ? >> + const unsigned int scale_len; >> bool has_comparator; >> }; >> >> @@ -108,10 +111,18 @@ static const unsigned int ads1115_data_rate[] = { >> * Translation from PGA bits to full-scale positive and negative input voltage >> * range in mV >> */ >> -static int ads1015_fullscale_range[] = { >> +static const int ads1015_fullscale_range[] = { > Technically unrelated but good fix and not hurting patch readability significantly > so perhaps not worth a separate patch. > >> 6144, 4096, 2048, 1024, 512, 256, 256, 256 >> }; >> >> +static const int ads1015_scale[] = { /* 12bit ADC */ >> + 256, 11, 512, 11, 1024, 11, 2048, 11, 4096, 11, 6144, 11 > I wonder if it's worth either using a 2D array and casting > a dimension away, or perhaps just formatting these pair wise > so we can see what is going on more obviously? > > I don't feel strongly about this so up to you. If we are trying to get rid of the type casts, then pair format it is.
On 3/22/22 22:39, Marek Vasut wrote: > On 3/22/22 22:15, Jonathan Cameron wrote: > > [...] > >>> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c >>> index 66431d1445d9b..b1257f65d7431 100644 >>> --- a/drivers/iio/adc/ti-ads1015.c >>> +++ b/drivers/iio/adc/ti-ads1015.c >>> @@ -81,6 +81,9 @@ struct ads1015_chip_data { >>> int num_channels; >>> const struct iio_info *info; >>> const unsigned int *data_rate; >> Should probably change this to signed. > > The data_rate values are unsigned ... why ? > >>> + const unsigned int data_rate_len; >>> + const unsigned int *scale; >> >> Why unsigned int given we use it as an array of signed ints? > > Scale is also unsigned ... why ? Nevermind, I turned all the ranges into unsigned int and sent V4.
diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c index 66431d1445d9b..b1257f65d7431 100644 --- a/drivers/iio/adc/ti-ads1015.c +++ b/drivers/iio/adc/ti-ads1015.c @@ -81,6 +81,9 @@ struct ads1015_chip_data { int num_channels; const struct iio_info *info; const unsigned int *data_rate; + const unsigned int data_rate_len; + const unsigned int *scale; + const unsigned int scale_len; bool has_comparator; }; @@ -108,10 +111,18 @@ static const unsigned int ads1115_data_rate[] = { * Translation from PGA bits to full-scale positive and negative input voltage * range in mV */ -static int ads1015_fullscale_range[] = { +static const int ads1015_fullscale_range[] = { 6144, 4096, 2048, 1024, 512, 256, 256, 256 }; +static const int ads1015_scale[] = { /* 12bit ADC */ + 256, 11, 512, 11, 1024, 11, 2048, 11, 4096, 11, 6144, 11 +}; + +static const int ads1115_scale[] = { /* 16bit ADC */ + 256, 15, 512, 15, 1024, 15, 2048, 15, 4096, 15, 6144, 15 +}; + /* * Translation from COMP_QUE field value to the number of successive readings * exceed the threshold values before an interrupt is generated @@ -166,6 +177,9 @@ static const struct iio_event_spec ads1015_events[] = { .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ .scan_index = _addr, \ .scan_type = { \ .sign = 's', \ @@ -189,6 +203,9 @@ static const struct iio_event_spec ads1015_events[] = { .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ .scan_index = _addr, \ .scan_type = { \ .sign = 's', \ @@ -470,7 +487,7 @@ static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate) { int i; - for (i = 0; i < ARRAY_SIZE(ads1015_data_rate); i++) { + for (i = 0; i < data->chip->data_rate_len; i++) { if (data->chip->data_rate[i] == rate) { data->channel_data[chan].data_rate = i; return 0; @@ -480,6 +497,32 @@ static int ads1015_set_data_rate(struct ads1015_data *data, int chan, int rate) return -EINVAL; } +static int ads1015_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + struct ads1015_data *data = iio_priv(indio_dev); + + if (chan->type != IIO_VOLTAGE) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + *type = IIO_VAL_FRACTIONAL_LOG2; + *vals = data->chip->scale; + *length = data->chip->scale_len; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_SAMP_FREQ: + *type = IIO_VAL_INT; + *vals = data->chip->data_rate; + *length = data->chip->data_rate_len; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + static int ads1015_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -828,60 +871,20 @@ static const struct iio_buffer_setup_ops ads1015_buffer_setup_ops = { .validate_scan_mask = &iio_validate_scan_mask_onehot, }; -static IIO_CONST_ATTR_NAMED(ads1015_scale_available, scale_available, - "3 2 1 0.5 0.25 0.125"); -static IIO_CONST_ATTR_NAMED(ads1115_scale_available, scale_available, - "0.1875 0.125 0.0625 0.03125 0.015625 0.007813"); - -static IIO_CONST_ATTR_NAMED(ads1015_sampling_frequency_available, - sampling_frequency_available, "128 250 490 920 1600 2400 3300"); -static IIO_CONST_ATTR_NAMED(ads1115_sampling_frequency_available, - sampling_frequency_available, "8 16 32 64 128 250 475 860"); - -static struct attribute *ads1015_attributes[] = { - &iio_const_attr_ads1015_scale_available.dev_attr.attr, - &iio_const_attr_ads1015_sampling_frequency_available.dev_attr.attr, - NULL, -}; - -static const struct attribute_group ads1015_attribute_group = { - .attrs = ads1015_attributes, -}; - -static struct attribute *ads1115_attributes[] = { - &iio_const_attr_ads1115_scale_available.dev_attr.attr, - &iio_const_attr_ads1115_sampling_frequency_available.dev_attr.attr, - NULL, -}; - -static const struct attribute_group ads1115_attribute_group = { - .attrs = ads1115_attributes, -}; - static const struct iio_info ads1015_info = { + .read_avail = ads1015_read_avail, .read_raw = ads1015_read_raw, .write_raw = ads1015_write_raw, .read_event_value = ads1015_read_event, .write_event_value = ads1015_write_event, .read_event_config = ads1015_read_event_config, .write_event_config = ads1015_write_event_config, - .attrs = &ads1015_attribute_group, -}; - -static const struct iio_info ads1115_info = { - .read_raw = ads1015_read_raw, - .write_raw = ads1015_write_raw, - .read_event_value = ads1015_read_event, - .write_event_value = ads1015_write_event, - .read_event_config = ads1015_read_event_config, - .write_event_config = ads1015_write_event_config, - .attrs = &ads1115_attribute_group, }; static const struct iio_info tla2024_info = { + .read_avail = ads1015_read_avail, .read_raw = ads1015_read_raw, .write_raw = ads1015_write_raw, - .attrs = &ads1015_attribute_group, }; static int ads1015_client_get_channels_config(struct i2c_client *client) @@ -1131,14 +1134,20 @@ static const struct ads1015_chip_data ads1015_data = { .num_channels = ARRAY_SIZE(ads1015_channels), .info = &ads1015_info, .data_rate = ads1015_data_rate, + .data_rate_len = ARRAY_SIZE(ads1015_data_rate), + .scale = ads1015_scale, + .scale_len = ARRAY_SIZE(ads1015_scale), .has_comparator = true, }; static const struct ads1015_chip_data ads1115_data = { .channels = ads1115_channels, .num_channels = ARRAY_SIZE(ads1115_channels), - .info = &ads1115_info, + .info = &ads1015_info, .data_rate = ads1115_data_rate, + .data_rate_len = ARRAY_SIZE(ads1115_data_rate), + .scale = ads1115_scale, + .scale_len = ARRAY_SIZE(ads1115_scale), .has_comparator = true, }; @@ -1147,6 +1156,9 @@ static const struct ads1015_chip_data tla2024_data = { .num_channels = ARRAY_SIZE(tla2024_channels), .info = &tla2024_info, .data_rate = ads1015_data_rate, + .data_rate_len = ARRAY_SIZE(ads1015_data_rate), + .scale = ads1015_scale, + .scale_len = ARRAY_SIZE(ads1015_scale), .has_comparator = false, };
Replace sysfs attributes with read_avail() callback. This also permits removal of ads1115_info, since the scale attribute tables are now part of chip data. 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> --- V3: New patch --- drivers/iio/adc/ti-ads1015.c | 102 +++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 45 deletions(-)