Re: [PATCH] iio: ad_sigma_delta: use unsigned long for timeout
On Mon, 2018-07-23 at 11:18 +0200, Nicholas Mc Guire wrote:
> wait_for_completion_timeout returns unsigned long not int so an
> appropriate
> variable is declared and the assignment and check fixed up.
>
Patch looks good.
Thanks for this
Alex
> Signed-off-by: Nicholas Mc Guire
> ---
>
> found by experimental coccinelle script
>
> As the timeout returned is always << INT_MAX there is no side-effect with
> the
> type conversion here, never the less proper types should be used.
>
> Patch was compile tested with: x86_64_defconfig + CONFIG_SPI=y,
> CONFIG_IIO=y,
> CONFIG_AD7793=y (implies CONFIG_AD_SIGMA_DELTA=y)
>
> Patch is against 4.18-rc5 (localversion-next is next-20180720)
>
> drivers/iio/adc/ad_sigma_delta.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ad_sigma_delta.c
> b/drivers/iio/adc/ad_sigma_delta.c
> index cf1b048..fc95107 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -209,6 +209,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta
> *sigma_delta,
> unsigned int mode, unsigned int channel)
> {
> int ret;
> + unsigned long timeout;
>
> ret = ad_sigma_delta_set_channel(sigma_delta, channel);
> if (ret)
> @@ -224,8 +225,8 @@ static int ad_sd_calibrate(struct ad_sigma_delta
> *sigma_delta,
>
> sigma_delta->irq_dis = false;
> enable_irq(sigma_delta->spi->irq);
> - ret = wait_for_completion_timeout(&sigma_delta->completion,
> 2*HZ);
> - if (ret == 0) {
> + timeout = wait_for_completion_timeout(&sigma_delta->completion,
> 2 * HZ);
> + if (timeout == 0) {
> sigma_delta->irq_dis = true;
> disable_irq_nosync(sigma_delta->spi->irq);
> ret = -EIO;
Re: [PATCH] iio: ad_sigma_delta: use unsigned long for timeout
On Thu, 2018-07-26 at 11:44 +, Ardelean, Alexandru wrote:
> On Mon, 2018-07-23 at 11:18 +0200, Nicholas Mc Guire wrote:
> > wait_for_completion_timeout returns unsigned long not int so an
> > appropriate
> > variable is declared and the assignment and check fixed up.
> >
>
> Patch looks good.
>
> Thanks for this
[ I forgot to add this ]
Reviewed-by: Alexandru Ardelean
>
> Alex
>
> > Signed-off-by: Nicholas Mc Guire
> > ---
> >
> > found by experimental coccinelle script
> >
> > As the timeout returned is always << INT_MAX there is no side-effect
> > with
> > the
> > type conversion here, never the less proper types should be used.
> >
> > Patch was compile tested with: x86_64_defconfig + CONFIG_SPI=y,
> > CONFIG_IIO=y,
> > CONFIG_AD7793=y (implies CONFIG_AD_SIGMA_DELTA=y)
> >
> > Patch is against 4.18-rc5 (localversion-next is next-20180720)
> >
> > drivers/iio/adc/ad_sigma_delta.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > b/drivers/iio/adc/ad_sigma_delta.c
> > index cf1b048..fc95107 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -209,6 +209,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta
> > *sigma_delta,
> > unsigned int mode, unsigned int channel)
> > {
> > int ret;
> > + unsigned long timeout;
> >
> > ret = ad_sigma_delta_set_channel(sigma_delta, channel);
> > if (ret)
> > @@ -224,8 +225,8 @@ static int ad_sd_calibrate(struct ad_sigma_delta
> > *sigma_delta,
> >
> > sigma_delta->irq_dis = false;
> > enable_irq(sigma_delta->spi->irq);
> > - ret = wait_for_completion_timeout(&sigma_delta->completion,
> > 2*HZ);
> > - if (ret == 0) {
> > + timeout = wait_for_completion_timeout(&sigma_delta-
> > >completion,
> > 2 * HZ);
> > + if (timeout == 0) {
> > sigma_delta->irq_dis = true;
> > disable_irq_nosync(sigma_delta->spi->irq);
> > ret =
> > -EIO;N�r��y���b�X��ǧv�^�){.n�+{��*"��^n�r���z���h&�� �G��
> > �h�(�階�ݢj"���m�z�ޖ���f���h���~�m�
Re: [PATCH v3 7/7] staging:iio:ad2s90: Move out of staging
On Fri, 2018-11-23 at 22:23 -0200, Matheus Tavares wrote:
> Move ad2s90 resolver driver out of staging to the main tree.
>
Acked-by: Alexandru Ardelean
> Signed-off-by: Matheus Tavares
> Signed-off-by: Victor Colombo
> ---
> Changes in v3:
> - none
>
> Changes in v2:
> - Disabled git move detection, to see the whole code, as Jonathan
> suggested
>
> drivers/iio/resolver/Kconfig | 10 ++
> drivers/iio/resolver/Makefile | 1 +
> drivers/iio/resolver/ad2s90.c | 131 ++
> drivers/staging/iio/resolver/Kconfig | 10 --
> drivers/staging/iio/resolver/Makefile | 1 -
> drivers/staging/iio/resolver/ad2s90.c | 131 --
> 6 files changed, 142 insertions(+), 142 deletions(-)
> create mode 100644 drivers/iio/resolver/ad2s90.c
> delete mode 100644 drivers/staging/iio/resolver/ad2s90.c
>
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> index 2ced9f22aa70..786801be54f6 100644
> --- a/drivers/iio/resolver/Kconfig
> +++ b/drivers/iio/resolver/Kconfig
> @@ -3,6 +3,16 @@
> #
> menu "Resolver to digital converters"
>
> +config AD2S90
> + tristate "Analog Devices ad2s90 driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices spi resolver
> + to digital converters, ad2s90, provides direct access via sysfs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad2s90.
> +
> config AD2S1200
> tristate "Analog Devices ad2s1200/ad2s1205 driver"
> depends on SPI
> diff --git a/drivers/iio/resolver/Makefile
> b/drivers/iio/resolver/Makefile
> index 4e1dccae07e7..398d82d50028 100644
> --- a/drivers/iio/resolver/Makefile
> +++ b/drivers/iio/resolver/Makefile
> @@ -2,4 +2,5 @@
> # Makefile for Resolver/Synchro drivers
> #
>
> +obj-$(CONFIG_AD2S90) += ad2s90.o
> obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/iio/resolver/ad2s90.c
> b/drivers/iio/resolver/ad2s90.c
> new file mode 100644
> index ..a41f5cb10da5
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s90.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ad2s90.c simple support for the ADI Resolver to Digital Converters:
> AD2S90
> + *
> + * Copyright (c) 2010-2010 Analog Devices Inc.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/*
> + * Although chip's max frequency is 2Mhz, it needs 600ns between CS and
> the
> + * first falling edge of SCLK, so frequency should be at most 1 / (2 *
> 6e-7)
> + */
> +#define AD2S90_MAX_SPI_FREQ_HZ 83
> +
> +struct ad2s90_state {
> + struct mutex lock; /* lock to protect rx buffer */
> + struct spi_device *sdev;
> + u8 rx[2] cacheline_aligned;
> +};
> +
> +static int ad2s90_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int *val,
> +int *val2,
> +long m)
> +{
> + int ret;
> + struct ad2s90_state *st = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_ANGL)
> + return -EINVAL;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + /* 2 * Pi / 2^12 */
> + *val = 6283; /* mV */
> + *val2 = 12;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + ret = spi_read(st->sdev, st->rx, 2);
> + if (ret < 0) {
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> + *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >>
> 4);
> +
> + mutex_unlock(&st->lock);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info ad2s90_info = {
> + .read_raw = ad2s90_read_raw,
> +};
> +
> +static const struct iio_chan_spec ad2s90_chan = {
> + .type = IIO_ANGL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad2s90_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad2s90_state *st;
> +
> + if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
> + dev_err(&spi->dev, "SPI CLK, %d Hz exceeds %d Hz\n",
> + spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
> + return -EINVAL;
> + }
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> + st = iio_priv(indio_dev);
> + spi_set_drvdata(spi, indio_dev);
> +
> + mutex_init(&st->lock);
> + st->sdev = spi;
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->info = &ad2s90_info;
> + indio_dev->modes
Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type
On Tue, 2018-11-20 at 22:30 +0530, Shreeya Patel wrote:
> ADT7316 driver no more uses platform data and hence use device tree
> data instead of platform data for assigning irq_type field.
> Switch case figures out the type of irq and if it's the default case
> then assign the default value to the irq_type i.e.
> irq_type = IRQF_TRIGGER_LOW
>
1 comment inline
> Signed-off-by: Shreeya Patel
> ---
> drivers/staging/iio/addac/adt7316.c | 21 +
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/addac/adt7316.c
> b/drivers/staging/iio/addac/adt7316.c
> index 9c72538baf9e..c647875a64f5 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -2101,8 +2101,7 @@ int adt7316_probe(struct device *dev, struct
> adt7316_bus *bus,
> {
> struct adt7316_chip_info *chip;
> struct iio_dev *indio_dev;
> - unsigned short *adt7316_platform_data = dev->platform_data;
> - int irq_type = IRQF_TRIGGER_LOW;
> + int irq_type;
> int ret = 0;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> @@ -2146,8 +2145,22 @@ int adt7316_probe(struct device *dev, struct
> adt7316_bus *bus,
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> if (chip->bus.irq > 0) {
> - if (adt7316_platform_data[0])
> - irq_type = adt7316_platform_data[0];
> + irq_type =
> + irqd_get_trigger_type(irq_get_irq_data(chip-
> >bus.irq));
> +
> + switch (irq_type) {
> + case IRQF_TRIGGER_HIGH:
> + case IRQF_TRIGGER_RISING:
> + break;
> + case IRQF_TRIGGER_LOW:
> + case IRQF_TRIGGER_FALLING:
> + break;
> + default:
> + dev_info(dev, "mode %d unsupported, using
> IRQF_TRIGGER_LOW\n",
> + irq_type);
> + irq_type = IRQF_TRIGGER_LOW;
> + break;
> + }
It would be an idea to move this part [together with
devm_request_threaded_irq()] into a "adt7316_setup_irq()" function. To un-
clutter the code in the adt7316_probe() function.
>
> ret = devm_request_threaded_irq(dev, chip->bus.irq,
> NULL,
Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote: > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote: > Hi, please see bellow > One note from me here. > > Hi, thank you for the review > > > > > > > > On Thu, 22 Nov 2018 11:01:00 + > > > "Popa, Stefan Serban" wrote: > > > > > > > > I think that instead of setting the gain directly, we should use > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 > > > > datasheet > > > > there > > > > is a formula from which the output code can be calculated: > > > > Code = 2^(N − 1) > > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user > > > > space, > > > > the > > > > driver can calculate the correct gain by using the formula above. > > > > Also, it > > > > would be useful to introduce scale available. > > > > Furthermore, there is a new > > > > ad7124 adc driver which does this exact thing. Take a look here: > > > > http > > > > s://gi > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124. > > > > c# > > > > L337. > > > > We have some questions about the code you provided to us: > > 1-) What is exactly the inputs for the write_raw function? > > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. > Setting the scale from user space looks something like this: > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . > Furthermore, in your write_raw() function, val=0 and val2=298. > Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate > the gain = Vref / (full_scale_voltage * scale). We only support two gains > (1 and 128), so we need to determine which one fits better with the > desired > scale. Finally, all we have left to do is to set the gain. > > > 2-) Could you give more details about the math around lines 346-348? > > Is it correct to assume that the multiplication at line 346 won't > > overflow? (vref is an uint) > > It is correct that Vref is in microvolts, so for example, Vref of 2.5V = > 25uV. It won't overflow since we use the Vref as nominator, while > full_scale_voltage and scale are the denominators. > [Regarding the AD7124] I guess I should be noted that the code can overflow, but in this case it shouldn't, because (according to the device datasheet) the maximum VREF can be 3600 mV. A user could specify (in the devicetree) something larger than 4300 mV (and that would overflow) but that would also make the readings useless as the external VREF would be invalid ; and there is also the risk of frying the chip in that case [something you really can't protect the user against]. The internal VREF however is 2500 mV, so things should be fine from that point of view. Typically, in datasheets (at least from Analog Devices) it's good to take a look at the specifications sections. [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with approximation of +/-0.2%) and for external reference it goes all the way up to AVDD, which has typical values of 2.9V - 3.6V. So, for u32 this code should be fine and not overflow. One small thing that can be confusing about that code in AD7124 is that it gets multiplied with 100LL (which is signed long-long), but that should be fine, since the operation should be converted to u32 (unsigned int) representation [by being assigned to vref], which ends up being fine in the end. > > > > And regarding our code: > > 1-) The val in our write_raw function should be, in case of GAIN, a > > number that best approximate the actual gain value of 1 or 128? For > > instance, if the user inputs 126, we should default to 128? > > We should not allow the the user to input the gain, he needs to input the > scale (see the mail from Jonathan and the above explanation). However, if > the calculated gain is one that is not supported, such as 126, we will > set > the closest matching value, in this case 128. > > > 2-) In the case of FILTER, is it the same? Is the user sending the > > value in mHz (milihertz)? > > Yes, it is the same with the FILTER. You need to add > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user > space, the input value should be in Hz, something like this: > root:/sys/bus/iio/devices/iio:device0> echo 10 > > in_voltage_sampling_frequency > > > > > Thank you
Re: [PATCH v4 4/4] staging: iio: ad7816: Add device tree table.
On Wed, 2018-11-14 at 23:16 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
>
This could have been just one patch.
Something like
[PATCH v4] staging: iio: ad7816: Add device tree table.
It's no longer a series, because the other patches were applied already.
I don't think you need to re-spin this [it's up to Jonathan though].
It's just something to keep in mind for later when re-spinning patches or
series of patches.
Alex
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/adc/ad7816.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..925f7086bc07 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,14 @@ static int ad7816_probe(struct spi_device *spi_dev)
> return 0;
> }
>
> +static const struct of_device_id ad7816_of_match[] = {
> + { .compatible = "adi,ad7816", },
> + { .compatible = "adi,ad7817", },
> + { .compatible = "adi,ad7818", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
> static const struct spi_device_id ad7816_id[] = {
> { "ad7816", ID_AD7816 },
> { "ad7817", ID_AD7817 },
> @@ -434,6 +442,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> static struct spi_driver ad7816_driver = {
> .driver = {
> .name = "ad7816",
> + .of_match_table = of_match_ptr(ad7816_of_match),
> },
> .probe = ad7816_probe,
> .id_table = ad7816_id,
Re: [PATCH v2 1/7] staging:iio:ad2s90: Add device tree support
On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> This patch adds device tree support to ad2s90 with standard
> device tree id table.
>
Hey,
Comment inline
> Signed-off-by: Matheus Tavares
> ---
> Changes in v2:
> - none
>
> drivers/staging/iio/resolver/ad2s90.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 3e257ac46f7a..6ffbac66b837 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -107,6 +107,12 @@ static int ad2s90_probe(struct spi_device *spi)
> return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> }
>
> +static const struct of_device_id ad2s90_of_match[] = {
> + { .compatible = "adi,ad2s90", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad2s90_of_match);
> +
> static const struct spi_device_id ad2s90_id[] = {
> { "ad2s90" },
> {}
> @@ -116,6 +122,7 @@ MODULE_DEVICE_TABLE(spi, ad2s90_id);
> static struct spi_driver ad2s90_driver = {
> .driver = {
> .name = "ad2s90",
> + .of_match_table = of_match_ptr(ad2s90_of_match),
I think you need to remove the of_match_ptr().
There was a comment from Jonathan on another thread about this.
See:
https://patchwork.kernel.org/patch/10682963/
So,
+ .of_match_table = of_match_ptr(ad2s90_of_match),
becomes
> + .of_match_table = ad2s90_of_match,
> },
> .probe = ad2s90_probe,
> .id_table = ad2s90_id,
Re: [PATCH v2 3/7] staging:iio:ad2s90: Add max frequency check at probe
On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> From: Alexandru Ardelean
>
> This patch adds a max frequency check at the beginning of ad2s90_probe
> function so that when it is set to a value above 0.83Mhz, dev_err is
> called with an appropriate message and -EINVAL is returned.
>
> The defined limit is 0.83Mhz instead of 2Mhz, which is the chip's max
> frequency as specified in the datasheet, because, as also specified in
> the datasheet, a 600ns delay is expected between the application of a
> logic LO to CS and the application of SCLK. Since the delay is not
> implemented in the spi code, to satisfy it, SCLK's period should be at
> most 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
> gives roughly 83Hz.
>
> Signed-off-by: Alexandru Ardelean
I think you can use "Suggested-by:" instead.
But this is also fine.
> Signed-off-by: Matheus Tavares
> ---
> Changes in v2:
> - Correctly credit Alexandru as the patch's author
>
> drivers/staging/iio/resolver/ad2s90.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 913d6fad2d4d..fe90f2056bff 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -19,6 +19,12 @@
> #include
> #include
>
> +/*
> + * Although chip's max frequency is 2Mhz, it needs 600ns between CS and
> the
> + * first falling edge of SCLK, so frequency should be at most 1 / (2 *
> 6e-7)
> + */
> +#define AD2S90_MAX_SPI_FREQ_HZ 83
> +
> struct ad2s90_state {
> struct mutex lock;
> struct spi_device *sdev;
> @@ -78,6 +84,12 @@ static int ad2s90_probe(struct spi_device *spi)
> struct iio_dev *indio_dev;
> struct ad2s90_state *st;
>
> + if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
> + dev_err(&spi->dev, "SPI CLK, %d Hz exceeds %d Hz\n",
> + spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
> + return -EINVAL;
> + }
> +
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
Re: [PATCH v2 4/7] dt-bindings:iio:resolver: Add docs for ad2s90
On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> This patch adds the device tree binding documentation for the ad2s90
> resolver-to-digital converter.
>
One minor comment inline.
> Signed-off-by: Matheus Tavares
> ---
> Changes in v2:
> - Rewritten 'spi-cpol and spi-cpha' item to say that the device can
> work in either mode (0,0) or (1,1) and explain how they should be
> specified in DT.
>
> .../bindings/iio/resolver/ad2s90.txt | 28 +++
> 1 file changed, 28 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> new file mode 100644
> index ..594417539938
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> @@ -0,0 +1,28 @@
> +Analog Devices AD2S90 Resolver-to-Digital Converter
> +
> +https://www.analog.com/en/products/ad2s90.html
> +
> +Required properties:
> + - compatible: should be "adi,ad2s90"
> + - reg: SPI chip select number for the device
> + - spi-max-frequency: set maximum clock frequency, must be 83
> + - spi-cpol and spi-cpha:
> +Either SPI mode (0,0) or (1,1) must be used, so specify none or
> both of
> +spi-cpha, spi-cpol.
For SPI properties it's a good idea to also reference the document for SPI
bindings.
Something like:
See for more details:
Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Note about max frequency:
> +Chip's max frequency, as specified in its datasheet, is 2Mhz. But a
> 600ns
> +delay is expected between the application of a logic LO to CS and
> the
> +application of SCLK, as also specified. And since the delay is not
> +implemented in the spi code, to satisfy it, SCLK's period should be
> at most
> +2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
> gives
> +roughly 83Hz.
> +
> +Example:
> +resolver@0 {
> + compatible = "adi,ad2s90";
> + reg = <0>;
> + spi-max-frequency = <83>;
> + spi-cpol;
> + spi-cpha;
> +};
Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read
On Sun, 2018-11-11 at 14:30 +, Jonathan Cameron wrote:
> On Tue, 6 Nov 2018 09:24:44 +
> "Ardelean, Alexandru" wrote:
>
> > On Mon, 2018-11-05 at 17:14 -0200, Renato Lui Geh wrote:
> > > The ad7780 driver previously did not read the correct device output,
> > > as
> > > it read an outdated value set at initialization. It now updates its
> > > voltage on read.
> > >
>
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to check it. I'm not that fussed about pushing this
> one as a fix, as most of the time these things are on fixed regulators
> anyway, but it is nice to do it right.
>
> > Looks good from my side.
>
> Ack?
Acked-by: Alexandru Ardelean
>
> Much preferred if you are willing to give a formal acknowledgement.
>
> Thanks,
>
> Jonathan
>
>
> >
> > Alex
> >
> > > Signed-off-by: Renato Lui Geh
> > > ---
> > > Changes in v3:
> > > - removed initialization (int voltage_uv = 0)
> > > - returns error when voltage_uv is null
> > > Changes in v4:
> > > - returns error when voltage_uv is negative
> > >
> > > drivers/staging/iio/adc/ad7780.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index b67412db0318..c7cb05cedbbc 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev
> > > *indio_dev,
> > > long m)
> > > {
> > > struct ad7780_state *st = iio_priv(indio_dev);
> > > + int voltage_uv;
> > >
> > > switch (m) {
> > > case IIO_CHAN_INFO_RAW:
> > > return ad_sigma_delta_single_conversion(indio_dev, chan,
> > > val);
> > > case IIO_CHAN_INFO_SCALE:
> > > - *val = st->int_vref_mv * st->gain;
> > > + voltage_uv = regulator_get_voltage(st->reg);
> > > + if (voltage_uv < 0)
> > > + return voltage_uv;
> > > + *val = (voltage_uv / 1000) * st->gain;
> > > *val2 = chan->scan_type.realbits - 1;
> > > return IIO_VAL_FRACTIONAL_LOG2;
> > > case IIO_CHAN_INFO_OFFSET:
>
>
Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update
On Sun, 2018-11-11 at 12:58 +, Jonathan Cameron wrote:
> On Thu, 8 Nov 2018 13:44:17 +
> "Ardelean, Alexandru" wrote:
>
> > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > through a new variable is_ad778x in chip_info.
> > >
> >
> > Looks good.
>
> Alex, formal tags definitely preferred! It's not as though a
> looks good is any less of a review than an Ack, it's just better
> hidden as people need to look at mailing list archives...
>
> Jonathan
>
Acked-by: Alexandru Ardelean
// Will remember that next time :)
Thanks
Alex
> >
> > Alex
> >
> > > Signed-off-by: Giuliano Belinassi
> > > ---
> > > Changes in v2:
> > > - Squashed is_ad778x declaration commit with the ad778x checkage
> > > - Changed is_ad778x type to bool
> > >
> > > drivers/staging/iio/adc/ad7780.c | 15 +++
> > > 1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index 91e016d534ed..9ec2b002539e 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > > struct iio_chan_specchannel;
> > > unsigned intpattern_mask;
> > > unsigned intpattern;
> > > + boolis_ad778x;
> > > };
> > >
> > > struct ad7780_state {
> > > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > > ad_sigma_delta *sigma_delta,
> > > ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > > return -EIO;
> > >
> > > - if (raw_sample & AD7780_GAIN)
> > > - st->gain = 1;
> > > - else
> > > - st->gain = 128;
> > > + if (chip_info->is_ad778x) {
> > > + if (raw_sample & AD7780_GAIN)
> > > + st->gain = 1;
> > > + else
> > > + st->gain = 128;
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > > ad7780_chip_info_tbl[] = {
> > > .channel = AD7780_CHANNEL(12, 24),
> > > .pattern = 0x5,
> > > .pattern_mask = 0x7,
> > > + .is_ad778x = false,
> > > },
> > > [ID_AD7171] = {
> > > .channel = AD7780_CHANNEL(16, 24),
> > > .pattern = 0x5,
> > > .pattern_mask = 0x7,
> > > + .is_ad778x = false,
> > > },
> > > [ID_AD7780] = {
> > > .channel = AD7780_CHANNEL(24, 32),
> > > .pattern = 0x1,
> > > .pattern_mask = 0x3,
> > > + .is_ad778x = true,
> > > },
> > > [ID_AD7781] = {
> > > .channel = AD7780_CHANNEL(20, 32),
> > > .pattern = 0x1,
> > > .pattern_mask = 0x3,
> > > + .is_ad778x = true,
> > > },
> > > };
> > >
>
>
Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface
On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
>
Patch looks good.
I do have some thoughts about it. See inline.
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/adc/ad7816.c | 80 ++--
> 1 file changed, 34 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index bf76a8620bdb..12c4e0ab4713 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -7,7 +7,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -44,9 +44,9 @@
>
> struct ad7816_chip_info {
> struct spi_device *spi_dev;
> - u16 rdwr_pin;
> - u16 convert_pin;
> - u16 busy_pin;
> + struct gpio_desc *rdwr_pin;
> + struct gpio_desc *convert_pin;
> + struct gpio_desc *busy_pin;
Only if you want to do a re-spin, here's an idea.
If you don't want to, feel free to disregard, since your patch is good.
You could compact things a bit; I know this wasn't what the initial code
was doing, but it's an option.
Something like:
enum {
GPIO_RWDR,
GPIO_CONVERT,
GPIO_BUSY,
__GPIO_MAX,
}
Then, what you end up having is:
struct ad7816_chip_info {
struct spi_device *spi_dev;
- u16 rdwr_pin;
- u16 convert_pin;
- u16 busy_pin;
+ struct gpio_desc *gpios[__GPIO_MAX];
// Continued below
> u8 oti_data[AD7816_CS_MAX + 1];
> u8 channel_id; /* 0 always be temperature */
> u8 mode;
> @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> *chip, u16 *data)
> int ret = 0;
> __be16 buf;
>
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
Obviously, in the above context, this becomes:
+ gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
+ gpiod_set_value(chip->gpios[GPIO_RWDR], 0);
> ret = spi_write(spi_dev, &chip->channel_id, sizeof(chip-
> >channel_id));
> if (ret < 0) {
> dev_err(&spi_dev->dev, "SPI channel setting error\n");
> return ret;
> }
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
>
> if (chip->mode == AD7816_PD) { /* operating mode 2 */
> - gpio_set_value(chip->convert_pin, 1);
> - gpio_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
> } else { /* operating mode 1 */
> - gpio_set_value(chip->convert_pin, 0);
> - gpio_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
> }
>
> - while (gpio_get_value(chip->busy_pin))
> + while (gpiod_get_value(chip->busy_pin))
> cpu_relax();
>
> - gpio_set_value(chip->rdwr_pin, 0);
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> ret = spi_read(spi_dev, &buf, sizeof(*data));
> if (ret < 0) {
> dev_err(&spi_dev->dev, "SPI data read error\n");
> @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> *chip, u8 data)
> struct spi_device *spi_dev = chip->spi_dev;
> int ret = 0;
>
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
> ret = spi_write(spi_dev, &data, sizeof(data));
> if (ret < 0)
> dev_err(&spi_dev->dev, "SPI oti data write error\n");
> @@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device
> *dev,
> struct ad7816_chip_info *chip = iio_priv(indio_dev);
>
> if (strcmp(buf, "full")) {
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
> chip->mode = AD7816_FULL;
> } else {
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 0);
> chip->mode = AD7816_PD;
> }
>
> @@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device *spi_dev)
> {
> struct ad7816_chip_info *chip;
> struct iio_dev *indio_dev;
> - unsigned short *pins = dev_get_platdata(&spi_dev->dev);
> int ret = 0;
> int i;
>
> - if (!pins) {
> - dev_err(&spi_dev->dev, "No necessary GPIO platform
> data.\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(&spi_dev->dev, sizeof(*chip));
> if (!indio_dev)
> return -ENOMEM;
> @@ -364,34 +358,28 @@ stat
Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface
On Fri, 2018-11-09 at 08:05 +, Ardelean, Alexandru wrote:
> On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> > instead of the deprecated old non-descriptor interface.
> >
>
> Patch looks good.
> I do have some thoughts about it. See inline.
>
Nevermind what I just said here; I just saw patch3: `iio: ad7816: Set RD/WR
pin and CONVST pin as outputs.`
This looks good as is in the context of the patch series.
>
> > Signed-off-by: Nishad Kamdar
> > ---
> > drivers/staging/iio/adc/ad7816.c | 80 ++--
> > 1 file changed, 34 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7816.c
> > b/drivers/staging/iio/adc/ad7816.c
> > index bf76a8620bdb..12c4e0ab4713 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -7,7 +7,7 @@
> > */
> >
> > #include
> > -#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -44,9 +44,9 @@
> >
> > struct ad7816_chip_info {
> > struct spi_device *spi_dev;
> > - u16 rdwr_pin;
> > - u16 convert_pin;
> > - u16 busy_pin;
> > + struct gpio_desc *rdwr_pin;
> > + struct gpio_desc *convert_pin;
> > + struct gpio_desc *busy_pin;
>
> Only if you want to do a re-spin, here's an idea.
> If you don't want to, feel free to disregard, since your patch is good.
>
> You could compact things a bit; I know this wasn't what the initial code
> was doing, but it's an option.
>
> Something like:
>
> enum {
> GPIO_RWDR,
> GPIO_CONVERT,
> GPIO_BUSY,
> __GPIO_MAX,
> }
>
> Then, what you end up having is:
>
> struct ad7816_chip_info {
> struct spi_device *spi_dev;
> -u16 rdwr_pin;
> -u16 convert_pin;
> -u16 busy_pin;
> +struct gpio_desc *gpios[__GPIO_MAX];
>
>
> // Continued below
>
>
> > u8 oti_data[AD7816_CS_MAX + 1];
> > u8 channel_id; /* 0 always be temperature */
> > u8 mode;
> > @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> > *chip, u16 *data)
> > int ret = 0;
> > __be16 buf;
> >
> > - gpio_set_value(chip->rdwr_pin, 1);
> > - gpio_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
>
> Obviously, in the above context, this becomes:
> +gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
> +gpiod_set_value(chip->gpios[GPIO_RWDR], 0);
>
> > ret = spi_write(spi_dev, &chip->channel_id, sizeof(chip-
> > > channel_id));
> >
> > if (ret < 0) {
> > dev_err(&spi_dev->dev, "SPI channel setting error\n");
> > return ret;
> > }
> > - gpio_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> >
> > if (chip->mode == AD7816_PD) { /* operating mode 2 */
> > - gpio_set_value(chip->convert_pin, 1);
> > - gpio_set_value(chip->convert_pin, 0);
> > + gpiod_set_value(chip->convert_pin, 1);
> > + gpiod_set_value(chip->convert_pin, 0);
> > } else { /* operating mode 1 */
> > - gpio_set_value(chip->convert_pin, 0);
> > - gpio_set_value(chip->convert_pin, 1);
> > + gpiod_set_value(chip->convert_pin, 0);
> > + gpiod_set_value(chip->convert_pin, 1);
> > }
> >
> > - while (gpio_get_value(chip->busy_pin))
> > + while (gpiod_get_value(chip->busy_pin))
> > cpu_relax();
> >
> > - gpio_set_value(chip->rdwr_pin, 0);
> > - gpio_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > ret = spi_read(spi_dev, &buf, sizeof(*data));
> > if (ret < 0) {
> > dev_err(&spi_dev->dev, "SPI data read error\n");
> > @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> > *chip, u8 data)
> > struct spi_device *spi_dev = chip->spi_dev;
> > int ret = 0;
> >
> > - gpio_set_value(chip->rdwr_pin, 1);
> > - gpio_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
> > ret = spi_write(spi_dev, &am
Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.
On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
One comment inline for this.
Thanks
Alex
>
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/adc/ad7816.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..b8a9149fbac1 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> return 0;
> }
>
> +static const struct of_device_id ad7816_of_match[] = {
> + { .compatible = "adi,ad7816", },
I think you also need to add:
+ { .compatible = "adi,ad7817", },
+ { .compatible = "adi,ad7818", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
> static const struct spi_device_id ad7816_id[] = {
> { "ad7816", ID_AD7816 },
> { "ad7817", ID_AD7817 },
> @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> static struct spi_driver ad7816_driver = {
> .driver = {
> .name = "ad7816",
> + .of_match_table = of_match_ptr(ad7816_of_match),
> },
> .probe = ad7816_probe,
> .id_table = ad7816_id,
Re: [PATCH 0/3] Add devicetree support for ad5933
On Mon, 2018-12-10 at 21:27 +, Jonathan Cameron wrote: > On Sat, 8 Dec 2018 22:10:43 +0100 > Greg KH wrote: > > > On Sat, Dec 08, 2018 at 04:56:45PM -0200, Marcelo Schmitt wrote: > > > Parts of this work came from contributions of Alexandru Ardelean and > > > Dragos Bogdan, I and Gabriel would like to thank for the insights > > > provided by their previous patches. Maybe it would be the case to add > > > them as co-authors of this patch set. > > > > That's what the Co-developed-by: tag is for, please use it :) > > > > Alexandru, Dragos. No idea how involved you were in the actual > patch... > > Work amongst amongst the 4 of you what you would like to do and > let us know! > Hey, To give a bit of context, I sent some patches some time ago that deal with this, but haven't had time to re-spin them [based on Jonathan's comments]. I've discussed with Dragos. From our side, any version that is decided with respect to the `Co- developed-by:` tag is fine. i.e. we don't need to be mentioned in the commits. I hope this is a sufficiently good answer [for the whole legal stuff]. > Patches look fine, though we need to let the DT maintainers have a few > days at least to get around to looking if they want to (they don't always > on simple bindings like these). > Thanks Alex > Jonathan >
Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read
On Mon, 2018-11-05 at 17:14 -0200, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
>
Looks good from my side.
Alex
> Signed-off-by: Renato Lui Geh
> ---
> Changes in v3:
> - removed initialization (int voltage_uv = 0)
> - returns error when voltage_uv is null
> Changes in v4:
> - returns error when voltage_uv is negative
>
> drivers/staging/iio/adc/ad7780.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..c7cb05cedbbc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv;
>
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
> case IIO_CHAN_INFO_SCALE:
> - *val = st->int_vref_mv * st->gain;
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (voltage_uv < 0)
> + return voltage_uv;
> + *val = (voltage_uv / 1000) * st->gain;
> *val2 = chan->scan_type.realbits - 1;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
Re: [PATCH v4 2/2] staging: iio: ad7780: remove unnecessary stashed voltage value
On Mon, 2018-11-05 at 17:16 -0200, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
>
Looks good from my side.
Alex
> Signed-off-by: Renato Lui Geh
> ---
> Changes in v3:
> - removed unnecessary int_vref_mv from ad7780_state
> Changes in v4:
> - removed voltage reading on probe
>
> drivers/staging/iio/adc/ad7780.c | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c7cb05cedbbc..52a914360574 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
> struct regulator*reg;
> struct gpio_desc*powerdown_gpio;
> unsigned intgain;
> - u16 int_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -165,7 +164,7 @@ static int ad7780_probe(struct spi_device *spi)
> {
> struct ad7780_state *st;
> struct iio_dev *indio_dev;
> - int ret, voltage_uv = 0;
> + int ret;
>
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> @@ -185,16 +184,10 @@ static int ad7780_probe(struct spi_device *spi)
> dev_err(&spi->dev, "Failed to enable specified AVdd
> supply\n");
> return ret;
> }
> - voltage_uv = regulator_get_voltage(st->reg);
>
> st->chip_info =
> &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>
> - if (voltage_uv)
> - st->int_vref_mv = voltage_uv / 1000;
> - else
> - dev_warn(&spi->dev, "Reference voltage unspecified\n");
> -
> spi_set_drvdata(spi, indio_dev);
>
> indio_dev->dev.parent = &spi->dev;
Re: [PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info
On Wed, 2018-11-07 at 16:49 -0200, Giuliano Belinassi wrote:
> This patch allows further checking of whatever the chip is (ad778x or
> ad717x).
Hey,
The patch looks good overall.
I only have one nitpick for this patch. See inline.
And you can squash this patch with patch `[PATCH 2/3] staging: iio: ad7780:
check if ad778x before gain update`.
In fact, the title of the squashed patch can just be `staging: iio: ad7780:
check if ad778x before gain update` ; because the code in this patch
implies that it's used to check if the device is an ad778x chip.
This patch doesn't have much value on it's own without the 2nd patch, and
you can do them in a single go.
/*
* Note: yes, I know that these subtle semantics between patch
* splitting & squashing can be a bit annoying ; I don't have a general
* recommendation for them, other than just to keep sending patches
*/
Thanks
Alex
>
> Signed-off-by: Giuliano Belinassi
> ---
> drivers/staging/iio/adc/ad7780.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..6e51bfdb076a 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> unsigned intpattern;
> + u8 is_ad778x;
You could make this `bool` type since you are assigning `true/false` values
to this field. It's generally good to be consistent between type names &
type values when using them [even though in C, these are pretty much the
same].
> };
>
> struct ad7780_state {
> @@ -135,21 +136,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
> .channel = AD7780_CHANNEL(12, 24),
> .pattern = 0x5,
> .pattern_mask = 0x7,
> + .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7780_CHANNEL(16, 24),
> .pattern = 0x5,
> .pattern_mask = 0x7,
> + .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> .pattern = 0x1,
> .pattern_mask = 0x3,
> + .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> .pattern = 0x1,
> .pattern_mask = 0x3,
> + .is_ad778x = true,
> },
> };
>
Re: [PATCH 2/3] staging: iio: ad7780: check if ad778x before gain update
On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating.
>
This looks good.
The only note is that it can be squashed with the 1st patch (which I noted
on the 1st patch).
> Signed-off-by: Giuliano Belinassi
> ---
> drivers/staging/iio/adc/ad7780.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 6e51bfdb076a..0a473aae52f2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -114,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> return -EIO;
>
> - if (raw_sample & AD7780_GAIN)
> - st->gain = 1;
> - else
> - st->gain = 128;
> + if (chip_info->is_ad778x) {
> + if (raw_sample & AD7780_GAIN)
> + st->gain = 1;
> + else
> + st->gain = 128;
> + }
>
> return 0;
> }
Re: [PATCH 3/3] staging: iio: ad7780: generates pattern_mask from PAT bits
On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks in the chip_info table were hardcoded. Now
> they
> are generated using the PAT macros, as described in the datasheets.
>
I like this change :)
I only have nitpicks.
See inline.
> Signed-off-by: Giuliano Belinassi
> ---
> drivers/staging/iio/adc/ad7780.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 0a473aae52f2..fa9e047b5191 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -31,6 +31,8 @@
> #define AD7780_PAT1 BIT(1)
> #define AD7780_PAT0 BIT(0)
>
> +#define AD7170_PAT2 BIT(2)
> +
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -137,25 +139,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7780_CHANNEL(12, 24),
> .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
If you are updating these pattern masks, you should update the default
patterns as well with the macros to be consistent.
And to be a bit more compact, you could define:
#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
#d
efine AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
#define AD7170_PATTERN (AD7780_PAT1 | AD7170_PAT2)
#define AD7780_PATTERN (AD7780_PAT0)
Then you can assign AD7170_PATTERN[_MASK] to AD7170/AD7171 &
AD7780_PATTERN[_MASK] to AD7780/AD7781.
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7780_CHANNEL(16, 24),
> .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1,
> .is_ad778x = true,
> },
> };
Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update
On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating
> through a new variable is_ad778x in chip_info.
>
Looks good.
Alex
> Signed-off-by: Giuliano Belinassi
> ---
> Changes in v2:
> - Squashed is_ad778x declaration commit with the ad778x checkage
> - Changed is_ad778x type to bool
>
> drivers/staging/iio/adc/ad7780.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..9ec2b002539e 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> unsigned intpattern;
> + boolis_ad778x;
> };
>
> struct ad7780_state {
> @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> return -EIO;
>
> - if (raw_sample & AD7780_GAIN)
> - st->gain = 1;
> - else
> - st->gain = 128;
> + if (chip_info->is_ad778x) {
> + if (raw_sample & AD7780_GAIN)
> + st->gain = 1;
> + else
> + st->gain = 128;
> + }
>
> return 0;
> }
> @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
> .channel = AD7780_CHANNEL(12, 24),
> .pattern = 0x5,
> .pattern_mask = 0x7,
> + .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7780_CHANNEL(16, 24),
> .pattern = 0x5,
> .pattern_mask = 0x7,
> + .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> .pattern = 0x1,
> .pattern_mask = 0x3,
> + .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> .pattern = 0x1,
> .pattern_mask = 0x3,
> + .is_ad778x = true,
> },
> };
>
Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits
On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks and patterns in the chip_info table were
> hardcoded. Now they are generated using the PAT macros, as described in
> the datasheets.
One comment about indentation/whitespace.
Rest looks good.
Alex
>
> Signed-off-by: Giuliano Belinassi
> ---
> Changes in v2:
> - Added the PATTERN and PATTERN_MASK macros
> - Update BIT macros alignment to match with PATTERN
> - Generate pattern mask with PATTERN_MASK macros.
>
> drivers/staging/iio/adc/ad7780.c | 40 +++-
> 1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 9ec2b002539e..ff8e3b2d0efc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -22,14 +22,22 @@
> #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)
> +#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)
Changing indentation here doesn't add much value; it's mostly
patch/whitespace noise.
While I agree that it looks nicer to indent all these to the same level,
you also need to think about the fact that the kernel git repo is already
pretty big as-is, so it's a good idea if a patch adds as much code/semantic
value [as possible] with as little change [as possible] and with as good of
readability [as possible].
[ Kind of sounds like a balance act between the 3 things ].
> +
> +#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)
>
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7780_CHANNEL(12, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7780_CHANNEL(16, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> };
Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value
Good catch. Acked-by: Alexandru Ardelean On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote: > Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET. > This was fixed by assigning the correct value instead. > > Signed-off-by: Renato Lui Geh > --- > drivers/staging/iio/adc/ad7780.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/adc/ad7780.c > b/drivers/staging/iio/adc/ad7780.c > index b67412db0318..91e016d534ed 100644 > --- a/drivers/staging/iio/adc/ad7780.c > +++ b/drivers/staging/iio/adc/ad7780.c > @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev, > *val2 = chan->scan_type.realbits - 1; > return IIO_VAL_FRACTIONAL_LOG2; > case IIO_CHAN_INFO_OFFSET: > - *val -= (1 << (chan->scan_type.realbits - 1)); > + *val = -(1 << (chan->scan_type.realbits - 1)); > return IIO_VAL_INT; > } >
Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read
On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
>
> Signed-off-by: Renato Lui Geh
> ---
> Changes in v3:
> - removed initialization (int voltage_uv = 0)
> - returns error when voltage_uv is null
>
> drivers/staging/iio/adc/ad7780.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..f2a11e9424cd 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv;
>
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
> case IIO_CHAN_INFO_SCALE:
> - *val = st->int_vref_mv * st->gain;
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (!voltage_uv)
This looks wrong.
I admit this was done in the same way in the probe function, but that looks
a bit wrong as well.
Typically, the return value of `regulator_get_voltage()` would get checked
with:
ret = regulator_get_voltage(st->reg);
if (ret < 0)
return ret;
*val = ret / 1000;
So, negative values are errors and zero & positive values are valid voltage
values.
> + return -EINVAL;
> + *val = (voltage_uv / 1000) * st->gain;
> *val2 = chan->scan_type.realbits - 1;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
Re: [PATCH v3 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value
On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
>
> Signed-off-by: Renato Lui Geh
> ---
> Changes in v3:
> - removed unnecessary int_vref_mv from ad7780_state
>
> drivers/staging/iio/adc/ad7780.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index f2a11e9424cd..f250370efcf7 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
> struct regulator*reg;
> struct gpio_desc*powerdown_gpio;
> unsigned intgain;
> - u16 int_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
> st->chip_info =
> &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>
> - if (voltage_uv)
> - st->int_vref_mv = voltage_uv / 1000;
> - else
> + if (!voltage_uv)
> dev_warn(&spi->dev, "Reference voltage unspecified\n");
I think you could remove this altogether, and also remove the entire
`voltage_uv = regulator_get_voltage(st->reg);` assignment.
It doesn't make much sense to read the voltage here, since it won't be used
in the probe part anymore.
>
> spi_set_drvdata(spi, indio_dev);
Re: [PATCH 2/2] dt-bindings: iio: adc: Add docs for AD7606 ADC
On Thu, 2018-10-18 at 12:12 +0300, Stefan Popa wrote:
> Document support for AD7606 Analog to Digital Converter.
>
Commments inline
> Signed-off-by: Stefan Popa
> ---
> .../devicetree/bindings/iio/adc/adi,ad7606.txt | 51
> ++
> MAINTAINERS| 1 +
> 2 files changed, 52 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/adc/adi,ad7606.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.txt
> b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.txt
> new file mode 100644
> index 000..dede581
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.txt
> @@ -0,0 +1,51 @@
> +Analog Devices AD7606 Simultaneous Sampling ADC
> +
> +Required properties for the AD7606:
> +
> + - compatible: Must be one of "adi,ad7605-4", "adi,ad7606-8",
> "adi,ad7606-6" or
> +"adi,ad7606-4".
> + - reg: SPI chip select number for the device
> + - spi-max-frequency: Max SPI frequency to use
> + see: Documentation/devicetree/bindings/spi/spi-bus.txt
> + - spi-cpha: See Documentation/devicetree/bindings/spi/spi-bus.txt
I don't know what the typical preference is (with respect to SPI docs), but
I'd just reference the "Documentation/devicetree/bindings/spi/spi-bus.txt
"doc here.
> + - avcc-supply: phandle to the Avcc power supply
> + - interrupts: IRQ line for the ADC
> + see: Documentation/devicetree/bindings/interrupt-
> controller/interrupts.txt
> + - conversion-start-gpio: must be the device tree identifier of the
> CONVST pin.
> + This logic input is used to initiate conversions
> on
> + the analog input channels.
> +
> +Optional properties:
> +
> + - reset-gpio: must be the device tree identifier of the RESET pin. If
> specified,
> +it will be asserted during driver probe.
> + - first-data-gpio: must be the device tree identifier of the FRSTDATA
> pin.
> + The FRSTDATA output indicates when the first channel,
> V1, is
> + being read back on either the parallel, byte or serial
> interface.
> +- standby-gpio: must be the device tree identifier of the STBY pin. This
> pin is used
> + to place the AD7606 into one of two power-down modes,
> Standby mode or
> + Shutdown mode.
> +- range-gpio: must be the device tree identifier of the RANGE pin. The
> polarity on
> + this pin determines the input range of the analog input
> channels. If
> + this pin is tied to a logic high, the analog input range is
> ±10V for
> + allchannels. If this pin is tied to a logic low, the analog
> input range
> + is ±5V for all channels.
> +
nitpick: `range-gpio` && `standby-gpio` entries are indented differently
than `reset-gpio` && `first-data-gpio`
> +Example:
> +
> + adc@0 {
> + compatible = "adi,ad7606-8";
> + reg = <0>;
> + spi-max-frequency = <100>;
> + spi-cpol;
> +
> + avcc-supply = <&adc_vref>;
> +
> + interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-parent = <&gpio>;
> +
> + conversion-start-gpio = <&gpio 17 0>;
> + reset-gpio = <&gpio 27 0>;
> + first-data-gpio = <&gpio 22 0>;
> + standby-gpio = <&gpio 24 0>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 843545d..6d63db4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -845,6 +845,7 @@ L:linux-...@vger.kernel.org
> W: http://ez.analog.com/community/linux-device-drivers
> S: Supported
> F: drivers/iio/adc/ad7606.c
> +F: Documentation/devicetree/bindings/iio/adc/ad7606.txt
>
> ANALOG DEVICES INC AD9389B DRIVER
> M: Hans Verkuil
Re: [PATCH] lib/scatterlist: introduce sg_nents_for_dma() helper
On Wed, 2019-02-27 at 16:15 +0200, Andy Shevchenko wrote: > [External] > > > On Wed, Feb 27, 2019 at 2:26 PM Ardelean, Alexandru > wrote: > > > > On Wed, 2019-02-27 at 11:46 +0200, Andy Shevchenko wrote: > > > [External] > > > > > > > > > +Cc Vinod > > > > > > On Wed, Feb 27, 2019 at 11:45 AM Andy Shevchenko > > > wrote: > > > > > > > > On Wed, Feb 27, 2019 at 10:51 AM Alexandru Ardelean > > > > wrote: > > > > > > > > > > From: Andy Shevchenko > > > > > > > > > > Sometimes the user needs to split each entry on the mapped > > > > > scatter > > > > > list > > > > > due to DMA length constrains. This helper returns a number of > > > > > entities > > > > > assuming that each of them is not bigger than supplied maximum > > > > > length. > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > Signed-off-by: Alexandru Ardelean > > > > > > > > Hmm... Usually we don't accept generic API without users. > > > > Do you have any use case in mind? > > > > Yep: this one: > > https://patchwork.kernel.org/patch/10814527/ > > > > But, I can rework this patch to work without the helper. > > Ah, okay, worth to mention this in comments (in addition to what you > put about the patch below). > > > > > > Patch was sent in 2016 initially to the DMA engine sub-system. > > > > > Link: > > > > > https://patchwork.kernel.org/patch/9389821/ > > > > > This was part of a larger series: > > > > > > > > > > https://patchwork.kernel.org/project/linux-dmaengine/list/?q=sg_nents_for_dma&archive=both&series=&submitter=&delegate=&state=* > > > > > > > > > > I'm not sure if this is supposed to go into DMAEngine or > > > > > lib/scatterlist. > > > > > It doesn't look like lib/scatterlist is managed by DMAEngine, so > > > > > (by > > > > > using > > > > > the `get_maintainers.pl` script) I'm sending this patch to this > > > > > group > > > > > of > > > > > parties. > > > > > > > > The problem the patch tried to solve is much deeper and correct > > > > solution should be more generic, i.e. > > > > each channel should provide a set of parameters, such as DMA > > > > segment > > > > size, to the users (via DMA engine API) and users should prepare > > > > the > > > > SG list according to the limits of the channel. > > > > In that case we don't need to re-split/re-allocate given SG list at > > > > all, which would simplify DMA slave drivers and their users. > > > > > > > > I don't think I managed to follow [or find] the full length of that > > discussion. Or, the conclusion wasn't that obvious to me, from what I > > found. > > I assumed this may have been dropped/forgotten. > > > > In any case, I am fine with just reworking. > > It's fine to apply it, my point that it help to solve a symptom in a > short-term (however this short-term can be easily a long one due to no > guarantee that all drivers using SG for DMA will be converted). > So, please, update the comments and resubmit as a new version. > There's so much uncertainty about this patch, that it makes it easier [and probably makes more sense] to just rework the AXI DMAC patch to not use this helper. This is also in the hope that one day we will get to the more generic API that you mentioned. > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support
On Thu, 2019-02-28 at 11:23 -0300, 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
> ---
> 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
> Changes in v4:
> - Removed useless macro
> - Added default case for switch to suppress warning
> - Removed chunks belonging to filter reading, adding these as a
>patch for itself
>
> drivers/staging/iio/adc/ad7780.c | 90 +---
> 1 file changed, 84 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..87fbcf510d45 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,12 @@
> #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> #define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>
> +#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 };
ad778x_odr_avail[2] is not used in this patch, so it should probably go
into the next one
(i.e. staging: iio: ad7780: add filter reading to ad778x )
one good way of catching stuff like this is to do interactive rebase and
compile your driver on each patch to see if the compiler catches this;
i suspect the compiler would have thrown an error for this change
>
> struct ad7780_chip_info
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,7 +56,10 @@ 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 intint_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -104,17 +113,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;
> + default:
> + break;
The indentation of the break statement is inconsistent with other places.
Still, it does not add much value adding this change as-is, since it does
not change any behavior, and is not an element needed by this change (i.e.
adding gain & filter support via gpios)
> }
>
> 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;
> + gpiod_set_value(st->filter_gpio, val);
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
>
Re: [PATCH v4 2/9] staging: iio: ad7780: add filter reading to ad778x
On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
>
>
> This patch adds the new feature of reading the filter odr value for
> ad778x chips. This value is stored in the chip's state struct whenever a
> read or write call is performed on the chip's driver.
>
> This feature requires sharing SAMP_FREQ. Since the ad717x does not have
> a filter option, the driver only shares the relevant info mask for the
> ad778x family.
>
> Signed-off-by: Renato Lui Geh
> ---
> Changes in v4:
> - As mentioned in the previous patch, this was originally as part of
>the 'add gain & filter gpio support' patch
>
This needs the ad778x_odr_avail[2] array to be moved to this patch.
Otherwise from what I can tell, this looks good.
> drivers/staging/iio/adc/ad7780.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 87fbcf510d45..7a68e90ddf14 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -59,6 +59,7 @@ struct ad7780_state {
> struct gpio_desc*gain_gpio;
> struct gpio_desc*filter_gpio;
> unsigned intgain;
> + unsigned intodr;
> unsigned intint_vref_mv;
>
> struct ad_sigma_delta sd;
> @@ -121,6 +122,9 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> 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;
> default:
> break;
> }
> @@ -163,6 +167,7 @@ static int ad7780_write_raw(struct iio_dev
> *indio_dev,
> val = 0;
> else
> val = 1;
> + st->odr = ad778x_odr_avail[val];
> gpiod_set_value(st->filter_gpio, val);
> break;
> default:
> @@ -184,6 +189,7 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
>
> if (chip_info->is_ad778x) {
> st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> + st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> }
>
> return 0;
> @@ -196,17 +202,19 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> };
>
> #define AD7780_CHANNEL(bits, wordsize) \
> + AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7170_CHANNEL(bits, wordsize) \
> AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
>
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> - .channel = AD7780_CHANNEL(12, 24),
> + .channel = AD7170_CHANNEL(12, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> - .channel = AD7780_CHANNEL(16, 24),
> + .channel = AD7170_CHANNEL(16, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> --
> 2.21.0
>
Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
>
>
> The AD7780 driver contains status pattern bits designed for checking
> whether serial transfers have been correctly performed. Pattern macros
> were previously generated through bit fields. This patch sets good
> pattern values directly and masks through GENMASK.
>
> Signed-off-by: Renato Lui Geh
> ---
> drivers/staging/iio/adc/ad7780.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 7a68e90ddf14..56c49e28f432 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -28,16 +29,13 @@
> #define AD7780_ID1 BIT(4)
> #define AD7780_ID0 BIT(3)
> #define AD7780_GAINBIT(2)
> -#define AD7780_PAT1BIT(1)
> -#define AD7780_PAT0BIT(0)
I don't see a problem to leave the bitfields; they can be read & matched
easier with the datasheet.
>
> -#define AD7780_PATTERN (AD7780_PAT0)
> -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
>
> -#define AD7170_PAT2BIT(2)
> +#define AD7780_PATTERN_GOOD1
It was also nice before that the PAT0..PAT2 bitfields were used to define a
good pattern, since it's easier to match with the datasheet.
> +#define AD7780_PATTERN_MASKGENMASK(1, 0)
I like the general usage of GENMASK, but I'm not sure in this case it's
worth doing. Maybe I missed a discussion somewhere, about doing this
change, but it is mostly a cosmetic without any functional change.
>
> -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> +#define AD7170_PATTERN_GOOD5
> +#define AD7170_PATTERN_MASKGENMASK(2, 0)
>
> #define AD7780_GAIN_MIDPOINT 64
> #define AD7780_FILTER_MIDPOINT 13350
> @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7170_CHANNEL(12, 24),
> - .pattern = AD7170_PATTERN,
> + .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7170_CHANNEL(16, 24),
> - .pattern = AD7170_PATTERN,
> + .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> - .pattern = AD7780_PATTERN,
> + .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> - .pattern = AD7780_PATTERN,
> + .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> --
> 2.21.0
>
Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask
On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote: > > > The ad7780 supports both the ad778x and ad717x families. Each chip has > a corresponding ID. This patch provides a mask for extracting ID values > from the status bits and also macros for the correct values for the > ad7170, ad7171, ad7780 and ad7781. > > Signed-off-by: Renato Lui Geh > --- > drivers/staging/iio/adc/ad7780.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7780.c > b/drivers/staging/iio/adc/ad7780.c > index 56c49e28f432..ad7617a3a141 100644 > --- a/drivers/staging/iio/adc/ad7780.c > +++ b/drivers/staging/iio/adc/ad7780.c > @@ -26,10 +26,14 @@ > #define AD7780_RDY BIT(7) > #define AD7780_FILTER BIT(6) > #define AD7780_ERR BIT(5) > -#define AD7780_ID1 BIT(4) > -#define AD7780_ID0 BIT(3) > #define AD7780_GAINBIT(2) > > +#define AD7170_ID 0 > +#define AD7171_ID 1 > +#define AD7780_ID 1 > +#define AD7781_ID 0 > + > +#define AD7780_ID_MASK (BIT(3) | BIT(4)) This also doesn't have any functionality change. The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused (maybe in a later patch they are ?). I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, since they're easier matched with the datasheet. > > #define AD7780_PATTERN_GOOD1 > #define AD7780_PATTERN_MASKGENMASK(1, 0) > -- > 2.21.0 >
Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init
On Thu, 2019-02-28 at 11:25 -0300, 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
> ---
> 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 ad7617a3a141..12aef0f101bc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -255,16 +255,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];
>
> @@ -284,7 +274,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) {
> @@ -295,7 +285,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,
> @@ -306,10 +296,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;
> + }
> +
I'm probably missing something, but other than the fact that this moves the
regulator init after the GPIOs init, it doesn't change much.
The order of the probe & remove is more-or-less the same.
The GPIOs will be free'd via devm_ API/stuff.
> ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> if (ret)
> goto error_disable_reg;
> --
> 2.21.0
>
Re: [PATCH v4 0/9] staging: iio: ad7780: move out of staging
On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote: > The patch-series is a bit big. I guess that the intent is to move this out-of-staging, but various patches are holding this in it's place. For patch series above a certain size, you could get many re-spins [V2,3,4... so on]. You could send some of the changes as individual patches, or group them in series of 1,2 or 3 patches. That way, you "parallelize" patch sending, and when you get reviews on each patch, you can re-spin them individually. You'll find over time that certain patches get accepted on V1, others on V2 and some on V7 [ hopefully, there isn't any frustration at that point ]. Well, this is a technique I use to distribute some of my upstream-patch- work, so that I can switch easier between internal-work & upstreaming-work. Coming back to this patch-series. My general input, is that the patches are fine over-all; some are just cosmetics/noise/a-different-way-of-doing-things-for-this-driver, and those usually can be left to preference [of the maintainer usually]. I do suggest to not hurry when re-spinning patches, and not change too much the number of patches in a new series. That can complicate things sometimes. But, if doing small patch-series or individual patches, you won't have this problem too much. Thanks Alex > > This series of patches contains the following: > - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x >family chips; > - Filter reading for the ad778x; > - Sets pattern macro values and mask for PATTERN status bits; > - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID >status bits checking; > - Moves regulator initialization to after GPIO init to maintain >consistency between probe and remove; > - Copyright edits, adding SPDX identifier and new copyright holder; > - Moves the ad7780 driver out of staging to the mainline; > - Adds device tree binding for the ad7780 driver. > > Renato Lui Geh (9): > staging: iio: ad7780: add gain & filter gpio support > staging: iio: ad7780: add filter reading to ad778x > staging: iio: ad7780: set pattern values and masks directly > staging:iio:ad7780: add chip ID values and mask > staging: iio: ad7780: move regulator to after GPIO init > staging: iio: ad7780: add SPDX identifier > staging: iio: ad7780: add new copyright holder > staging: iio: ad7780: moving ad7780 out of staging > staging: iio: ad7780: add device tree binding > > Changelog: > *v3 > - SPDX and regulator init as patches > - Renamed filter to odr and ad778x_filter to ad778x_odr_avail > - Removed unnecessary regulator disabling > - Removed unnecessary AD_SD_CHANNEL macro > - Changed unsigned int to unsigned long long to avoid overflow > *v4 > - Split gain & filter patch into two, with the new commit adding only >filter reading > - Changed pattern values to direct values, and added pattern mask > - Added ID values and mask > - Added new copyright holder > - Added device tree binding to the ad7780 driver > > .../bindings/iio/adc/adi,ad7780.txt | 48 +++ > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7780.c | 365 ++ > drivers/staging/iio/adc/Kconfig | 13 - > drivers/staging/iio/adc/Makefile | 1 - > drivers/staging/iio/adc/ad7780.c | 277 - > 7 files changed, 426 insertions(+), 291 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt > create mode 100644 drivers/iio/adc/ad7780.c > delete mode 100644 drivers/staging/iio/adc/ad7780.c > > -- > 2.21.0 >
Re: [PATCH] lib/scatterlist: introduce sg_nents_for_dma() helper
On Wed, 2019-02-27 at 11:46 +0200, Andy Shevchenko wrote: > [External] > > > +Cc Vinod > > On Wed, Feb 27, 2019 at 11:45 AM Andy Shevchenko > wrote: > > > > On Wed, Feb 27, 2019 at 10:51 AM Alexandru Ardelean > > wrote: > > > > > > From: Andy Shevchenko > > > > > > Sometimes the user needs to split each entry on the mapped scatter > > > list > > > due to DMA length constrains. This helper returns a number of > > > entities > > > assuming that each of them is not bigger than supplied maximum > > > length. > > > > > > Signed-off-by: Andy Shevchenko > > > Signed-off-by: Alexandru Ardelean > > > > Hmm... Usually we don't accept generic API without users. > > Do you have any use case in mind? Yep: this one: https://patchwork.kernel.org/patch/10814527/ But, I can rework this patch to work without the helper. > > > > > Patch was sent in 2016 initially to the DMA engine sub-system. > > > Link: > > > https://patchwork.kernel.org/patch/9389821/ > > > This was part of a larger series: > > > > > > https://patchwork.kernel.org/project/linux-dmaengine/list/?q=sg_nents_for_dma&archive=both&series=&submitter=&delegate=&state=* > > > > > > I'm not sure if this is supposed to go into DMAEngine or > > > lib/scatterlist. > > > It doesn't look like lib/scatterlist is managed by DMAEngine, so (by > > > using > > > the `get_maintainers.pl` script) I'm sending this patch to this group > > > of > > > parties. > > > > The problem the patch tried to solve is much deeper and correct > > solution should be more generic, i.e. > > each channel should provide a set of parameters, such as DMA segment > > size, to the users (via DMA engine API) and users should prepare the > > SG list according to the limits of the channel. > > In that case we don't need to re-split/re-allocate given SG list at > > all, which would simplify DMA slave drivers and their users. > > I don't think I managed to follow [or find] the full length of that discussion. Or, the conclusion wasn't that obvious to me, from what I found. I assumed this may have been dropped/forgotten. In any case, I am fine with just reworking. > > We discussed this topic back in 2016 with Vinod on LPC, but seems it's > > not critical to solve. My case is to improve DMA performance for 8250 > > UART. > > > > -- > > With Best Regards, > > Andy Shevchenko > > > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH] iio: dac: ad5592r-base: Replace indio_dev->mlock with own device lock
On Wed, 2020-05-20 at 09:18 +0300, Sergiu Cuciurean wrote:
> [External]
>
> As part of the general cleanup of indio_dev->mlock, this change replaces
> it with a local lock on the device's state structure.
>
This requires a V2.
It produces a warning:
159 | struct iio_dev *iio_dev = iio_priv_to_dev(st);
| ^~~
drivers/iio/dac/ad5592r-base.c: In function ‘ad5592r_set_channel_modes’:
drivers/iio/dac/ad5592r-base.c:200:18: warning: unused variable ‘iio_dev’
[-Wunused-variable]
200 | struct iio_dev *iio_dev = iio_priv_to_dev(st);
You can remove both instances of
struct iio_dev *iio_dev = iio_priv_to_dev(st);
> Signed-off-by: Sergiu Cuciurean
> ---
> drivers/iio/dac/ad5592r-base.c | 28 +++-
> drivers/iio/dac/ad5592r-base.h | 1 +
> 2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-
> base.c
> index e2110113e884..10109eb81db2 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -166,10 +166,10 @@ static int ad5592r_reset(struct ad5592r_state *st)
> udelay(1);
> gpiod_set_value(gpio, 1);
> } else {
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
> /* Writing this magic value resets the device */
> st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
> }
>
> udelay(250);
> @@ -247,7 +247,7 @@ static int ad5592r_set_channel_modes(struct
> ad5592r_state *st)
> }
> }
>
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
>
> /* Pull down unused pins to GND */
> ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
> @@ -285,7 +285,7 @@ static int ad5592r_set_channel_modes(struct
> ad5592r_state *st)
> ret = -EIO;
>
> err_unlock:
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> @@ -314,11 +314,11 @@ static int ad5592r_write_raw(struct iio_dev
> *iio_dev,
> if (!chan->output)
> return -EINVAL;
>
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = st->ops->write_dac(st, chan->channel, val);
> if (!ret)
> st->cached_dac[chan->channel] = val;
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> case IIO_CHAN_INFO_SCALE:
> if (chan->type == IIO_VOLTAGE) {
> @@ -333,12 +333,12 @@ static int ad5592r_write_raw(struct iio_dev
> *iio_dev,
> else
> return -EINVAL;
>
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
>
> ret = st->ops->reg_read(st, AD5592R_REG_CTRL,
> &st->cached_gp_ctrl);
> if (ret < 0) {
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> @@ -360,7 +360,7 @@ static int ad5592r_write_raw(struct iio_dev *iio_dev,
>
> ret = st->ops->reg_write(st, AD5592R_REG_CTRL,
>st->cached_gp_ctrl);
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret;
> }
> @@ -382,7 +382,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
>
> if (!chan->output) {
> ret = st->ops->read_adc(st, chan->channel,
> &read_val);
> @@ -419,7 +419,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
> } else {
> int mult;
>
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
>
> if (chan->output)
> mult = !!(st->cached_gp_ctrl &
> @@ -437,7 +437,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
> case IIO_CHAN_INFO_OFFSET:
> ret = ad5592r_get_vref(st);
>
> - mutex_lock(&iio_dev->mlock);
> + mutex_lock(&st->lock);
>
> if (st->cached_gp_ctrl & AD5592R_REG_CTRL_ADC_RANGE)
> *val = (-34365 * 25) / ret;
> @@ -450,7 +450,7 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
> }
>
> unlock:
> - mutex_unlock(&iio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> @@ -625,6 +625,8 @@ int ad5592r_pr
RE: [PATCH 5/6] include: fpga: adi-axi-common.h: add definitions for supported FPGAs
-Original Message-
From: Tom Rix
Sent: Wednesday, August 5, 2020 7:02 PM
To: Ardelean, Alexandru ;
linux-...@vger.kernel.org; linux-f...@vger.kernel.org;
linux-kernel@vger.kernel.org
Cc: mturque...@baylibre.com; sb...@kernel.org; m...@kernel.org; Caprioru,
Mircea
Subject: Re: [PATCH 5/6] include: fpga: adi-axi-common.h: add definitions for
supported FPGAs
[External]
On 8/4/20 4:06 AM, Alexandru Ardelean wrote:
> From: Mircea Caprioru
>
> All (newer) FPGA IP cores supported by Analog Devices, store
> information in the synthesized designs. This information describes
> various parameters, including the family of boards on which this is
> deployed, speed-grade, and so on.
>
> Currently, some of these definitions are deployed mostly on Xilinx
> boards, but they have been considered also for FPGA boards from other vendors.
>
> The register definitions are described at this link:
> https://wiki.analog.com/resources/fpga/docs/hdl/regmap
> (the 'Base (common to all cores)' section).
>
> Signed-off-by: Mircea Caprioru
> Signed-off-by: Alexandru Ardelean
> ---
> include/linux/fpga/adi-axi-common.h | 37
> +
> 1 file changed, 37 insertions(+)
>
> diff --git a/include/linux/fpga/adi-axi-common.h
> b/include/linux/fpga/adi-axi-common.h
> index 141ac3f251e6..7cca2d62cc45 100644
> --- a/include/linux/fpga/adi-axi-common.h
> +++ b/include/linux/fpga/adi-axi-common.h
> @@ -13,6 +13,9 @@
>
> #define ADI_AXI_REG_VERSION 0x
>
> +#define ADI_AXI_REG_FPGA_INFO0x001C
> +#define ADI_AXI_REG_FPGA_VOLTAGE 0x0140
> +
> #define ADI_AXI_PCORE_VER(major, minor, patch) \
> (((major) << 16) | ((minor) << 8) | (patch))
>
> @@ -20,4 +23,38 @@
> #define ADI_AXI_PCORE_VER_MINOR(version) (((version) >> 8) & 0xff)
> #define ADI_AXI_PCORE_VER_PATCH(version) ((version) & 0xff)
>
> +#define ADI_AXI_INFO_FPGA_VOLTAGE(val) ((val) & 0x)
> +
> +#define ADI_AXI_INFO_FPGA_TECH(info) (((info) >> 24) & 0xff)
> +#define ADI_AXI_INFO_FPGA_FAMILY(info) (((info) >> 16) & 0xff)
> +#define ADI_AXI_INFO_FPGA_SPEED_GRADE(info) (((info) >> 8) & 0xff)
> +
> +enum adi_axi_fgpa_technology {
> enum types are defined but not used. It would be better to convert all of
> these to #defines.
>
> These are only the Xilinx values. Need to add the Intel values from
> https://urldefense.com/v3/__https://github.com/analogdevicesinc/hdl/blob/master/library/scripts/adi_intel_device_info_enc.tcl__;!!A3Ni8CS0y2Y!tpLxcnpvcjFVEYz0mfRNW03dYet-iklk2s_eG4FmRmeyMOcldd8f-zpebB5NnJLOjl1rNw$
>
> The #define names need to include XILINX or INTEL.
Ah, good point.
This patch was initially written before the Intel stuff was added.
Will update.
Apologies for the mis-formatted reply/email; will need to include my Gmail
account to reply neater here.
> Tom
> + ADI_AXI_FPGA_TECH_UNKNOWN = 0,
> + ADI_AXI_FPGA_TECH_SERIES7,
> + ADI_AXI_FPGA_TECH_ULTRASCALE,
> + ADI_AXI_FPGA_TECH_ULTRASCALE_PLUS,
> +};
> +
> +enum adi_axi_fpga_family {
> + ADI_AXI_FPGA_FAMILY_UNKNOWN = 0,
> + ADI_AXI_FPGA_FAMILY_ARTIX,
> + ADI_AXI_FPGA_FAMILY_KINTEX,
> + ADI_AXI_FPGA_FAMILY_VIRTEX,
> + ADI_AXI_FPGA_FAMILY_ZYNQ,
> +};
> +
> +enum adi_axi_fpga_speed_grade {
> + ADI_AXI_FPGA_SPEED_UNKNOWN = 0,
> + ADI_AXI_FPGA_SPEED_1= 10,
> + ADI_AXI_FPGA_SPEED_1L = 11,
> + ADI_AXI_FPGA_SPEED_1H = 12,
> + ADI_AXI_FPGA_SPEED_1HV = 13,
> + ADI_AXI_FPGA_SPEED_1LV = 14,
> + ADI_AXI_FPGA_SPEED_2= 20,
> + ADI_AXI_FPGA_SPEED_2L = 21,
> + ADI_AXI_FPGA_SPEED_2LV = 22,
> + ADI_AXI_FPGA_SPEED_3= 30,
> +};
> +
> #endif /* ADI_AXI_COMMON_H_ */
RE: [PATCH 1/2] clk: axi-clkgen: add support for ZynqMP (UltraScale)
> -Original Message-
> From: Tom Rix
> Sent: Thursday, December 24, 2020 4:03 PM
> To: Ardelean, Alexandru ; linux-
> c...@vger.kernel.org; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: mturque...@baylibre.com; sb...@kernel.org; robh...@kernel.org;
> l...@metafoo.de; linux-f...@vger.kernel.org; m...@kernel.org; Bogdan,
> Dragos ; Mathias Tausen
>
> Subject: Re: [PATCH 1/2] clk: axi-clkgen: add support for ZynqMP (UltraScale)
>
> [External]
>
>
> On 12/21/20 6:42 AM, Alexandru Ardelean wrote:
> > From: Dragos Bogdan
> >
> > This IP core also works and is supported on the Xilinx ZynqMP
> > (UltraScale) FPGA boards.
> > This patch enables the driver to be available on these platforms as well.
> >
> > Since axi-clkgen is now supported on ZYNQMP, we need to make sure the
> > max/min frequencies of the PFD and VCO are respected.
> >
> > This change adds two new compatible strings to select limits for Zynq
> > or ZynqMP from the device data (in the OF table). The old compatible
> > string (i.e. adi,axi-clkgen-2.00.a) is the same as
> > adi,zynq-axi-clkgen-2.00.a, since the original version of this driver
> > was designed on top of that platform.
> >
Apologies for the late reply on this, this looks like it went to some folder in
my inbox and I forgot about it.
And Happy New Year :)
> > Signed-off-by: Dragos Bogdan
> > Signed-off-by: Mathias Tausen
> > Signed-off-by: Alexandru Ardelean
> > ---
> >
> > This is a re-spin of an older series.
> > It needed to wait a txt -> yaml dt conversion:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20201013143421.84188-1-alexandru.ardel...@analog.com/__;!!A
> > 3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrDY
> > iPPDLdVc-wg$
> >
> > It's 2 patches squashed into one:
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20200929144417.89816-12-alexandru.ardel...@analog.com/__;!!
> > A3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrD
> > YiPPAkwzbuMA$
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux
> > -clk/patch/20200929144417.89816-14-alexandru.ardel...@analog.com/__;!!
> > A3Ni8CS0y2Y!sPnN7f9iWjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-
> pZzEaFqTrD
> > YiPPCaD6QXfQ$
> >
> > The series from where all this started is:
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-clk/20200929
> > 144417.89816-1-
> alexandru.ardel...@analog.com/__;!!A3Ni8CS0y2Y!sPnN7f9i
> > WjhGkjI-Gu8RTo73gVDV6x0-5m42Pu2kZwvVbHN-pZzEaFqTrDYiPPAjuz4oHg$
> >
> > Well, v4 was the point where I decided to split this into smaller
> > series, and also do the conversion of the binding to yaml.
> >
> > drivers/clk/Kconfig | 2 +-
> > drivers/clk/clk-axi-clkgen.c | 15 +++
> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > 85856cff506c..252333e585e7 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -247,7 +247,7 @@ config CLK_TWL6040
> >
> > config COMMON_CLK_AXI_CLKGEN
> > tristate "AXI clkgen driver"
> > - depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> > + depends on ARCH_ZYNQ || ARCH_ZYNQMP || MICROBLAZE ||
> COMPILE_TEST
> > help
> > Support for the Analog Devices axi-clkgen pcore clock generator for
> Xilinx
> > FPGAs. It is commonly used in Analog Devices' reference designs.
> > diff --git a/drivers/clk/clk-axi-clkgen.c
> > b/drivers/clk/clk-axi-clkgen.c index ad86e031ba3e..a413c13334ff 100644
> > --- a/drivers/clk/clk-axi-clkgen.c
> > +++ b/drivers/clk/clk-axi-clkgen.c
> > @@ -108,6 +108,13 @@ static uint32_t axi_clkgen_lookup_lock(unsigned int
> m)
> > return 0x1f1f00fa;
> > }
> >
>
> Could something like
>
> #ifdef ARCH_ZYNQMP
So, we decided not to do this in an older discussion:
https://lore.kernel.org/linux-clk/20200929153040.GA114067@archbook/
https://lore.kernel.org/linux-clk/20200930171607.GA121420@archbook/
Because, these drivers should also be usable in cases where a ZynqMP or Zynq
device can be plugged in via PCIe on a host device.
Thinking about it now, I think I should remove the "depends on ARCH_ZYNQ ||
ARCH_ZYNQMP" limitation.
>
> > +static const struct axi_clkgen_limits axi_clkgen_zynqmp_default_limits = {
> > + .fpfd_min = 1,
> > + .fpfd_max = 45,
> > +
RE: [PATCH v5 2/3] spi: Add SPI_NO_TX/RX support
> -Original Message- > From: Andy Shevchenko > Sent: Monday, December 21, 2020 4:37 PM > To: Ardelean, Alexandru > Cc: linux-spi ; devicetree > ; Linux Kernel Mailing List ker...@vger.kernel.org>; Bogdan, Dragos ; > Mark Brown ; Rob Herring > Subject: Re: [PATCH v5 2/3] spi: Add SPI_NO_TX/RX support > > On Mon, Dec 21, 2020 at 4:34 PM Andy Shevchenko > wrote: > > On Mon, Dec 21, 2020 at 4:15 PM Alexandru Ardelean > > wrote: > > > > > > From: Dragos Bogdan > > > > > > Transmit/receive only is a valid SPI mode. For example, the MOSI/TX > > > line might be missing from an ADC while for a DAC the MISO/RX line > > > may be optional. This patch adds these two new modes: SPI_NO_TX and > > > SPI_NO_RX. This way, the drivers will be able to identify if any of > > > these two lines is missing and to adjust the transfers accordingly. > > > > > > Signed-off-by: Dragos Bogdan > > > > Missed Co-developed-by: Alexandru ... ? Nah, that's fine from my side without that tag. > > > > Anyway, looks good to me, > > Reviewed-by: Andy Shevchenko > > One nit, though... > > > > - "setup: can not select dual and quad at the same time\n"); > > > + "setup: can not select any two of dual, quad and no-rx/tx > > > " > > > + "at the same time\n"); > > Can we avoid splitting string literals which are assumed to be on one line > when > printed? It ends up at about 96 cols, but it's within limits. The patch may have been written before the new 100 col-width limit. > > -- > With Best Regards, > Andy Shevchenko
RE: [PATCH v3 1/4] Input: adp5589-keys - add default platform data
> -Original Message-
> From: Alexandru Ardelean
> Sent: Friday, November 27, 2020 1:14 PM
> To: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Cc: l...@metafoo.de; dmitry.torok...@gmail.com; robh...@kernel.org;
> Ardelean, Alexandru
> Subject: [PATCH v3 1/4] Input: adp5589-keys - add default platform data
>
> From: Lars-Peter Clausen
>
> If no platform data is supplied use a dummy platform data that configures the
> device in GPIO only mode. This change adds a adp5589_kpad_pdata_get() helper
> that returns the default platform-data. This can be later extended to load
> configuration from device-trees or ACPI.
>
Ping on this for the input subsystem.
Since patch 4 was applied by Rob, maybe for input, only the first 3 should be
applied.
Or, should I re-send just the first 3?
Thanks
Alex
> Signed-off-by: Lars-Peter Clausen
> Signed-off-by: Alexandru Ardelean
> ---
>
> Changelog v2 - v3:
> * https://lore.kernel.org/linux-input/20201124082255.13427-1-
> alexandru.ardel...@analog.com/
> * added patch 'dt-bindings: add ADP5585/ADP5589 entries to trivial-devices'
>
> drivers/input/keyboard/adp5589-keys.c | 33 +++
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/adp5589-keys.c
> b/drivers/input/keyboard/adp5589-keys.c
> index e2cdf14d90cd..742bf4b97dbb 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -369,6 +369,25 @@ static const struct adp_constants const_adp5585 = {
> .reg= adp5585_reg,
> };
>
> +static const struct adp5589_gpio_platform_data adp5589_default_gpio_pdata
> = {
> + .gpio_start = -1,
> +};
> +
> +static const struct adp5589_kpad_platform_data adp5589_default_pdata = {
> + .gpio_data = &adp5589_default_gpio_pdata, };
> +
> +static const struct adp5589_kpad_platform_data *adp5589_kpad_pdata_get(
> + struct device *dev)
> +{
> + const struct adp5589_kpad_platform_data *pdata =
> +dev_get_platdata(dev);
> +
> + if (!pdata)
> + pdata = &adp5589_default_pdata;
> +
> + return pdata;
> +}
> +
> static int adp5589_read(struct i2c_client *client, u8 reg) {
> int ret = i2c_smbus_read_byte_data(client, reg); @@ -498,7 +517,8 @@
> static int adp5589_build_gpiomap(struct adp5589_kpad *kpad, static int
> adp5589_gpio_add(struct adp5589_kpad *kpad) {
> struct device *dev = &kpad->client->dev;
> - const struct adp5589_kpad_platform_data *pdata =
> dev_get_platdata(dev);
> + const struct adp5589_kpad_platform_data *pdata =
> + adp5589_kpad_pdata_get(dev);
> const struct adp5589_gpio_platform_data *gpio_data = pdata-
> >gpio_data;
> int i, error;
>
> @@ -619,7 +639,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad) {
> struct i2c_client *client = kpad->client;
> const struct adp5589_kpad_platform_data *pdata =
> - dev_get_platdata(&client->dev);
> + adp5589_kpad_pdata_get(&client->dev);
> u8 (*reg) (u8) = kpad->var->reg;
> unsigned char evt_mode1 = 0, evt_mode2 = 0, evt_mode3 = 0;
> unsigned char pull_mask = 0;
> @@ -824,7 +844,7 @@ static int adp5589_keypad_add(struct adp5589_kpad
> *kpad, unsigned int revid) {
> struct i2c_client *client = kpad->client;
> const struct adp5589_kpad_platform_data *pdata =
> - dev_get_platdata(&client->dev);
> + adp5589_kpad_pdata_get(&client->dev);
> struct input_dev *input;
> unsigned int i;
> int error;
> @@ -948,7 +968,7 @@ static int adp5589_probe(struct i2c_client *client, {
> struct adp5589_kpad *kpad;
> const struct adp5589_kpad_platform_data *pdata =
> - dev_get_platdata(&client->dev);
> + adp5589_kpad_pdata_get(&client->dev);
> unsigned int revid;
> int error, ret;
>
> @@ -958,11 +978,6 @@ static int adp5589_probe(struct i2c_client *client,
> return -EIO;
> }
>
> - if (!pdata) {
> - dev_err(&client->dev, "no platform data?\n");
> - return -EINVAL;
> - }
> -
> kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
> if (!kpad)
> return -ENOMEM;
> --
> 2.27.0
RE: [PATCH] Input: adp5589-keys - mark suspend and resume methods as __maybe_unused
> -Original Message-
> From: Dmitry Torokhov
> Sent: Thursday, November 19, 2020 9:24 AM
> To: linux-in...@vger.kernel.org
> Cc: Ardelean, Alexandru ; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] Input: adp5589-keys - mark suspend and resume methods as
> __maybe_unused
>
> [External]
>
> This improves compile coverage of the code; unused code will be dropped by the
> linker.
>
Acked-by: Alexandru Ardelean
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/input/keyboard/adp5589-keys.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/adp5589-keys.c
> b/drivers/input/keyboard/adp5589-keys.c
> index 31145a85c819..a9b69a268c09 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -1016,8 +1016,7 @@ static int adp5589_probe(struct i2c_client *client,
> return 0;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int adp5589_suspend(struct device *dev)
> +static int __maybe_unused adp5589_suspend(struct device *dev)
> {
> struct adp5589_kpad *kpad = dev_get_drvdata(dev);
> struct i2c_client *client = kpad->client; @@ -1033,7 +1032,7 @@ static
> int adp5589_suspend(struct device *dev)
> return 0;
> }
>
> -static int adp5589_resume(struct device *dev)
> +static int __maybe_unused adp5589_resume(struct device *dev)
> {
> struct adp5589_kpad *kpad = dev_get_drvdata(dev);
> struct i2c_client *client = kpad->client; @@ -1048,7 +1047,6 @@ static
> int adp5589_resume(struct device *dev)
>
> return 0;
> }
> -#endif
>
> static SIMPLE_DEV_PM_OPS(adp5589_dev_pm_ops, adp5589_suspend,
> adp5589_resume);
>
> --
> 2.29.2.299.gdc1121823c-goog
>
>
> --
> Dmitry
RE: [PATCH] Input: adp5589-keys - use BIT()
> -Original Message- > From: Dmitry Torokhov > Sent: Thursday, November 19, 2020 9:25 AM > To: linux-in...@vger.kernel.org > Cc: Ardelean, Alexandru ; linux- > ker...@vger.kernel.org > Subject: [PATCH] Input: adp5589-keys - use BIT() > > [External] > > Let's use BIT() macro instead of explicitly shifting '1'. As a first iteration for cleaning up bitmask stuff, this looks good. So: Acked-by: Alexandru Ardelean As a continuation, in some places, some GENMASK() and FIELD_GET() macros would be an idea for some contiguous bits. I can do that later. For now, my todo-list doesn't include these conversions. Thanks Alex > > Signed-off-by: Dmitry Torokhov > --- > drivers/input/keyboard/adp5589-keys.c | 69 ++- > 1 file changed, 35 insertions(+), 34 deletions(-) > > diff --git a/drivers/input/keyboard/adp5589-keys.c > b/drivers/input/keyboard/adp5589-keys.c > index a9b69a268c09..70c8d1c298ee 100644 > --- a/drivers/input/keyboard/adp5589-keys.c > +++ b/drivers/input/keyboard/adp5589-keys.c > @@ -7,6 +7,7 @@ > * Copyright (C) 2010-2011 Analog Devices Inc. > */ > > +#include > #include > #include > #include > @@ -153,48 +154,48 @@ > #define ADP5589_5_MAN_ID 0x02 > > /* GENERAL_CFG Register */ > -#define OSC_EN (1 << 7) > +#define OSC_EN BIT(7) > #define CORE_CLK(x) (((x) & 0x3) << 5) > -#define LCK_TRK_LOGIC(1 << 4)/* ADP5589 only */ > -#define LCK_TRK_GPI (1 << 3)/* ADP5589 only */ > -#define INT_CFG (1 << 1) > -#define RST_CFG (1 << 0) > +#define LCK_TRK_LOGICBIT(4) /* ADP5589 only */ > +#define LCK_TRK_GPI BIT(3) /* ADP5589 only */ > +#define INT_CFG BIT(1) > +#define RST_CFG BIT(0) > > /* INT_EN Register */ > -#define LOGIC2_IEN (1 << 5)/* ADP5589 only */ > -#define LOGIC1_IEN (1 << 4) > -#define LOCK_IEN (1 << 3)/* ADP5589 only */ > -#define OVRFLOW_IEN (1 << 2) > -#define GPI_IEN (1 << 1) > -#define EVENT_IEN(1 << 0) > +#define LOGIC2_IEN BIT(5) /* ADP5589 only */ > +#define LOGIC1_IEN BIT(4) > +#define LOCK_IEN BIT(3) /* ADP5589 only */ > +#define OVRFLOW_IEN BIT(2) > +#define GPI_IEN BIT(1) > +#define EVENT_IENBIT(0) > > /* Interrupt Status Register */ > -#define LOGIC2_INT (1 << 5)/* ADP5589 only */ > -#define LOGIC1_INT (1 << 4) > -#define LOCK_INT (1 << 3)/* ADP5589 only */ > -#define OVRFLOW_INT (1 << 2) > -#define GPI_INT (1 << 1) > -#define EVENT_INT(1 << 0) > +#define LOGIC2_INT BIT(5) /* ADP5589 only */ > +#define LOGIC1_INT BIT(4) > +#define LOCK_INT BIT(3) /* ADP5589 only */ > +#define OVRFLOW_INT BIT(2) > +#define GPI_INT BIT(1) > +#define EVENT_INTBIT(0) > > /* STATUS Register */ > -#define LOGIC2_STAT (1 << 7)/* ADP5589 only */ > -#define LOGIC1_STAT (1 << 6) > -#define LOCK_STAT(1 << 5)/* ADP5589 only */ > +#define LOGIC2_STAT BIT(7) /* ADP5589 only */ > +#define LOGIC1_STAT BIT(6) > +#define LOCK_STATBIT(5) /* ADP5589 only */ > #define KEC 0x1F > > /* PIN_CONFIG_D Register */ > -#define C4_EXTEND_CFG(1 << 6)/* RESET2 */ > -#define R4_EXTEND_CFG(1 << 5)/* RESET1 */ > +#define C4_EXTEND_CFGBIT(6) /* RESET2 */ > +#define R4_EXTEND_CFGBIT(5) /* RESET1 */ > > /* LOCK_CFG */ > -#define LOCK_EN (1 << 0) > +#define LOCK_EN BIT(0) > > #define PTIME_MASK 0x3 > #define LTIME_MASK 0x3 /* ADP5589 only */ > > /* Key Event Register xy */ > -#define KEY_EV_PRESSED (1 << 7) > -#define KEY_EV_MASK (0x7F) > +#define KEY_EV_PRESSED BIT(7) > +#define KEY_EV_MASK 0x7F > > #define KEYP_MAX_EVENT 16 > #define ADP5589_MAXGPIO 19 > @@ -472,7 +473,7 @@ static int adp5589_build_gpiomap(struct adp5589_kpad > *kpad, > memset(pin_used, false, sizeof(pin_used)); > > for (i = 0; i < kpad->var->maxgpio; i++) > - if (pdata->keypad_en_mask & (1 << i)) > + if (pdata->keypad_en_mask & BIT(i)) > pin_used[i] = true; > > for (i = 0; i < kpad->gpimapsize; i++) @@ -651,13 +652,13 @@ static int > adp5589_setup(stru
RE: [PATCH v2] uio/uio_pci_generic: remove unneeded pci_set_drvdata()
> -Original Message-
> From: Alexandru Ardelean
> Sent: Monday, November 23, 2020 4:35 PM
> To: linux-kernel@vger.kernel.org; k...@vger.kernel.org
> Cc: m...@redhat.com; gre...@linuxfoundation.org; Ardelean, Alexandru
>
> Subject: [PATCH v2] uio/uio_pci_generic: remove unneeded pci_set_drvdata()
>
> The pci_get_drvdata() was moved during commit ef84928cff58
> ("uio/uio_pci_generic: use device-managed function equivalents").
>
> Storing a private object with pci_set_drvdata() doesn't make sense since that
> change, since there is no more pci_get_drvdata() call in the driver to
> retrieve the
> information.
>
> This change removes it.
>
> Fixes: ef84928cff58 ("io/uio_pci_generic: use device-managed function
> equivalents")
> Signed-off-by: Alexandru Ardelean
> ---
Forgot the changelog
Apologies.
Changelog v1 -> v2:
* added Fixes tag
* updated commit comment a bit from V1
> drivers/uio/uio_pci_generic.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index
> 1c6c09e1280d..b8e44d16279f 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -101,13 +101,7 @@ static int probe(struct pci_dev *pdev,
>"no support for interrupts?\n");
> }
>
> - err = devm_uio_register_device(&pdev->dev, &gdev->info);
> - if (err)
> - return err;
> -
> - pci_set_drvdata(pdev, gdev);
> -
> - return 0;
> + return devm_uio_register_device(&pdev->dev, &gdev->info);
> }
>
> static struct pci_driver uio_pci_driver = {
> --
> 2.17.1
RE: [PATCH v2 1/3] spi: convert to BIT() all spi_device flags
> -Original Message- > From: Andy Shevchenko > Sent: Tuesday, November 24, 2020 1:50 PM > To: kernel test robot > Cc: Ardelean, Alexandru ; linux-spi s...@vger.kernel.org>; devicetree ; Linux Kernel > Mailing List ; kbuild-...@lists.01.org; Mark > Brown ; Rob Herring ; Bogdan, > Dragos > Subject: Re: [PATCH v2 1/3] spi: convert to BIT() all spi_device flags > > On Tue, Nov 24, 2020 at 1:42 PM kernel test robot wrote: > > > All warnings (new ones prefixed by >>): > > > >In file included from drivers/spi/spidev.c:26: > > >> include/uapi/linux/spi/spidev.h:33: warning: "SPI_CPHA" redefined > > 33 | #define SPI_CPHA 0x01 > > Argh! Can we have only one set of flags? > My bad here for not catching this earlier. It might be an idea to create a "include/uapi/linux/spi/spi.h" and include this in " include/uapi/linux/spi/spidev.h " Then the " include/uapi/linux/spi/spi.h " would also be included in " include/linux/spi/spi.h " We would naturally drop the BIT() macros for the uapi header. > -- > With Best Regards, > Andy Shevchenko
RE: [PATCH v2 3/3] Input: adp5589-keys - add basic devicetree support
> -Original Message-
> From: Lars-Peter Clausen
> Sent: Tuesday, November 24, 2020 10:43 AM
> To: Ardelean, Alexandru ; linux-
> in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: dmitry.torok...@gmail.com
> Subject: Re: [PATCH v2 3/3] Input: adp5589-keys - add basic devicetree support
>
> [External]
>
> On 11/24/20 9:22 AM, Alexandru Ardelean wrote:
> > error = devm_add_action_or_reset(&client->dev,
> > adp5589_clear_config, @@ -1078,6 +1098,13 @@ static int __maybe_unused
> > adp5589_resume(struct device *dev)
> >
> > static SIMPLE_DEV_PM_OPS(adp5589_dev_pm_ops, adp5589_suspend,
> > adp5589_resume);
> >
> > +static const struct of_device_id adp5589_of_match[] = {
> > + { .compatible = "adi,adp5585", .data =
> &adp5589_chip_info_tbl[ADP5585_01] },
> > + { .compatible = "adi,adp5585-02", .data =
> &adp5589_chip_info_tbl[ADP5585_02] },
> > + { .compatible = "adi,adp5589", .data =
> > +&adp5589_chip_info_tbl[ADP5589] },
>
> I think we need to add these to
> Documentation/devicetree/bindings/trivial-devices.yaml
>
Ack
Will send a V3 in the next few days.
RE: [PATCH v3 1/3] spi: uapi: unify SPI modes into a single spi.h header
> -Original Message-
> From: Andy Shevchenko
> Sent: Friday, November 27, 2020 4:13 PM
> To: Ardelean, Alexandru
> Cc: linux-spi ; devicetree
> ; Linux Kernel Mailing List ker...@vger.kernel.org>; Rob Herring ; Mark Brown
> ; Bogdan, Dragos
> Subject: Re: [PATCH v3 1/3] spi: uapi: unify SPI modes into a single spi.h
> header
>
> On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean
> wrote:
> >
> > This change moves all the SPI mode bits into a separate 'spi.h' header
> > in uapi. This is meant to re-use these definitions inside the kernel
> > as well as export them to userspace (via uapi).
>
> uapi -> UAPI (or uAPI) here and everywhere else where it makes sense.
>
> > The SPI mode definitions have usually been duplicated between between
> > 'include/linux/spi/spi.h' and 'include/uapi/linux/spi/spidev.h', so
> > whenever adding a new entry, this would need to be put in both headers.
> >
> > They've been moved from 'include/linux/spi/spi.h', since that seems a
> > bit more complete; the bits have descriptions and there is the
> SPI_MODE_X_MASK.
> >
> > For now, this change does a simple move; no conversions to BIT()
> > macros are being done at this point. This can be done later, as that
> > requires also another header inclusion (the 'const.h' header).
> > The change as-is makes this 'spi.h' header more standalone.
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> >
> > Personally, I am not sure whether to convert the bitfield tos _BITUL()
> > macros or not. I feel that not-having these macros makes this uapi
> > spi.h header more standalone.
> > If there's a strong insistence to use those _BITUL() macros, I'll do it.
> > I'm hesitant now, because it requires that this spi.h includes the
> > 'const.h' header.
>
> _BITUL is a part of uAPI, why not to use it?
> In general BIT() type of macros makes values easier to read and less error
> prone
> (in big numbers it's easy to miss 0).
> It's not a strong opinion, it's just the rationale behind how I see it.
Yeah I understand.
I guess since this is a new header in UAPI, I can use the _BITUL macro.
Sometimes I look at some code and remember how some people tend to use some
things.
Like, I would expect some people to maybe just copy this header as-is in some
"libspi".
So, then: do we make it easier for those people or not?
But again, since this is a new header, the concern may be reduced by just using
_BITUL() from the start.
I sometimes have these arguments with myself whenever going into API code.
Kernel code is easy: all the users of that code in the kernel, and we try not
to bother too much with out-of-tree drivers. Those get impossible to care about.
>
> > Changelog v2 -> v3:
> > *
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-spi/20201124
> > 102152.16548-1-
> alexandru.ardel...@analog.com/__;!!A3Ni8CS0y2Y!oNpULNID
> > JsvU1BX8CZgZYoS90mW3rZ2dHrZOwTRS4ndZ-_rLq3eJt22Rj1RQafWyw1-3YA$
> > * dropped 'spi: convert to BIT() all spi_device flags '
> > added 'spi: uapi: unify SPI modes into a single spi.h header'
> >
> > include/linux/spi/spi.h | 22 +--
> > include/uapi/linux/spi/spi.h| 47 +
> > include/uapi/linux/spi/spidev.h | 30 +
> > 3 files changed, 49 insertions(+), 50 deletions(-) create mode
> > 100644 include/uapi/linux/spi/spi.h
> >
> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index
> > aa09fdc8042d..a4fedb33d34b 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > struct dma_chan;
> > struct property_entry;
> > @@ -165,27 +166,6 @@ struct spi_device {
> > u8 bits_per_word;
> > boolrt;
> > u32 mode;
> > -#defineSPI_CPHA0x01/* clock phase */
> > -#defineSPI_CPOL0x02/* clock polarity */
> > -#defineSPI_MODE_0 (0|0) /* (original
> > MicroWire) */
> > -#defineSPI_MODE_1 (0|SPI_CPHA)
> > -#defineSPI_MODE_2 (SPI_CPOL|0)
> > -#defineSPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> > -#defineSPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
> > -#defineSPI_CS_HIGH 0x04
RE: [PATCH v3 2/3] spi: Add SPI_NO_TX/RX support
> -Original Message- > From: Andy Shevchenko > Sent: Friday, November 27, 2020 4:24 PM > To: Ardelean, Alexandru > Cc: linux-spi ; devicetree > ; Linux Kernel Mailing List ker...@vger.kernel.org>; Rob Herring ; Mark Brown > ; Bogdan, Dragos > Subject: Re: [PATCH v3 2/3] spi: Add SPI_NO_TX/RX support > > On Fri, Nov 27, 2020 at 4:22 PM Andy Shevchenko > wrote: > > On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean > > wrote: > > ... > > > > --- a/include/uapi/linux/spi/spi.h > > > +++ b/include/uapi/linux/spi/spi.h > > > @@ -43,5 +43,7 @@ > > > #defineSPI_TX_OCTAL0x2000 /* transmit with > > > 8 wires */ > > > #defineSPI_RX_OCTAL0x4000 /* receive with 8 > > > wires */ > > > #defineSPI_3WIRE_HIZ 0x8000 /* high impedance > > > turnaround > */ > > > +#defineSPI_NO_TX 0x1 /* no transmit > > > wire */ > > > +#defineSPI_NO_RX 0x2 /* no receive > > > wire */ > > > > Is it really material for uAPI? > > Perhaps we may have something like > > SPI_MODE_USER_MASK in uAPI and > > in internal headers Hmm, in a way this could make sense for some SPIDEVs as well, to set these flags and get an error if they try to TX with the NO_TX flag set. Not really sure about this. Initially I mechanically added these here as an inertia to the previous patch which is just unifying the headers. Any other opinions? Thoughts? Mark? > > > > SPI_MODE_KERNEL_MASK with > > static_assert(_USER_MASK & _KERNEL_MASK); // check conditional > > > > ? > > And logically start bits for the kernel from the end (31, 30, ...). > > -- > With Best Regards, > Andy Shevchenko
RE: [PATCH v3 3/3] spi: dt-bindings: document zero value for spi-{rx,tx}-bus-width properties
> -Original Message-
> From: Andy Shevchenko
> Sent: Friday, November 27, 2020 4:26 PM
> To: Ardelean, Alexandru
> Cc: linux-spi ; devicetree
> ; Linux Kernel Mailing List ker...@vger.kernel.org>; Rob Herring ; Mark Brown
> ; Bogdan, Dragos
> Subject: Re: [PATCH v3 3/3] spi: dt-bindings: document zero value for
> spi-{rx,tx}-
> bus-width properties
>
> On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean
> wrote:
> >
> > Following a change to the SPI framework, providing a value of zero for
> > 'spi-rx-bus-width' and 'spi-tx-bus-width' is now possible and will
> > essentially mean than no RX or TX is allowed.
>
> than -> that
>
> I'm wondering if we have a possibility to strict this for controllers that
> always
> duplex (I assume that schema works in a way that the generic bindings is most
> relaxed in comparison to the device specific ones in case of the same binding
> in
> question).
No idea here.
>
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH] net: phy: adin: add signal mean square error registers to phy-stats
> -Original Message- > From: Andrew Lunn > Sent: Thursday, December 3, 2020 4:16 PM > To: Ardelean, Alexandru > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; > hkallwe...@gmail.com; li...@armlinux.org.uk; da...@davemloft.net; > k...@kernel.org; Redmond, Catherine ; > Murray, Brian ; Baylov, Danail > ; OBrien, Maurice > > Subject: Re: [PATCH] net: phy: adin: add signal mean square error registers to > phy-stats > > On Thu, Dec 03, 2020 at 10:07:19AM +0200, Alexandru Ardelean wrote: > > When the link is up on the ADIN1300/ADIN1200, the signal quality on > > each pair is indicated in the mean square error register for each pair > > (MSE_A, MSE_B, MSE_C, and MSE_D registers, Address 0x8402 to Address > > 0x8405, Bits[7:0]). > > > > These values can be useful for some industrial applications. > > > > This change implements support for these registers using the PHY > > statistics mechanism. > > There was a discussion about values like these before. If i remember > correctly, it > was for a BroadReach PHY. I thought we decided to add them to the link state > information? > Oh, this is new. I've had this MSE patch lying around in a branch since last year sometime. Wasn't sure whether to put it in the phy-stats. > Ah, found it. > > commit 68ff5e14759e7ac1aac7bc75ac5b935e390fa2b3 > Author: Oleksij Rempel > Date: Wed May 20 08:29:15 2020 +0200 > > net: phy: tja11xx: add SQI support > > and > > ommit 8066021915924f58ed338bf38208215f5a7355f6 > Author: Oleksij Rempel > Date: Wed May 20 08:29:14 2020 +0200 > > ethtool: provide UAPI for PHY Signal Quality Index (SQI) > > Can you convert your MSE into SQI? I'll take a look and try to understand the SQI spec. It's neat that there's a common place where to put this. Thanks Alex > > Andrew
RE: linux-next: build warning after merge of the spi tree
> -Original Message- > From: Stephen Rothwell > Sent: Monday, January 4, 2021 2:07 AM > To: Mark Brown > Cc: Ardelean, Alexandru ; Andy Shevchenko > ; Linux Kernel Mailing List ker...@vger.kernel.org>; Linux Next Mailing List > Subject: linux-next: build warning after merge of the spi tree > > [External] > > Hi all, > > After merging the spi tree, today's linux-next build (arm > multi_v7_defconfig) produced this warning: > Apologies for this. Will send a fix. > In file included from include/linux/device.h:15, > from include/linux/dmaengine.h:8, > from drivers/spi/spi-stm32.c:11: > drivers/spi/spi-stm32.c: In function 'stm32_spi_prepare_msg': > drivers/spi/spi-stm32.c:1030:20: warning: format '%d' expects argument of type > 'int', but argument 4 has type 'long unsigned int' [-Wformat=] > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > |^~~ > include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' >19 | #define dev_fmt(fmt) fmt > | ^~~ > drivers/spi/spi-stm32.c:1030:2: note: in expansion of macro 'dev_dbg' > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ^~~ > drivers/spi/spi-stm32.c:1030:27: note: format string is defined here > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ~^ > | | > | int > | %ld > In file included from include/linux/device.h:15, > from include/linux/dmaengine.h:8, > from drivers/spi/spi-stm32.c:11: > drivers/spi/spi-stm32.c:1030:20: warning: format '%d' expects argument of type > 'int', but argument 5 has type 'long unsigned int' [-Wformat=] > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > |^~~ > include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' >19 | #define dev_fmt(fmt) fmt > | ^~~ > drivers/spi/spi-stm32.c:1030:2: note: in expansion of macro 'dev_dbg' > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ^~~ > drivers/spi/spi-stm32.c:1030:35: note: format string is defined here > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ~^ > | | > | int > | %ld > In file included from include/linux/device.h:15, > from include/linux/dmaengine.h:8, > from drivers/spi/spi-stm32.c:11: > drivers/spi/spi-stm32.c:1030:20: warning: format '%d' expects argument of type > 'int', but argument 6 has type 'long unsigned int' [-Wformat=] > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > |^~~ > include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' >19 | #define dev_fmt(fmt) fmt > | ^~~ > drivers/spi/spi-stm32.c:1030:2: note: in expansion of macro 'dev_dbg' > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ^~~ > drivers/spi/spi-stm32.c:1030:48: note: format string is defined here > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > | ~^ > || > |int > | %ld > In file included from include/linux/device.h:15, > from include/linux/dmaengine.h:8, > from drivers/spi/spi-stm32.c:11: > drivers/spi/spi-stm32.c:1030:20: warning: format '%d' expects argument of type > 'int', but argument 7 has type 'long unsigned int' [-Wformat=] > 1030 | dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n", > |^~~ > include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' >19
RE: [PATCH] spi: stm32: update dev_dbg() print format for SPI params
> -Original Message-
> From: Andy Shevchenko
> Sent: Monday, January 4, 2021 3:51 PM
> To: Ardelean, Alexandru
> Cc: linux-spi ; Linux Kernel Mailing List ker...@vger.kernel.org>; Mark Brown ; Stephen Rothwell
>
> Subject: Re: [PATCH] spi: stm32: update dev_dbg() print format for SPI params
>
> [External]
>
> On Mon, Jan 4, 2021 at 10:55 AM Alexandru Ardelean
> wrote:
> >
> > With the introduction of the 'include/uapi/linux/spi/spi.h' header,
> > the type of the macros are enforced to 'unsigned long int' via the
> > _BITUL() macro.
> >
> > This causes some -Wformat warnings in the spi-stm32 driver.
> > This patch changes the printf() specifiers from '%d' to '%lu' to
> > accommodate for this change.
> >
> > Fixes: f7005142dace ("spi: uapi: unify SPI modes into a single spi.h
> > header")
> > Reported-by: Stephen Rothwell
>
> LKP also reported this before.
>
Ack;
Will add it
> ...
>
> > - dev_dbg(spi->dev, "cpol=%d cpha=%d lsb_first=%d cs_high=%d\n",
> > + dev_dbg(spi->dev, "cpol=%lu cpha=%lu lsb_first=%lu
> > + cs_high=%lu\n",
> > spi_dev->mode & SPI_CPOL,
> > spi_dev->mode & SPI_CPHA,
> > spi_dev->mode & SPI_LSB_FIRST,
>
> Wouldn't the output be a bit awful with all these?
>
> I think the proper fix is to add !! to each bit mask.
Fine by me
>
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v2 1/3] clk: axi-clkgen: remove ARCH dependency in Kconfig
> -Original Message- > From: Moritz Fischer > Sent: Wednesday, January 27, 2021 4:39 AM > To: Ardelean, Alexandru > Cc: linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux- > ker...@vger.kernel.org; mturque...@baylibre.com; sb...@kernel.org; > robh...@kernel.org; l...@metafoo.de; linux-f...@vger.kernel.org; > m...@kernel.org; Bogdan, Dragos > Subject: Re: [PATCH v2 1/3] clk: axi-clkgen: remove ARCH dependency in Kconfig > > Alexandru, > > On Tue, Jan 26, 2021 at 01:08:24PM +0200, Alexandru Ardelean wrote: > > The intent is to be able to run this driver to access the IP core in > > setups where FPGA board is also connected via a PCIe bus. In such > > cases the number of combinations explodes, where the host system can > > be an x86 with Xilinx Zynq/ZynqMP/Microblaze board connected via PCIe. > > Or even a ZynqMP board with a ZynqMP/Zynq/Microblaze connected via PCIe. > > > > To accommodate for these cases, this change removes the limitation for > > this driver to be compilable only on Zynq/Microblaze architectures. > > > > Signed-off-by: Dragos Bogdan > > Signed-off-by: Alexandru Ardelean > > --- > > drivers/clk/Kconfig | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index > > 85856cff506c..d8c2d4593926 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -247,7 +247,6 @@ config CLK_TWL6040 > > > > config COMMON_CLK_AXI_CLKGEN > > tristate "AXI clkgen driver" > > - depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST > Umhhh ... no dependencies? How are you accessing your registers? You seem to > be using device tree, probably: > > depends on HAS_IOMEM || COMPILE_TEST > depends on OF > > at least? Please double check your dependencies. Agreed. Will re-spin. This is a n00b mistake on my part Thanks > > help > > Support for the Analog Devices axi-clkgen pcore clock generator for > Xilinx > > FPGAs. It is commonly used in Analog Devices' reference designs. > > -- > > 2.17.1 > > > > - Moritz
RE: [PATCH] uio/uio_pci_generic: remove unneeded pci_set_drvdata()
> -Original Message-
> From: Greg KH
> Sent: Friday, November 20, 2020 5:46 PM
> To: Ardelean, Alexandru
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] uio/uio_pci_generic: remove unneeded pci_set_drvdata()
>
> [External]
>
> On Thu, Nov 19, 2020 at 04:59:06PM +0200, Alexandru Ardelean wrote:
> > The pci_get_drvdata() was moved during commit ef84928cff58
> > ("uio/uio_pci_generic: use device-managed function equivalents").
> >
> > I should have notice that the pci_set_drvdata() requires a
> > pci_get_drvdata() for it to make sense.
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> >
> > Apologies for not noticing this sooner.
> > If this can be squashed into commit ef84928cff58 , then it's also fine.
> > I've started seeing that there actually more xxx_set_drvdata()
> > leftovers in the entire kernel, and I pinged the checkpatch crew to
> > add a check for this.
> >
> > https://urldefense.com/v3/__https://lore.kernel.org/lkml/CA*U=Dspy5*RE
> >
> 9agcLr6eY9DCMa1c5**b0jleugmmbrxz4yl...@mail.gmail.com/T/*u__;KysrK
> ysj!
> >
> !A3Ni8CS0y2Y!q3fJW4rKvEHQ7BDt1PaK4Cbexv4wbivUKBeDjo7ZwNXYwOLBawA
> Eq1Jaj
> > mhYxftX6DAJpg$
>
> I can't squash existing public commits. Can you resend this and add a
> "Fixes:"
> tag to it to show what commit it fixes so we can track this properly?
>
Sure, will re-send in the next couple of days.
Thanks
Alex
> thanks,
>
> greg k-h
Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging
On Sun, 2019-09-08 at 12:09 +0100, Jonathan Cameron wrote: > On Mon, 2 Sep 2019 13:26:02 + > "Ardelean, Alexandru" wrote: > > > On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote: > > > Move ADIS16240 driver from staging to mainline. > > > > > > The ADIS16240 is a fully integrated digital shock detection > > > and recorder system. > > > > Hey, > > > > Comments inline. > > > > I'll probably take a look in the next days again. > > There seem to be some ABI/sysfs attributes that need to be resolved before > > moving this out of staging. > > Absolutely. It is a 'new' type of device so there are definitely some > corners that need discussing before we move out of staging and commit > to maintaining the ABI moving forwards. > > That is the real reason this driver was still in staging! No one > had been through the process of proposing the ABI and responding to > questions etc. > > The issue with impact sensors has always been that they don't really fit > our normal model for buffers or triggers. > > So normally a trigger (if exposed in IIO) is used as one trigger > causes 1 set of samples (so like a frame trigger for a camera). > > These devices tend to work in a mode where one trigger causes data > to be captured for a period of time. In this part that's the event > recorder function > > No one is realistically going to buy an impact sensor to just use it > as an accelerometer which is what this driver is currently doing. > I suppose we could just leave support in that form for now, but > I'm no sure how much use it is to anyone. > > Analog Devices people, worth working out how to support the event > recorder? For that someone needs to have hardware as it is complex > to say the least! Worth it: yes. But we don't have any resources to allocate for this [at this point in time]. > > We could move it out but might be worth adding a comment somewhere > saying this only really supports direct access to channels, and > not the event recorder functionality. > I guess, I would vote for leaving it in staging. It's also a way to mark it as a work-in-progress/not-done/still-needs-something kind of thing. If we move it now, it gets the status of "everything-resolved" which is not yet the case. Thanks for the insight/background info. Much of it was discussed before my time. > > Jonathan Alex > > > > > Signed-off-by: Rodrigo Ribeiro Carvalho > > > --- > > > drivers/iio/accel/Kconfig | 12 + > > > drivers/iio/accel/Makefile| 1 + > > > drivers/iio/accel/adis16240.c | 454 ++ > > > drivers/staging/iio/accel/Kconfig | 12 - > > > drivers/staging/iio/accel/Makefile| 1 - > > > drivers/staging/iio/accel/adis16240.c | 454 -- > > > 6 files changed, 467 insertions(+), 467 deletions(-) > > > create mode 100644 drivers/iio/accel/adis16240.c > > > delete mode 100644 drivers/staging/iio/accel/adis16240.c > > > > Looks like MAINTAINERS file also needs to be updated, also with the DT > > bindings file. > > I think checkpatch usually complains about these. > > > > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > > > index d4ef35aeb579..91fd8741c95f 100644 > > > --- a/drivers/iio/accel/Kconfig > > > +++ b/drivers/iio/accel/Kconfig > > > @@ -30,6 +30,18 @@ config ADIS16209 > > > To compile this driver as a module, say M here: the module will be > > > called adis16209. > > > > > > +config ADIS16240 > > > + tristate "Analog Devices ADIS16240 Programmable Impact Sensor and > > > Recorder" > > > + depends on SPI > > > + select IIO_ADIS_LIB > > > + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER > > > + help > > > + Say Y here to build support for Analog Devices adis16240 programmable > > > + impact Sensor and recorder. > > > + > > > + To compile this driver as a module, say M here: the module will be > > > + called adis16240. > > > + > > > config ADXL345 > > > tristate > > > > > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile > > > index 56bd0215e0d4..f7e025a86dd9 100644 > > > --- a/drivers/iio/accel/Makefile > > > +++ b/drivers/iio/accel/Makefile > > > @@ -6,6 +6,7 @@ > > > # When adding new entries keep the list in alphabetical order > > >
Re: [PATCH] net: stmmac: socfpga: re-use the `interface` parameter from platform data
On Sat, 2019-09-07 at 20:54 +0800, kbuild test robot wrote:
> [External]
>
> Hi Alexandru,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
Hmm, this error should be expectable I guess: I applied this on net-next/master.
Alex
> [cannot apply to v5.3-rc7 next-20190904]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/net-stmmac-socfpga-re-use-the-interface-parameter-from-platform-data/20190907-190627
> config: sparc64-allmodconfig (attached as .config)
> compiler: sparc64-linux-gcc (GCC) 7.4.0
> reproduce:
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.4.0 make.cross ARCH=sparc64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/dma-mapping.h:7:0,
> from include/linux/skbuff.h:30,
> from include/linux/if_ether.h:19,
> from include/uapi/linux/ethtool.h:19,
> from include/linux/ethtool.h:18,
> from include/linux/phy.h:16,
> from
> drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c:11:
>drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c: In function
> 'socfpga_gen5_set_phy_mode':
> > > drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c:264:44: error:
> > > 'phymode' undeclared (first use in this
> > > function); did you mean 'phy_modes'?
> dev_err(dwmac->dev, "bad phy mode %d\n", phymode);
>^
>include/linux/device.h:1499:32: note: in definition of macro 'dev_err'
> _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
>^~~
>drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c:264:44: note: each
> undeclared identifier is reported only once
> for each function it appears in
> dev_err(dwmac->dev, "bad phy mode %d\n", phymode);
>^
>include/linux/device.h:1499:32: note: in definition of macro 'dev_err'
> _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
>^~~
>drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c: In function
> 'socfpga_gen10_set_phy_mode':
>drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c:340:6: error:
> 'phymode' undeclared (first use in this
> function); did you mean 'phy_modes'?
> phymode == PHY_INTERFACE_MODE_MII ||
> ^~~
> phy_modes
>
> vim +264 drivers/net//ethernet/stmicro/stmmac/dwmac-socfpga.c
>
> 40ae25505fe834 Dinh Nguyen2019-06-05 255
> 40ae25505fe834 Dinh Nguyen2019-06-05 256 static int
> socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
> 40ae25505fe834 Dinh Nguyen2019-06-05 257 {
> 40ae25505fe834 Dinh Nguyen2019-06-05 258 struct regmap
> *sys_mgr_base_addr = dwmac->sys_mgr_base_addr;
> 40ae25505fe834 Dinh Nguyen2019-06-05 259 u32 reg_offset =
> dwmac->reg_offset;
> 40ae25505fe834 Dinh Nguyen2019-06-05 260 u32 reg_shift =
> dwmac->reg_shift;
> 40ae25505fe834 Dinh Nguyen2019-06-05 261 u32 ctrl, val, module;
> 40ae25505fe834 Dinh Nguyen2019-06-05 262
> 6169afbe4a340b Alexandru Ardelean 2019-09-06 263 if
> (socfpga_set_phy_mode_common(dwmac, &val)) {
> 801d233b7302ee Dinh Nguyen2014-03-26 @264
> dev_err(dwmac->dev, "bad phy mode %d\n", phymode);
> 801d233b7302ee Dinh Nguyen2014-03-26 265 return -EINVAL;
> 801d233b7302ee Dinh Nguyen2014-03-26 266 }
> 801d233b7302ee Dinh Nguyen2014-03-26 267
> b4834c86e11baf Ley Foon Tan 2014-08-20 268 /* Overwrite val to
> GMII if splitter core is enabled. The
> phymode here
> b4834c86e11baf Ley Foon Tan 2014-08-20 269 * is the actual phy
> mode on phy hardware, but phy interface
> from
> b4834c86e11baf Ley Foon Tan 2014-08-20 270 * EMAC core is GMII.
> b4834c86e11baf Ley Foon Tan 2014-08-20 271 */
> b4834c86e11baf Ley Foon Tan 2014-08-20 272 if
> (dwmac->splitter_base)
> b4834c86e11baf Ley Foon Tan 2014-08-20 273 val =
> SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
> b4834c86e11baf Ley Foon Tan 2014-08-20 274
> 70cb136f773083 Joachim Eastwood 2016-05-01 275 /* Assert reset to the
> enet controller before changing the phy
> mode */
> bc8a2d9bcbf1ca Dinh Nguyen2018-06-19 276
> reset_control_assert(dwmac->stmmac_ocp_rst);
> 70cb136f773083 Joachim Eastwood 2016-05-01 277
> reset_control_assert(dwmac->stmmac_rst);
> 70
Re: [PATCH v3 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable
On Tue, 2019-09-10 at 10:00 +0200, Michal Kubecek wrote: > [External] > > On Mon, Sep 09, 2019 at 04:12:50PM +0300, Alexandru Ardelean wrote: > > The `phy_tunable_id` has been named `ETHTOOL_PHY_EDPD` since it looks like > > this feature is common across other PHYs (like EEE), and defining > > `ETHTOOL_PHY_ENERGY_DETECT_POWER_DOWN` seems too long. > > > > The way EDPD works, is that the RX block is put to a lower power mode, > > except for link-pulse detection circuits. The TX block is also put to low > > power mode, but the PHY wakes-up periodically to send link pulses, to avoid > > lock-ups in case the other side is also in EDPD mode. > > > > Currently, there are 2 PHY drivers that look like they could use this new > > PHY tunable feature: the `adin` && `micrel` PHYs. > > > > The ADIN's datasheet mentions that TX pulses are at intervals of 1 second > > default each, and they can be disabled. For the Micrel KSZ9031 PHY, the > > datasheet does not mention whether they can be disabled, but mentions that > > they can modified. > > > > The way this change is structured, is similar to the PHY tunable downshift > > control: > > * a `ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL` value is exposed to cover a default > > TX interval; some PHYs could specify a certain value that makes sense > > * `ETHTOOL_PHY_EDPD_NO_TX` would disable TX when EDPD is enabled > > * `ETHTOOL_PHY_EDPD_DISABLE` will disable EDPD > > > > This should allow PHYs to: > > * enable EDPD and not enable TX pulses (interval would be 0) > > * enable EDPD and configure TX pulse interval; note that TX interval units > > would be PHY specific; we could consider `seconds` as units, but it could > > happen that some PHYs would be prefer milliseconds as a unit; > > a maximum of 65533 units should be sufficient > > Sorry for missing the discussion on previous version but I don't really > like the idea of leaving the choice of units to PHY. Both for manual > setting and system configuration, it would be IMHO much more convenient > to have the interpretation universal for all NICs. > I was also a bit uncertain/undecided here about letting PHYs decide units. And I also wasn't sure what to propose. Not proposing a unit now, would have allowed us to decide later based on what PHYs implement (generally) in the future. I know that may make me look a bit lazy [for not proposing units in ethtool], but tbh it's more of lack-of-experience with ethtool (or these APIs) evolve over time. > Seconds as units seem too coarse and maximum of ~18 hours way too big. > Milliseconds would be more practical from granularity point of view, > would maximum of ~65 seconds be sufficient? I think 65 seconds is more than enough. I feel that, if you plug-in an eth cable and don't have a link in a minute, then it's not great (no matter how much of a power-saver you are). Actually, voicing out these units makes it simpler for a decision to go in favor for milliseconds. So: thank you :) Alex > > Michal Kubecek > > > * disable EDPD > > > > Signed-off-by: Alexandru Ardelean
Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240
On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
>
> Signed-off-by: Rodrigo Ribeiro Carvalho
> ---
> I have doubt about what maintainer I may to put in that documentation. I
> put Alexandru as maintainer because he reviewed my last patch on this
> driver, so I think that he is a good candidate.
Fine to list me as maintainer for this.
Reviewed-by: Alexandru Ardelean
> .../bindings/iio/accel/adi,adis16240.yaml | 55 +++
> 1 file changed, 55 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index ..08019b51611c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> + - Alexandru Ardelean
> +
> +description: |
> + ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> + SPI interface.
> +https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> + compatible:
> +enum:
> + - adi,adis16240
> +
> + reg:
> +maxItems: 1
> +
> + spi-cpha: true
> +
> + spi-cpol: true
> +
> + interrupts:
> +maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +examples:
> + - |
> +#include
> +#include
> +spi0 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +/* Example for a SPI device node */
> +accelerometer@0 {
> +compatible = "adi,adis16240";
> +reg = <0>;
> +spi-max-frequency = <250>;
> +spi-cpol;
> +spi-cpha;
> +interrupt-parent = <&gpio0>;
> +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +};
Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging
On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote: > Move ADIS16240 driver from staging to mainline. > > The ADIS16240 is a fully integrated digital shock detection > and recorder system. Hey, Comments inline. I'll probably take a look in the next days again. There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging. > > Signed-off-by: Rodrigo Ribeiro Carvalho > --- > drivers/iio/accel/Kconfig | 12 + > drivers/iio/accel/Makefile| 1 + > drivers/iio/accel/adis16240.c | 454 ++ > drivers/staging/iio/accel/Kconfig | 12 - > drivers/staging/iio/accel/Makefile| 1 - > drivers/staging/iio/accel/adis16240.c | 454 -- > 6 files changed, 467 insertions(+), 467 deletions(-) > create mode 100644 drivers/iio/accel/adis16240.c > delete mode 100644 drivers/staging/iio/accel/adis16240.c Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file. I think checkpatch usually complains about these. > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index d4ef35aeb579..91fd8741c95f 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -30,6 +30,18 @@ config ADIS16209 > To compile this driver as a module, say M here: the module will be > called adis16209. > > +config ADIS16240 > + tristate "Analog Devices ADIS16240 Programmable Impact Sensor and > Recorder" > + depends on SPI > + select IIO_ADIS_LIB > + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER > + help > + Say Y here to build support for Analog Devices adis16240 programmable > + impact Sensor and recorder. > + > + To compile this driver as a module, say M here: the module will be > + called adis16240. > + > config ADXL345 > tristate > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile > index 56bd0215e0d4..f7e025a86dd9 100644 > --- a/drivers/iio/accel/Makefile > +++ b/drivers/iio/accel/Makefile > @@ -6,6 +6,7 @@ > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_ADIS16201) += adis16201.o > obj-$(CONFIG_ADIS16209) += adis16209.o > +obj-$(CONFIG_ADIS16240) += adis16240.o > obj-$(CONFIG_ADXL345) += adxl345_core.o > obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o > obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c > new file mode 100644 > index ..82099db4bf0c > --- /dev/null > +++ b/drivers/iio/accel/adis16240.c > @@ -0,0 +1,454 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * ADIS16240 Programmable Impact Sensor and Recorder driver > + * > + * Copyright 2010 Analog Devices Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#define ADIS16240_STARTUP_DELAY 220 /* ms */ > + > +/* Flash memory write count */ > +#define ADIS16240_FLASH_CNT 0x00 > + > +/* Output, power supply */ > +#define ADIS16240_SUPPLY_OUT 0x02 > + > +/* Output, x-axis accelerometer */ > +#define ADIS16240_XACCL_OUT 0x04 > + > +/* Output, y-axis accelerometer */ > +#define ADIS16240_YACCL_OUT 0x06 > + > +/* Output, z-axis accelerometer */ > +#define ADIS16240_ZACCL_OUT 0x08 > + > +/* Output, auxiliary ADC input */ > +#define ADIS16240_AUX_ADC0x0A > + > +/* Output, temperature */ > +#define ADIS16240_TEMP_OUT 0x0C > + > +/* Output, x-axis acceleration peak */ > +#define ADIS16240_XPEAK_OUT 0x0E > + > +/* Output, y-axis acceleration peak */ > +#define ADIS16240_YPEAK_OUT 0x10 > + > +/* Output, z-axis acceleration peak */ > +#define ADIS16240_ZPEAK_OUT 0x12 > + > +/* Output, sum-of-squares acceleration peak */ > +#define ADIS16240_XYZPEAK_OUT0x14 > + > +/* Output, Capture Buffer 1, X and Y acceleration */ > +#define ADIS16240_CAPT_BUF1 0x16 > + > +/* Output, Capture Buffer 2, Z acceleration */ > +#define ADIS16240_CAPT_BUF2 0x18 > + > +/* Diagnostic, error flags */ > +#define ADIS16240_DIAG_STAT 0x1A > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute. > +/* Diagnostic, event counter */ > +#define ADIS16240_EVNT_CNTR 0x1C > + > +/* Diagnostic, check sum value from firmware test */ > +#define ADIS16240_CHK_SUM0x1E > + > +/* Calibration, x-axis acceleration offset adjustment */ > +#define ADIS16240_XACCL_OFF 0x20 > + > +/* Calibration, y-axis acceleration offset adjustment */ > +#define ADIS16240_YACCL_OFF 0x22 > + > +/* Calibration, z-axis acceleration offset adjustment */ > +#define ADIS16240_ZACCL_OFF 0x24 > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute. > +/* Clock, hour and minute */ > +#define ADIS16240_CLK_TIME 0x2E > + > +/* Clock, mont
Re: [PATCH v4 11/14] net: phy: adin: implement Energy Detect Powerdown mode
On Wed, 2019-08-14 at 10:57 -0700, Florian Fainelli wrote: > [External] > > > > On 8/12/2019 4:23 AM, Alexandru Ardelean wrote: > > The ADIN PHYs support Energy Detect Powerdown mode, which puts the PHY into > > a low power mode when there is no signal on the wire (typically cable > > unplugged). > > This behavior is enabled by default, but can be disabled via device > > property. > > We could consider adding a PHY tunable, having this as a Device Tree > property amounts to putting a policy inside DT, which is frowned upon. That would be interesting actually, and I would also prefer it over static DT. Maybe for this patch, I'll just enable EDPD by default and see about a tuna option. > > > Signed-off-by: Alexandru Ardelean > > Other than that, the code looks fine: > > Reviewed-by: Florian Fainelli
Re: [PATCH] iio: dac: ad5380: fix incorrect assignment to val
On Thu, 2019-08-15 at 12:58 +0100, Colin King wrote:
> [External]
>
Reviewed-by: Alexandru Ardelean
> From: Colin Ian King
>
> Currently the pointer val is being incorrectly incremented
> instead of the value pointed to by val. Fix this by adding
> in the missing * indirection operator.
>
> Addresses-Coverity: ("Unused value")
> Fixes: c03f2c536818 ("staging:iio:dac: Add AD5380 driver")
> Signed-off-by: Colin Ian King
> ---
> drivers/iio/dac/ad5380.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/dac/ad5380.c b/drivers/iio/dac/ad5380.c
> index 4335214800d2..2ebe08326048 100644
> --- a/drivers/iio/dac/ad5380.c
> +++ b/drivers/iio/dac/ad5380.c
> @@ -220,7 +220,7 @@ static int ad5380_read_raw(struct iio_dev *indio_dev,
> if (ret)
> return ret;
> *val >>= chan->scan_type.shift;
> - val -= (1 << chan->scan_type.realbits) / 2;
> + *val -= (1 << chan->scan_type.realbits) / 2;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> *val = 2 * st->vref;
Re: [PATCH v2 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable
On Wed, 2019-09-04 at 21:53 +0200, Andrew Lunn wrote: > [External] > > On Wed, Sep 04, 2019 at 07:23:21PM +0300, Alexandru Ardelean wrote: > > Hi Alexandru > > Somewhere we need a comment stating what EDPD means. Here would be a > good place. ack > > > +#define ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL 0x7fff > > +#define ETHTOOL_PHY_EDPD_NO_TX 0x8000 > > +#define ETHTOOL_PHY_EDPD_DISABLE 0 > > I think you are passing a u16. So why not 0xfffe and 0x? We also > need to make it clear what the units are for interval. This file I initially thought about keeping this u8 and going with 0xff & 0xfe. But 254 or 253 could be too small to specify the value of an interval. Also (maybe due ti all the coding-patterns that I saw over the course of some time), make me feel that I should add a flag somewhere. Bottom line is: 0xfffe and 0x also work from my side, if it is acceptable (by the community). Another approach I considered, was to maybe have this EDPD just do enable & disable (which is sufficient for the `adin` PHY & `micrel` as well). That would mean that if we would ever want to configure the TX interval (in the future), we would need an extra PHY- tunable parameter just for that; because changing the enable/disable behavior would be dangerous. And also, deferring the TX-interval configuration, does not sound like good design/pattern, since it can allow for tons of PHY-tunable parameters for every little knob. > specifies the contract between the kernel and user space. So we need > to clearly define what we mean here. Lots of comments are better than > no comments. Will come back with more comments. > >Andrew
Re: [PATCH v2 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable
On Wed, 2019-09-04 at 22:03 +0200, Andrew Lunn wrote:
> [External]
>
> On Wed, Sep 04, 2019 at 07:23:22PM +0300, Alexandru Ardelean wrote:
> > This driver becomes the first user of the kernel's `ETHTOOL_PHY_EDPD`
> > phy-tunable feature.
> > EDPD is also enabled by default on PHY config_init, but can be disabled via
> > the phy-tunable control.
> >
> > When enabling EDPD, it's also a good idea (for the ADIN PHYs) to enable TX
> > periodic pulses, so that in case the other PHY is also on EDPD mode, there
> > is no lock-up situation where both sides are waiting for the other to
> > transmit.
> >
> > Via the phy-tunable control, TX pulses can be disabled if specifying 0
> > `tx-interval` via ethtool.
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> > drivers/net/phy/adin.c | 50 ++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index 4dec83df048d..742728ab2a5d 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -26,6 +26,11 @@
> >
> > #define ADIN1300_RX_ERR_CNT0x0014
> >
> > +#define ADIN1300_PHY_CTRL_STATUS2 0x0015
> > +#define ADIN1300_NRG_PD_EN BIT(3)
> > +#define ADIN1300_NRG_PD_TX_ENBIT(2)
> > +#define ADIN1300_NRG_PD_STATUS BIT(1)
> > +
> > #define ADIN1300_PHY_CTRL2 0x0016
> > #define ADIN1300_DOWNSPEED_AN_100_EN BIT(11)
> > #define ADIN1300_DOWNSPEED_AN_10_EN BIT(10)
> > @@ -328,12 +333,51 @@ static int adin_set_downshift(struct phy_device
> > *phydev, u8 cnt)
> > ADIN1300_DOWNSPEEDS_EN);
> > }
> >
> > +static int adin_get_edpd(struct phy_device *phydev, u16 *tx_interval)
> > +{
> > + int val;
> > +
> > + val = phy_read(phydev, ADIN1300_PHY_CTRL_STATUS2);
> > + if (val < 0)
> > + return val;
> > +
> > + if (ADIN1300_NRG_PD_EN & val) {
> > + if (val & ADIN1300_NRG_PD_TX_EN)
> > + *tx_interval = 1;
>
> What does 1 mean? 1 pico second, one hour? Anything but zero seconds?
> Does the datasheet specify what it actually does? Maybe you should be
> using ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL here, to indicate you actually
> have no idea, but it is the default for this PHY?
This PHY currently has a 1 second TX interval.
As specified by the datasheet (page 22 Energy-Detect Powerdown Mode section):
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
"The PHY monitors the line for signal energy and sends a link pulse once every
second"
Currently there is no control exposed to configure this interval; but there
could be some registers (that are not
exposed yet) that could control this.
Micrel's datasheet mentions (page 27 3.16.1ENERGY-DETECT POWER-DOWN MODE
section):
http://ww1.microchip.com/downloads/en/devicedoc/2117f.pdf
"In EDPD Mode, the KSZ9031RNX shuts down all transceiver blocks, except for the
transmitter and energy detect cir-cuits.
Power can be reduced further by extending the time interval between the
transmissions of link pulses to check forthe
presence of a link partner."
I did not check for Micrel to see how that interval is controlled.
The reason for doing this TX interval control was mostly based on this mention
from Micrel's datasheet, with
consideration that it could work ADIN & other future PHYs.
>
> > + else
> > + *tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
> > + } else {
> > + *tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
> > +{
> > + u16 val;
> > +
> > + if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
> > + return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
> > + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
> > +
> > + val = ADIN1300_NRG_PD_EN;
> > + if (tx_interval != ETHTOOL_PHY_EDPD_NO_TX)
> > + val |= ADIN1300_NRG_PD_TX_EN;
>
> So you silently accept any interval? That sounds wrong. You really
> should be returning -EINVAL for any value other than, either 1, or
> maybe ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, if you change the get
> function.
ack;
will use ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, ETHTOOL_PHY_EDPD_NO_TX && 1 as
acceptable values here
>
> Andrew
Re: [PATCH] net: stmmac: socfpga: re-use the `interface` parameter from platform data
On Tue, 2019-09-10 at 17:46 +0200, David Miller wrote: > [External] > > From: David Miller > Date: Tue, 10 Sep 2019 17:45:44 +0200 (CEST) > > > From: Alexandru Ardelean > > Date: Fri, 6 Sep 2019 15:30:54 +0300 > > > > > The socfpga sub-driver defines an `interface` field in the `socfpga_dwmac` > > > struct and parses it on init. > > > > > > The shared `stmmac_probe_config_dt()` function also parses this from the > > > device-tree and makes it available on the returned `plat_data` (which is > > > the same data available via `netdev_priv()`). > > > > > > All that's needed now is to dig that information out, via some > > > `dev_get_drvdata()` && `netdev_priv()` calls and re-use it. > > > > > > Signed-off-by: Alexandru Ardelean > > > > This doesn't build even on net-next. > Right. My bad. I think I got confused with multiple/cross-testing and probably this change didn't even get compiled. Apologies for this. Will send a good version. Alex > Specifically: > > drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c: In function > ‘socfpga_gen5_set_phy_mode’: > drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:264:44: error: ‘phymode’ > undeclared (first use in this function); > did you mean ‘phy_modes’? > 264 | dev_err(dwmac->dev, "bad phy mode %d\n", phymode); > |^~~ > ./include/linux/device.h:1499:32: note: in definition of macro ‘dev_err’ > 1499 | _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__) > |^~~ > drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:264:44: note: each > undeclared identifier is reported only once for > each function it appears in > 264 | dev_err(dwmac->dev, "bad phy mode %d\n", phymode); > |^~~ > ./include/linux/device.h:1499:32: note: in definition of macro ‘dev_err’ > 1499 | _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__) > |^~~ > drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c: In function > ‘socfpga_gen10_set_phy_mode’: > drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:340:6: error: ‘phymode’ > undeclared (first use in this function); > did you mean ‘phy_modes’? > 340 | phymode == PHY_INTERFACE_MODE_MII || > | ^~~ > | phy_modes
Re: [PATCH v2] dt-bindings: iio: accel: add binding documentation for ADIS16240
On Thu, 2019-09-12 at 18:39 -0300, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
>
> Signed-off-by: Rodrigo Ribeiro Carvalho
> ---
> V2:
> - Remove true constant for spi-cpha and spi-cpol
> - Add description field for spi-cpha and spi-cpol
> - Add maxItems field for spi-cpha and spi-cpol
>
> .../bindings/iio/accel/adi,adis16240.yaml | 61 +++
> 1 file changed, 61 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index ..4b1bd2419604
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> + - Alexandru Ardelean
> +
> +description: |
> + ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> + SPI interface.
> +https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> + compatible:
> +enum:
> + - adi,adis16240
> +
> + reg:
> +maxItems: 1
> +
> + spi-cpha:
> +description: |
> + See Documentation/devicetree/bindings/spi/spi-controller.yaml
> +maxItems: 1
Description for standard properties is not required.
For spi-cpha/cpol just "true" seems sufficient.
So
spi-cpha: true
spi-cpol: true
> +
> + spi-cpol: |
> +description: |
> + See Documentation/devicetree/bindings/spi/spi-controller.yaml
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
If spi-cpha & spi-cpol are true, they should typically be also required.
Though, I think Rob would answer things better here.
> +examples:
> + - |
> +#include
> +#include
> +spi0 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +/* Example for a SPI device node */
> +accelerometer@0 {
> +compatible = "adi,adis16240";
> +reg = <0>;
> +spi-max-frequency = <250>;
> +spi-cpol;
> +spi-cpha;
> +interrupt-parent = <&gpio0>;
> +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +};
Re: [PATCH v4 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable
On Sun, 2019-09-15 at 08:11 -0700, Florian Fainelli wrote:
> [External]
>
>
>
> On 9/14/2019 8:29 AM, Andrew Lunn wrote:
> > On Thu, Sep 12, 2019 at 07:28:12PM +0300, Alexandru Ardelean wrote:
> >
> > > +static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
> > > +{
> > > + u16 val;
> > > +
> > > + if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
> > > + return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
> > > + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
> > > +
> > > + val = ADIN1300_NRG_PD_EN;
> > > +
> > > + switch (tx_interval) {
> > > + case 1000: /* 1 second */
> > > + /* fallthrough */
> > > + case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
> > > + val |= ADIN1300_NRG_PD_TX_EN;
> > > + /* fallthrough */
> > > + case ETHTOOL_PHY_EDPD_NO_TX:
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return phy_modify(phydev, ADIN1300_PHY_CTRL_STATUS2,
> > > + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN),
> > > + val);
> > > +}
> > > +
> > >
> > > + rc = adin_set_edpd(phydev, 1);
> > > + if (rc < 0)
> > > + return rc;
> >
> > Hi Alexandru
> >
> > Shouldn't this be adin_set_edpd(phydev, 1000);
>
> That does sound like the intended use, or use
> ETHTOOL_PHY_EDPD_DFLT_TX_MSECS, with that fixed:
Ack.
Many thanks for catching this.
I missed it when re-spinning.
>
> Reviewed-by: Florian Fainelli
Re: [PATCH 2/2] dt-bindings: net: dwmac: document 'mac-mode' property
On Fri, 2019-09-13 at 15:36 +0100, Rob Herring wrote:
> [External]
>
> On Fri, Sep 06, 2019 at 04:02:56PM +0300, Alexandru Ardelean wrote:
> > This change documents the 'mac-mode' property that was introduced in the
> > 'stmmac' driver to support passive mode converters that can sit in-between
> > the MAC & PHY.
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index c78be15704b9..ebe4537a7cce 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -112,6 +112,14 @@ properties:
> >reset-names:
> > const: stmmaceth
> >
> > + mac-mode:
> > +maxItems: 1
>
> Is this an array because {min,max}Items is for arrays? It should be
> defined as a string with possible values.
>
> As this property is the same as another, you can do this:
>
> $ref: ethernet-controller.yaml#/properties/phy-connection-type
>
> Unless only a small subset of those values are valid here, then you may
> want to list them here.
>
Ack.
Thank you.
Will investigate and re-spin.
> > +description:
> > + The property is identical to 'phy-mode', and assumes that there is
> > mode
> > + converter in-between the MAC & PHY (e.g. GMII-to-RGMII). This
> > converter
> > + can be passive (no SW requirement), and requires that the MAC operate
> > + in a different mode than the PHY in order to function.
> > +
> >snps,axi-config:
> > $ref: /schemas/types.yaml#definitions/phandle
> > description:
> > --
> > 2.20.1
> >
Re: [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct
On Sun, 2019-09-15 at 11:14 +0100, Jonathan Cameron wrote:
> [External]
>
> On Fri, 13 Sep 2019 14:55:49 +0300
> Alexandru Ardelean wrote:
>
> > The ADIS library is one of the few users of the new `cs_change_delay`
> > parameter for an spi_transfer.
> >
> > The introduction of the `spi_delay` struct, requires that the users of of
> > `cs_change_delay` get an update. This change updates the ADIS library.
> >
> > Signed-off-by: Alexandru Ardelean
>
> Looks to me like the build is broken between patches 3 and 4.
> Don't do that as it breaks bisectability.
>
> If you are changing an interface like this it has to occur in one patch,
> of you have to have intermediate code that deals with the smooth transition.
>
> Otherwise, looks like a sensible bit of rework to me.
I thought about it, but wasn't sure.
Will create a v3 with patches 3 & 4 squashed.
Thanks
Alex
>
> Jonathan
>
> > ---
> > drivers/iio/imu/adis.c | 24
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > index 1631c255deab..2cd2cc2316c6 100644
> > --- a/drivers/iio/imu/adis.c
> > +++ b/drivers/iio/imu/adis.c
> > @@ -39,24 +39,24 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->write_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .tx_buf = adis->tx + 2,
> > .bits_per_word = 8,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->write_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .tx_buf = adis->tx + 4,
> > .bits_per_word = 8,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->write_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .tx_buf = adis->tx + 6,
> > .bits_per_word = 8,
> > @@ -139,16 +139,16 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->write_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .tx_buf = adis->tx + 2,
> > .bits_per_word = 8,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->read_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .tx_buf = adis->tx + 4,
> > .rx_buf = adis->rx,
> > @@ -156,8 +156,8 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> > .len = 2,
> > .cs_change = 1,
> > .delay_usecs = adis->data->read_delay,
> > - .cs_change_delay = adis->data->cs_change_delay,
> > - .cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > + .cs_change_delay.value = adis->data->cs_change_delay,
> > + .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> > }, {
> > .rx_buf = adis->rx + 2,
> > .bits_per_word = 8,
Re: [PATCH v2] net: stmmac: socfpga: re-use the `interface` parameter from platform data
On Sun, 2019-09-15 at 19:51 +0100, David Miller wrote: > [External] > > From: Alexandru Ardelean > Date: Thu, 12 Sep 2019 16:28:50 +0300 > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > b/drivers/net/ethernet/stmicro/stmmac/dwmac- > > socfpga.c > > index c141fe783e87..5b6213207c43 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > ... > > +static inline int socfpga_get_plat_phymode(struct socfpga_dwmac *dwmac) > > Please do not use the inline keyword in foo.c files, let the compiler device. Ack. Will remove.
Re: [RFC PATCH 03/15] spi: make `cs_change_delay` the first user of the `spi_delay` logic
On Mon, 2019-09-16 at 13:25 +0100, Mark Brown wrote: > [External] > > On Fri, Sep 13, 2019 at 02:45:38PM +0300, Alexandru Ardelean wrote: > > > - u16 cs_change_delay; > > - u8 cs_change_delay_unit; > > + struct spi_delaycs_change_delay; > > This breaks the build as there is a user of this interface. Ack. Jonathan pointed this out. There's a V3 that changes both this and it's user (in IIO). V3: https://lore.kernel.org/linux-iio/20190916071024.21447-1-alexandru.ardel...@analog.com/T/#t V2: https://lore.kernel.org/linux-iio/20190913115549.3823-1-alexandru.ardel...@analog.com/T/#t [ archive is from the IIO list ] Well, I'm hoping you are referring to the same user. On a general note: I apologise for the amount of noise/spam I am doing here. Still adjusting to how to do things/changes that touch 2 subsystems, especially when trees are not quite in-sync.
Re: [RFC PATCH 03/15] spi: make `cs_change_delay` the first user of the `spi_delay` logic
On Mon, 2019-09-16 at 13:47 +0100, Mark Brown wrote: > [External] > > On Mon, Sep 16, 2019 at 12:37:12PM +, Ardelean, Alexandru wrote: > > > > This breaks the build as there is a user of this interface. > > Ack. > > Jonathan pointed this out. > > There's a V3 that changes both this and it's user (in IIO). > > That v3 seems to be a small subset of this series? Ack. V3 is the first 4 patches from this series. Well, patches 3 & 4 are squashed. I am 100% convinced that the entire series is a good idea. In the sense that a `struct spi_delay` may be a good idea, but at the same time, it may be un-needed. All I wanted to do, was to add another delay somewhere, and got lost in the rework of current delays. I thought about proposing just the first 4 patches [on their own], but I thought that showing the current series as-is now, may be a good idea as well [to gather some feedback].
Re: [RFC PATCH 03/15] spi: make `cs_change_delay` the first user of the `spi_delay` logic
On Mon, 2019-09-16 at 14:43 +0100, Mark Brown wrote: > [External] > > On Mon, Sep 16, 2019 at 01:04:42PM +, Ardelean, Alexandru wrote: > > On Mon, 2019-09-16 at 13:47 +0100, Mark Brown wrote: > > > That v3 seems to be a small subset of this series? > > Ack. > > V3 is the first 4 patches from this series. > > Well, patches 3 & 4 are squashed. > > I am 100% convinced that the entire series is a good idea. Something happened here to the "not" word. Probably got lost in an alternate dimension ¯\_(ツ)_/¯ . Was supposed to be: "I am not 100% convinced that the entire series is a good idea." > > In the sense that a `struct spi_delay` may be a good idea, but at the > > same time, it may be un-needed. > > All I wanted to do, was to add another delay somewhere, and got lost in > > the rework of current delays. > > I thought about proposing just the first 4 patches [on their own], but > > I thought that showing the current series as-is > > now, may be a good idea as well [to gather some feedback]. > > I think it makes more sense to review as a whole series rather than only > a part of the conversion, it doesn't really help to only do part of it. > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. Ack. Problem is: I have to re-setup my email client every now-n-then since the work-email server has some issues with Linux email clients. And I sometimes forget to configure this. [ Exchange does not always get along well with non-Outlook clients ]
Re: [PATCH 2/2] dt-bindings: net: dwmac: document 'mac-mode' property
On Mon, 2019-09-16 at 12:49 +0300, Alexandru Ardelean wrote:
> On Fri, 2019-09-13 at 15:36 +0100, Rob Herring wrote:
> > [External]
> >
> > On Fri, Sep 06, 2019 at 04:02:56PM +0300, Alexandru Ardelean wrote:
> > > This change documents the 'mac-mode' property that was introduced in
> > > the
> > > 'stmmac' driver to support passive mode converters that can sit in-
> > > between
> > > the MAC & PHY.
> > >
> > > Signed-off-by: Alexandru Ardelean
> > > ---
> > > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 8
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > index c78be15704b9..ebe4537a7cce 100644
> > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > @@ -112,6 +112,14 @@ properties:
> > >reset-names:
> > > const: stmmaceth
> > >
> > > + mac-mode:
> > > +maxItems: 1
> >
> > Is this an array because {min,max}Items is for arrays? It should be
> > defined as a string with possible values.
> >
> > As this property is the same as another, you can do this:
> >
> > $ref: ethernet-controller.yaml#/properties/phy-connection-type
> >
> > Unless only a small subset of those values are valid here, then you
> > may
> > want to list them here.
> >
>
> Ack.
> Thank you.
>
> Will investigate and re-spin.
Looking at '$ref: ethernet-controller.yaml#/properties/phy-connection-type'
it looks like 'mac-mode' could cover almost all 'phy-connection-type'
except for a few (1 or 2). The 'dwmac' driver is pretty complex/big.
There was a note that Andrew made on a previous change, that we could have
a 'mac-mode' (similar to 'phy-mode') and that could become generic (either
in phylib or maybe somewhere else in netdev).
In any case, the conclusion [from my side] would be that
'$ref: ethernet-controller.yaml#/properties/phy-connection-type'
could work, and be sufficiently future-proof.
Thanks
Alex
>
>
> > > +description:
> > > + The property is identical to 'phy-mode', and assumes that
> > > there is mode
> > > + converter in-between the MAC & PHY (e.g. GMII-to-RGMII). This
> > > converter
> > > + can be passive (no SW requirement), and requires that the MAC
> > > operate
> > > + in a different mode than the PHY in order to function.
> > > +
> > >snps,axi-config:
> > > $ref: /schemas/types.yaml#definitions/phandle
> > > description:
> > > --
> > > 2.20.1
> > >
Re: [PATCH] dt-bindings: net: dwmac: fix 'mac-mode' type
On Tue, 2019-09-17 at 14:41 +0200, Andrew Lunn wrote:
> [External]
>
> On Tue, Sep 17, 2019 at 01:30:52PM +0300, Alexandru Ardelean wrote:
> > The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
> > which are enums of mode strings.
> >
> > The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
> > enum (except for 1 or 2). But in general, there may be a case where
> > 'mac-mode' becomes more generic and is moved as part of phylib or netdev.
> >
> > In any case, the 'mac-mode' field should be made an enum, and it also makes
> > sense to just reference the 'phy-connection-type' from
> > 'ethernet-controller.yaml'. That will also make it more future-proof for new
> > modes.
> >
> > Signed-off-by: Alexandru Ardelean
>
> Hi Alexandru
>
> Adding a Fixes: tag would be good. Just reply in this thread, and
> patchwork will do magic to append it to the patch.
>
Oops. Good point.
Thanks for the tip.
Let's see if Rob agrees as well.
Fixes: 9c15d3597c62 ("dt-bindings: net: dwmac: document 'mac-mode' property")
> Reviewed-by: Andrew Lunn
>
> Andrew
Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor
On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote: > [External] > Hey, Comments inline. I thought I sent an initial review, but seems to have gotten lost [maybe in my email client]. Oh well. I managed to re-do it anyway. I tried to group them this time. The more prominent part is [3]; this driver needs a bit more error checking on regmap_() returns. Generally some notes: - Is there a need to implement the 32Khz or 32Mhz clock calibration routines on startup? Some drivers need this, some don't/ - From the functional diagram, it looks like maybe the VREF would be needed to be hooked via a regulator framework; but this could be done later - Just curios here: there is gesture mode as well; will that be implemented later? Or will there be other modes implemented? If I remember anything else I may come back with a reply. Thanks Alex > Add initial support for Analog Devices ADUX1020 Photometric sensor. > Only proximity mode has been enabled for now. > > Signed-off-by: Manivannan Sadhasivam > --- > drivers/iio/light/Kconfig| 11 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/adux1020.c | 783 +++ Does MAINTAINERS need updating as well? > 3 files changed, 795 insertions(+) > create mode 100644 drivers/iio/light/adux1020.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 08d7e1ef2186..3f8c8689cd89 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -32,6 +32,17 @@ config ADJD_S311 > This driver can also be built as a module. If so, the module > will be called adjd_s311. > > +config ADUX1020 > + tristate "ADUX1020 photometric sensor" > + select REGMAP_I2C > + depends on I2C > + help > + Say Y here if you want to build a driver for the Analog Devices > + ADUX1020 photometric sensor. > + > + To compile this driver as a module, choose M here: the > + module will be called adux1020. > + > config AL3320A > tristate "AL3320A ambient light sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index 00d1f9b98f39..5d650ce46a40 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -6,6 +6,7 @@ > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_ACPI_ALS) += acpi-als.o > obj-$(CONFIG_ADJD_S311) += adjd_s311.o > +obj-$(CONFIG_ADUX1020) += adux1020.o > obj-$(CONFIG_AL3320A)+= al3320a.o > obj-$(CONFIG_APDS9300) += apds9300.o > obj-$(CONFIG_APDS9960) += apds9960.o > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c > new file mode 100644 > index ..d0b76e5b44f1 > --- /dev/null > +++ b/drivers/iio/light/adux1020.c > @@ -0,0 +1,783 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * adux1020.c - Support for Analog Devices ADUX1020 photometric sensor Maybe drop the adux1020.c part? I think something like this should be sufficient: "Analog Devices ADUX1020 photometric sensor" > + * > + * Copyright (C) 2019 Linaro Ltd. > + * Author: Manivannan Sadhasivam > + * > + * TODO: Triggered buffer support Maybe drop the TODO? It's not needed for mainline. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#define ADUX1020_REGMAP_NAME "adux1020_regmap" > +#define ADUX1020_DRV_NAME"adux1020" > + > +/* System registers */ > +#define ADUX1020_REG_CHIP_ID 0x08 > +#define ADUX1020_REG_SLAVE_ADDRESS 0x09 > + > +#define ADUX1020_REG_SW_RESET0x0f > +#define ADUX1020_REG_INT_ENABLE 0x1c > +#define ADUX1020_REG_INT_POLARITY0x1d > +#define ADUX1020_REG_PROX_TH_ON1 0x2a > +#define ADUX1020_REG_PROX_TH_OFF10x2b > +#define ADUX1020_REG_PROX_TYPE 0x2f > +#define ADUX1020_REG_TEST_MODES_3 0x32 > +#define ADUX1020_REG_FORCE_MODE 0x33 > +#define ADUX1020_REG_FREQUENCY 0x40 > +#define ADUX1020_REG_LED_CURRENT 0x41 > +#define ADUX1020_REG_OP_MODE0x45 > +#define ADUX1020_REG_INT_MASK 0x48 > +#define ADUX1020_REG_INT_STATUS 0x49 > +#define ADUX1020_REG_DATA_BUFFER0x60 > + > +/* Chip ID bits */ > +#define ADUX1020_CHIP_ID_MASKGENMASK(11, 0) > +#define ADUX1020_CHIP_ID 0x03fc > + > +#define ADUX1020_MODE_OUT_SHIFT 4 I'm seeing a few _SHIFT macros. Maybe use the FIELD_PREP() macro where possible? [1] > +#define ADUX1020_MODE_OUT_PROX_I 1 > +#define ADUX1020_MODE_OUT_PROX_XY3 > + > +#define ADUX1020_SW_RESETBIT(1) > +#define ADUX1020_FIFO_FLUSH BIT(15) > +#define ADUX1020_OP_MODE_MASKGENMASK(3, 0) > +#define ADUX1020_DATA_OUT_MODE_MASK GENMASK(7,
Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions
On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> On Sun, 6 Oct 2019 10:12:01 +0100
> Jonathan Cameron wrote:
>
> > On Thu, 26 Sep 2019 14:18:04 +0300
> > Alexandru Ardelean wrote:
> >
> > > This will allow more flexible control to group reads & writes into a
> > > single
> > > lock (particularly the state_lock).
> > >
> > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > simplest fix
> > > would have been to just add another lock, which would not be a good
> > > idea on
> > > the long-run.
> > >
> > > Signed-off-by: Alexandru Ardelean
> > Applied to the togreg branch of iio.git and pushed out as testing etc.
> >
> > Jonathan
> >
> 0-day found a potential issue (kind of) in the read functions.
>
> > > ---
> > > drivers/iio/imu/adis.c | 34 +--
> > > include/linux/iio/imu/adis.h | 114
> > > ++-
> > > 2 files changed, 128 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > index 3c2d896e3a96..4f3be011c898 100644
> > > --- a/drivers/iio/imu/adis.c
> > > +++ b/drivers/iio/imu/adis.c
> > > @@ -26,7 +26,14 @@
> > > #define ADIS_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > > #define ADIS_GLOB_CMD_SW_RESET BIT(7)
> > >
> > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > +/**
> > > + * __adis_write_reg() - write N bytes to register (unlocked version)
> > > + * @adis: The adis device
> > > + * @reg: The address of the lower of the two registers
> > > + * @value: The value to write to device (up to 4 bytes)
> > > + * @size: The size of the @value (in bytes)
> > > + */
> > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > unsigned int value, unsigned int size)
> > > {
> > > unsigned int page = reg / ADIS_PAGE_SIZE;
> > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > > },
> > > };
> > >
> > > - mutex_lock(&adis->state_lock);
> > > -
> > > spi_message_init(&msg);
> > >
> > > if (adis->current_page != page) {
> > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > > adis->tx[3] = value & 0xff;
> > > break;
> > > default:
> > > - ret = -EINVAL;
> > > - goto out_unlock;
> > > + return -EINVAL;
> > > }
> > >
> > > xfers[size].cs_change = 0;
> > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis, unsigned
> > > int reg,
> > > adis->current_page = page;
> > > }
> > >
> > > -out_unlock:
> > > - mutex_unlock(&adis->state_lock);
> > > -
> > > return ret;
> > > }
> > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > >
> > > /**
> > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > + * __adis_read_reg() - read N bytes from register (unlocked version)
> > > * @adis: The adis device
> > > * @reg: The address of the lower of the two registers
> > > * @val: The value read back from the device
> > > + * @size: The size of the @val buffer
> > > */
> > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > unsigned int *val, unsigned int size)
> > > {
> > > unsigned int page = reg / ADIS_PAGE_SIZE;
> > > @@ -188,15 +190,14 @@ int adis_read_reg(struct adis *adis, unsigned
> > > int reg,
> > > spi_message_add_tail(&xfers[3], &msg);
> > > break;
> > > default:
> > > - ret = -EINVAL;
> > > - goto out_unlock;
> > > + return -EINVAL;
> > > }
> > >
> > > ret = spi_sync(adis->spi, &msg);
> > > if (ret) {
> > > dev_err(&adis->spi->dev, "Failed to read register 0x%02X:
> > > %d\n",
> > > reg, ret);
> > > - goto out_unlock;
> > > + return ret;
> > > } else {
> > > adis->current_page = page;
> > > }
> > > @@ -210,12 +211,9 @@ int adis_read_reg(struct adis *adis, unsigned
> > > int reg,
> > > break;
> > > }
> > >
> > > -out_unlock:
> > > - mutex_unlock(&adis->state_lock);
> > > -
> > > return ret;
> > > }
> > > -EXPORT_SYMBOL_GPL(adis_read_reg);
> > > +EXPORT_SYMBOL_GPL(__adis_read_reg);
> > >
> > > #ifdef CONFIG_DEBUG_FS
> > >
> > > diff --git a/include/linux/iio/imu/adis.h
> > > b/include/linux/iio/imu/adis.h
> > > index 3ed5eceaac2d..3a028c40e04e 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -75,11 +75,121 @@ int adis_init(struct adis *adis, struct iio_dev
> > > *indio_dev,
> > > struct spi_device *spi, const struct adis_data *data);
> > > int adis_reset(struct adis *adis);
> > >
> > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > unsigned int val, unsigned int size);
> > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > +int __adis_read_reg(struct adis *
Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions
On Tue, 2019-10-08 at 06:54 +, Ardelean, Alexandru wrote:
> [External]
>
> On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> > On Sun, 6 Oct 2019 10:12:01 +0100
> > Jonathan Cameron wrote:
> >
> > > On Thu, 26 Sep 2019 14:18:04 +0300
> > > Alexandru Ardelean wrote:
> > >
> > > > This will allow more flexible control to group reads & writes into
> > > > a
> > > > single
> > > > lock (particularly the state_lock).
> > > >
> > > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > > simplest fix
> > > > would have been to just add another lock, which would not be a good
> > > > idea on
> > > > the long-run.
> > > >
> > > > Signed-off-by: Alexandru Ardelean
> > > Applied to the togreg branch of iio.git and pushed out as testing
> > > etc.
> > >
> > > Jonathan
> > >
> > 0-day found a potential issue (kind of) in the read functions.
> >
> > > > ---
> > > > drivers/iio/imu/adis.c | 34 +--
> > > > include/linux/iio/imu/adis.h | 114
> > > > ++-
> > > > 2 files changed, 128 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > > index 3c2d896e3a96..4f3be011c898 100644
> > > > --- a/drivers/iio/imu/adis.c
> > > > +++ b/drivers/iio/imu/adis.c
> > > > @@ -26,7 +26,14 @@
> > > > #define ADIS_MSC_CTRL_DATA_RDY_DIO2BIT(0)
> > > > #define ADIS_GLOB_CMD_SW_RESET BIT(7)
> > > >
> > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > +/**
> > > > + * __adis_write_reg() - write N bytes to register (unlocked
> > > > version)
> > > > + * @adis: The adis device
> > > > + * @reg: The address of the lower of the two registers
> > > > + * @value: The value to write to device (up to 4 bytes)
> > > > + * @size: The size of the @value (in bytes)
> > > > + */
> > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > unsigned int value, unsigned int size)
> > > > {
> > > > unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > > },
> > > > };
> > > >
> > > > - mutex_lock(&adis->state_lock);
> > > > -
> > > > spi_message_init(&msg);
> > > >
> > > > if (adis->current_page != page) {
> > > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > int
> > > > reg,
> > > > adis->tx[3] = value & 0xff;
> > > > break;
> > > > default:
> > > > - ret = -EINVAL;
> > > > - goto out_unlock;
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > xfers[size].cs_change = 0;
> > > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis,
> > > > unsigned
> > > > int reg,
> > > > adis->current_page = page;
> > > > }
> > > >
> > > > -out_unlock:
> > > > - mutex_unlock(&adis->state_lock);
> > > > -
> > > > return ret;
> > > > }
> > > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > > >
> > > > /**
> > > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > > + * __adis_read_reg() - read N bytes from register (unlocked
> > > > version)
> > > > * @adis: The adis device
> > > > * @reg: The address of the lower of the two registers
> > > > * @val: The value read back from the device
> > > > + * @size: The size of the @val buffer
> > > > */
> > > > -int adis_read_reg(struct adis *adis, unsigned int reg,
> > > > +int __adis_read_reg(struct adis *adis, unsigned int reg,
> > > > unsigned int *val, unsigned int size)
> > > > {
> > > > unsigned int page = reg / AD
Re: [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions
On Tue, 2019-10-08 at 08:47 +, Ardelean, Alexandru wrote:
> [External]
>
> On Tue, 2019-10-08 at 06:54 +, Ardelean, Alexandru wrote:
> > [External]
> >
> > On Mon, 2019-10-07 at 22:16 +0100, Jonathan Cameron wrote:
> > > On Sun, 6 Oct 2019 10:12:01 +0100
> > > Jonathan Cameron wrote:
> > >
> > > > On Thu, 26 Sep 2019 14:18:04 +0300
> > > > Alexandru Ardelean wrote:
> > > >
> > > > > This will allow more flexible control to group reads & writes
> > > > > into
> > > > > a
> > > > > single
> > > > > lock (particularly the state_lock).
> > > > >
> > > > > The end-goal is to remove the indio_dev->mlock usage, and the
> > > > > simplest fix
> > > > > would have been to just add another lock, which would not be a
> > > > > good
> > > > > idea on
> > > > > the long-run.
> > > > >
> > > > > Signed-off-by: Alexandru Ardelean
> > > > >
> > > > Applied to the togreg branch of iio.git and pushed out as testing
> > > > etc.
> > > >
> > > > Jonathan
> > > >
> > > 0-day found a potential issue (kind of) in the read functions.
> > >
> > > > > ---
> > > > > drivers/iio/imu/adis.c | 34 +--
> > > > > include/linux/iio/imu/adis.h | 114
> > > > > ++-
> > > > > 2 files changed, 128 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > > > index 3c2d896e3a96..4f3be011c898 100644
> > > > > --- a/drivers/iio/imu/adis.c
> > > > > +++ b/drivers/iio/imu/adis.c
> > > > > @@ -26,7 +26,14 @@
> > > > > #define ADIS_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > > > > #define ADIS_GLOB_CMD_SW_RESET BIT(7)
> > > > >
> > > > > -int adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > +/**
> > > > > + * __adis_write_reg() - write N bytes to register (unlocked
> > > > > version)
> > > > > + * @adis: The adis device
> > > > > + * @reg: The address of the lower of the two registers
> > > > > + * @value: The value to write to device (up to 4 bytes)
> > > > > + * @size: The size of the @value (in bytes)
> > > > > + */
> > > > > +int __adis_write_reg(struct adis *adis, unsigned int reg,
> > > > > unsigned int value, unsigned int size)
> > > > > {
> > > > > unsigned int page = reg / ADIS_PAGE_SIZE;
> > > > > @@ -70,8 +77,6 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > > },
> > > > > };
> > > > >
> > > > > - mutex_lock(&adis->state_lock);
> > > > > -
> > > > > spi_message_init(&msg);
> > > > >
> > > > > if (adis->current_page != page) {
> > > > > @@ -96,8 +101,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > > > int
> > > > > reg,
> > > > > adis->tx[3] = value & 0xff;
> > > > > break;
> > > > > default:
> > > > > - ret = -EINVAL;
> > > > > - goto out_unlock;
> > > > > + return -EINVAL;
> > > > > }
> > > > >
> > > > > xfers[size].cs_change = 0;
> > > > > @@ -113,20 +117,18 @@ int adis_write_reg(struct adis *adis,
> > > > > unsigned
> > > > > int reg,
> > > > > adis->current_page = page;
> > > > > }
> > > > >
> > > > > -out_unlock:
> > > > > - mutex_unlock(&adis->state_lock);
> > > > > -
> > > > > return ret;
> > > > > }
> > > > > -EXPORT_SYMBOL_GPL(adis_write_reg);
> > > > > +EXPORT_SYMBOL_GPL(__adis_write_reg);
> > > > >
> > > > > /**
> > > > > - * adis_read_reg() - read 2 bytes from a 16-bit register
> > > > > + * __adis_read_reg() - read N bytes from re
Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor
On Wed, 2019-10-09 at 15:15 +0530, Manivannan Sadhasivam wrote: > [External] > > Hi Ardelean, > > Thanks for the quick review! > > On Tue, Oct 08, 2019 at 06:52:50AM +, Ardelean, Alexandru wrote: > > On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote: > > > [External] > > > > > > > Hey, > > > > Comments inline. > > > > I thought I sent an initial review, but seems to have gotten lost > > [maybe in > > my email client]. > > Oh well. I managed to re-do it anyway. > > > > I tried to group them this time. > > > > The more prominent part is [3]; this driver needs a bit more error > > checking > > on regmap_() returns. > > > > Yes, agree. I forgot that I'm not working on memory mapped region ;-) > > > Generally some notes: > > - Is there a need to implement the 32Khz or 32Mhz clock calibration > > routines on startup? Some drivers need this, some don't/ > > Calibration is required to have the precise reading but it is not a > blocker. > We can add it later. Fine from my side. I should have also mentioned this earlier [that the calibration can be added later]. > > > - From the functional diagram, it looks like maybe the VREF would be > > needed > > to be hooked via a regulator framework; but this could be done later > > Right but the reference board schematics is not very clear about VREF and > neither the sensor datasheet. That's why I intentionally left it. Will > get > in touch with the board vendor to figure out what is the recommended VREF > voltage and submit a patch later. ack; well, typically [for the driver] VREF support is just added in the driver and then the board device-tree just implements what it needs [that is, if it needs it]; some board designs just have a fixed value, and the driver does not need to care/know about the VREF regulator; the idea of adding regulator support in the driver is to allow the driver to initialize it [if there is a device-tree entry for it] but this can be done later; > > > - Just curios here: there is gesture mode as well; will that be > > implemented > > later? Or will there be other modes implemented? > > Currently only proximity mode is implemented. There are gesture and > sample > modes and I left those as a TODO. But I'm not sure whether IIO is > supporting > gesture mode properly or not. I don't have any input on this at the moment [about gesture support & IIO]. I'd have to investigate. Maybe Jonathan has some thoughts. > > > If I remember anything else I may come back with a reply. > > > > Sure. > > > Thanks > > Alex > > > > > Add initial support for Analog Devices ADUX1020 Photometric sensor. > > > Only proximity mode has been enabled for now. > > > > > > Signed-off-by: Manivannan Sadhasivam < > > > manivannan.sadhasi...@linaro.org> > > > --- > > > drivers/iio/light/Kconfig| 11 + > > > drivers/iio/light/Makefile | 1 + > > > drivers/iio/light/adux1020.c | 783 > > > +++ > > > > Does MAINTAINERS need updating as well? > > > > I don't prefer to have MAINTAINERS entry for small drivers like this. > Anyway, get_maintainers will return my mailing address based on the > commit signing. ack > > > > 3 files changed, 795 insertions(+) > > > create mode 100644 drivers/iio/light/adux1020.c > > > > > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > > > index 08d7e1ef2186..3f8c8689cd89 100644 > > > --- a/drivers/iio/light/Kconfig > > > +++ b/drivers/iio/light/Kconfig > > > @@ -32,6 +32,17 @@ config ADJD_S311 > > > This driver can also be built as a module. If so, the module > > > will be called adjd_s311. > > > > > > +config ADUX1020 > > > + tristate "ADUX1020 photometric sensor" > > > + select REGMAP_I2C > > > + depends on I2C > > > + help > > > + Say Y here if you want to build a driver for the Analog Devices > > > + ADUX1020 photometric sensor. > > > + > > > + To compile this driver as a module, choose M here: the > > > + module will be called adux1020. > > > + > > > config AL3320A > > > tristate "AL3320A ambient light sensor" > > > depends on I2C > > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > > > index 00d1f9b98f39..5d650ce46a40 100644 > > > --- a/drivers/iio/light/M
Re: [PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock
On Sun, 2019-10-06 at 10:06 +0100, Jonathan Cameron wrote:
> [External]
>
> On Sun, 6 Oct 2019 09:53:33 +0100
> Jonathan Cameron wrote:
>
> > On Thu, 26 Sep 2019 14:18:03 +0300
> > Alexandru Ardelean wrote:
> >
> > > The lock can be extended a bit to protect other elements that are not
> > > particular to just TX/RX. Another idea would have been to just add a
> > > new
> > > `state_lock`, but that would mean 2 locks which would be redundant,
> > > and
> > > probably cause more potential for dead-locks.
> > >
> > > What will be done in the next patches, will be to add some unlocked
> > > versions for read/write_reg functions.
> > >
> > > Signed-off-by: Alexandru Ardelean
> >
> > Would be good to document the scope of the lock as a comment when it
> > is defined. What exactly is 'state' in this case?
> As this can be done as a follow up and the rest of the series is fine
> as is...
>
Will do.
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
>
> Thanks,
>
> Jonathan
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/imu/adis.c| 10 +-
> > > drivers/iio/imu/adis_buffer.c | 4 ++--
> > > include/linux/iio/imu/adis.h | 2 +-
> > > 3 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > > index 1631c255deab..3c2d896e3a96 100644
> > > --- a/drivers/iio/imu/adis.c
> > > +++ b/drivers/iio/imu/adis.c
> > > @@ -70,7 +70,7 @@ int adis_write_reg(struct adis *adis, unsigned int
> > > reg,
> > > },
> > > };
> > >
> > > - mutex_lock(&adis->txrx_lock);
> > > + mutex_lock(&adis->state_lock);
> > >
> > > spi_message_init(&msg);
> > >
> > > @@ -114,7 +114,7 @@ int adis_write_reg(struct adis *adis, unsigned
> > > int reg,
> > > }
> > >
> > > out_unlock:
> > > - mutex_unlock(&adis->txrx_lock);
> > > + mutex_unlock(&adis->state_lock);
> > >
> > > return ret;
> > > }
> > > @@ -166,7 +166,7 @@ int adis_read_reg(struct adis *adis, unsigned int
> > > reg,
> > > },
> > > };
> > >
> > > - mutex_lock(&adis->txrx_lock);
> > > + mutex_lock(&adis->state_lock);
> > > spi_message_init(&msg);
> > >
> > > if (adis->current_page != page) {
> > > @@ -211,7 +211,7 @@ int adis_read_reg(struct adis *adis, unsigned int
> > > reg,
> > > }
> > >
> > > out_unlock:
> > > - mutex_unlock(&adis->txrx_lock);
> > > + mutex_unlock(&adis->state_lock);
> > >
> > > return ret;
> > > }
> > > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
> > > int adis_init(struct adis *adis, struct iio_dev *indio_dev,
> > > struct spi_device *spi, const struct adis_data *data)
> > > {
> > > - mutex_init(&adis->txrx_lock);
> > > + mutex_init(&adis->state_lock);
> > > adis->spi = spi;
> > > adis->data = data;
> > > iio_device_set_drvdata(indio_dev, adis);
> > > diff --git a/drivers/iio/imu/adis_buffer.c
> > > b/drivers/iio/imu/adis_buffer.c
> > > index 9ac8356d9a95..bf581a2c321d 100644
> > > --- a/drivers/iio/imu/adis_buffer.c
> > > +++ b/drivers/iio/imu/adis_buffer.c
> > > @@ -123,7 +123,7 @@ static irqreturn_t adis_trigger_handler(int irq,
> > > void *p)
> > > return -ENOMEM;
> > >
> > > if (adis->data->has_paging) {
> > > - mutex_lock(&adis->txrx_lock);
> > > + mutex_lock(&adis->state_lock);
> > > if (adis->current_page != 0) {
> > > adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> > > adis->tx[1] = 0;
> > > @@ -138,7 +138,7 @@ static irqreturn_t adis_trigger_handler(int irq,
> > > void *p)
> > >
> > > if (adis->data->has_paging) {
> > > adis->current_page = 0;
> > > - mutex_unlock(&adis->txrx_lock);
> > > + mutex_unlock(&adis->state_lock);
> > > }
> > >
> > > iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > > diff --git a/include/linux/iio/imu/adis.h
> > > b/include/linux/iio/imu/adis.h
> > > index 4c53815bb729..3ed5eceaac2d 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -61,7 +61,7 @@ struct adis {
> > > const struct adis_data *data;
> > > struct adis_burst *burst;
> > >
> > > - struct mutextxrx_lock;
> > > + struct mutexstate_lock;
> > > struct spi_message msg;
> > > struct spi_transfer *xfer;
> > > unsigned intcurrent_page;
Re: [PATCH 1/2] dt-bindings: iio: light: Add binding for ADUX1020
On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> [External]
>
> Add devicetree binding for Analog Devices ADUX1020 Photometric
> sensor.
>
Hey,
Thanks for the patches.
This dt-binding docs is in text format.
dt-binding docs now need to be in YAML format.
Also, patches for dt-bindings docs usually come after the driver is added.
So, this patch should be the second in the series, not the first.
Alex
> Signed-off-by: Manivannan Sadhasivam
> ---
> .../bindings/iio/light/adux1020.txt | 22 +++
> 1 file changed, 22 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/light/adux1020.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
> b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> new file mode 100644
> index ..e896dda30e36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> @@ -0,0 +1,22 @@
> +Analog Devices ADUX1020 Photometric sensor
> +
> +Link to datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
> +
> +Required properties:
> +
> + - compatible: should be "adi,adux1020"
> + - reg: the I2C address of the sensor
> +
> +Optional properties:
> +
> + - interrupts: interrupt mapping for IRQ as documented in
> + Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> +Example:
> +
> +adux1020@64 {
> + compatible = "adi,adux1020";
> + reg = <0x64>;
> + interrupt-parent = <&msmgpio>;
> + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
> +};
Re: [PATCH 1/2] dt-bindings: iio: light: Add binding for ADUX1020
On Mon, 2019-10-07 at 18:10 +0530, Manivannan Sadhasivam wrote:
> [External]
>
> Hi Ardelean,
>
> On 7 October 2019 3:51:16 PM IST, "Ardelean, Alexandru" <
> alexandru.ardel...@analog.com> wrote:
> > On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> > > [External]
> > >
> > > Add devicetree binding for Analog Devices ADUX1020 Photometric
> > > sensor.
> > >
> >
> > Hey,
> >
> > Thanks for the patches.
> >
> > This dt-binding docs is in text format.
> > dt-binding docs now need to be in YAML format.
> >
>
> Sure. I can convert to YAML binding.
>
> > Also, patches for dt-bindings docs usually come after the driver is
> > added.
> > So, this patch should be the second in the series, not the first.
> >
>
> I don't think so. The convention is to put dt-bindings patch upfront for
> all subsystems. Not sure if IIO differs here.
Now that you mention, I'm not sure either.
We typically sent the dt-bindings one last, so I assumed it was the
default.
>
> Thanks,
> Mani
> > Alex
> >
> > > Signed-off-by: Manivannan Sadhasivam
> >
> > > ---
> > > .../bindings/iio/light/adux1020.txt | 22
> > +++
> > > 1 file changed, 22 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/iio/light/adux1020.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > new file mode 100644
> > > index ..e896dda30e36
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > @@ -0,0 +1,22 @@
> > > +Analog Devices ADUX1020 Photometric sensor
> > > +
> > > +Link to datasheet:
> > >
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
> > > +
> > > +Required properties:
> > > +
> > > + - compatible: should be "adi,adux1020"
> > > + - reg: the I2C address of the sensor
> > > +
> > > +Optional properties:
> > > +
> > > + - interrupts: interrupt mapping for IRQ as documented in
> > > +
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > > +
> > > +Example:
> > > +
> > > +adux1020@64 {
> > > + compatible = "adi,adux1020";
> > > + reg = <0x64>;
> > > + interrupt-parent = <&msmgpio>;
> > > + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
> > > +};
Re: [PATCH] iio: imu: adis16400: release allocated memory on failure
On Wed, 2019-09-18 at 11:57 -0500, Navid Emamdoost wrote:
> [External]
>
Hey,
Good catch.
One comment inline.
> In adis_update_scan_mode, if allocation for adis->buffer fails,
> previously allocated adis->xfer needs to be released.
>
> Signed-off-by: Navid Emamdoost
> ---
> drivers/iio/imu/adis_buffer.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> index 9ac8356d9a95..c5d7e368a636 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -78,8 +78,10 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
> return -ENOMEM;
>
> adis->buffer = kcalloc(indio_dev->scan_bytes, 2, GFP_KERNEL);
> - if (!adis->buffer)
> + if (!adis->buffer) {
> + kfree(adis->xfer);
Maybe also do "adis->xfer = NULL" here.
The idea is to make sure that the pointer is not double-free'd by some
other function (i.e. adis_cleanup_buffer_and_trigger() or another
adis_update_scan_mode() call).
> return -ENOMEM;
> + }
>
> rx = adis->buffer;
> tx = rx + scan_count;
Re: [PATCH] iio: imu: adis16400: fix memory leak
On Wed, 2019-09-18 at 12:03 -0500, Navid Emamdoost wrote:
> [External]
>
Hey,
Thanks for this patch as well.
Comments inline here as well.
> In adis_update_scan_mode_burst, if adis->buffer allocation fails release
> the adis->xfer.
>
> Signed-off-by: Navid Emamdoost
> ---
> drivers/iio/imu/adis_buffer.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> index 9ac8356d9a95..1dbe25572a39 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -35,8 +35,10 @@ static int adis_update_scan_mode_burst(struct iio_dev
> *indio_dev,
> return -ENOMEM;
>
> adis->buffer = kzalloc(burst_length + sizeof(u16), GFP_KERNEL);
> - if (!adis->buffer)
> + if (!adis->buffer) {
> + kfree(adis->xfer);
Same as the other patch: it would be a good idea to do "adis->xfer = NULL"
here.
> return -ENOMEM;
> + }
>
> tx = adis->buffer + burst_length;
> tx[0] = ADIS_READ_REG(adis->burst->reg_cmd);
Re: [PATCH v2] iio: imu: adis16400: release allocated memory on failure
On Thu, 2019-09-19 at 10:50 -0500, Navid Emamdoost wrote:
> In adis_update_scan_mode, if allocation for adis->buffer fails,
> previously allocated adis->xfer needs to be released.
>
> v2: added adis->xfer = NULL to avoid any potential double free.
Reviewed-by: Alexandru Ardelean
> Signed-off-by: Navid Emamdoost
> ---
> drivers/iio/imu/adis_buffer.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> index 9ac8356d9a95..f446ff497809 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -78,8 +78,11 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
> return -ENOMEM;
>
> adis->buffer = kcalloc(indio_dev->scan_bytes, 2, GFP_KERNEL);
> - if (!adis->buffer)
> + if (!adis->buffer) {
> + kfree(adis->xfer);
> + adis->xfer = NULL;
> return -ENOMEM;
> + }
>
> rx = adis->buffer;
> tx = rx + scan_count;
Re: [PATCH v2] iio: imu: adis16400: fix memory leak
On Thu, 2019-09-19 at 10:56 -0500, Navid Emamdoost wrote:
> In adis_update_scan_mode_burst, if adis->buffer allocation fails release
> the adis->xfer.
>
> v2: set adis->xfer = NULL to avoid any potential double free.
>
Reviewed-by: Alexandru Ardelean
> Signed-off-by: Navid Emamdoost
> ---
> drivers/iio/imu/adis_buffer.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> index 9ac8356d9a95..78fe83c1f4fe 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -35,8 +35,11 @@ static int adis_update_scan_mode_burst(struct iio_dev
> *indio_dev,
> return -ENOMEM;
>
> adis->buffer = kzalloc(burst_length + sizeof(u16), GFP_KERNEL);
> - if (!adis->buffer)
> + if (!adis->buffer) {
> + kfree(adis->xfer);
> + adis->xfer = NULL;
> return -ENOMEM;
> + }
>
> tx = adis->buffer + burst_length;
> tx[0] = ADIS_READ_REG(adis->burst->reg_cmd);
Re: [EXT] [PATCH v3] serial: imx: adapt rx buffer and dma periods
On Wed, 2019-09-25 at 10:14 -0500, Adam Ford wrote:
> [External]
>
> On Fri, Sep 20, 2019 at 2:06 AM Philipp Puschmann
> wrote:
> > Hi Andy,
> >
> > Am 20.09.19 um 05:42 schrieb Andy Duan:
> > > From: Philipp Puschmann Sent: Thursday,
> > > September 19, 2019 10:51 PM
> > > > Using only 4 DMA periods for UART RX is very few if we have a high
> > > > frequency
> > > > of small transfers - like in our case using Bluetooth with many
> > > > small packets
> > > > via UART - causing many dma transfers but in each only filling a
> > > > fraction of a
> > > > single buffer. Such a case may lead to the situation that DMA RX
> > > > transfer is
> > > > triggered but no free buffer is available. When this happens dma
> > > > channel ist
> > > > stopped - with the patch
> > > > "dmaengine: imx-sdma: fix dma freezes" temporarily only - with the
> > > > possible
> > > > consequences that:
>
> I have an i.MX6Q with Wl1837MOD on UART 2 with flow control, and I am
> getting Bluetooth transfer timeouts.
> (see imx6-logicpd-som.dtsi)
>
> On top of 5.3.1, I have installed:
>
> dmaengine: imx-sdma: fix buffer ownership
> dmaengine: imx-sdma: fix dma freezes
> dmaengine: imx-sdma: drop redundant variable
> dmaengine: imx-sdma: fix kernel hangs with SLUB slab allocator
> serial: imx: adapt rx buffer and dma periods
>
> and I still get timeouts:
>
> [ 66.632006] Bluetooth: hci0: command 0xff36 tx timeout
> [ 76.790499] Bluetooth: hci0: command 0x1001 tx timeout
> [ 87.110488] Bluetooth: hci0: command 0xff36 tx timeout
> [ 97.270507] Bluetooth: hci0: command 0x1001 tx timeout
> [ 107.590457] Bluetooth: hci0: command 0xff36 tx timeout
> [ 117.750477] Bluetooth: hci0: command 0x1001 tx timeout
> [ 226.390499] Bluetooth: hci0: command 0xfe38 tx timeout
> [ 231.590735] Bluetooth: hci0: command tx timeout
>
> I did a bisect and found the start of my problems came from
>
> 361deb7243d2 ("dmaengine: dmatest: wrap src & dst data into a struct")
That commit only touches `drivers/dma/dmatest.c`
Are you using that module?
It's a "unit-test" module for testing DMAengine drivers.
The only way that can break anything [from what I can tell], is if it is
being run. It will probably put the DMA into a weird state (it is a test-
module after-all), and it may require some DMAs to be reset.
I admit it would be nice that the test-module would put the DMA back into a
normal-working state, but that effort could be big for some cases.
>
> This happened sometime between v5.0 and v5.1
>
> Is there a patch I missed somewhere? Do I need to change my device
> tree configuration somehow to allocate the proper DMA memory?
>
>
>
> > > > with disabled hw flow control:
> > > > If enough data is incoming on UART port the RX FIFO runs over and
> > > > characters will be lost. What then happens depends on upper
> > > > layer.
> > > >
> > > > with enabled hw flow control:
> > > > If enough data is incoming on UART port the RX FIFO reaches a
> > > > level
> > > > where CTS is deasserted and remote device sending the data stops.
> > > > If it fails to stop timely the i.MX' RX FIFO may run over and
> > > > data
> > > > get lost. Otherwise it's internal TX buffer may getting filled to
> > > > a point where it runs over and data is again lost. It depends on
> > > > the remote device how this case is handled and if it is
> > > > recoverable.
> > > >
> > > > Obviously we want to avoid having no free buffers available. So we
> > > > decrease
> > > > the size of the buffers and increase their number and the total
> > > > buffer size.
> > > >
> > > > Signed-off-by: Philipp Puschmann
> > > > Reviewed-by: Lucas Stach
> > > > ---
> > > >
> > > > Changelog v3:
> > > > - enhance description
> > > >
> > > > Changelog v2:
> > > > - split this patch from series "Fix UART DMA freezes for iMX6"
> > > > - add Reviewed-by tag
> > > >
> > > > drivers/tty/serial/imx.c | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index
> > > > 87c58f9f6390..51dc19833eab 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -1034,8 +1034,6 @@ static void imx_uart_timeout(struct
> > > > timer_list *t)
> > > > }
> > > > }
> > > >
> > > > -#define RX_BUF_SIZE(PAGE_SIZE)
> > > > -
> > > > /*
> > > > * There are two kinds of RX DMA interrupts(such as in the MX6Q):
> > > > * [1] the RX DMA buffer is full.
> > > > @@ -1118,7 +1116,8 @@ static void imx_uart_dma_rx_callback(void
> > > > *data) }
> > > >
> > > > /* RX DMA buffer periods */
> > > > -#define RX_DMA_PERIODS 4
> > > > +#define RX_DMA_PERIODS 16
> > > > +#define RX_BUF_SIZE(PAGE_SIZE / 4)
> > > >
> > > Why to decrease the DMA RX buffer size here ?
> > >
> > > The current DMA implementation support DMA cyclic mode, one SDMA BD
> > > receive one Bluetooth frame can
> > > bring better performance.
> > > As you know, fo
Re: [PATCH] iio: imu: adis16480: clean up a condition
On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote: > [External] > > The "t" variable is unsigned so it can't be less than zero. We really > are just trying to prevent divide by zero bugs so just checking against > zero is sufficient. > > Signed-off-by: Dan Carpenter > --- > drivers/iio/imu/adis16480.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c > index b99d73887c9f..e144e567675d 100644 > --- a/drivers/iio/imu/adis16480.c > +++ b/drivers/iio/imu/adis16480.c > @@ -318,7 +318,7 @@ static int adis16480_set_freq(struct iio_dev > *indio_dev, int val, int val2) > unsigned int t, reg; I would just change the type of "t" to "int". Especially, since "val" & "val2" are "int". Thanks for the catch :) Alex > > t = val * 1000 + val2 / 1000; > - if (t <= 0) > + if (t == 0) > return -EINVAL; > > /*
Re: [PATCH 2/5] drivers: spi: core: Add optional stall delay between cs_change transfers
On Tue, 2019-07-09 at 15:12 +0100, Mark Brown wrote: > On Wed, Jun 26, 2019 at 07:34:38PM +0100, Jonathan Cameron wrote: > > On Tue, 25 Jun 2019 16:13:25 +0300 > > Alexandru Ardelean wrote: > > > > > Some devices like the ADIS16460 IMU require a stall period between > > > transfers, i.e. between when the CS is de-asserted and re-asserted. The > > > default value of 10us is not enough. This change makes the delay > > > configurable for when the next CS change goes active. > > This looks like cs_change_delay. > > As documented in SubmittingPatches please send patches to the > maintainers for the code you would like to change. The normal kernel > workflow is that people apply patches from their inboxes, if they aren't > copied they are likely to not see the patch at all and it is much more > difficult to apply patches. Ack. [Sorry for the late reply; I'm balancing other stuff as well and terrible at it] I'll probably update my practice to also include maintainers via --cc to `git send-email`. Up until now, I would send emails to lists [as much as possible] and try to not include people directly. My assumption was that the list is enough. I'm still adjusting to how things get done in the various Linux kernel subsystems/subgroups.
Re: [PATCH 1/4][V2] drivers: spi: core: Add optional delay between cs_change transfers
On Thu, 2019-07-18 at 13:50 +0100, Mark Brown wrote: > On Wed, Jul 17, 2019 at 02:51:06PM +0300, Alexandru Ardelean wrote: > > Some devices like the ADIS16460 IMU require a stall period between > > transfers, i.e. between when the CS is de-asserted and re-asserted. The > > default value of 10us is not enough. This change makes the delay > > configurable for when the next CS change goes active. > > To repeat my previous feedback: > > > This looks like cs_change_delay. Ack. Will use `cs_change_delay`. I have no strong preference regarding the name. > > Please use subject lines matching the style for the subsystem. This > makes it easier for people to identify relevant patches. Ack. Will look for SPI subsystem specific subject lines and use them. > > Please don't ignore review comments, people are generally making them > for a reason and are likely to have the same concerns if issues remain > unaddressed. Having to repeat the same comments can get repetitive and > make people question the value of time spent reviewing. If you disagree > with the review comments that's fine but you need to reply and discuss > your concerns so that the reviewer can understand your decisions. [ the following part should not be considered in a disrespectful tone ; the intent is nowhere near that, but text- communication has a design-flaw where a disrespectful tone may be interpreted [where there isn't one] ] My intent wasn't to ignore the review comment. Sorry if it came out like that. I assumed a patch re-spin was preferred vs a verbal discussion. Some people prefer patch re-spins as a basis for discussion. Various people have various preferences. Also, I wasn't sure how soon I'd get a reply back on this, since various people/maintainers have various reply-times. And I also [sometimes] have longer reply-back-times [which doesn't help either]. And I wasn't sure if `cs_change_delay` was fully intentional/ad-literam, or whether it was a feedback of the sorts "along-the-lines of `cs_change_delay`". While looking at `struct spi_transfer` the other "delay" fields seem to be: `word_delay_usecs` & `delay_usecs`, which is why I assumed `cs_change_delay_usecs` was preferred [though I will admit, it is a long var-name]. And the conclusion [from my side] is: maybe I rushed this a bit and maybe it annoyed you. Not my intention, and it'll take me a bit to adjust to your style. Moving forward. 1. I will use `cs_change_delay` as field name 2. I will use SPI subsystem subject line; I will admit I forget this stuff periodically Is there anything else I should consider? Or anything else to discuss? I'm open to elements I may have forgotten/omitted unintentionally. Thanks Alex
Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants
On Sat, 2020-05-02 at 19:25 +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 12:31:28 +0300
> Alexandru Ardelean wrote:
>
> > This change cleans up the driver's probe function to use only devm_
> > function variants. This also gets rid of the remove function and moves the
> > clock & regulator de-initializations to the 'ad5933_cleanup()' callback.
> >
> > Signed-off-by: Alexandru Ardelean
>
> Basic rule of thumb. Whatever you register with devm_add_action_or_reset
> should only cleanup one one thing done in the probe path.
> There is almost always a race if you do more than one bit of cleanup
> per such callback + it's harder to review as it fails the 'obviously correct
> test'.
I did not know that.
That idea missed me up until now.
Will re-spin.
Thanks
Alex
>
> Jonathan
>
> > ---
> > .../staging/iio/impedance-analyzer/ad5933.c | 59 ---
> > 1 file changed, 23 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..06a6dcd7883b 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> > .postdisable = ad5933_ring_postdisable,
> > };
> >
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > +static int ad5933_register_ring_funcs_and_init(struct device *dev,
> > + struct iio_dev *indio_dev)
> > {
> > struct iio_buffer *buffer;
> >
> > - buffer = iio_kfifo_allocate();
> > + buffer = devm_iio_kfifo_allocate(dev);
> > if (!buffer)
> > return -ENOMEM;
> >
> > @@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
> > }
> > }
> >
> > +static void ad5933_cleanup(void *data)
> > +{
> > + struct ad5933_state *st = data;
> > +
> > + clk_disable_unprepare(st->mclk);
> > + regulator_disable(st->reg);
>
> Please do two separate callbacks so that these can be handled
> in the correct places. I.e. you do something then immediately
> register the handler to undo it.
>
> Currently you can end up disabling a clock you haven't enabled
> (which I am fairly sure will give you an error message).
>
> > +}
> > +
> > static int ad5933_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
> > dev_err(&client->dev, "Failed to enable specified VDD
> > supply\n");
> > return ret;
> > }
> > +
> > + ret = devm_add_action_or_reset(&client->dev, ad5933_cleanup, st);
> > + if (ret)
> > + return ret;
> > +
> > ret = regulator_get_voltage(st->reg);
> >
> > if (ret < 0)
> > - goto error_disable_reg;
> > + return ret;
> >
> > st->vref_mv = ret / 1000;
> >
> > st->mclk = devm_clk_get(&client->dev, "mclk");
> > if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
> > ret = PTR_ERR(st->mclk);
> > - goto error_disable_reg;
> > + return ret;
> > }
> >
> > if (!IS_ERR(st->mclk)) {
> > ret = clk_prepare_enable(st->mclk);
> > if (ret < 0)
> > - goto error_disable_reg;
> > + return ret;
> > ext_clk_hz = clk_get_rate(st->mclk);
> > }
> >
> > @@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
> > indio_dev->channels = ad5933_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >
> > - ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > + ret = ad5933_register_ring_funcs_and_init(&client->dev, indio_dev);
> > if (ret)
> > - goto error_disable_mclk;
> > + return ret;
> >
> > ret = ad5933_setup(st);
> > if (ret)
> > - goto error_unreg_ring;
> > -
> > - ret = iio_device_register(indio_dev);
> > - if (ret)
> > - goto error_unreg_ring;
> > -
> > - return 0;
> > -
> > -error_unreg_ring:
> > - iio_kfifo_free(indio_dev->buffer);
> > -error_disable_mclk:
> > - clk_disable_unprepare(st->mclk);
> > -error_disable_reg:
> > - regulator_disable(st->reg);
> > -
> > - return ret;
> > -}
> > -
> > -static int ad5933_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > - struct ad5933_state *st = iio_priv(indio_dev);
> > -
> > - iio_device_unregister(indio_dev);
> > - iio_kfifo_free(indio_dev->buffer);
> > - regulator_disable(st->reg);
> > - clk_disable_unprepare(st->mclk);
> > + return ret;
> >
> > - return 0;
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > }
> >
> > static const struct i2c_device_id ad5933_id[] = {
> > @@ -801,7 +789,6 @@ static struct i2c_driver ad59
Re: [PATCH] staging: iio: ad2s1210: Fix SPI reading
On Sun, 2020-05-03 at 12:37 +0100, Jonathan Cameron wrote:
> [External]
>
> On Wed, 29 Apr 2020 10:21:29 +0300
> Alexandru Ardelean wrote:
>
> > From: Dragos Bogdan
> >
> > If the serial interface is used, the 8-bit address should be latched using
> > the rising edge of the WR/FSYNC signal.
> >
> > This basically means that a CS change is required between the first byte
> > sent, and the second one.
> > This change splits the single-transfer transfer of 2 bytes into 2 transfers
> > with a single byte, and CS change in-between.
> >
> > Signed-off-by: Dragos Bogdan
> > Signed-off-by: Alexandru Ardelean
>
> Fixes tag would have been nice. I've had a go by picking a patch where I
> refactored this code, but I think the issue probably predates that one.
> Its in 2011 so I doubt anyone will try going past that with backports ;)
>
Apologies again for not considering Fixes tag.
At this point, I feel bad for repeating the apology, as it may sound like hollow
words.
But, I guess this could have skipped going through the fixes route.
The patch has been living in our tree for a while.
> Applied to the fixes-togreg branch of iio.git and marked for stable.
>
> I'm guessing this means you have hardware and hope to get this one out
> of staging shortly? *crosses fingers* :)
Sorry, but not at this point in time.
I just fished this from our tree.
I also handle our kernel upgrades [on our side], and whenever I do an update,
some upstreamed patches disappear from our tree, and some stand-out and I wonder
how come this wasn't sent upstream.
This is one of them.
I don't know if I'll be able to find someone [in the near future] to allocate
this to [for moving out-of-staging].
Right now, the priority [on our side] is the high-speed converters.
>
> Jonathan
>
> > ---
> > drivers/staging/iio/resolver/ad2s1210.c | 17 -
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index 4b25a3a314ed..ed404355ea4c 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -130,17 +130,24 @@ static int ad2s1210_config_write(struct ad2s1210_state
> > *st, u8 data)
> > static int ad2s1210_config_read(struct ad2s1210_state *st,
> > unsigned char address)
> > {
> > - struct spi_transfer xfer = {
> > - .len = 2,
> > - .rx_buf = st->rx,
> > - .tx_buf = st->tx,
> > + struct spi_transfer xfers[] = {
> > + {
> > + .len = 1,
> > + .rx_buf = &st->rx[0],
> > + .tx_buf = &st->tx[0],
> > + .cs_change = 1,
> > + }, {
> > + .len = 1,
> > + .rx_buf = &st->rx[1],
> > + .tx_buf = &st->tx[1],
> > + },
> > };
> > int ret = 0;
> >
> > ad2s1210_set_mode(MOD_CONFIG, st);
> > st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
> > st->tx[1] = AD2S1210_REG_FAULT;
> > - ret = spi_sync_transfer(st->sdev, &xfer, 1);
> > + ret = spi_sync_transfer(st->sdev, xfers, 2);
> > if (ret < 0)
> > return ret;
> >
Re: [PATCH v2 1/2] iio: Move scan mask management to the core
On Sun, 2020-05-03 at 13:51 +0100, Jonathan Cameron wrote:
> On Wed, 29 Apr 2020 18:17:39 +0300
> Alexandru Ardelean wrote:
>
> > From: Lars-Peter Clausen
> >
> > Let the core handle the buffer scan mask management including allocation
> > and channel selection. Having this handled in a central place rather than
> > open-coding it all over the place will make it easier to change the
> > implementation (if needed).
> > At the very least, this change abstracts scan-mask management away from
> > buffer implementations.
> >
> > Signed-off-by: Lars-Peter Clausen
> > Signed-off-by: Alexandru Ardelean
>
> I'm not 100% happy with including the buffer_impl.h header in the inkern
> code, but I can't see a simple way around it.
I actually also tried to not do it [I suspect Lars may have tried as well].
But you end up either having to expose 'struct iio_channel' outside of
'inkern.c' or having to include 'iio/consumer.h' in industrialio-buffer.c
Since there will be a V3, I will probably try to look again at a potential
different solution. Maybe there is a better solution I haven't considered.
>
> However, there are some missing statics in here and I'm feeling lazy
> so not going to fix them up for you.
>
No problem from my side.
I'll fix them up.
I prefer it when people don't cleanup after me. Makes me feel less guilty.
Though I will admit, it's a double-edged feeling: I am greatful when someone
cleans up after me, but I also feel slightly guilty [about it].
> Thanks,
>
> Jonathan
>
> > ---
> >
> > Changelog v1 -> v2:
> > - split away from initial series; the `buffer->channel_mask` attribute
> > requires a bit more dicussion; or may even be dropped; just these 2
> > patches helps with diff-ing 2 trees, as applying patches between my
> > work tree & IIO has fewer conflicts
> > - return -EINVAL if masklength is 0 in iio_buffer_alloc_scanmask()
> > - convert 2nd parameter to `unsigned int masklength` in
> > iio_buffer_alloc_scanmask()
> >
> > drivers/iio/buffer/industrialio-buffer-cb.c | 17 +-
> > drivers/iio/industrialio-buffer.c | 36 +++--
> > drivers/iio/inkern.c| 15 +
> > include/linux/iio/consumer.h| 10 ++
> > 4 files changed, 59 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c
> > b/drivers/iio/buffer/industrialio-buffer-cb.c
> > index 47c96f7f4976..dc5bb2ab533a 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > @@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer
> > *buffer)
> > {
> > struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> >
> > - bitmap_free(cb_buff->buffer.scan_mask);
> > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > kfree(cb_buff);
> > }
> >
> > @@ -72,27 +72,26 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct
> > device *dev,
> > }
> >
> > cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > - cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev-
> > >masklength,
> > - GFP_KERNEL);
> > - if (cb_buff->buffer.scan_mask == NULL) {
> > - ret = -ENOMEM;
> > +
> > + ret = iio_buffer_alloc_scanmask(&cb_buff->buffer,
> > + cb_buff->indio_dev->masklength);
> > + if (ret)
> > goto error_release_channels;
> > - }
> > +
> > chan = &cb_buff->channels[0];
> > while (chan->indio_dev) {
> > if (chan->indio_dev != cb_buff->indio_dev) {
> > ret = -EINVAL;
> > goto error_free_scan_mask;
> > }
> > - set_bit(chan->channel->scan_index,
> > - cb_buff->buffer.scan_mask);
> > + iio_buffer_channel_enable(&cb_buff->buffer, chan);
> > chan++;
> > }
> >
> > return cb_buff;
> >
> > error_free_scan_mask:
> > - bitmap_free(cb_buff->buffer.scan_mask);
> > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > error_release_channels:
> > iio_channel_release_all(cb_buff->channels);
> > error_free_cb_buff:
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index eae39eaf49af..c6b63f4474ff 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > }
> > EXPORT_SYMBOL(iio_buffer_init);
> >
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > + unsigned int masklength)
> > +{
> > + if (!masklength)
> > + return -EINVAL;
> > +
> > + buffer->scan_mask = bitmap_zalloc(masklength, GFP_KERNEL);
> > + if (buffer->scan_mask == NULL)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > +
> > +void iio_buffer_free_sc
Re: [PATCH v6 3/6] iio: core: register chardev only if needed
On Sun, 2020-05-03 at 16:39 +0100, Jonathan Cameron wrote:
> [External]
>
> On Mon, 27 Apr 2020 16:10:57 +0300
> Alexandru Ardelean wrote:
>
> > The final intent is to localize all buffer ops into the
> > industrialio-buffer.c file, to be able to add support for multiple buffers
> > per IIO device.
> >
> > We only need a chardev if we need to support buffers and/or events.
> >
> > With this change, a chardev will be created only if an IIO buffer is
> > attached OR an event_interface is configured.
> >
> > Otherwise, no chardev will be created, and the IIO device will get
> > registered with the 'device_add()' call.
> >
> > Quite a lot of IIO devices don't really need a chardev, so this is a minor
> > improvement to the IIO core, as the IIO device will take up (slightly)
> > fewer resources.
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> > drivers/iio/industrialio-core.c | 25 ++---
> > 1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index 32c489139cd2..51e279c60793 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1682,6 +1682,15 @@ static int iio_check_unique_scan_index(struct iio_dev
> > *indio_dev)
> >
> > static const struct iio_buffer_setup_ops noop_ring_setup_ops;
> >
> > +static const struct file_operations iio_event_fileops = {
> > + .owner = THIS_MODULE,
> > + .llseek = noop_llseek,
> > + .unlocked_ioctl = iio_event_ioctl_wrapper,
>
> Unfortunately this doesn't exist until the next patch...
crap;
i copied this from the wrong place when i re-ordered this, and forgot to run a
build on each patchset
will fix
>
> > + .compat_ioctl = compat_ptr_ioctl,
> > + .open = iio_chrdev_open,
> > + .release = iio_chrdev_release,
> > +};
> > +
> > int __iio_device_register(struct iio_dev *indio_dev, struct module
> > *this_mod)
> > {
> > int ret;
> > @@ -1732,11 +1741,18 @@ int __iio_device_register(struct iio_dev *indio_dev,
> > struct module *this_mod)
> > indio_dev->setup_ops == NULL)
> > indio_dev->setup_ops = &noop_ring_setup_ops;
> >
> > - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > + if (indio_dev->buffer)
> > + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > + else if (indio_dev->event_interface)
> > + cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> >
> > indio_dev->chrdev.owner = this_mod;
> >
> > - ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > + if (indio_dev->buffer || indio_dev->event_interface)
> > + ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > + else
> > + ret = device_add(&indio_dev->dev);
> > +
> > if (ret < 0)
> > goto error_unreg_eventset;
> >
> > @@ -1760,7 +1776,10 @@ EXPORT_SYMBOL(__iio_device_register);
> > **/
> > void iio_device_unregister(struct iio_dev *indio_dev)
> > {
> > - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> > + if (indio_dev->buffer || indio_dev->event_interface)
> > + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> > + else
> > + device_del(&indio_dev->dev);
> >
> > mutex_lock(&indio_dev->info_exist_lock);
> >
Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper
On Mon, 2020-04-27 at 13:00 +, Ardelean, Alexandru wrote: > [External] > > On Mon, 2020-04-27 at 12:20 +, eugen.hris...@microchip.com wrote: > > [External] > > > > On 15.04.2020 09:33, Ardelean, Alexandru wrote: > > > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > > > > On Tue, 14 Apr 2020 12:22:45 + > > > > wrote: > > > > > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > > Alexandru Ardelean wrote: > > > > > > > > > > > > > This change moves the logic to check if the current channel is the > > > > > > > touchscreen channel to a separate helper. > > > > > > > This reduces some code duplication, but the main intent is to re- > > > > > > > use > > > > > > > this > > > > > > > in the next patches. > > > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean > > > > > > Eugen / Ludovic, > > > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > > > Hi Jonathan, > > > > > > > > > > Does the patch apply correctly for you ? > > > > > > > > I haven't tried yet :) > > > > > > > > > > I've rebased this patchset on top of current iio/testing and it still > > > applies. > > > > > > > Hi Alex, > > > > I tried this patch on top of my tree (however I am testing with an older > > kernel 5.4) , and I have issues starting the buffer after you moved my > > code to the preenable callback. > > > > Namely, on the line: > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > return -EINVAL; > In the meantime I found this patch: https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-l...@metafoo.de/ from about ~5 years ago; if this patch is a valid proposal, it could fix this case as well; well, it might break others, so applying it [now] would need some general review of all pre/post enable/disable hooks > Apologies for the breakage. > > For the touch-part I don't see that code being executed. > > But a question is: does the driver need to check for the currentmode? > Or is that something that the IIO core should do? > > > And with this , the preenable fails on my side, because the current mode > > is not yet switched to triggered. > > > > I do remember adding this line with a specific reason. It may be related > > to touchscreen operations, but I have to retest the touch with and > > without this line and your patch. > > > > Meanwhile, maybe you have any suggestions on how to fix the buffer ? > > Well, there was the question of whether iio_triggered_buffer_postenable() [to > attach the pollfunc] makes sense to be called first/last in the old > at91_adc_buffer_postenable(), and the answer was 'last'; so then one solution > was to move things to preenable(). > > Going back to the old patch isn't ideal, as the idea was to make the position > of > iio_triggered_buffer_postenable() consistent across all drivers, so that it > can > be removed [and moved to the IIO core]. > > But if we need revert the patch, then I guess it's fine. > The only solution I see right now [for going forward], is to remove that check > for 'currrentmode' > > > This check here makes any sense to you ? > > I think Jonathan may have to add some input here, but I think that in this > current situation, checking 'currentmode' looks like is re-validating how the > device was configured via the IIO framework. > I am not sure if it's needed or not. > > > Thanks, > > Eugen > > > > > > > I will try to test it , if I manage to apply it. > > > > > I can only test the ADC though because at this moment I do not have a > > > > > touchscreen at disposal. > > > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > > > Reviewed-by: Eugen Hristev > > > > > > > > > > By the way, I do not know if my two pending patches on this driver > > > > > will > > > > > conflict or not. > > > > > > > > As this is a long term rework patch at heart, there isn't any particular > &g
Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper
On Thu, 2020-04-30 at 07:30 +, Ardelean, Alexandru wrote:
> On Mon, 2020-04-27 at 13:00 +0000, Ardelean, Alexandru wrote:
> > [External]
> >
> > On Mon, 2020-04-27 at 12:20 +, eugen.hris...@microchip.com wrote:
> > > [External]
> > >
> > > On 15.04.2020 09:33, Ardelean, Alexandru wrote:
> > >
> > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote:
> > > > > On Tue, 14 Apr 2020 12:22:45 +
> > > > > wrote:
> > > > >
> > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote:
> > > > > > > On Wed, 4 Mar 2020 10:42:18 +0200
> > > > > > > Alexandru Ardelean wrote:
> > > > > > >
> > > > > > > > This change moves the logic to check if the current channel is
> > > > > > > > the
> > > > > > > > touchscreen channel to a separate helper.
> > > > > > > > This reduces some code duplication, but the main intent is to
> > > > > > > > re-
> > > > > > > > use
> > > > > > > > this
> > > > > > > > in the next patches.
> > > > > > > >
> > > > > > > > Signed-off-by: Alexandru Ardelean > > > > > > > >
> > > > > > > Eugen / Ludovic,
> > > > > > >
> > > > > > > Have you had a chance to look at this series?
> > > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > Does the patch apply correctly for you ?
> > > > >
> > > > > I haven't tried yet :)
> > > > >
> > > >
> > > > I've rebased this patchset on top of current iio/testing and it still
> > > > applies.
> > > >
> > >
> > > Hi Alex,
> > >
> > > I tried this patch on top of my tree (however I am testing with an older
> > > kernel 5.4) , and I have issues starting the buffer after you moved my
> > > code to the preenable callback.
> > >
> > > Namely, on the line:
> > >
> > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > > return -EINVAL;
>
> In the meantime I found this patch:
> https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-l...@metafoo.de/__;!!A3Ni8CS0y2Y!ocQuNvFF_8rd-cCvMNXU0cTk9mLezpCPyzelQyhbxMGdKgFo0_JTgTD1q1VU-kj10aqxxA$
>
>
> from about ~5 years ago;
>
> if this patch is a valid proposal, it could fix this case as well;
> well, it might break others, so applying it [now] would need some general
> review
> of all pre/post enable/disable hooks
>
So, apologies if this will start to seem like spamming.
I decided to do a bit of shell magic for this:
get_files() {
git grep -w iio_buffer_setup_ops | grep drivers | cut -d: -f1 | sort | uniq
}
for file in $(get_files) ; do
if grep -q currentmode $file ; then
echo $file
fi
done
It finds 4 drivers.
Though, `get_files()` will return 56 files.
drivers/iio/accel/bmc150-accel-core.c
drivers/iio/adc/at91-sama5d2_adc.c
drivers/iio/adc/stm32-dfsdm-adc.c
drivers/iio/magnetometer/rm3100-core.c
The rm3100 driver doesn't do any checks in the setup_ops for 'currentmode' as
far as I could see.
So, Lars' patch could work nicely to fix this current case and not break others.
Semantically though, it would sound nicer to have a 'nextmode' parameter
somewhere; maybe on the setup_ops(indio_dev, nextmode)?
Though, only those 3 drivers would really ever use it; so doing it like that
sounds like overkill.
So, we're left with Lars' patch or we could add an 'indio_dev->nextmode' field,
that may be used in just these 3 drivers [which again: sounds overkill at this
point in time].
Alternatively, this 'indio_dev->currentmode' could be removed from all these 3
drivers somehow. But that needs testing and a thorough understanding of all 3
drivers and what they're doing, to do properly.
@Jonathan: what do you think?
In any case, pending a reply, I'll send Lars' patch.
Even if we come to a different conclusion we have something to start with.
But, if the conclusion is that Lars' patch is a good solution now, it can be
applied.
> > Apologies for the breakage.
> >
> > For the touch-part I don't see that code being executed.
> >
> > But a question is: does the driver need to check for the currentmode?
> > Or is that something that the IIO core should do?
&
Re: [PATCH] iio: remove iio_get_debugfs_dentry() helper
On Thu, 2020-04-30 at 11:52 +0300, Alexandru Ardelean wrote:
> Not used anywhere.
> Looks like it was forgotten in iio.h
>
Actually, disregard this.
I've found places where 'indio_dev->debugfs_dentry' is accessed directly and
that should have used this instead.
Apologies for the spam.
> Signed-off-by: Alexandru Ardelean
> ---
> include/linux/iio/iio.h | 16
> 1 file changed, 16 deletions(-)
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 38c4ea505394..1a1da52c55cf 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -696,22 +696,6 @@ static inline bool iio_buffer_enabled(struct iio_dev
> *indio_dev)
> INDIO_BUFFER_SOFTWARE);
> }
>
> -/**
> - * iio_get_debugfs_dentry() - helper function to get the debugfs_dentry
> - * @indio_dev: IIO device structure for device
> - **/
> -#if defined(CONFIG_DEBUG_FS)
> -static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev
> *indio_dev)
> -{
> - return indio_dev->debugfs_dentry;
> -}
> -#else
> -static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev
> *indio_dev)
> -{
> - return NULL;
> -}
> -#endif
> -
> ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>
> int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
Re: [PATCH] dmaengine: axi-dmac: simple device_config operation implemented
On Mon, 2019-10-14 at 12:31 +0530, Vinod Koul wrote:
> [External]
>
Hey,
> On 13-09-19, 17:54, Alexandru Ardelean wrote:
> > From: Rodrigo Alencar
> >
> > dmaengine_slave_config is called by dmaengine_pcm_hw_params when using
> > axi-i2s with axi-dmac. If device_config is NULL, -ENOSYS is returned,
> > which breaks the snd_pcm_hw_params function.
> > This is a fix for the error:
>
> and what is that?
>
> > $ aplay -D plughw:ADAU1761 /usr/share/sounds/alsa/Front_Center.wav
> > Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit
> > Little Endian, Rate 48000 Hz, Mono
> > axi-i2s 43c2.axi-i2s: ASoC: 43c2.axi-i2s hw params failed: -38
Error is above this line [code -38].
> > aplay: set_params:1403: Unable to install hw params:
> > ACCESS: RW_INTERLEAVED
> > FORMAT: S16_LE
> > SUBFORMAT: STD
> > SAMPLE_BITS: 16
> > FRAME_BITS: 16
> > CHANNELS: 1
> > RATE: 48000
> > PERIOD_TIME: 125000
> > PERIOD_SIZE: 6000
> > PERIOD_BYTES: 12000
> > PERIODS: 4
> > BUFFER_TIME: 50
> > BUFFER_SIZE: 24000
> > BUFFER_BYTES: 48000
> > TICK_TIME: 0
> >
> > Signed-off-by: Rodrigo Alencar
> > Signed-off-by: Alexandru Ardelean
> > ---
> >
> > Note: Fixes tag not added intentionally.
> >
> > drivers/dma/dma-axi-dmac.c | 16
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> > index a0ee404b736e..ab2677343202 100644
> > --- a/drivers/dma/dma-axi-dmac.c
> > +++ b/drivers/dma/dma-axi-dmac.c
> > @@ -564,6 +564,21 @@ static struct dma_async_tx_descriptor
> > *axi_dmac_prep_slave_sg(
> > return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
> > }
> >
> > +static int axi_dmac_device_config(struct dma_chan *c,
> > + struct dma_slave_config *slave_config)
> > +{
> > + struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
> > + struct axi_dmac *dmac = chan_to_axi_dmac(chan);
> > +
> > + /* no configuration required, a sanity check is done instead */
> > + if (slave_config->direction != chan->direction) {
>
> slave_config->direction is a deprecated field, pls dont use that
ack
any alternative recommendations of what to do in this case?
i can take a look, but if you have something on-the-top-of-your-head, i'm
open to suggestions
we can also just drop this completely and let userspace fail
>
> > + dev_err(dmac->dma_dev.dev, "Direction not supported by this
> > DMA Channel");
> > + return -EINVAL;
>
> So you intent to support slave dma but do not use dma_slave_config.. how
> are you getting the slave address and other details?
This DMA controller is a bit special.
It gets synthesized in FPGA, so the configuration is fixed and cannot be
changed at runtime. Maybe later we would allow/implement this
functionality, but this is a question for my HDL colleagues.
Two things are done (in this order):
1. For some paramters, axi_dmac_parse_chan_dt() is used to determine things
from device-tree; as it's an FPGA core, things are synthesized once and
cannot change (yet)
2. For other parameters, the axi_dmac_detect_caps() is used to guess some
of them at probe time, by doing some reg reads/writes
I'll admit that maybe the whole approach could be done a bit
differently/better. But I guess this approach was chosen by the fact that
it's FPGA.
Btw: if I'm talking crap, or I may sound like I don't know what I'm talking
about, that could also be true. I am not quite versed in the DMAEngine
framework.
Thanks
Alex
>
> Thanks
Re: [PATCH] dmaengine: axi-dmac: simple device_config operation implemented
On Wed, 2019-10-16 at 10:38 +0530, Vinod Koul wrote: > [External] > > On 15-10-19, 23:06, Lars-Peter Clausen wrote: > > > > > This DMA controller is a bit special. > > > > It gets synthesized in FPGA, so the configuration is fixed and > > > > cannot be > > > > changed at runtime. Maybe later we would allow/implement this > > > > functionality, but this is a question for my HDL colleagues. > > > > > > > > Two things are done (in this order): > > > > 1. For some paramters, axi_dmac_parse_chan_dt() is used to > > > > determine things > > > > from device-tree; as it's an FPGA core, things are synthesized once > > > > and > > > > cannot change (yet) > > > > 2. For other parameters, the axi_dmac_detect_caps() is used to > > > > guess some > > > > of them at probe time, by doing some reg reads/writes > > > > > > So the question for you hw folks is how would a controller work with > > > multiple slave devices, do they need to synthesize it everytime? > > > > > > Rather than that why cant they make the peripheral addresses > > > programmable so that you dont need updating fpga everytime! > > > > The DMA has a direct connection to the peripheral and the peripheral > > data port is not connected to the general purpose memory interconnect. > > So you can't write to it by an MMIO address and there is no > > address > > that needs to be configured. For an FPGA based design this is quite a > > good solution in terms of resource usage, performance and simplicity. A > > direct connection requires less resources than connection it to the > > central memory interconnect, while at the same time having lower > > latency > > and not eating up any additional bandwidth on the central memory > > connect. > > thanks for explanation! also many thanks [from my side] for the explanations :) > > > So slave config in this case is a noop and all it can do is verify that > > the requested configuration matches the available configuration. > > okay so noop it is! >
Re: [PATCH] staging: iio: ad9834: add a check for devm_clk_get
On Wed, 2019-10-16 at 22:25 +0800, Chuhong Yuan wrote:
> ad9834_probe misses a check for devm_clk_get and may cause problems.
> Add a check like what ad9832 does to fix it.
>
This could also use a Fixes tag, but not a big deal.
Reviewed-by: Alexandru Ardelean
> Signed-off-by: Chuhong Yuan
> ---
> drivers/staging/iio/frequency/ad9834.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c
> b/drivers/staging/iio/frequency/ad9834.c
> index 038d6732c3fd..23026978a5a5 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -417,6 +417,10 @@ static int ad9834_probe(struct spi_device *spi)
> st = iio_priv(indio_dev);
> mutex_init(&st->lock);
> st->mclk = devm_clk_get(&spi->dev, NULL);
> + if (IS_ERR(st->mclk)) {
> + ret = PTR_ERR(st->mclk);
> + goto error_disable_reg;
> + }
>
> ret = clk_prepare_enable(st->mclk);
> if (ret) {
Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK
On Tue, 2019-05-07 at 17:44 -0300, Melissa Wen wrote:
> [External]
>
>
> On 05/06, Ardelean, Alexandru wrote:
> > On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> > > [External]
> > >
> > >
> > > On Sat, May 4, 2019 at 1:25 AM Melissa Wen
> > > wrote:
> > > >
> > > > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and
> > > > mask
> > > > in
> > > > one go. This makes the code more readable than explicit masking
> > > > followed
> > > > by a shift.
> > > >
> > >
> > > This looks neat.
> > > I'd have to remember to ack it from my work email.
> >
> > Acked-by: Alexandru Ardelean
> >
> > >
> > > One minor comment inline, which would be the object of a new patch.
> > >
> > > > Signed-off-by: Melissa Wen
> > > > ---
> > > > drivers/staging/iio/cdc/ad7150.c | 6 +-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > > > b/drivers/staging/iio/cdc/ad7150.c
> > > > index 24601ba7db88..4ba46fb6ac02 100644
> > > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > > @@ -5,6 +5,7 @@
> > > > * Copyright 2010-2011 Analog Devices Inc.
> > > > */
> > > >
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -44,6 +45,9 @@
> > > > #define AD7150_SN0_REG 0x16
> > > > #define AD7150_ID_REG 0x17
> > > >
> > > > +/* AD7150 masks */
> > > > +#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
> > > > +
> > > > /**
> > > > * struct ad7150_chip_info - instance specific chip data
> > > > * @client: i2c client for this device
> > > > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct
> > > > iio_dev
> > > > *indio_dev,
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > > > - threshtype = (ret >> 5) & 0x03;
> > > > + threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > > > adaptive = !!(ret & 0x80);
> > >
> > > Why not also do something similar for the `adaptive` value ?
>
> Hi Alexandru,
>
> Yes, I'm working on it! However, taking a look at the driver datasheet
> (Table
> 13, page 19), there seems to be a little mistake in choosing the variable
> name
> and its meaning, since when bit(7) is 1 (true) the threshold is fixed,
> and not
> adaptive. For this reason, I removed it from this patchset to mature the
> solution. I will send it as a bug fix if I prove it's necessary.
> Do you have any advice or feeling about it to share?
>
Good find.
Since this is a bug-fix, typically it's good to fix the code as-is now
[even if it isn't neat code], and then do the conversions to neat code.
Bug-fixes always take priority over cosmetic changes.
So, I would send the bug-fix as-soon-as-possible, and then update things.
> P.s.: Sorry for having previously sent an email with HTML.
>
> Melissa
>
> > >
> > > >
> > > > switch (type) {
> > > > --
> > > > 2.20.1
> > > >
Re: [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper
On Mon, 2019-05-06 at 17:46 +0300, andriy.shevche...@linux.intel.com wrote: > [External] > > > On Mon, May 06, 2019 at 04:45:43PM +0300, Alexandru Ardelean wrote: > > On Fri, Apr 26, 2019 at 5:27 PM andriy.shevche...@linux.intel.com > > wrote: > > > > > > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote: > > > > > > > Hmm, I actually did not give much thought to that -1. > > > > I'll check into this and see about a V3. > > > > It may make more sense to just fix the original > > > > `__sysfs_match_string()`, but I'll need to go through the users of > > > > this function and see. > > > > > > I was thinking about existing users of such (with "gaps") cases. > > > Not all of them have NULL there and would like to avoid some members. > > > Though, I think that we may ignore NULL items if -1 is supplied. > > > > > > Think as well about ARRAY_SIZE() as given to that. > > > > > > > I am a bit vague on what you are proposing. > > Is it: > > > > a) Leave __sysfs_match_string() as-is and introduce a new > > `__sysfs_match_string_with_gaps()` helper/variant ? > > b) Fix __sysfs_match_string() to break/exit on the first NULL, only if > > -1 is provided ? > > > > Either is fine, but I wanted to clarify. > > The current logic something like "-1 to go till first NULL" and > ARRAY_SIZE() in > *some* cases is basically the synonym to above. > > What I meant is to check if there is *any* case where ARRAY_SIZE() > behaves in > the same way as -1. Those cases should be fixed accordingly. > > Otherwise, the b) is what would be preferred according to the discussion. > Ack. I sent a series. I guess this is the noisiest I've ever been. And I feel a bit bad/guilty [for generating that much noise], but I'll probably grab a beer later to treat it. I'll probably learn something from this. Guilt-tripped experiences are pretty learnful. Thanks Alex > > > And consider to fix match_string() accordingly. > > -- > With Best Regards, > Andy Shevchenko > >
