Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support
On Tue, 5 Feb 2019 15:13:00 -0200
Renato Lui Geh wrote:
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
>
> Signed-off-by: Renato Lui Geh
> Signed-off-by: Giuliano Belinassi
> Co-developed-by: Giuliano Belinassi
Comments inline.
> ---
> Changes in v3:
> - Renamed ad7780_chip_info's filter to odr
> - Renamed ad778x_filter to ad778x_odr_avail
> - Changed vref variable from unsigned int to unsigned long long to
> avoid overflow
> - Removed unnecessary AD_SD_CHANNEL macro
>
> drivers/staging/iio/adc/ad7780.c | 95 ++--
> 1 file changed, 89 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..6e4357800d31 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,15 @@
> #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> #define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>
> +#define AD7780_GAIN_GPIO 0
> +#define AD7780_FILTER_GPIO 1
What are these for?
> +
> +#define AD7780_GAIN_MIDPOINT 64
> +#define AD7780_FILTER_MIDPOINT 13350
> +
> +static const unsigned int ad778x_gain[2] = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 1, 16700 };
> +
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,7 +59,11 @@ struct ad7780_state {
> const struct ad7780_chip_info *chip_info;
> struct regulator*reg;
> struct gpio_desc*powerdown_gpio;
> - unsigned intgain;
> + struct gpio_desc*gain_gpio;
> + struct gpio_desc*filter_gpio;
> + unsigned intgain;
> + unsigned intodr;
> + unsigned intint_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> voltage_uv = regulator_get_voltage(st->reg);
> if (voltage_uv < 0)
> return voltage_uv;
> - *val = (voltage_uv / 1000) * st->gain;
> + voltage_uv /= 1000;
> + *val = voltage_uv * st->gain;
> *val2 = chan->scan_type.realbits - 1;
> + st->int_vref_mv = voltage_uv;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> *val = -(1 << (chan->scan_type.realbits - 1));
> return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->odr;
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> }
>
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long m)
> +{
> + struct ad7780_state *st = iio_priv(indio_dev);
> + const struct ad7780_chip_info *chip_info = st->chip_info;
> + unsigned long long vref;
> + unsigned int full_scale, gain;
> +
> + if (!chip_info->is_ad778x)
> + return 0;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + if (val != 0)
> + return -EINVAL;
> +
> + vref = st->int_vref_mv * 100LL;
> + full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> + gain = DIV_ROUND_CLOSEST(vref, full_scale);
> + gain = DIV_ROUND_CLOSEST(gain, val2);
> + st->gain = gain;
> + if (gain < AD7780_GAIN_MIDPOINT)
> + gain = 0;
> + else
> + gain = 1;
> + gpiod_set_value(st->gain_gpio, gain);
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> + val = 0;
> + else
> + val = 1;
> + st->odr = ad778x_odr_avail[val];
> + gpiod_set_value(st->filter_gpio, val);
> + break;
> + }
> +
> + return 0;
> +}
> +
> static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>unsigned int raw_sample)
> {
> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> return -EIO;
>
> if (chip_info->is_ad778x) {
> - if (raw_sample & AD7780_GAIN)
> - st->gain = 1;
> - else
> - st->gain = 128;
> + st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> + st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> }
>
> return 0;
> @@ -173,6 +232,7 @@ static const struct ad7780_chip_in
Re: [PATCH v3 2/4] staging: iio: ad7780: move regulator to after GPIO init
On Tue, 5 Feb 2019 15:13:21 -0200
Renato Lui Geh wrote:
> To maintain consistency between ad7780_probe and ad7780_remove orders,
> regulator initialization has been moved to after GPIO initializations.
>
> Signed-off-by: Renato Lui Geh
This looks fine, will pick up with the earlier patches when ready.
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/ad7780.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 6e4357800d31..7804cd2b273e 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -250,16 +250,6 @@ static int ad7780_probe(struct spi_device *spi)
>
> ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
>
> - st->reg = devm_regulator_get(&spi->dev, "avdd");
> - if (IS_ERR(st->reg))
> - return PTR_ERR(st->reg);
> -
> - ret = regulator_enable(st->reg);
> - if (ret) {
> - dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> - return ret;
> - }
> -
> st->chip_info =
> &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>
> @@ -279,7 +269,7 @@ static int ad7780_probe(struct spi_device *spi)
> ret = PTR_ERR(st->powerdown_gpio);
> dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
> ret);
> - goto error_disable_reg;
> + return ret;
> }
>
> if (st->chip_info->is_ad778x) {
> @@ -290,7 +280,7 @@ static int ad7780_probe(struct spi_device *spi)
> ret = PTR_ERR(st->gain_gpio);
> dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
> ret);
> - goto error_disable_reg;
> + return ret;
> }
>
> st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> @@ -301,10 +291,20 @@ static int ad7780_probe(struct spi_device *spi)
> dev_err(&spi->dev,
> "Failed to request filter GPIO: %d\n",
> ret);
> - goto error_disable_reg;
> + return ret;
> }
> }
>
> + st->reg = devm_regulator_get(&spi->dev, "avdd");
> + if (IS_ERR(st->reg))
> + return PTR_ERR(st->reg);
> +
> + ret = regulator_enable(st->reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> + return ret;
> + }
> +
> ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> if (ret)
> goto error_disable_reg;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging
On Tue, 5 Feb 2019 15:14:03 -0200 Renato Lui Geh wrote: > Move ad7780 ADC driver out of staging and into the mainline. > > The ad7780 is a sigma-delta analog to digital converter. This driver provides > reading voltage values and status bits from both the ad778x and ad717x series. > Its interface also allows writing on the FILTER and GAIN GPIO pins on the > ad778x. > > Signed-off-by: Renato Lui Geh > Signed-off-by: Giuliano Belinassi > Co-developed-by: Giuliano Belinassi This needs a device tree binding doc which should be reviewed before we move the driver out of staging. Make sure to cc the dt-binding maintainers and list. Doesn't really matter if that patch is before or after this one in the series but needs to be in the same series. There are a few more minor tidy ups that would be nice to have inline given you are doing a v4. Stuff like this could have been cleaned up after moving out of staging (nothing wrong with improving non staging drivers after all) but always better to do it whilst we remember! > --- > Changes in v3: > - Changes unrelated to moving the driver to main tree were resent as > individual patches > > drivers/iio/adc/Kconfig | 13 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7780.c | 359 +++ > drivers/staging/iio/adc/Kconfig | 13 -- > drivers/staging/iio/adc/Makefile | 1 - > drivers/staging/iio/adc/ad7780.c | 359 --- > 6 files changed, 373 insertions(+), 373 deletions(-) > create mode 100644 drivers/iio/adc/ad7780.c > delete mode 100644 drivers/staging/iio/adc/ad7780.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index f3cc7a31bce5..2cdee166d0e9 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -108,6 +108,19 @@ config AD7766 > To compile this driver as a module, choose M here: the module will be > called ad7766. > > +config AD7780 > + tristate "Analog Devices AD7780 and similar ADCs driver" > + depends on SPI > + depends on GPIOLIB || COMPILE_TEST > + select AD_SIGMA_DELTA > + help > + Say yes here to build support for Analog Devices AD7170, AD7171, > + AD7780 and AD7781 SPI analog to digital converters (ADC). > + If unsure, say N (but it's safe to say "Y"). I wouldn't bother with this statement, doesn't add any real info! > + > + To compile this driver as a module, choose M here: the > + module will be called ad7780. > + > config AD7791 > tristate "Analog Devices AD7791 ADC driver" > depends on SPI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index ea5031348052..b48852157115 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o > obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o > obj-$(CONFIG_AD7606) += ad7606.o > obj-$(CONFIG_AD7766) += ad7766.o > +obj-$(CONFIG_AD7780) += ad7780.o > obj-$(CONFIG_AD7791) += ad7791.o > obj-$(CONFIG_AD7793) += ad7793.o > obj-$(CONFIG_AD7887) += ad7887.o > diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c > new file mode 100644 > index ..163e3c983598 > --- /dev/null > +++ b/drivers/iio/adc/ad7780.c > @@ -0,0 +1,359 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver > + * > + * Copyright 2011 Analog Devices Inc. I think you have done more than enough to this driver to add an additional copyright line if you want to! > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#define AD7780_RDY BIT(7) > +#define AD7780_FILTERBIT(6) > +#define AD7780_ERR BIT(5) > +#define AD7780_ID1 BIT(4) > +#define AD7780_ID0 BIT(3) > +#define AD7780_GAIN BIT(2) > +#define AD7780_PAT1 BIT(1) > +#define AD7780_PAT0 BIT(0) These two bits of pattern don't really add anything. I'd drop them in favour of something like #define AD7780_PATTERN_GOOD 1 #define AD7780_PATTERN_MASK GENMASK(1, 0) Same for ID for that matter. These aren't one bit fields, so we shouldn't ever present them as such (though the datasheet confusingly sort of does so!) > + > +#define AD7780_PATTERN (AD7780_PAT0) > +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1) > + > +#define AD7170_PAT2 BIT(2) > + > +#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2) > +#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2) I'd use a value for the pattern directly and GENMASK for the mask. > + > +#define AD7780_GAIN_GPIO 0 > +#define AD7780_FILTER_GPIO 1 > + > +#define AD7780_GAIN_MIDPOINT 64 > +#define AD7780_FILTER_MIDPOINT 13350 > + > +static const unsigned i
[PATCH] Staging: mt7621-pci: Fix space required coding style
Fix some space required coding style.
Signed-off-by: Guilherme Tadashi Maeoka
---
drivers/staging/mt7621-pci/pci-mt7621.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
b/drivers/staging/mt7621-pci/pci-mt7621.c
index 31310b6fb7db..fd8990e61120 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -90,7 +90,7 @@
#define PCIE_CLK_GEN_ENBIT(31)
#define PCIE_CLK_GEN_DIS 0
-#define PCIE_CLK_GEN1_DIS GENMASK(30,24)
+#define PCIE_CLK_GEN1_DIS GENMASK(30, 24)
#define PCIE_CLK_GEN1_EN (BIT(27) | BIT(25))
#define RALINK_PCI_IO_MAP_BASE 0x1e16
#define MEMORY_BASE0x0
@@ -714,7 +714,7 @@ static void mt7621_pcie_enable_ports(struct mt7621_pcie
*pcie)
static int mt7621_pcie_init_virtual_bridges(struct mt7621_pcie *pcie)
{
u32 pcie_link_status = 0;
- u32 val= 0;
+ u32 val = 0;
struct mt7621_pcie_port *port;
list_for_each_entry(port, &pcie->ports, list) {
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types
On Wed, 6 Feb 2019 14:25:41 +0200
Beniamin Bia wrote:
> From: Beniamin Bia
>
> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
>
> Signed-off-by: Beniamin Bia
Hi,
How is this related to the one a few hours earlier?
V2?
If you do accidentally send twice (and realise it obviously) please
reply to say what you want ignored. For now I'll ignore this version.
I'm guessing it was fighting with your work email system.
Thanks,
Jonathan
> ---
> drivers/staging/iio/frequency/ad9834.c | 97 +-
> 1 file changed, 80 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c
> b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
> /* Registers */
>
> #define AD9834_REG_CMD 0
> -#define AD9834_REG_FREQ0 BIT(14)
> -#define AD9834_REG_FREQ1 BIT(15)
> +#define AD9834_REG_FREQ(chann) (BIT(14) << (chann))
> #define AD9834_REG_PHASE0(BIT(15) | BIT(14))
> #define AD9834_REG_PHASE1(BIT(15) | BIT(14) | BIT(13))
>
> @@ -81,6 +80,9 @@ struct ad9834_state {
> struct spi_message freq_msg;
> struct mutexlock; /* protect sensor state */
>
> + unsigned long frequency0;
> + unsigned long frequency1;
> +
> /*
>* DMA (thus cache coherency maintenance) requires the
>* transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
> ID_AD9838,
> };
>
> +#define AD9833_CHANNEL(_chan) {
> \
> + .type = IIO_ALTVOLTAGE, \
> + .indexed = 1, \
> + .output = 1,\
> + .address = (_chan), \
> + .channel = (_chan), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) \
> +}
> +
> +static const struct iio_chan_spec ad9833_channels[] = {
> + AD9833_CHANNEL(0),
> + AD9833_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad9834_channels[] = {
> + AD9833_CHANNEL(0),
> + AD9833_CHANNEL(1),
> +};
> +
> static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long
> fout)
> {
> unsigned long long freqreg = (u64)fout * (u64)BIT(AD9834_FREQ_BITS);
> @@ -113,6 +134,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
> {
> unsigned long clk_freq;
> unsigned long regval;
> + int ret;
>
> clk_freq = clk_get_rate(st->mclk);
>
> @@ -121,13 +143,22 @@ static int ad9834_write_frequency(struct ad9834_state
> *st,
>
> regval = ad9834_calc_freqreg(clk_freq, fout);
>
> - st->freq_data[0] = cpu_to_be16(addr | (regval &
> + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ(addr) | (regval &
> RES_MASK(AD9834_FREQ_BITS / 2)));
> - st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ(addr) | ((regval >>
> (AD9834_FREQ_BITS / 2)) &
> RES_MASK(AD9834_FREQ_BITS / 2)));
>
> - return spi_sync(st->spi, &st->freq_msg);
> + ret = spi_sync(st->spi, &st->freq_msg);
> + if (ret)
> + return ret;
> +
> + if (addr == 0)
> + st->frequency0 = fout;
> + else
> + st->frequency1 = fout;
> +
> + return 0;
> }
>
> static int ad9834_write_phase(struct ad9834_state *st,
> @@ -140,6 +171,40 @@ static int ad9834_write_phase(struct ad9834_state *st,
> return spi_sync(st->spi, &st->msg);
> }
>
> +static int ad9834_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int *val, int *val2, long mask)
> +{
> + struct ad9834_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> + if (chan->address == 0)
> + *val = st->frequency0;
> + else
> + *val = st->frequency1;
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad9834_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ad9834_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> + return ad9834_write_frequency(st, chan->address, val);
> + default:
> +
Re: [PATCH net-next 01/16] Documentation: networking: switchdev: Update port parent ID section
Sat, Feb 09, 2019 at 01:32:33AM CET, f.faine...@gmail.com wrote:
>Update the section about switchdev drivers having to implement a
>switchdev_port_attr_get() function to return
>SWITCHDEV_ATTR_ID_PORT_PARENT_ID since that is no longer valid after
>commit bccb30254a4a ("net: Get rid of
>SWITCHDEV_ATTR_ID_PORT_PARENT_ID").
>
>Fixes: bccb30254a4a ("net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID")
>Signed-off-by: Florian Fainelli
>---
> Documentation/networking/switchdev.txt | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/networking/switchdev.txt
>b/Documentation/networking/switchdev.txt
>index f3244d87512a..2842f63ad47b 100644
>--- a/Documentation/networking/switchdev.txt
>+++ b/Documentation/networking/switchdev.txt
>@@ -92,11 +92,11 @@ device.
> Switch ID
> ^
>
>-The switchdev driver must implement the switchdev op switchdev_port_attr_get
>-for SWITCHDEV_ATTR_ID_PORT_PARENT_ID for each port netdev, returning the same
>-physical ID for each port of a switch. The ID must be unique between switches
>-on the same system. The ID does not need to be unique between switches on
>-different systems.
>+The switchdev driver must implement the net_device operation
>+ndo_get_port_parent_id for each port netdev, returning the same physical ID
Double space. Otherwise, this looks fine.
Acked-by: Jiri Pirko
>+for each port of a switch. The ID must be unique between switches on the same
>+system. The ID does not need to be unique between switches on different
>+systems.
>
> The switch ID is used to locate ports on a switch and to know if aggregated
> ports belong to the same switch.
>--
>2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types
On Wed, 6 Feb 2019 14:05:41 +0200
Beniamin Bia wrote:
> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
>
> Signed-off-by: Beniamin Bia
> ---
> drivers/staging/iio/frequency/ad9834.c | 97 +-
> 1 file changed, 80 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c
> b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
> /* Registers */
>
> #define AD9834_REG_CMD 0
> -#define AD9834_REG_FREQ0 BIT(14)
> -#define AD9834_REG_FREQ1 BIT(15)
> +#define AD9834_REG_FREQ(chann) (BIT(14) << (chann))
That is rather confusing...
Perhaps we can represent this as a variable that is 3 bits wide, with values
that
correspond to the different settings.
So define
#define AD7834_REG_ADDR_MASK GENMASK(15,13)
#define AD7834_REG_FREQ0 2
#define AD7834_REG_FREQ1 4
#define AD7834_REG_PHASE0 6
#define AD7834_REG_PHASE0 7
And use FIELD_SET(reg, AD7834_REG_ADDR_MASK, AD7834_REG_FREQ0) etc.
or something along those lines. It's a bit nasty because in the FREQ
case the bottom bit is actually part of the value.
Hmm. May just need two definitions of the mask. how about.
#define AD7834_REG_FREQ_MASK GENMASK(15,14)
#define AD7834_REG_FREQ0 1
#define AD7934_REG_FREQ1 2
#define AD7834_REG_PHASE_MASK GENMASK(15,13)
#define AD7834_REG_PHASE0 6
#define AD7834_REG_PHASE1 7
?
> #define AD9834_REG_PHASE0(BIT(15) | BIT(14))
> #define AD9834_REG_PHASE1(BIT(15) | BIT(14) | BIT(13))
>
> @@ -81,6 +80,9 @@ struct ad9834_state {
> struct spi_message freq_msg;
> struct mutexlock; /* protect sensor state */
>
> + unsigned long frequency0;
> + unsigned long frequency1;
> +
> /*
>* DMA (thus cache coherency maintenance) requires the
>* transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
> ID_AD9838,
> };
>
> +#define AD9833_CHANNEL(_chan) {
> \
> + .type = IIO_ALTVOLTAGE, \
> + .indexed = 1, \
> + .output = 1,\
> + .address = (_chan), \
Not a lot of point in setting them both if they are always the same.
> + .channel = (_chan), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) \
> +}
> +
> +static const struct iio_chan_spec ad9833_channels[] = {
> + AD9833_CHANNEL(0),
> + AD9833_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad9834_channels[] = {
> + AD9833_CHANNEL(0),
> + AD9833_CHANNEL(1),
> +};
Umm. That needs some explaining. Why have two identical arrays?
I'll guess that later patches will make them different?
That is fine, but you need a comment in the patch description to give
the reasoning.
> +
> static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long
> fout)
> {
> unsigned long long freqreg = (u64)fout * (u64)BIT(AD9834_FREQ_BITS);
> @@ -113,6 +134,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
> {
> unsigned long clk_freq;
> unsigned long regval;
> + int ret;
>
> clk_freq = clk_get_rate(st->mclk);
>
> @@ -121,13 +143,22 @@ static int ad9834_write_frequency(struct ad9834_state
> *st,
>
> regval = ad9834_calc_freqreg(clk_freq, fout);
>
> - st->freq_data[0] = cpu_to_be16(addr | (regval &
> + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ(addr) | (regval &
> RES_MASK(AD9834_FREQ_BITS / 2)));
> - st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ(addr) | ((regval >>
> (AD9834_FREQ_BITS / 2)) &
> RES_MASK(AD9834_FREQ_BITS / 2)));
>
> - return spi_sync(st->spi, &st->freq_msg);
> + ret = spi_sync(st->spi, &st->freq_msg);
> + if (ret)
> + return ret;
> +
> + if (addr == 0)
> + st->frequency0 = fout;
How about moving to an array you can just index with addr?
(or the enum I suggest below).
> + else
> + st->frequency1 = fout;
> +
> + return 0;
> }
>
> static int ad9834_write_phase(struct ad9834_state *st,
> @@ -140,6 +171,40 @@ static int ad9834_write_phase(struct ad9834_state *st,
> return spi_sync(st->spi, &st->msg);
> }
>
> +static int ad9834_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_sp
Re: [PATCH 2/2] staging: iio: frequency: ad9834: Move phase and scale to standard iio attribute
On Wed, 6 Feb 2019 14:05:42 +0200
Beniamin Bia wrote:
> The custom phase and scale attributes were moved to standard iio types.
>
> Signed-off-by: Beniamin Bia
Similar comments apply as to patch 1.
Jonathan
> ---
> drivers/staging/iio/frequency/ad9834.c | 54 +++---
> 1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c
> b/drivers/staging/iio/frequency/ad9834.c
> index 370e8263899e..3ecf976ddefe 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -30,8 +30,7 @@
>
> #define AD9834_REG_CMD 0
> #define AD9834_REG_FREQ(chann) (BIT(14) << (chann))
> -#define AD9834_REG_PHASE0(BIT(15) | BIT(14))
> -#define AD9834_REG_PHASE1(BIT(15) | BIT(14) | BIT(13))
> +#define AD9834_REG_PHASE(chann) (BIT(15) | BIT(14) | ((chann) << 13UL))
>
> /* Command Control Bits */
>
> @@ -82,6 +81,8 @@ struct ad9834_state {
>
> unsigned long frequency0;
> unsigned long frequency1;
> + unsigned long phase0;
> + unsigned long phase1;
>
> /*
>* DMA (thus cache coherency maintenance) requires the
> @@ -109,6 +110,8 @@ enum ad9834_supported_device_ids {
> .address = (_chan), \
> .channel = (_chan), \
> .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) \
> + | BIT(IIO_CHAN_INFO_PHASE),\
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> }
>
> static const struct iio_chan_spec ad9833_channels[] = {
> @@ -164,11 +167,22 @@ static int ad9834_write_frequency(struct ad9834_state
> *st,
> static int ad9834_write_phase(struct ad9834_state *st,
> unsigned long addr, unsigned long phase)
> {
> + int ret;
> +
> if (phase > BIT(AD9834_PHASE_BITS))
> return -EINVAL;
> - st->data = cpu_to_be16(addr | phase);
> + st->data = cpu_to_be16(AD9834_REG_PHASE(addr) | phase);
> +
> + ret = spi_sync(st->spi, &st->msg);
> + if (ret)
> + return ret;
>
> - return spi_sync(st->spi, &st->msg);
> + if (addr == 0)
> + st->phase0 = phase;
> + else
> + st->phase1 = phase;
> +
> + return 0;
> }
>
> static int ad9834_read_raw(struct iio_dev *indio_dev,
> @@ -184,6 +198,16 @@ static int ad9834_read_raw(struct iio_dev *indio_dev,
> else
> *val = st->frequency1;
> return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PHASE:
> + if (chan->address == 0)
> + *val = st->phase0;
> + else
> + *val = st->phase1;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + /*1 hz */
> + *val = 1;
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -198,6 +222,8 @@ static int ad9834_write_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_FREQUENCY:
> return ad9834_write_frequency(st, chan->address, val);
> + case IIO_CHAN_INFO_PHASE:
> + return ad9834_write_phase(st, chan->address, val);
> default:
> return -EINVAL;
> }
> @@ -222,10 +248,6 @@ static ssize_t ad9834_write(struct device *dev,
>
> mutex_lock(&st->lock);
> switch ((u32)this_attr->address) {
> - case AD9834_REG_PHASE0:
> - case AD9834_REG_PHASE1:
> - ret = ad9834_write_phase(st, this_attr->address, val);
> - break;
> case AD9834_OPBITEN:
> if (st->control & AD9834_MODE) {
> ret = -EINVAL; /* AD9843 reserved mode */
> @@ -385,12 +407,8 @@ static
> IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
> */
>
> static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
> -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
>
> -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9834_write, AD9834_REG_PHASE0);
> -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9834_write, AD9834_REG_PHASE1);
> static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);
> -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
>
> static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
> ad9834_write, AD9834_PIN_SW);
> @@ -401,10 +419,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0,
> ad9834_store_wavetype, 0);
> static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
>
> static struct attribute *ad9834_attributes[] = {
> - &iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
> - &iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
> - &iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> - &iio_const_attr_out_altv
Re: [PATCH net-next 02/16] mlxsw: spectrum: Check bridge flags during prepare phase
Sat, Feb 09, 2019 at 01:32:34AM CET, f.faine...@gmail.com wrote: >In preparation for getting rid of switchdev_port_attr_get(), have mlxsw >check for the bridge flags being set through switchdev_port_attr_set() >with the SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS attribute identifier. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 03/16] staging: fsl-dpaa2: ethsw: Check bridge port flags during set
Sat, Feb 09, 2019 at 01:32:35AM CET, f.faine...@gmail.com wrote: >In preparation for removing SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, >have ethsw check that the bridge port flags that are being set are >supported. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 04/16] net: dsa: Add setter for SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
Sat, Feb 09, 2019 at 01:32:36AM CET, f.faine...@gmail.com wrote: >In preparation for removing SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, >add support for a function that processes the >SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS attribute and returns not supported >for any flag set, since DSA does not currently support toggling those >bridge port attributes (yet). > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 07/16] net: Remove SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT
Sat, Feb 09, 2019 at 01:32:39AM CET, f.faine...@gmail.com wrote: >Now that we have converted the bridge code and the drivers to check for >bridge port(s) flags at the time we try to set them, there is no need >for a get() -> set() sequence anymore and >SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT therefore becomes unused. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 06/16] net: bridge: Stop calling switchdev_port_attr_get()
Sat, Feb 09, 2019 at 01:32:38AM CET, f.faine...@gmail.com wrote: >Now that all switchdev drivers have been converted to checking the >bridge port flags during the prepare phase of the >switchdev_port_attr_set(), we can move straight to trying to set the >desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 05/16] rocker: Check bridge flags during prepare phase
Sat, Feb 09, 2019 at 01:32:37AM CET, f.faine...@gmail.com wrote: >In preparation for getting rid of switchdev_port_attr_get(), have rocker >check for the bridge flags being set through switchdev_port_attr_set() >with the SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS attribute identifier. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 08/16] net: Get rid of switchdev_port_attr_get()
Sat, Feb 09, 2019 at 01:32:40AM CET, f.faine...@gmail.com wrote: >With the bridge no longer calling switchdev_port_attr_get() to obtain >the supported bridge port flags from a driver but instead trying to set >the bridge port flags directly and relying on driver to reject >unsupported configurations, we can effectively get rid of >switchdev_port_attr_get() entirely since this was the only place where >it was called. > >Signed-off-by: Florian Fainelli Acked-by: Jiri Pirko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 09/16] switchdev: Add SWITCHDEV_PORT_ATTR_SET
Sat, Feb 09, 2019 at 01:32:41AM CET, f.faine...@gmail.com wrote:
>In preparation for allowing switchdev enabled drivers to veto specific
>attribute settings from within the context of the caller, introduce a
>new switchdev notifier type for port attributes.
>
>Suggested-by: Ido Schimmel
>Signed-off-by: Florian Fainelli
>---
> include/net/switchdev.h | 9 +
> 1 file changed, 9 insertions(+)
>
>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>index 96cd3e795f7f..4c5f7e5430cf 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -141,6 +141,8 @@ enum switchdev_notifier_type {
> SWITCHDEV_VXLAN_FDB_ADD_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_DEL_TO_DEVICE,
> SWITCHDEV_VXLAN_FDB_OFFLOADED,
>+
>+ SWITCHDEV_PORT_ATTR_SET, /* Blocking. */
This is not UAPI. You can put it next to SWITCHDEV_PORT_OBJ_ADD and
SWITCHDEV_PORT_OBJ_DEL. Up to you.
With or without that:
Acked-by: Jiri Pirko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 10/16] rocker: Handle SWITCHDEV_PORT_ATTR_SET
Sat, Feb 09, 2019 at 01:32:42AM CET, f.faine...@gmail.com wrote:
>Following patches will change the way we communicate getting or setting
Just "setting", no "getting".
>a port's attribute and use a blocking notifier to perform those tasks.
>
>Prepare rocker to support receiving notifier events targeting
>SWITCHDEV_PORT_ATTR_SET and simply translate that into the existing
>rocker_port_attr_set call.
>
>Signed-off-by: Florian Fainelli
>---
> drivers/net/ethernet/rocker/rocker_main.c | 20
> 1 file changed, 20 insertions(+)
>
>diff --git a/drivers/net/ethernet/rocker/rocker_main.c
>b/drivers/net/ethernet/rocker/rocker_main.c
>index ff3f14504f4f..f10e4888ecff 100644
>--- a/drivers/net/ethernet/rocker/rocker_main.c
>+++ b/drivers/net/ethernet/rocker/rocker_main.c
>@@ -2811,6 +2811,24 @@ rocker_switchdev_port_obj_event(unsigned long event,
>struct net_device *netdev,
> return notifier_from_errno(err);
> }
>
>+static int
>+rocker_switchdev_port_attr_event(unsigned long event, struct net_device
>*netdev,
>+ struct switchdev_notifier_port_attr_info
>+ *port_attr_info)
>+{
>+ int err = -EOPNOTSUPP;
>+
>+ switch (event) {
>+ case SWITCHDEV_PORT_ATTR_SET:
Do you expect some other event to be handled in
rocker_switchdev_port_attr_event()? Because you have SWITCHDEV_PORT_ATTR_SET
selected in case here and in rocker_switchdev_blocking_event.
Perhaps you can rename rocker_switchdev_port_attr_event() to
rocker_switchdev_port_attr_set_event() and avoid this switchcase.
>+ err = rocker_port_attr_set(netdev, port_attr_info->attr,
>+ port_attr_info->trans);
>+ break;
>+ }
>+
>+ port_attr_info->handled = true;
>+ return notifier_from_errno(err);
>+}
>+
> static int rocker_switchdev_blocking_event(struct notifier_block *unused,
> unsigned long event, void *ptr)
> {
>@@ -2823,6 +2841,8 @@ static int rocker_switchdev_blocking_event(struct
>notifier_block *unused,
> case SWITCHDEV_PORT_OBJ_ADD:
> case SWITCHDEV_PORT_OBJ_DEL:
> return rocker_switchdev_port_obj_event(event, dev, ptr);
>+ case SWITCHDEV_PORT_ATTR_SET:
>+ return rocker_switchdev_port_attr_event(event, dev, ptr);
> }
>
> return NOTIFY_DONE;
>--
>2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 10/16] rocker: Handle SWITCHDEV_PORT_ATTR_SET
Sat, Feb 09, 2019 at 07:21:47PM CET, j...@resnulli.us wrote:
[...]
>>+static int
>>+rocker_switchdev_port_attr_event(unsigned long event, struct net_device
>>*netdev,
>>+ struct switchdev_notifier_port_attr_info
>>+ *port_attr_info)
>>+{
>>+ int err = -EOPNOTSUPP;
>>+
>>+ switch (event) {
>>+ case SWITCHDEV_PORT_ATTR_SET:
>
>Do you expect some other event to be handled in
>rocker_switchdev_port_attr_event()? Because you have SWITCHDEV_PORT_ATTR_SET
>selected in case here and in rocker_switchdev_blocking_event.
>Perhaps you can rename rocker_switchdev_port_attr_event() to
>rocker_switchdev_port_attr_set_event() and avoid this switchcase.
Same comment applies on next 4 patches.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 15/16] net: switchdev: Replace port attr set SDO with a notification
Sat, Feb 09, 2019 at 01:36:18AM CET, f.faine...@gmail.com wrote:
>On 2/8/19 4:32 PM, Florian Fainelli wrote:
>> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
>> from all clients, which were migrated to use switchdev notification in
>> the previous patches.
>>
>> Add a new function switchdev_port_attr_notify() that sends the switchdev
>> notifications SWITCHDEV_PORT_ATTR_SET.
>>
>> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
>> likewise.
>>
>> Signed-off-by: Florian Fainelli
>> ---
>> include/net/switchdev.h | 18
>> net/switchdev/switchdev.c | 92 ++-
>> 2 files changed, 22 insertions(+), 88 deletions(-)
>>
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index 4c5f7e5430cf..5387ff6f41c5 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -111,21 +111,6 @@ void *switchdev_trans_item_dequeue(struct
>> switchdev_trans *trans);
>>
>> typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
>>
>> -/**
>> - * struct switchdev_ops - switchdev operations
>> - *
>> - * @switchdev_port_attr_get: Get a port attribute (see switchdev_attr).
>> - *
>> - * @switchdev_port_attr_set: Set a port attribute (see switchdev_attr).
>> - */
>> -struct switchdev_ops {
>> -int (*switchdev_port_attr_get)(struct net_device *dev,
>> - struct switchdev_attr *attr);
>> -int (*switchdev_port_attr_set)(struct net_device *dev,
>> - const struct switchdev_attr *attr,
>> - struct switchdev_trans *trans);
>> -};
>> -
>
>This and the hunk below bisection, I will move that into patch #16 after
>receiving feedback on this.
Looks good. Thanks!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 10/16] rocker: Handle SWITCHDEV_PORT_ATTR_SET
Le 2/9/19 à 10:21 AM, Jiri Pirko a écrit :
> Sat, Feb 09, 2019 at 01:32:42AM CET, f.faine...@gmail.com wrote:
>> Following patches will change the way we communicate getting or setting
>
> Just "setting", no "getting".
>
>
>> a port's attribute and use a blocking notifier to perform those tasks.
>>
>> Prepare rocker to support receiving notifier events targeting
>> SWITCHDEV_PORT_ATTR_SET and simply translate that into the existing
>> rocker_port_attr_set call.
>>
>> Signed-off-by: Florian Fainelli
>> ---
>> drivers/net/ethernet/rocker/rocker_main.c | 20
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/rocker/rocker_main.c
>> b/drivers/net/ethernet/rocker/rocker_main.c
>> index ff3f14504f4f..f10e4888ecff 100644
>> --- a/drivers/net/ethernet/rocker/rocker_main.c
>> +++ b/drivers/net/ethernet/rocker/rocker_main.c
>> @@ -2811,6 +2811,24 @@ rocker_switchdev_port_obj_event(unsigned long event,
>> struct net_device *netdev,
>> return notifier_from_errno(err);
>> }
>>
>> +static int
>> +rocker_switchdev_port_attr_event(unsigned long event, struct net_device
>> *netdev,
>> + struct switchdev_notifier_port_attr_info
>> + *port_attr_info)
>> +{
>> +int err = -EOPNOTSUPP;
>> +
>> +switch (event) {
>> +case SWITCHDEV_PORT_ATTR_SET:
>
> Do you expect some other event to be handled in
> rocker_switchdev_port_attr_event()? Because you have SWITCHDEV_PORT_ATTR_SET
> selected in case here and in rocker_switchdev_blocking_event.
> Perhaps you can rename rocker_switchdev_port_attr_event() to
> rocker_switchdev_port_attr_set_event() and avoid this switchcase.
That's a good point, I have PORT_ATTR_{GET_SET} initially which was the
reason for the switch/case statement, will change it according to your
suggestion. Thanks!
--
Florian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
