On Tue, 14 Nov 2023 22:05:33 +0200
Alisa-Dariana Roman <[email protected]> wrote:
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
>
> The default configuration is hardcoded in order to have a stable number
> of channels.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
Not directly related to this patch (which looks fine to me)
but any idea why 3db_frequency_available is not using read_avail?
Seems sensible to convert it over given all the other cases are using that
and it will allow dropping at least some of the attributes infrastructure
for some devices.
Random aside on the fact that we should be able to do cleanup.h magic
to deal with the fwnode_handle_put() in error paths. +CC Peter Z, Andy S and Rafael,
mostly so they can shout if someone has already done it. That would avoid one
of our more common bugs with property handling and drop a bunch of lines in
every driver looping over fwnodes.
(my lore search foo isn't finding anything).
I have a bunch of other cleanup.h stuff to send out, but if no one points
out a nasty flaw, I'll circle back to this one in a week or two.
Jonathan
> ---
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 48e0357564af..0532678ad665 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
...
> +static int ad7192_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct ad7192_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->sd.spi->dev;
> + struct fwnode_handle *child;
Not specific to this driver, but someone (maybe me if I ever get
time) should look at a cleanup.h class for fwnode_handle. Should be easy to do
and would get rid of all the manual fwnode_handle_put calls once and for all!
Would look something like (completely untested)
DEFINE_FREE(fwnode_handle_put, struct fwnode_handle, if (_T) fwnode_handle_put(_T));
struct fwnode_handle __free(fwnode_handle_put) *child = NULL;
minor benefit in this driver, but much larger in others that are parsing lots
of attributes under the fwnode.
> + int ret;
> +
> + device_for_each_child_node(dev, child) {
> + ret = ad7192_parse_channel(indio_dev, child);
> + if (ret) {
> + fwnode_handle_put(child);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int ad7192_probe(struct spi_device *spi)
> {
> struct ad7192_state *st;
> @@ -1150,6 +1282,12 @@ static int ad7192_probe(struct spi_device *spi)
> }
> }
>
> + if (st->chip_info->chip_id == CHIPID_AD7194) {
You match on 7194, then call a function named ad7192_xxx
which with that name we'd expect to apply to either the
ad7192 or to all parts in this driver.
So that function is not well named- it's find to rename it
to ad7194_parse_channels(). However...
> + ret = ad7192_parse_channels(indio_dev);
I'd prefer to see this done via a callback in chip_info
if (st->chip_info->channel_parse) {
ret = st->chip_info->channel_parse(indio_dev);
...
> + if (ret)
> + return ret;
> + }
> +
> ret = ad7192_setup(indio_dev);
> if (ret)
> return ret;
> @@ -1161,6 +1299,7 @@ static const struct of_device_id ad7192_of_match[] = {
> { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> + { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
> { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> @@ -1170,6 +1309,7 @@ static const struct spi_device_id ad7192_ids[] = {
> { "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
> { "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
> { "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
> + { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
> { "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> @@ -1186,6 +1326,6 @@ static struct spi_driver ad7192_driver = {
> module_spi_driver(ad7192_driver);
>
> MODULE_AUTHOR("Michael Hennerich <[email protected]>");
> -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
> +MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7194, AD7195 ADC");
Maybe time to switch to 'and similar' here.
Thanks,
Jonathan
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
On 26.11.2023 18:34, Jonathan Cameron wrote:
> On Tue, 14 Nov 2023 22:05:33 +0200
> Alisa-Dariana Roman <[email protected]> wrote:
>
>> Unlike the other AD719Xs, AD7194 has configurable differential
>> channels. The default configuration for these channels can be changed
>> from the devicetree.
>>
>> The default configuration is hardcoded in order to have a stable number
>> of channels.
>>
>> Also modify config AD7192 description for better scaling.
>>
>> Signed-off-by: Alisa-Dariana Roman <[email protected]>
>
> Not directly related to this patch (which looks fine to me)
> but any idea why 3db_frequency_available is not using read_avail?
>
> Seems sensible to convert it over given all the other cases are using that
> and it will allow dropping at least some of the attributes infrastructure
> for some devices.
Thank you very much for the feedback!
I actually tried then to use read_avail for the 3db frequencies, but it
required a greater rework. If I remember correctly, the four possible
frequency choices need to be stored in the ad7192_state for it to work.
Should I add a patch with these changes?
Kind regards,
Alisa-Dariana Roman
On Fri, 2 Feb 2024 16:32:14 +0200
Alisa-Dariana Roman <[email protected]> wrote:
> On 26.11.2023 18:34, Jonathan Cameron wrote:
> > On Tue, 14 Nov 2023 22:05:33 +0200
> > Alisa-Dariana Roman <[email protected]> wrote:
> >
> >> Unlike the other AD719Xs, AD7194 has configurable differential
> >> channels. The default configuration for these channels can be changed
> >> from the devicetree.
> >>
> >> The default configuration is hardcoded in order to have a stable number
> >> of channels.
> >>
> >> Also modify config AD7192 description for better scaling.
> >>
> >> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> >
> > Not directly related to this patch (which looks fine to me)
> > but any idea why 3db_frequency_available is not using read_avail?
> >
> > Seems sensible to convert it over given all the other cases are using that
> > and it will allow dropping at least some of the attributes infrastructure
> > for some devices.
>
> Thank you very much for the feedback!
>
> I actually tried then to use read_avail for the 3db frequencies, but it
> required a greater rework. If I remember correctly, the four possible
> frequency choices need to be stored in the ad7192_state for it to work.
> Should I add a patch with these changes?
>
If will end up a tiny bit more complex than currently because of the need
to stash them in an array rather than simply print the strings directly.
But not a lot and should allow you to get rid of a small amount of other code.
Not urgent, but I think this would be a nice change in the longer term
Thanks,
Jonathan