| Message ID | 20231105193132.47009-1-alisadariana@gmail.com |
|---|---|
| Headers | show |
| Series | iio: adc: ad7192: Add support for AD7194 | expand |
On 05/11/2023 20:31, alisadariana@gmail.com wrote: > From: Alisa-Dariana Roman <alisa.roman@analog.com> > > Replace of.h and corresponding functions with preferred device specific > functions. > > Also replace of_device_get_match_data function with > spi_get_device_match_data. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- > drivers/iio/adc/ad7192.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c > index adc3cbe92d6e..48e0357564af 100644 > --- a/drivers/iio/adc/ad7192.c > +++ b/drivers/iio/adc/ad7192.c > @@ -17,7 +17,6 @@ > #include <linux/err.h> > #include <linux/sched.h> > #include <linux/delay.h> > -#include <linux/of.h> > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > @@ -364,19 +363,19 @@ static inline bool ad7192_valid_external_frequency(u32 freq) > freq <= AD7192_EXT_FREQ_MHZ_MAX); > } > > -static int ad7192_of_clock_select(struct ad7192_state *st) > +static int ad7192_device_clock_select(struct ad7192_state *st) > { > - struct device_node *np = st->sd.spi->dev.of_node; > + struct device *dev = &st->sd.spi->dev; > unsigned int clock_sel; > > clock_sel = AD7192_CLK_INT; > > /* use internal clock */ > if (!st->mclk) { > - if (of_property_read_bool(np, "adi,int-clock-output-enable")) > + if (device_property_read_bool(dev, "adi,int-clock-output-enable")) > clock_sel = AD7192_CLK_INT_CO; > } else { > - if (of_property_read_bool(np, "adi,clock-xtal")) > + if (device_property_read_bool(dev, "adi,clock-xtal")) > clock_sel = AD7192_CLK_EXT_MCLK1_2; > else > clock_sel = AD7192_CLK_EXT_MCLK2; > @@ -385,9 +384,10 @@ static int ad7192_of_clock_select(struct ad7192_state *st) > return clock_sel; > } > > -static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np) > +static int ad7192_setup(struct iio_dev *indio_dev) > { > struct ad7192_state *st = iio_priv(indio_dev); > + struct device *dev = &st->sd.spi->dev; > bool rej60_en, refin2_en; > bool buf_en, bipolar, burnout_curr_en; > unsigned long long scale_uv; > @@ -416,26 +416,26 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np) > > st->conf = FIELD_PREP(AD7192_CONF_GAIN_MASK, 0); > > - rej60_en = of_property_read_bool(np, "adi,rejection-60-Hz-enable"); > + rej60_en = device_property_read_bool(dev, "adi,rejection-60-Hz-enable"); Not strictly related to your patch, but where are these properties documented? Best regards, Krzysztof
On Sun, Nov 05, 2023 at 09:31:29PM +0200, alisadariana@gmail.com wrote: > From: Alisa-Dariana Roman <alisa.roman@analog.com> > > Replace of.h and corresponding functions with preferred device specific > functions. > > Also replace of_device_get_match_data function with of_device_get_match_data() > spi_get_device_match_data. spi_get_device_match_data() With the above fixed, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> The first patch should be documentation of the properties as Krzysztof noted. P.S. Also consider using or taking an ideas from the "smart" script [1] I wrote to send patches, it will put the better list of people into the proper places, including maintainers and mailing lists. [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
On Sun, Nov 05, 2023 at 09:31:31PM +0200, alisadariana@gmail.com wrote: > From: Alisa-Dariana Roman <alisa.roman@analog.com> > > 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. ... > config AD7192 > - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver" > + tristate "Analog Devices AD7190 AD7192 AD7193 AD7194 AD7195 ADC driver" This doesn't scale. Please change this and below like: tristate "Analog Devices AD719x ADC driver" > depends on SPI > select AD_SIGMA_DELTA > help > Say yes here to build support for Analog Devices AD7190, > - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC). > + AD7192, AD7193, AD7194 or AD7195 SPI analog to digital converters (ADC). Say yes here to build support for Analog Devices SPI analog to digital converters (ADC): - AD7190 - AD7192 - AD7193 - AD7194 - AD7195 > If unsure, say N (but it's safe to say "Y"). With above change adding a new one will be just a mater of adding a single line. ... > +static int ad7192_parse_channel(struct iio_dev *indio_dev, > + struct fwnode_handle *child) > +{ > + u32 reg, ain[2]; > + int ret; > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + return ret; > + if (reg < AD7194_CH_DIFF_NR_MIN || reg > AD7194_CH_DIFF_NR_MAX) > + return -EINVAL; in_range() > + ret = fwnode_property_read_u32_array(child, "diff-channels", ain, > + ARRAY_SIZE(ain)); > + if (ret) > + return ret; > + > + if (ain[0] < AD7194_CH_AIN_MIN || ain[0] > AD7194_CH_AIN_MAX || > + ain[1] < AD7194_CH_AIN_MIN || ain[1] > AD7194_CH_AIN_MAX) Ditto. > + return -EINVAL; > + > + ad7194_channels[reg].channel = ain[0]; > + ad7194_channels[reg].channel2 = ain[1]; > + ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]); > + > + return 0; > +}
On Mon, 6 Nov 2023 12:27:29 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, Nov 05, 2023 at 09:31:31PM +0200, alisadariana@gmail.com wrote: > > From: Alisa-Dariana Roman <alisa.roman@analog.com> > > > > 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. > > ... > > > config AD7192 > > - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver" > > + tristate "Analog Devices AD7190 AD7192 AD7193 AD7194 AD7195 ADC driver" > > This doesn't scale. Please change this and below like: > > tristate "Analog Devices AD719x ADC driver" Also tends to cause trouble given habit of manufacturers to not have consistent naming. "AD7194 and similar ADC driver" would be my preference.
On 06.11.2023 11:24, Krzysztof Kozlowski wrote: > On 05/11/2023 20:31, alisadariana@gmail.com wrote: >> From: Alisa-Dariana Roman <alisa.roman@analog.com> >> >> Replace of.h and corresponding functions with preferred device specific >> functions. >> >> Also replace of_device_get_match_data function with >> spi_get_device_match_data. >> >> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> >> --- >> drivers/iio/adc/ad7192.c | 32 +++++++++++++++----------------- >> 1 file changed, 15 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c >> index adc3cbe92d6e..48e0357564af 100644 >> --- a/drivers/iio/adc/ad7192.c >> +++ b/drivers/iio/adc/ad7192.c >> @@ -17,7 +17,6 @@ >> #include <linux/err.h> >> #include <linux/sched.h> >> #include <linux/delay.h> >> -#include <linux/of.h> >> >> #include <linux/iio/iio.h> >> #include <linux/iio/sysfs.h> >> @@ -364,19 +363,19 @@ static inline bool ad7192_valid_external_frequency(u32 freq) >> freq <= AD7192_EXT_FREQ_MHZ_MAX); >> } >> >> -static int ad7192_of_clock_select(struct ad7192_state *st) >> +static int ad7192_device_clock_select(struct ad7192_state *st) >> { >> - struct device_node *np = st->sd.spi->dev.of_node; >> + struct device *dev = &st->sd.spi->dev; >> unsigned int clock_sel; >> >> clock_sel = AD7192_CLK_INT; >> >> /* use internal clock */ >> if (!st->mclk) { >> - if (of_property_read_bool(np, "adi,int-clock-output-enable")) >> + if (device_property_read_bool(dev, "adi,int-clock-output-enable")) >> clock_sel = AD7192_CLK_INT_CO; >> } else { >> - if (of_property_read_bool(np, "adi,clock-xtal")) >> + if (device_property_read_bool(dev, "adi,clock-xtal")) >> clock_sel = AD7192_CLK_EXT_MCLK1_2; >> else >> clock_sel = AD7192_CLK_EXT_MCLK2; >> @@ -385,9 +384,10 @@ static int ad7192_of_clock_select(struct ad7192_state *st) >> return clock_sel; >> } >> >> -static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np) >> +static int ad7192_setup(struct iio_dev *indio_dev) >> { >> struct ad7192_state *st = iio_priv(indio_dev); >> + struct device *dev = &st->sd.spi->dev; >> bool rej60_en, refin2_en; >> bool buf_en, bipolar, burnout_curr_en; >> unsigned long long scale_uv; >> @@ -416,26 +416,26 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np) >> >> st->conf = FIELD_PREP(AD7192_CONF_GAIN_MASK, 0); >> >> - rej60_en = of_property_read_bool(np, "adi,rejection-60-Hz-enable"); >> + rej60_en = device_property_read_bool(dev, "adi,rejection-60-Hz-enable"); > > Not strictly related to your patch, but where are these properties > documented? > > > Best regards, > Krzysztof > Thank you for the feedback! The properties are documented in Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml. But the properties related to the clock configuration are indeed missing. I will add them. Kind regards, Alisa-Dariana Roman
From: Alisa-Dariana Roman <alisa.roman@analog.com> Dear maintainers, I am submitting a series of patches to improve the ad7192 driver by adding suport for AD7194 also. Please consider applying these patches in order. Thank you for your time and attention. Kind regards, Alisa-Dariana Roman (3): iio: adc: ad7192: Use device api dt-bindings: iio: adc: ad7192: Add AD7194 support iio: adc: ad7192: Add AD7194 support .../bindings/iio/adc/adi,ad7192.yaml | 69 +++++++ drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad7192.c | 176 ++++++++++++++++-- 3 files changed, 228 insertions(+), 21 deletions(-)