RE: [PATCH] binder: ipc namespace support for android binder
>> Hi >> We are working for running android in container, but we found that binder >> is >> not isolated by ipc namespace. Since binder is a form of IPC and therefore >> should >> be tied to ipc namespace. With this patch, we can run more than one android >> container on one host. >> This patch move "binder_procs" and "binder_context" into ipc_namespace, >> driver will find the context from it when opening. Althought statistics in >> debugfs >> remain global. >> >> Signed-off-by: choury zhou >> --- >> drivers/android/Kconfig | 2 +- >> drivers/android/binder.c | 126 +- >> include/linux/ipc_namespace.h | 14 >> ipc/namespace.c | 4 ++ >> 4 files changed, 111 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig >> index 432e9ad77070..09883443b2da 100644 >> --- a/drivers/android/Kconfig >> +++ b/drivers/android/Kconfig >> @@ -10,7 +10,7 @@ if ANDROID >> >> config ANDROID_BINDER_IPC >> bool "Android Binder IPC Driver" >> - depends on MMU >> + depends on MMU && SYSVIPC > > NAK. We can't force SYSVIPC on for Android. The notion of running > binder in a container is reasonable, but needs to be done without > explicit requirement for SYSVIPC. binder-in-container is a topic in > the android microconf at Linux plumbers -- are you going to be at LPC? > We have no plan to going to attend LPC temporarily. > It's not obvious from this patch where this dependency comes > from...why is SYSVIPC required? I'd like to not have to require IPC_NS > either for devices. Yes, the patch is not highly dependent on SYSVIPC, but it will be convenient if require it. I will update it to drop dependency of it in V2 patch. This patch doesn't need IPC_NS set at present. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 18/18] staging: vchiq: add more tasks to the TODO list
> Nicolas Saenz Julienne hat am 26. Oktober 2018 um > 15:48 geschrieben: > > > The more the better. Please try to find a better commit log ;-) > > Signed-off-by: Nicolas Saenz Julienne > --- > .../staging/vc04_services/interface/vchi/TODO | 46 ++- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/vc04_services/interface/vchi/TODO > b/drivers/staging/vc04_services/interface/vchi/TODO > index 0b3ec75ff627..bf2135826431 100644 > --- a/drivers/staging/vc04_services/interface/vchi/TODO > +++ b/drivers/staging/vc04_services/interface/vchi/TODO > @@ -27,8 +27,8 @@ unused. > 3) Make driver more portable > > Building this driver with arm/multi_v7_defconfig or arm64/defconfig > -leads to data corruption during the following command: > - > +leads to data corruption during the following command: > + I assume this wasn't intended. Btw after applying Phil's patch series. This point can be dropped. Beside that i'm fine with the additions. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 16/18] staging: vchiq_arm: rework probe and init functions
> Nicolas Saenz Julienne hat am 26. Oktober 2018 um > 15:48 geschrieben: > > > Some operations performed in the probe function should have been > implemented in the init function. Namely class and dev region creations. > Please explain the why in the commit log ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores
> Nicolas Saenz Julienne hat am 26. Oktober 2018 um > 15:48 geschrieben: > > > It is preferred in the kernel to avoid using semaphores to wait for > events, as they are optimised for the opposite situation; where the > common case is that they are available and may block only occasionally. > FYI see this thread: https://lkml.org/lkml/2008/4/11/323. > > Also completions are semantically more explicit in this case. > Since patch #11 #12 and #13 doing the same, they could be fold. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 15/18] stagning: vchiq_core: fix logic redundancy in parse_open
> Nicolas Saenz Julienne hat am 26. Oktober 2018 um > 15:48 geschrieben: > > > We update sync to reflect that the firmware version is compatible with > that option. We don't need to check both of them again further down the > code. please fix the typo in the subject s/stagning/staging/ for this patch and patch #4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.
On Sun, Oct 28, 2018 at 02:51:08PM +, Jonathan Cameron wrote:
> On Sun, 28 Oct 2018 13:23:23 +0530
> Nishad Kamdar wrote:
>
> > Replace platform data with device tree support.
> >
> > Signed-off-by: Nishad Kamdar
> The whole gpio in or out thing makes less and less sense to
> me and seems to contradict the datasheet.
>
> If I'm not missing something I would just get rid of the option.
> If I am missing something (not hard in the datasheet which, whilst
> fairly clear, is a rather long ;)
>
> Jonathan
>
Ok, I'll drop it.
> > ---
> > drivers/staging/iio/resolver/Kconfig| 1 +
> > drivers/staging/iio/resolver/ad2s1210.c | 17 -
> > drivers/staging/iio/resolver/ad2s1210.h | 17 -
> > 3 files changed, 9 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/Kconfig
> > b/drivers/staging/iio/resolver/Kconfig
> > index 6a469ee6101f..cc1202ff813d 100644
> > --- a/drivers/staging/iio/resolver/Kconfig
> > +++ b/drivers/staging/iio/resolver/Kconfig
> > @@ -15,6 +15,7 @@ config AD2S90
> >
> > config AD2S1210
> > tristate "Analog Devices ad2s1210 driver"
> > + depends on OF
> > depends on SPI
> > depends on GPIOLIB || COMPILE_TEST
> > help
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index 0234869e9d74..5ecdb5785132 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -76,7 +77,6 @@ struct ad2s1210_gpio {
> > };
> >
> > struct ad2s1210_state {
> > - const struct ad2s1210_platform_data *pdata;
> > struct mutex lock;
> > struct spi_device *sdev;
> > struct gpio_desc *sample;
> > @@ -84,6 +84,7 @@ struct ad2s1210_state {
> > struct gpio_desc *a1;
> > struct gpio_desc *res0;
> > struct gpio_desc *res1;
> > + bool gpioin;
> > unsigned int fclkin;
> > unsigned int fexcit;
> > bool hysteresis;
> > @@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device
> > *dev,
> > }
> > st->resolution
> > = ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> > - if (st->pdata->gpioin) {
> > + if (st->gpioin) {
> > data = ad2s1210_read_resolution_pin(st);
> > if (data != st->resolution)
> > dev_warn(dev, "ad2s1210: resolution settings not
> > match\n");
> > @@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device
> > *dev,
> > }
> > st->resolution
> > = ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> > - if (st->pdata->gpioin) {
> > + if (st->gpioin) {
> > data = ad2s1210_read_resolution_pin(st);
> > if (data != st->resolution)
> > dev_warn(dev, "ad2s1210: resolution settings not
> > match\n");
> > @@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
> > int ret;
> >
> > mutex_lock(>lock);
> > - if (st->pdata->gpioin)
> > + if (st->gpioin)
> > st->resolution = ad2s1210_read_resolution_pin(st);
> > else
> > ad2s1210_set_resolution_pin(st);
> > @@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state
> > *st)
> > int ret, i;
> > struct spi_device *spi = st->sdev;
> > struct ad2s1210_gpio *pin;
> > - unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> > + unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> >
> > struct ad2s1210_gpio gpios[] = {
> > { .ptr = >sample, .name = "sample", .flags = GPIOD_IN },
> > @@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
> > {
> > struct iio_dev *indio_dev;
> > struct ad2s1210_state *st;
> > + struct device_node *np = spi->dev.of_node;
> > int ret;
> >
> > - if (!spi->dev.platform_data)
> > - return -EINVAL;
> > -
> > indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
> > if (!indio_dev)
> > return -ENOMEM;
> > st = iio_priv(indio_dev);
> > - st->pdata = spi->dev.platform_data;
> > + st->gpioin = of_property_read_bool(np, "gpioin");
>
> Hmm. This bothered me in the earlier patch. How are we meant to configure
> these pins... (and this time I actually loaded the datasheet)
>
> A0 and A1 always seem to be control signals written to the device so
> I don't really understand why we would ever want them to be 'inputs'
> on our host.
>
> RES0 and RES1 are also control signals. Clearly marked as logic inputs.
>
> The only thing I can think here is there is an evaluation board out there
> were we are not in control of these, some other controller is.
> That is a pretty weird board if so, hence I would only support the version
> where we use GPIO outputs from the host.
>
> This will further simplify patch 1 and get
Re: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice
Hi Nicolas, > Nicolas Saenz Julienne hat am 26. Oktober 2018 um > 15:48 geschrieben: > > > vchiq_init_state() initialises a series of semaphores to then call > remote_event_create() on the same semaphores, which initializes them > again. i would prefer to have all init stuff at one place in vchiq_init_state() and drop this ugliness from remote_event_create() instead. Is this possible? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On Sun, Oct 28, 2018 at 02:36:07PM +, Jonathan Cameron wrote:
> On Sun, 28 Oct 2018 13:21:25 +0530
> Nishad Kamdar wrote:
>
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface.
> >
> > Signed-off-by: Nishad Kamdar
> Hi Nishad,
>
> Sorry it took me most of the week to get to this. It's a trade off when you
> want to make fast progress, but I would suggest always leaving a few days
> between patches to see if there are other review comments coming in.
>
Ok, I'll keep that in mind.
> I don't particularly mind myself (as I'm very good at ignoring older versions
> ;)
> but I do feel some time got wasted here on your part.
>
> Anyhow, what you have here is correct but the drive to get things as a nice
> array has lead to a weird mixture of being all array based and not being at
> all.
> Some suggestions in line that will hopefully tidy this up for you.
>
> I'm basically suggesting adding a layer of indirection where you don't
> currently
> have one (on the actual reads and writes) so that you get more consistency
> with the setup code rather than adding a lot of fiddly indirection just in
> the setup code.
>
> Note, what I've given is very much meant to be an illustration. It's probably
> full of bugs and silly naming etc, so don't take it as being right!
>
> Jonathan
>
> > ---
> > drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
> > drivers/staging/iio/resolver/ad2s1210.h | 3 -
> > 2 files changed, 50 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index ac13b99bd9cb..93c3c70ce62e 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -15,7 +15,7 @@
> > #include
> > #include
> > #include
> > -#include
> > +#include
> > #include
> >
> > #include
> > @@ -69,10 +69,21 @@ enum ad2s1210_mode {
> >
> > static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
> >
> > +struct ad2s1210_gpio {
> > + struct gpio_desc **ptr;
> > + const char *name;
> > + unsigned long flags;
> > +};
> > +
> > struct ad2s1210_state {
> > const struct ad2s1210_platform_data *pdata;
> > struct mutex lock;
> > struct spi_device *sdev;
>
> With the setup below this becomes
> struct gpio_desc *gpios[5];
>
> > + struct gpio_desc *sample;
> > + struct gpio_desc *a0;
> > + struct gpio_desc *a1;
> > + struct gpio_desc *res0;
> > + struct gpio_desc *res1;
> > unsigned int fclkin;
> > unsigned int fexcit;
> > bool hysteresis;
> > @@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
> > static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
> > struct ad2s1210_state *st)
> > {
> > - gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
> > - gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
> > + gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
> > + gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
> > st->mode = mode;
> > }
> >
> > @@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct
> > ad2s1210_state *st)
> >
> > static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state
> > *st)
> > {
> > - int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
> > - gpio_get_value(st->pdata->res[1]);
> > + int resolution = (gpiod_get_value(st->res0) << 1) |
> > + gpiod_get_value(st->res1);
> >
> > return ad2s1210_resolution_value[resolution];
> > }
> > @@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
> >
> > static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
> > {
> > - gpio_set_value(st->pdata->res[0],
> > - ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > - gpio_set_value(st->pdata->res[1],
> > - ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> > + gpiod_set_value(st->res0,
> > + ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > + gpiod_set_value(st->res1,
> > + ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> > }
> >
> > static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> > @@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device
> > *dev,
> > int ret;
> >
> > mutex_lock(>lock);
> > - gpio_set_value(st->pdata->sample, 0);
> > + gpiod_set_value(st->sample, 0);
> > /* delay (2 * tck + 20) nano seconds */
> > udelay(1);
> > - gpio_set_value(st->pdata->sample, 1);
> > + gpiod_set_value(st->sample, 1);
> > ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> > if (ret < 0)
> > goto error_ret;
> > - gpio_set_value(st->pdata->sample, 0);
> > - gpio_set_value(st->pdata->sample, 1);
> > + gpiod_set_value(st->sample, 0);
> > +
Re: [PATCH RFC 00/11] staging: vc04_services: Improve driver load/unload
Hi Nicolas,
> Nicolas Saenz Julienne hat am 26. Oktober 2018 um
> 13:06 geschrieben:
>
>
> Hi Stefan,
>
> On Thu, 2018-10-25 at 17:29 +0200, Stefan Wahren wrote:
> > This patch series improves the load/unload of bcm2835 camera and
> > audio
> > drivers. It has been tested with Raspberry Pi 3 B and a camera module
> > V1.
> >
> > This series based on current linux-next and Phil Elwell's series
> > ("Improve VCHIQ
> > cache line size handling"). After Nicolas' series ("staging:
> > vc04_services:
> > Some dead code removal") has been applied, i will rebase my series.
> >
> > Stefan Wahren (11):
> > staging: bcm2835-camera: Abort probe if there is no camera
> > staging: bcm2835-camera: fix module autoloading
> > staging: bcm2835-camera: Move module info to the end
> > staging: vchiq_arm: Fix platform device unregistration
> > staging: vchiq_arm: Fix camera device registration
> > staging: vchiq_arm: Register a platform device for audio
> > staging: bcm2835-audio: Enable compile test
> > staging: bcm2835-audio: use module_platform_driver() macro
> > staging: bcm2835-audio: Drop DT dependency
> > staging: bcm2835-camera: Provide more specific probe error messages
> > staging: bcm2835-camera: Add hint about possible faulty config
> >
> > .../staging/vc04_services/bcm2835-audio/Kconfig| 2 +-
> > .../staging/vc04_services/bcm2835-audio/bcm2835.c | 61 ++
> > ---
> > .../vc04_services/bcm2835-camera/bcm2835-camera.c | 78
> > +++---
> > .../vc04_services/bcm2835-camera/mmal-vchiq.c | 5 +-
> > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 27 ++--
> > 5 files changed, 102 insertions(+), 71 deletions(-)
> >
>
> I prefer Dan's approach to error checking in vchiq_probe(). Apart from
> that seems good to me.
>
> Reviewed-by: Nicolas Saenz Julienne
unfortunately there is a issue with this series, after enabling the memleak
detector i'm getting this:
unreferenced object 0xec9c9300 (size 64):
comm "systemd-udevd", pid 182, jiffies 4294937996 (age 1376.140s)
hex dump (first 32 bytes):
ff ff ff ff 00 00 00 00 2f 70 6c 61 74 66 6f 72 /platfor
6d 2f 73 6f 63 2f 33 66 30 30 62 38 34 30 2e 6d m/soc/3f00b840.m
backtrace:
[<9d7676d1>] vchiq_register_child+0x58/0x74 [vchiq]
[<6a2780cc>] vchiq_probe+0x1c0/0x264 [vchiq]
[<278d830e>] platform_drv_probe+0x48/0x98
[] really_probe+0x228/0x2d0
[<489d6b89>] driver_probe_device+0x60/0x164
[] __driver_attach+0xd0/0xd4
[<042acada>] bus_for_each_dev+0x74/0xb4
[] bus_add_driver+0x18c/0x210
[] driver_register+0x7c/0x114
[] do_one_initcall+0x54/0x1fc
[<9420261f>] do_init_module+0x64/0x1f4
[<571c859a>] load_module+0x1dfc/0x24bc
[<06885682>] sys_finit_module+0xac/0xd8
[<85e18c3d>] __sys_trace_return+0x0/0x20
[<0051c54d>] 0xbecb0898
[<0a0ced8e>] 0x
>
> Regards,
> Nicolas
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Which SPDX Identifier for files without explicit GPL version
On 10/28/18 3:13 PM, Greg Kroah-Hartman wrote: On Sun, Oct 28, 2018 at 02:22:40PM +0100, Michael Straube wrote: On 10/28/18 12:27 PM, Greg Kroah-Hartman wrote: On Sun, Oct 28, 2018 at 11:04:06AM +0100, Michael Straube wrote: Hi, which GPL version should be used in SPDX Identifiers for files that are GPL licensed but do not mention any version? It is not clear to me after reading license-rules.rst. For example: /** * Copyright(c) 2008 - 2010 Realtek Corporation. All rights reserved. * * This program is distributed in the hope that it will be useful, but WITHOUT * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for * more details. * * The full GNU General Public License is included in this distribution in the * file called LICENSE. Perhaps look up what version of the GPL that was originally contained in the tarball that this driver was bundled with? I think that should be on their website somewhere... If not, it's v1, not a good thing, but all we have to go on. Also, try emailing: * Contact Information: * wlanfae That address :) thanks, greg k-h Ah, there is a file 'license' in the root directory of this driver (rtl8192e) that I overlooked. It contains GPLv2. So v2 applies to all files with the above shortened boiler plate? Yes. So add a SPDX line to the file and delete the boiler plate text so we don't have this question again in the future. Then delete that opy of the GPL so we don't have files we don't need :) thanks, greg k-h Alright, thanks. :) There are two files left with no GPL version mentioned and no reference to a file with the license text. I'm unsure how to handle the two files, so better asking to get it right. Are they also covered by the license file in the root directory? /* IEEE 802.11 SoftMAC layer * Copyright (c) 2005 Andrea Merello * * Mostly extracted from the rtl8180-sa2400 driver for the * in-kernel generic ieee802.11 stack. * * Few lines might be stolen from other part of the rtllib * stack. Copyright who own it's copyright * * WPA code stolen from the ipw2200 driver. * Copyright who own it's copyright. * * released under the GPL */ /* IEEE 802.11 SoftMAC layer * Copyright (c) 2005 Andrea Merello * * Mostly extracted from the rtl8180-sa2400 driver for the * in-kernel generic ieee802.11 stack. * * Some pieces of code might be stolen from ipw2100 driver * copyright of who own it's copyright ;-) * * PS wx handler mostly stolen from hostap, copyright who * own it's copyright ;-) * * released under the GPL */ Perhaps you could say what GPL version to use in the SPDX line, Andrea? Thanks, Michael ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] staging: iio: ad7780: update voltage on read
On Sun, 28 Oct 2018 13:52:32 -0300 Renato Lui Geh wrote: > Hi Jonathan, > > Thank you for the review. > > >> + voltage_uv = regulator_get_voltage(st->reg); > >> + if (voltage_uv) > >> + st->int_vref_mv = voltage_uv/1000; > >>*val = st->int_vref_mv * st->gain; > >Is there actually a reason (now) to have the stashed value > >of int_vref_mv in the state structure? > > From probe: > > if (voltage_uv) > st->int_vref_mv = voltage_uv / 1000; > else > dev_warn(>dev, "Reference voltage unspecified\n"); > > So the idea was to, when voltage_uv = 0, return the previous voltage. > Should I instead handle this as an error the same way as in probe? > I would return it as an error. I can't really see how we would get this to occur if the bindings are all correct and appropriate driver support is there for the regulator to actually let us use it. If we wanted to handle the case of no regulator having been provided cleanly then we should it using an optional regulator get, and not provide the scale attribute at all if we can't know what it will read. This is a weird enough corner case though that I just wouldn't bother handling it as anything other than an error. > Thanks, > Renato Thanks, Jonathan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] staging: iio: ad7780: update voltage on read
Hi Jonathan, Thank you for the review. + voltage_uv = regulator_get_voltage(st->reg); + if (voltage_uv) + st->int_vref_mv = voltage_uv/1000; *val = st->int_vref_mv * st->gain; Is there actually a reason (now) to have the stashed value of int_vref_mv in the state structure? From probe: if (voltage_uv) st->int_vref_mv = voltage_uv / 1000; else dev_warn(>dev, "Reference voltage unspecified\n"); So the idea was to, when voltage_uv = 0, return the previous voltage. Should I instead handle this as an error the same way as in probe? Thanks, Renato ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/6] staging:iio:ad2s90: Add scale info and improve error handling
On Fri, 26 Oct 2018 22:59:59 -0300 Matheus Tavares wrote: > This patch set adds scale info to ad2s90's single channel, improve > error handling in it's functions and fix a possible race condition > issue. > > The goal with this patch set is to address the points discussed in the > mailing list in an effort to move ad2s90.c out of staging. Thanks, A good series in general. A few suggested improvements. If I haven't commented on a patch, usually it means I'm happy with it and will pick it up with the rest of the series. Jonathan > > Changes in v2: > - Added my S-o-B in patch 5. > > Matheus Tavares (5): > staging:iio:ad2s90: Make read_raw return spi_read's error code > staging:iio:ad2s90: Make probe handle spi_setup failure > staging:iio:ad2s90: Remove always overwritten assignment > staging:iio:ad2s90: Move device registration to the end of probe > staging:iio:ad2s90: Check channel type at read_raw > > Victor Colombo (1): > staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and > read_raw > > drivers/staging/iio/resolver/ad2s90.c | 55 ++- > 1 file changed, 37 insertions(+), 18 deletions(-) > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 5/6] staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and read_raw
On Fri, 26 Oct 2018 23:00:04 -0300
Matheus Tavares wrote:
> This patch adds the IIO_CHAN_INFO_SCALE mask to ad2s90_chan and
> implements the relative read behavior at ad2s90_read_raw.
>
> Signed-off-by: Victor Colombo
> Signed-off-by: Matheus Tavares
Hi,
A suggestion inline. This is a common case that we have infrastructure
to simplify. + I think your scale factor is very slightly wrong.
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s90.c | 32 ++-
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index b4a6a89c11b0..52b656875ed1 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -34,19 +34,31 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> int ret;
> struct ad2s90_state *st = iio_priv(indio_dev);
>
> - mutex_lock(>lock);
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> + *val = 0;
> + *val2 = 1534355;
Definitely 2^12 - 1? That's a bit unusual if true as it would imply
that 2^12 - 1 and 0 were the same value.
Imagine a smaller version with on 2^2 bits so 0, 1, 2, 3
Values of each are
0, M_PI/2, M_PI, 3*M_PI/2
So the multiplier is 2*M_PI/(2^2) not 2*M_PI/(2^2 - 1)
1/2 vs 2/3 * M_PI
Now this is a very common case so we have the return type
IIO_VAL_FRACTIONAL_LOG2 to give a more obvious and potentially
more accurate representation.
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(>lock);
> +
> + ret = spi_read(st->sdev, st->rx, 2);
> + if (ret < 0) {
> + mutex_unlock(>lock);
> + return ret;
> + }
> +
> + *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>
> - ret = spi_read(st->sdev, st->rx, 2);
> - if (ret < 0) {
> mutex_unlock(>lock);
> - return ret;
> - }
>
> - *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> -
> - mutex_unlock(>lock);
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
>
> - return IIO_VAL_INT;
> + return -EINVAL;
> }
>
> static const struct iio_info ad2s90_info = {
> @@ -57,7 +69,7 @@ static const struct iio_chan_spec ad2s90_chan = {
> .type = IIO_ANGL,
> .indexed = 1,
> .channel = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> };
>
> static int ad2s90_probe(struct spi_device *spi)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/6] staging:iio:ad2s90: Make probe handle spi_setup failure
On Fri, 26 Oct 2018 23:00:01 -0300
Matheus Tavares wrote:
> Previously, ad2s90_probe ignored the return code from spi_setup, not
> handling its possible failure. This patch makes ad2s90_probe check if
> the code is an error code and, if so, do the following:
>
> - Call dev_err with an appropriate error message.
> - Return the spi_setup's error code, aborting the execution of the
> rest of the function.
>
> Signed-off-by: Matheus Tavares
> ---
> drivers/staging/iio/resolver/ad2s90.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 11fac9f90148..d6a42e3f1bd8 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -88,7 +88,12 @@ static int ad2s90_probe(struct spi_device *spi)
> /* need 600ns between CS and the first falling edge of SCLK */
> spi->max_speed_hz = 83;
> spi->mode = SPI_MODE_3;
> - spi_setup(spi);
> + ret = spi_setup(spi);
> +
> + if (ret < 0) {
> + dev_err(>dev, "spi_setup failed!\n");
> + return ret;
> + }
I would have reordered this first to be before the iio_device_register call.
The reason being that it would avoid this comment.
Drop the return ret out of the block above and return ret unconditionally.
I don't mind too much as I know this is moving later, but I only know that
because of the earlier discussion ;) Few reviewers read the whole patch
set before responding to the early patches - it's just too much like hard
work. So if you can do things in an order that minimizes standard responses
then that's great.
Patch is fine though - could be solved by a comment in the intro that
says the code in question will move in patch X.
Jonathan
>
> return 0;
> }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
On Fri, 26 Oct 2018 23:00:00 -0300
Matheus Tavares wrote:
> Previously, when spi_read returned an error code inside ad2s90_read_raw,
> the code was ignored and IIO_VAL_INT was returned. This patch makes the
> function return the error code returned by spi_read when it fails.
>
> Signed-off-by: Matheus Tavares
Hi Matheus,
One quick process note is that it takes people a while to get around to
reviewing
a series, so whilst it's tempting to very quickly send out a fix the moment
someone points out something that needs fixing, it is perhaps better to wait
at least a few days to see if you can pick up a few more reviews before you
do a V2.
A few comments on this one inline. I think it can be done 'slightly'
(and I mean only slightly) nicer than the version you have. Result is the
same though.
Thanks,
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s90.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 59586947a936..11fac9f90148 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> struct ad2s90_state *st = iio_priv(indio_dev);
>
> mutex_lock(>lock);
> +
Unconnected change. I'm not against the change in principle but please
group white space tidying up in it's own patch.
> ret = spi_read(st->sdev, st->rx, 2);
> - if (ret)
> - goto error_ret;
> + if (ret < 0) {
> + mutex_unlock(>lock);
> + return ret;
I'd actually prefer to keep the return path the same as before as then
it is easy (if the function gets more complex in future) to be sure
that all paths unlock the mutex.
> + }
> +
> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>
> -error_ret:
> mutex_unlock(>lock);
>
> return IIO_VAL_INT;
The 'standard' if slightly nasty way of doing this is:
return ret ? ret : IIO_VAL_INT;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: iio: ad7780: fix offset read value
On Thu, 25 Oct 2018 19:30:37 -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 It obviously doesn't make much difference, but as you are respinning anyway... Please could you always put fixes at the beginning of a set as it makes it easier to back port them. It think this one is simple enough that it is worth applying to the stable kernels. (naturally I could do that anyway as the rebase is trivial, so I'm just encouraging good practice by asking you to do it in v3!) Thanks, Jonathan > --- > 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 27972563bb6a..06700fe554a2 100644 > --- a/drivers/staging/iio/adc/ad7780.c > +++ b/drivers/staging/iio/adc/ad7780.c > @@ -100,7 +100,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; > } > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] staging: iio: ad7780: update voltage on read
On Thu, 25 Oct 2018 19:30:04 -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
> ---
> drivers/staging/iio/adc/ad7780.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..27972563bb6a 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,11 +87,15 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv = 0;
No initialization needed as it is always set in the paths where it is used
(before it is used).
>
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> case IIO_CHAN_INFO_SCALE:
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (voltage_uv)
> + st->int_vref_mv = voltage_uv/1000;
> *val = st->int_vref_mv * st->gain;
Is there actually a reason (now) to have the stashed value
of int_vref_mv in the state structure?
Also, I think we are always replacing the value that is retrieved
in probe so we can drop reading it there as well.
Thanks,
Jonathan
> *val2 = chan->scan_type.realbits - 1;
> return IIO_VAL_FRACTIONAL_LOG2;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: olpc_dcon: olpc_dcon_xo_1.c: Switch to the gpio descriptor interface
Hi Nishad, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on v4.19 next-20181019] [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/Nishad-Kamdar/staging-olpc_dcon-olpc_dcon_xo_1-c-Switch-to-the-gpio-descriptor-interface/20181028-124517 config: i386-randconfig-n0-10280159 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): >> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.o:drivers/staging/olpc_dcon/olpc_dcon.h:70: >> multiple definition of `dcon_blank' drivers/staging/olpc_dcon/olpc_dcon.o:drivers/staging/olpc_dcon/olpc_dcon.h:70: first defined here >> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.o:drivers/staging/olpc_dcon/olpc_dcon.h:69: >> multiple definition of `dcon_load' drivers/staging/olpc_dcon/olpc_dcon.o:drivers/staging/olpc_dcon/olpc_dcon.h:69: first defined here >> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.o:drivers/staging/olpc_dcon/olpc_dcon.h:68: >> multiple definition of `dcon_irq' drivers/staging/olpc_dcon/olpc_dcon.o:drivers/staging/olpc_dcon/olpc_dcon.h:68: first defined here >> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.o:drivers/staging/olpc_dcon/olpc_dcon.h:67: >> multiple definition of `dcon_stat1' drivers/staging/olpc_dcon/olpc_dcon.o:drivers/staging/olpc_dcon/olpc_dcon.h:67: first defined here >> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.o:drivers/staging/olpc_dcon/olpc_dcon.h:66: >> multiple definition of `dcon_stat0' drivers/staging/olpc_dcon/olpc_dcon.o:drivers/staging/olpc_dcon/olpc_dcon.h:66: first defined here --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: iio: ad5933: Remove unnecessary blank line
On Mon, 22 Oct 2018 17:41:27 -0300 Victor Colombo wrote: > This patch fixes the checkpatch.pl warning: > > WARNING: Blank lines aren't necessary before a close brace '}' > > Signed-off-by: Victor Colombo Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to ignore it (we'll be testing the other patches I applied today rather than this one which can't really be wrong :) Thanks, Jonathan > --- > drivers/staging/iio/impedance-analyzer/ad5933.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c > b/drivers/staging/iio/impedance-analyzer/ad5933.c > index 7a216ea90784..f9bcb8310e21 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -267,7 +267,6 @@ static void ad5933_calc_out_ranges(struct ad5933_state > *st) > > for (i = 0; i < 4; i++) > st->range_avail[i] = normalized_3v3[i] * st->vref_mv / 3300; > - > } > > /* ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: iio: ad5933: Remove unnecessary space on casting
On Mon, 22 Oct 2018 17:40:25 -0300
Victor Colombo wrote:
> This patch fixes the checkpatch.pl warning:
>
> WARNING: No space is necessary after a cast
>
> Signed-off-by: Victor Colombo
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it and see if we are missing something.
Thanks,
Jonathan
> ---
> drivers/staging/iio/impedance-analyzer/ad5933.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 8a920f675b83..7a216ea90784 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -210,7 +210,7 @@ static int ad5933_set_freq(struct ad5933_state *st,
> u8 d8[4];
> } dat;
>
> - freqreg = (u64) freq * (u64) (1 << 27);
> + freqreg = (u64)freq * (u64)(1 << 27);
> do_div(freqreg, st->mclk_hz / 4);
>
> switch (reg) {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adt7316: Switch to the gpio descriptor interface
On Wed, 24 Oct 2018 22:03:54 +0530
Nishad Kamdar wrote:
> On Mon, Oct 22, 2018 at 11:22:15PM +0530, Shreeya Patel wrote:
> > On Mon, 2018-10-22 at 22:44 +0530, Nishad Kamdar wrote:
> > > Use the gpiod interface instead of the deprecated old non-descriptor
> > > interface for ldac_pin.
> > >
> > > Signed-off-by: Nishad Kamdar
> > > ---
> >
> > Hi Nishad,
> >
> > I have been working on implementing device tree bindings for this
> > driver and removing platform data from it.
> >
> > I've sent a series of patches for it to Jonathan. I didn't send it
> > on the mailing list as I wanted to confirm from Jonathan if we wants
> > something more to be done in the series. That series also includes
> > the change which you are proposing here.
> > I am really sorry that you didn't know about this.
> > If you are planning to send any other patches for this driver then
> > let me know before you start implementing it so that we can
> > co-ordinate before doing any work.
> >
> > Thanks
> >
> >
> > > drivers/staging/iio/addac/adt7316.c | 17 -
> > > 1 file changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/addac/adt7316.c
> > > b/drivers/staging/iio/addac/adt7316.c
> > > index 3f22d1088713..94f945ba0097 100644
> > > --- a/drivers/staging/iio/addac/adt7316.c
> > > +++ b/drivers/staging/iio/addac/adt7316.c
> > > @@ -8,7 +8,7 @@
> > > */
> > >
> > > #include
> > > -#include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -177,7 +177,7 @@
> > >
> > > struct adt7316_chip_info {
> > > struct adt7316_bus bus;
> > > - u16 ldac_pin;
> > > + struct gpio_desc*ldac_pin;
> > > u16 int_mask; /* 0x2f */
> > > u8 config1;
> > > u8 config2;
> > > @@ -950,8 +950,8 @@ static ssize_t adt7316_store_update_DAC(struct
> > > device *dev,
> > > if (ret)
> > > return -EIO;
> > > } else {
> > > - gpio_set_value(chip->ldac_pin, 0);
> > > - gpio_set_value(chip->ldac_pin, 1);
> > > + gpiod_set_value(chip->ldac_pin, 0);
> > > + gpiod_set_value(chip->ldac_pin, 1);
> > > }
> > >
> > > return len;
> > > @@ -2120,7 +2120,14 @@ int adt7316_probe(struct device *dev, struct
> > > adt7316_bus *bus,
> > > else
> > > return -ENODEV;
> > >
> > > - chip->ldac_pin = adt7316_platform_data[1];
> > > + chip->ldac_pin = devm_gpiod_get(dev, "ldac",
> > > GPIOD_OUT_HIGH);
> > > + if (IS_ERR(chip->ldac_pin)) {
> > > + ret = PTR_ERR(chip->ldac_pin);
> > > + dev_err(dev, "Failed to request ldac GPIO: %d\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > +
> > > if (chip->ldac_pin) {
> > > chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDCA;
> > > if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
> Hello Shreeya,
>
> Sure no problem.
>
> Thanks and regards,
> Nishad
Hi Nishad,
Thanks for being so understanding on this. I would normally operate
a policy of first to post on the list (assuming they update reasonably
quickly) is the on I apply, but as you are fine with it and this will
save Shreeya a little time (by avoiding a rebase) I'll wait for Shreeya's
public version.
I once discovered I was one of 3 people working on new drivers for a
single camera sensor years ago. All 3 got posted in the same week
despite the part having been around for years before that. Sometimes
we get unlucky - the nature of opensource ;)
Jonathan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.
On Sat, 27 Oct 2018 17:49:03 +0200
Slawomir Stepien wrote:
> Hi
>
> On paź 26, 2018 18:55, Nishad Kamdar wrote:
> > Add device tree table for matching vendor ID
> > and support for retrieving platform data
> > from device tree.
>
> So maybe you should make 2 commits?
>
> > Signed-off-by: Nishad Kamdar
> > ---
> > drivers/staging/iio/resolver/ad2s1210.c | 43 -
> > 1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index 93c3c70ce62e..9fd5461c4ed0 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state
> > *st)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_OF
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct ad2s1210_platform_data *pdata;
> > +
> > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return NULL;
> > +
> > + pdata->gpioin = of_property_read_bool(np, "adi,gpioin");
>
> Why here you are adding "adi", but you are not adding it to the gpios in prev
> commit?
>
> I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I
> do
> not understand why adding vendor id to props is needed...
This is not standard ABI, hence we define it as being under the scope of
a particular manufacturer (and hope they use it consistently ;)
So this should have a the prefix, as should the gpios now you mention it.
We have gotten that bit wrong a lot in the past in IIO. If the gpio is
standard, i.e. reset or similar it doesn't need it, but for very much device
specific gpios like these it should have the prefix
(similar reasons to any other property - different manufacturers might
use the same name for different things).
Jonathan
>
> > +
> > + return pdata;
> > +}
> > +#else
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > +#endif
> > +
> > static int ad2s1210_probe(struct spi_device *spi)
> > {
> > struct iio_dev *indio_dev;
> > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi)
> > if (!indio_dev)
> > return -ENOMEM;
> > st = iio_priv(indio_dev);
> > - st->pdata = spi->dev.platform_data;
> > + if (spi->dev.of_node) {
> > + st->pdata = ad2s1210_parse_dt(>dev);
> > + if (!st->pdata)
> > + return -EINVAL;
> > + } else {
> > + st->pdata = spi->dev.platform_data;
> > + }
> > +
> > + if (!st->pdata) {
> > + dev_err(>dev, "ad2s1210: no platform data supplied\n");
> > + return -EINVAL;
> > + }
> > +
>
> Why not just use only device-tree here? The ad2s1210_platform_data has now
> only
> one entry... In that case remember to add "depends on OF" in Kconfig.
>
> > ret = ad2s1210_setup_gpios(st);
> > if (ret < 0)
> > return ret;
> > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi)
> > return 0;
> > }
> >
> > +static const struct of_device_id ad2s1210_of_match[] = {
> > + { .compatible = "adi,ad2s1210", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
> > +
> > static const struct spi_device_id ad2s1210_id[] = {
> > { "ad2s1210" },
> > {}
> > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id);
> > static struct spi_driver ad2s1210_driver = {
> > .driver = {
> > .name = DRV_NAME,
> > + .of_match_table = of_match_ptr(ad2s1210_of_match),
> > },
> > .probe = ad2s1210_probe,
> > .remove = ad2s1210_remove,
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.
On Sun, 28 Oct 2018 13:23:23 +0530
Nishad Kamdar wrote:
> Replace platform data with device tree support.
>
> Signed-off-by: Nishad Kamdar
The whole gpio in or out thing makes less and less sense to
me and seems to contradict the datasheet.
If I'm not missing something I would just get rid of the option.
If I am missing something (not hard in the datasheet which, whilst
fairly clear, is a rather long ;)
Jonathan
> ---
> drivers/staging/iio/resolver/Kconfig| 1 +
> drivers/staging/iio/resolver/ad2s1210.c | 17 -
> drivers/staging/iio/resolver/ad2s1210.h | 17 -
> 3 files changed, 9 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/Kconfig
> b/drivers/staging/iio/resolver/Kconfig
> index 6a469ee6101f..cc1202ff813d 100644
> --- a/drivers/staging/iio/resolver/Kconfig
> +++ b/drivers/staging/iio/resolver/Kconfig
> @@ -15,6 +15,7 @@ config AD2S90
>
> config AD2S1210
> tristate "Analog Devices ad2s1210 driver"
> + depends on OF
> depends on SPI
> depends on GPIOLIB || COMPILE_TEST
> help
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> b/drivers/staging/iio/resolver/ad2s1210.c
> index 0234869e9d74..5ecdb5785132 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -76,7 +77,6 @@ struct ad2s1210_gpio {
> };
>
> struct ad2s1210_state {
> - const struct ad2s1210_platform_data *pdata;
> struct mutex lock;
> struct spi_device *sdev;
> struct gpio_desc *sample;
> @@ -84,6 +84,7 @@ struct ad2s1210_state {
> struct gpio_desc *a1;
> struct gpio_desc *res0;
> struct gpio_desc *res1;
> + bool gpioin;
> unsigned int fclkin;
> unsigned int fexcit;
> bool hysteresis;
> @@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
> }
> st->resolution
> = ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> - if (st->pdata->gpioin) {
> + if (st->gpioin) {
> data = ad2s1210_read_resolution_pin(st);
> if (data != st->resolution)
> dev_warn(dev, "ad2s1210: resolution settings not
> match\n");
> @@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device
> *dev,
> }
> st->resolution
> = ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> - if (st->pdata->gpioin) {
> + if (st->gpioin) {
> data = ad2s1210_read_resolution_pin(st);
> if (data != st->resolution)
> dev_warn(dev, "ad2s1210: resolution settings not
> match\n");
> @@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
> int ret;
>
> mutex_lock(>lock);
> - if (st->pdata->gpioin)
> + if (st->gpioin)
> st->resolution = ad2s1210_read_resolution_pin(st);
> else
> ad2s1210_set_resolution_pin(st);
> @@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
> int ret, i;
> struct spi_device *spi = st->sdev;
> struct ad2s1210_gpio *pin;
> - unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> + unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
>
> struct ad2s1210_gpio gpios[] = {
> { .ptr = >sample, .name = "sample", .flags = GPIOD_IN },
> @@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
> {
> struct iio_dev *indio_dev;
> struct ad2s1210_state *st;
> + struct device_node *np = spi->dev.of_node;
> int ret;
>
> - if (!spi->dev.platform_data)
> - return -EINVAL;
> -
> indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
> st = iio_priv(indio_dev);
> - st->pdata = spi->dev.platform_data;
> + st->gpioin = of_property_read_bool(np, "gpioin");
Hmm. This bothered me in the earlier patch. How are we meant to configure
these pins... (and this time I actually loaded the datasheet)
A0 and A1 always seem to be control signals written to the device so
I don't really understand why we would ever want them to be 'inputs'
on our host.
RES0 and RES1 are also control signals. Clearly marked as logic inputs.
The only thing I can think here is there is an evaluation board out there
were we are not in control of these, some other controller is.
That is a pretty weird board if so, hence I would only support the version
where we use GPIO outputs from the host.
This will further simplify patch 1 and get rid fo the need for this
non standard bit of devicetree binding.
> ret = ad2s1210_setup_gpios(st);
> if (ret < 0)
> return ret;
> diff --git a/drivers/staging/iio/resolver/ad2s1210.h
>
Re: [PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On Sun, 28 Oct 2018 13:21:25 +0530
Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
>
> Signed-off-by: Nishad Kamdar
Hi Nishad,
Sorry it took me most of the week to get to this. It's a trade off when you
want to make fast progress, but I would suggest always leaving a few days
between patches to see if there are other review comments coming in.
I don't particularly mind myself (as I'm very good at ignoring older versions ;)
but I do feel some time got wasted here on your part.
Anyhow, what you have here is correct but the drive to get things as a nice
array has lead to a weird mixture of being all array based and not being at all.
Some suggestions in line that will hopefully tidy this up for you.
I'm basically suggesting adding a layer of indirection where you don't currently
have one (on the actual reads and writes) so that you get more consistency
with the setup code rather than adding a lot of fiddly indirection just in
the setup code.
Note, what I've given is very much meant to be an illustration. It's probably
full of bugs and silly naming etc, so don't take it as being right!
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
> drivers/staging/iio/resolver/ad2s1210.h | 3 -
> 2 files changed, 50 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> b/drivers/staging/iio/resolver/ad2s1210.c
> index ac13b99bd9cb..93c3c70ce62e 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -15,7 +15,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
>
> #include
> @@ -69,10 +69,21 @@ enum ad2s1210_mode {
>
> static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
>
> +struct ad2s1210_gpio {
> + struct gpio_desc **ptr;
> + const char *name;
> + unsigned long flags;
> +};
> +
> struct ad2s1210_state {
> const struct ad2s1210_platform_data *pdata;
> struct mutex lock;
> struct spi_device *sdev;
With the setup below this becomes
struct gpio_desc *gpios[5];
> + struct gpio_desc *sample;
> + struct gpio_desc *a0;
> + struct gpio_desc *a1;
> + struct gpio_desc *res0;
> + struct gpio_desc *res1;
> unsigned int fclkin;
> unsigned int fexcit;
> bool hysteresis;
> @@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
> static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
>struct ad2s1210_state *st)
> {
> - gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
> - gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
> + gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
> + gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
> st->mode = mode;
> }
>
> @@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct
> ad2s1210_state *st)
>
> static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
> {
> - int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
> - gpio_get_value(st->pdata->res[1]);
> + int resolution = (gpiod_get_value(st->res0) << 1) |
> + gpiod_get_value(st->res1);
>
> return ad2s1210_resolution_value[resolution];
> }
> @@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
>
> static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
> {
> - gpio_set_value(st->pdata->res[0],
> -ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> - gpio_set_value(st->pdata->res[1],
> -ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> + gpiod_set_value(st->res0,
> + ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> + gpiod_set_value(st->res1,
> + ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> }
>
> static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> @@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> int ret;
>
> mutex_lock(>lock);
> - gpio_set_value(st->pdata->sample, 0);
> + gpiod_set_value(st->sample, 0);
> /* delay (2 * tck + 20) nano seconds */
> udelay(1);
> - gpio_set_value(st->pdata->sample, 1);
> + gpiod_set_value(st->sample, 1);
> ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> if (ret < 0)
> goto error_ret;
> - gpio_set_value(st->pdata->sample, 0);
> - gpio_set_value(st->pdata->sample, 1);
> + gpiod_set_value(st->sample, 0);
> + gpiod_set_value(st->sample, 1);
> error_ret:
> mutex_unlock(>lock);
>
> @@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> s16 vel;
>
> mutex_lock(>lock);
> - gpio_set_value(st->pdata->sample, 0);
> + gpiod_set_value(st->sample, 0);
Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
> On 25 Oct 2018, at 08:37, Takashi Iwai wrote: > > On Thu, 25 Oct 2018 00:02:34 +0200, > Kirill Marinushkin wrote: >> When you play sound - the pointer increments. >>> >>> Unfortunately, when you play sound, the pointer does not actually >>> increment, for up to about 10 milliseconds. I know of no way to actually >>> access the true “live” position of the frame that is being played at any >>> instant; hence the desire to estimate it. >>> >> >> Your vision of situation in the opposite from my vision. What you see as a >> symptom - I see as a root cause. As I see, you should fix the >> pointer-not-incrementing. Why do you think that it's okay that the pointer is >> not updating during sound play? Why do you insist that there is a delay? I >> don't >> understand why we are so stuck here. > > Well, in the API POV, it's nothing wrong to keep hwptr sticking while > updating only delay value. It implies that the hardware chip doesn't > provide the hwptr update. > > Though, usually the delay value is provided also from the hardware, > e.g. reading the link position or such. It's a typical case like > USB-audio, where the hwptr gets updated and the delay indicates the > actual position *behind* hwptr. That works because hwptr shows the > position in the ring buffer at which you can access the data. And it > doesn't mean that hwptr is the actually playing position, but it can > be ahead of the current position, when many samples are queued on > FIFO. The delay is provided to correct the position back to the > actual point. > > But, this also doesn't mean that the delay shouldn't be used for the > purpose like this patchset, either. OTOH, providing a finer hwptr > value would be likely more apps-friendly; there must be many programs > that don't evaluate the delay value properly. > > So, I suppose that hwptr update might be a better option if the code > won't become too complex. Let's see. Indeed. It will take me a few days to look into this… Regards Mike > One another thing I'd like to point out is that the value given in the > patch is nothing but an estimated position, optimistically calculated > via the system timer. Mike and I had already discussion in another > thread, and another possible option would be to provide the proper > timestamp-vs-hwptr pair, instead of updating the timestamp always at > the status read. > > Maybe it's worth to have a module option to suppress this optimistic > hwptr update behavior, in case something went wrong with clocks? > > > thanks, > > Takashi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay
Hi Kirill. Thanks for the post. Mike > On 25 Oct 2018, at 18:20, Kirill Marinushkin wrote: > > Hello Takashi, Mike, > > @Takashi > > On 10/25/18 09:37, Takashi Iwai wrote: >> Well, in the API POV, it's nothing wrong to keep hwptr sticking while >> updating only delay value. It implies that the hardware chip doesn't >> provide the hwptr update. > > Thank you for the clarification. Modifying `runtime->delay` from the `pointer` > function looked wrong for me. Now I understand the motivation and the > use-case. > I will be more careful when analyzing the code which doesn't fit my > expectations. > > @Mike > > I was wrong. You can ignore my comments. Please don't take them personal: it's > all about having high-quality code in kernel. > > Best Regards, > Kirill > ___ > Alsa-devel mailing list > alsa-de...@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Which SPDX Identifier for files without explicit GPL version
On Sun, Oct 28, 2018 at 02:22:40PM +0100, Michael Straube wrote: > On 10/28/18 12:27 PM, Greg Kroah-Hartman wrote: > > On Sun, Oct 28, 2018 at 11:04:06AM +0100, Michael Straube wrote: > > > Hi, > > > > > > which GPL version should be used in SPDX Identifiers for files that > > > are GPL licensed but do not mention any version? It is not clear to > > > me after reading license-rules.rst. > > > > > > For example: > > > > > > /** > > > * Copyright(c) 2008 - 2010 Realtek Corporation. All rights reserved. > > > * > > > * This program is distributed in the hope that it will be useful, but > > > WITHOUT > > > * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > > * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > > for > > > * more details. > > > * > > > * The full GNU General Public License is included in this distribution > > > in the > > > * file called LICENSE. > > > > Perhaps look up what version of the GPL that was originally contained in > > the tarball that this driver was bundled with? I think that should be > > on their website somewhere... > > > > If not, it's v1, not a good thing, but all we have to go on. Also, try > > emailing: > > > > > * Contact Information: > > > * wlanfae > > > > That address :) > > > > thanks, > > > > greg k-h > > > > Ah, there is a file 'license' in the root directory of this driver (rtl8192e) > that I overlooked. It contains GPLv2. So v2 applies to all files with the > above > shortened boiler plate? Yes. So add a SPDX line to the file and delete the boiler plate text so we don't have this question again in the future. Then delete that opy of the GPL so we don't have files we don't need :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Which SPDX Identifier for files without explicit GPL version
On 10/28/18 12:27 PM, Greg Kroah-Hartman wrote: On Sun, Oct 28, 2018 at 11:04:06AM +0100, Michael Straube wrote: Hi, which GPL version should be used in SPDX Identifiers for files that are GPL licensed but do not mention any version? It is not clear to me after reading license-rules.rst. For example: /** * Copyright(c) 2008 - 2010 Realtek Corporation. All rights reserved. * * This program is distributed in the hope that it will be useful, but WITHOUT * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for * more details. * * The full GNU General Public License is included in this distribution in the * file called LICENSE. Perhaps look up what version of the GPL that was originally contained in the tarball that this driver was bundled with? I think that should be on their website somewhere... If not, it's v1, not a good thing, but all we have to go on. Also, try emailing: * Contact Information: * wlanfae That address :) thanks, greg k-h Ah, there is a file 'license' in the root directory of this driver (rtl8192e) that I overlooked. It contains GPLv2. So v2 applies to all files with the above shortened boiler plate? Thanks, Michael ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, 28 Oct 2018, Himanshu Jha wrote:
> On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > The "possible alignement issues" in CHECK report is difficult to figure
> > > out by just doing a glance analysis. :)
> > >
> > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> >
> > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > int? But my little experiments suggest that the size is the smallest that
> > fits the requested bits and alignment chosen by the compiler, regardless of
> > the type.
>
> Yes, correct!
> And we can't use sizeof on bitfields *directly*, nor reference it using a
> pointer.
>
> It can be applied only when these bitfields are wrapped in a structure.
>
> Testing:
>
> #include
> #include
>
> struct S {
> bool a:1;
> bool b:1;
> bool c:1;
> bool d:1;
> };
>
> int main(void)
> {
> printf("%zu\n", sizeof(struct S));
> }
>
> Output: 1
>
> If I change all bool to unsigned int, output is: *4*.
>
> So, conclusion is compiler doesn't squeeze the size less than
> native size of the datatype i.e., if we changed all members to
> unsigned int:1,
> total width = 4 bits
> padding = 4 bits
>
> Therefore, total size should have been = 1 byte!
> But since sizeof(unsigned int) == 4, it can't be squeezed to
> less than it.
This conclusion does not seem to be correct, if you try the following
program. I get 4 for everything, meaning that the four unsigned int bits
are getting squeezed into one byte when it is convenient.
#include
#include
struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};
struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};
int main(void)
{
printf("%zu\n", sizeof(struct S1));
printf("%zu\n", sizeof(struct S2));
printf("%zu\n", sizeof(unsigned int));
}
> Well, int x:1 can either have 0..1 or -1..0 range due implementation
> defined behavior as I said in the previous reply.
>
> If you really want to consider negative values, then make it explicit
> using `signed int x:1` which make range guaranteed to be -1..0
The code wants booleans, not negative values.
julia
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Which SPDX Identifier for files without explicit GPL version
On Sun, Oct 28, 2018 at 11:04:06AM +0100, Michael Straube wrote: > Hi, > > which GPL version should be used in SPDX Identifiers for files that > are GPL licensed but do not mention any version? It is not clear to > me after reading license-rules.rst. > > For example: > > /** > * Copyright(c) 2008 - 2010 Realtek Corporation. All rights reserved. > * > * This program is distributed in the hope that it will be useful, but WITHOUT > * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > * more details. > * > * The full GNU General Public License is included in this distribution in the > * file called LICENSE. Perhaps look up what version of the GPL that was originally contained in the tarball that this driver was bundled with? I think that should be on their website somewhere... If not, it's v1, not a good thing, but all we have to go on. Also, try emailing: > * Contact Information: > * wlanfae That address :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > The "possible alignement issues" in CHECK report is difficult to figure
> > out by just doing a glance analysis. :)
> >
> > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
>
> If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> int? But my little experiments suggest that the size is the smallest that
> fits the requested bits and alignment chosen by the compiler, regardless of
> the type.
Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a
pointer.
It can be applied only when these bitfields are wrapped in a structure.
Testing:
#include
#include
struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};
int main(void)
{
printf("%zu\n", sizeof(struct S));
}
Output: 1
If I change all bool to unsigned int, output is: *4*.
So, conclusion is compiler doesn't squeeze the size less than
native size of the datatype i.e., if we changed all members to
unsigned int:1,
total width = 4 bits
padding = 4 bits
Therefore, total size should have been = 1 byte!
But since sizeof(unsigned int) == 4, it can't be squeezed to
less than it.
> bool x:1 has the advantage that anything that is not 0 is considered true.
Yes, implicit conversion rules for boolean.
> So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.
Well, int x:1 can either have 0..1 or -1..0 range due implementation
defined behavior as I said in the previous reply.
If you really want to consider negative values, then make it explicit
using `signed int x:1` which make range guaranteed to be -1..0
Regardless, integer conversion rules will kick in.
> But the :1 adds instructions, so at least for only one bool, where little
> space is saved, it is probably not worth it.
True, we should reply on a promised guideline rather than possibility.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH 1/2] staging: vboxvideo: Change uint32_t to u32
Hi,
On 26-10-18 21:04, Shayenne da Luz Moura wrote:
This change was suggested by checkpath.pl.
CHECK: Prefer kernel type 'u32' over 'uint32_t'
Signed-off-by: Shayenne da Luz Moura
So as already mentioned in response to the coverletter of
the first posting of this series.
The drm headers use uint32_t in the prototype definition of the callback
functions we are defining, so the vboxvideo code should use the same even
if the compiler does not warn about the callback implementation having
different parameter types in this case.
To be precise, the page_flip member of struct drm_crtc_funcs is defined as:
int (*page_flip)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event,
uint32_t flags,
struct drm_modeset_acquire_ctx *ctx);
So our implementation before your patch exactly matches the prototype
which IMHO is the right thing to do.
Regards,
Hans
---
drivers/staging/vboxvideo/vbox_mode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vboxvideo/vbox_mode.c
b/drivers/staging/vboxvideo/vbox_mode.c
index 79836c8fb909..8a1b117990b8 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -311,7 +311,7 @@ static int vbox_crtc_mode_set(struct drm_crtc *crtc,
static int vbox_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event,
- uint32_t page_flip_flags,
+ u32 page_flip_flags,
struct drm_modeset_acquire_ctx *ctx)
{
struct vbox_private *vbox = crtc->dev->dev_private;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Which SPDX Identifier for files without explicit GPL version
Hi, which GPL version should be used in SPDX Identifiers for files that are GPL licensed but do not mention any version? It is not clear to me after reading license-rules.rst. For example: /** * Copyright(c) 2008 - 2010 Realtek Corporation. All rights reserved. * * This program is distributed in the hope that it will be useful, but WITHOUT * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for * more details. * * The full GNU General Public License is included in this distribution in the * file called LICENSE. * * Contact Information: * wlanfae */ or: /* IEEE 802.11 SoftMAC layer * Copyright (c) 2005 Andrea Merello * * Mostly extracted from the rtl8180-sa2400 driver for the * in-kernel generic ieee802.11 stack. * * Some pieces of code might be stolen from ipw2100 driver * copyright of who own it's copyright ;-) * * PS wx handler mostly stolen from hostap, copyright who * own it's copyright ;-) * * released under the GPL */ Should it be GPL-2.0 ? Regards, Michael ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] staging: rtl8188eu: change return type of rtw_hal_xmit()
On 10/28/18 1:08 AM, Joe Perches wrote:
On Sat, 2018-10-27 at 15:57 -0700, Joe Perches wrote:
On Sat, 2018-10-27 at 22:28 +0200, Michael Straube wrote:
The function rtw_hal_xmit() returns true or false.
Change the return type from s32 to bool.
[]
diff --git a/drivers/staging/rtl8188eu/hal/rtl8188eu_xmit.c
b/drivers/staging/rtl8188eu/hal/rtl8188eu_xmit.c
[]
@@ -598,7 +598,7 @@ bool rtl8188eu_xmitframe_complete(struct adapter *adapt,
*truedump packet directly
*false enqueue packet
*/
-s32 rtw_hal_xmit(struct adapter *adapt, struct xmit_frame *pxmitframe)
+bool rtw_hal_xmit(struct adapter *adapt, struct xmit_frame *pxmitframe)
{
s32 res;
Does "s32 res" need changing to bool too?
Perhaps all the functions regardless of types
with returns of only _SUCCESS and _FAIL could be
converted to bool.
"s32 res" is not used for return value, so it does not need changing to bool.
But it could be converted too.
I'll keep that, and converting functions only returning _SUCCESS and _FAIL to
bool, in mind for future patches. Thanks.
Perhaps _SUCCESS / _FAIL could be replaced with true / false throughout the
driver to get rid of the defines? Or is that a bad idea?
Regards,
Michael
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
> The "possible alignement issues" in CHECK report is difficult to figure > out by just doing a glance analysis. :) > > Linus also suggested to use bool as the base type i.e., `bool x:1` but > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes. If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type. bool x:1 has the advantage that anything that is not 0 is considered true. So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false. But the :1 adds instructions, so at least for only one bool, where little space is saved, it is probably not worth it. julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/7] staging: vc04_services: Some dead code removal
Hi Dave, > Dave Stevenson hat am 26. Oktober 2018 um > 19:15 geschrieben: > > > Thanks Stefan. > I've picked up your latest patches which mean I can get the driver > loaded via the (almost) approved method. > I do seem to still have issues with not getting the expected address > ranges, so the driver/VPU was trying to map cached alias memory. As > your patches only came through yesterday I haven't had a chance to dig > through why yet. I've done a temporary hack to ensure we always map > the uncached alias, but that can't persist. does it mean with DT probing it worked before and with platform change it's broken? Or anything else cause this regression in 4.19? > The networking issue has been resolved :-) > > I've pushed where I've got to to > https://github.com/6by9/linux/tree/rpi-4.14.y-codecs-push-pt2b > It's a touch messy due to integrating in your patches in the last 24 > hours. It needs a full rebase so that my changes are on top of yours > rather than haphazard. > As we're moving to 4.19 fairly soon I may well abandon my 4.14 tree > and jump to either that or directly on staging. I'll see where I get > to early next week. Sorry, but there is no need for a quick shot against a downstream 4.14. I assumed you make your changes against upstream linux-next + Phil's and my patches. You can use https://github.com/anholt/linux/commits/bcm2835-audio until 4.20-rc1 is out. Using 4.14 or 4.19 doesn't make any sense to me. Regards Stefan > > Dave ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.
Replace platform data with device tree support.
Signed-off-by: Nishad Kamdar
---
drivers/staging/iio/resolver/Kconfig| 1 +
drivers/staging/iio/resolver/ad2s1210.c | 17 -
drivers/staging/iio/resolver/ad2s1210.h | 17 -
3 files changed, 9 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/iio/resolver/Kconfig
b/drivers/staging/iio/resolver/Kconfig
index 6a469ee6101f..cc1202ff813d 100644
--- a/drivers/staging/iio/resolver/Kconfig
+++ b/drivers/staging/iio/resolver/Kconfig
@@ -15,6 +15,7 @@ config AD2S90
config AD2S1210
tristate "Analog Devices ad2s1210 driver"
+ depends on OF
depends on SPI
depends on GPIOLIB || COMPILE_TEST
help
diff --git a/drivers/staging/iio/resolver/ad2s1210.c
b/drivers/staging/iio/resolver/ad2s1210.c
index 0234869e9d74..5ecdb5785132 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -76,7 +77,6 @@ struct ad2s1210_gpio {
};
struct ad2s1210_state {
- const struct ad2s1210_platform_data *pdata;
struct mutex lock;
struct spi_device *sdev;
struct gpio_desc *sample;
@@ -84,6 +84,7 @@ struct ad2s1210_state {
struct gpio_desc *a1;
struct gpio_desc *res0;
struct gpio_desc *res1;
+ bool gpioin;
unsigned int fclkin;
unsigned int fexcit;
bool hysteresis;
@@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
}
st->resolution
= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
- if (st->pdata->gpioin) {
+ if (st->gpioin) {
data = ad2s1210_read_resolution_pin(st);
if (data != st->resolution)
dev_warn(dev, "ad2s1210: resolution settings not
match\n");
@@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
}
st->resolution
= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
- if (st->pdata->gpioin) {
+ if (st->gpioin) {
data = ad2s1210_read_resolution_pin(st);
if (data != st->resolution)
dev_warn(dev, "ad2s1210: resolution settings not
match\n");
@@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
int ret;
mutex_lock(>lock);
- if (st->pdata->gpioin)
+ if (st->gpioin)
st->resolution = ad2s1210_read_resolution_pin(st);
else
ad2s1210_set_resolution_pin(st);
@@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
int ret, i;
struct spi_device *spi = st->sdev;
struct ad2s1210_gpio *pin;
- unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
+ unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
struct ad2s1210_gpio gpios[] = {
{ .ptr = >sample, .name = "sample", .flags = GPIOD_IN },
@@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
{
struct iio_dev *indio_dev;
struct ad2s1210_state *st;
+ struct device_node *np = spi->dev.of_node;
int ret;
- if (!spi->dev.platform_data)
- return -EINVAL;
-
indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
st = iio_priv(indio_dev);
- st->pdata = spi->dev.platform_data;
+ st->gpioin = of_property_read_bool(np, "gpioin");
ret = ad2s1210_setup_gpios(st);
if (ret < 0)
return ret;
diff --git a/drivers/staging/iio/resolver/ad2s1210.h
b/drivers/staging/iio/resolver/ad2s1210.h
index 63d479b20a6c..e69de29bb2d1 100644
--- a/drivers/staging/iio/resolver/ad2s1210.h
+++ b/drivers/staging/iio/resolver/ad2s1210.h
@@ -1,17 +0,0 @@
-/*
- * ad2s1210.h plaform data for the ADI Resolver to Digital Converters:
- * AD2S1210
- *
- * Copyright (c) 2010-2010 Analog Devices Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#ifndef _AD2S1210_H
-#define _AD2S1210_H
-
-struct ad2s1210_platform_data {
- boolgpioin;
-};
-#endif /* _AD2S1210_H */
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 2/3] staging: iio: ad2s1210: Add device tree table.
Add device tree table for matching vendor ID.
Signed-off-by: Nishad Kamdar
---
drivers/staging/iio/resolver/ad2s1210.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c
b/drivers/staging/iio/resolver/ad2s1210.c
index 93c3c70ce62e..0234869e9d74 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -724,6 +724,12 @@ static int ad2s1210_remove(struct spi_device *spi)
return 0;
}
+static const struct of_device_id ad2s1210_of_match[] = {
+ { .compatible = "adi,ad2s1210", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
+
static const struct spi_device_id ad2s1210_id[] = {
{ "ad2s1210" },
{}
@@ -733,6 +739,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id);
static struct spi_driver ad2s1210_driver = {
.driver = {
.name = DRV_NAME,
+ .of_match_table = of_match_ptr(ad2s1210_of_match),
},
.probe = ad2s1210_probe,
.remove = ad2s1210_remove,
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi Sasha,
On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
Agreed!
FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207
As Steve says:
"Thus, changing:
int a : 1;
int b : 1;
int c : 1;
int d : 1;
to
bool a;
bool b;
bool c;
bool d;
at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."
In the above cases, we have all bitfields members and no non-bitfields
members.
But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9
Implementation Defined
--
* Whether a ''plain'' int bit-field is treated as a signed int
bit-field or as an unsigned int bit-field.
eg: `int foo:3` may have a range from 0..7 or -4..3
So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.
Also, such an change can also handle unsigned overflow
or better said "wrapping".
* Whether a bit-field can straddle a storage-unit boundary.
So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.
* The alignment of non-bit-field members of structures.
...
The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)
Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.
Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.
But if anyone is interested check this:
https://godbolt.org/
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
Use the gpiod interface instead of the deprecated old non-descriptor
interface.
Signed-off-by: Nishad Kamdar
---
drivers/staging/iio/resolver/ad2s1210.c | 92 ++---
drivers/staging/iio/resolver/ad2s1210.h | 3 -
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c
b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..93c3c70ce62e 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -15,7 +15,7 @@
#include
#include
#include
-#include
+#include
#include
#include
@@ -69,10 +69,21 @@ enum ad2s1210_mode {
static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
+struct ad2s1210_gpio {
+ struct gpio_desc **ptr;
+ const char *name;
+ unsigned long flags;
+};
+
struct ad2s1210_state {
const struct ad2s1210_platform_data *pdata;
struct mutex lock;
struct spi_device *sdev;
+ struct gpio_desc *sample;
+ struct gpio_desc *a0;
+ struct gpio_desc *a1;
+ struct gpio_desc *res0;
+ struct gpio_desc *res1;
unsigned int fclkin;
unsigned int fexcit;
bool hysteresis;
@@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
struct ad2s1210_state *st)
{
- gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
- gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
+ gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
+ gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
st->mode = mode;
}
@@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct
ad2s1210_state *st)
static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
{
- int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
- gpio_get_value(st->pdata->res[1]);
+ int resolution = (gpiod_get_value(st->res0) << 1) |
+ gpiod_get_value(st->res1);
return ad2s1210_resolution_value[resolution];
}
@@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
{
- gpio_set_value(st->pdata->res[0],
- ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
- gpio_set_value(st->pdata->res[1],
- ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
+ gpiod_set_value(st->res0,
+ ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
+ gpiod_set_value(st->res1,
+ ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
}
static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
@@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
int ret;
mutex_lock(>lock);
- gpio_set_value(st->pdata->sample, 0);
+ gpiod_set_value(st->sample, 0);
/* delay (2 * tck + 20) nano seconds */
udelay(1);
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 1);
ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
if (ret < 0)
goto error_ret;
- gpio_set_value(st->pdata->sample, 0);
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 0);
+ gpiod_set_value(st->sample, 1);
error_ret:
mutex_unlock(>lock);
@@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
s16 vel;
mutex_lock(>lock);
- gpio_set_value(st->pdata->sample, 0);
+ gpiod_set_value(st->sample, 0);
/* delay (6 * tck + 20) nano seconds */
udelay(1);
@@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
}
error_ret:
- gpio_set_value(st->pdata->sample, 1);
+ gpiod_set_value(st->sample, 1);
/* delay (2 * tck + 20) nano seconds */
udelay(1);
mutex_unlock(>lock);
@@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
{
- unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
- struct gpio ad2s1210_gpios[] = {
- { st->pdata->sample, GPIOF_DIR_IN, "sample" },
- { st->pdata->a[0], flags, "a0" },
- { st->pdata->a[1], flags, "a1" },
- { st->pdata->res[0], flags, "res0" },
- { st->pdata->res[0], flags, "res1" },
+ int ret, i;
+ struct spi_device *spi = st->sdev;
+ struct ad2s1210_gpio *pin;
+ unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
+
+ struct ad2s1210_gpio gpios[] = {
+ { .ptr = >sample, .name = "sample", .flags = GPIOD_IN },
+ { .ptr = >a0, .name = "a0", .flags = flags },
+
[PATCH v6 0/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface.
Use the gpiod interface instead of the deprecated old non-descriptor
Changes in v6:
- Split device tree table addition and device tree support
addition in two patches.
- Replace platform data with device tree support.
- Rename boolean property.
Changes in v5:
- Add device tree support.
- Add device tree table for matching vendor ID.
- Add Support for retrieving platform data from device tree.
Changes in v4:
- Add spaces after { and before } in gpios[]
initialization.
- Check the correct pointer for error.
- Align the dev_err msg to existing format in the code.
Changes in v3:
- Use a pointer to pointer for gpio_desc in
struct ad2s1210_gpio as it will be used to
modify a pointer.
- Use dot notation to initialize the structure.
- Use a pointer variable to avoid writing gpios[i].
Changes in v2:
- Use the spi_device struct embedded in st instead
of passing it as an argument to ad2s1210_setup_gpios().
- Use an array of structs to reduce redundant code in
in ad2s1210_setup_gpios().
- Remove ad2s1210_free_gpios() as devm API is being used.
-
Nishad Kamdar (3):
staging: iio: ad2s1210: Switch to the gpio descriptor interface
staging: iio: ad2s1210: Add device tree table.
staging: iio: ad2s1210: Add device tree support.
drivers/staging/iio/resolver/Kconfig| 1 +
drivers/staging/iio/resolver/ad2s1210.c | 114 +---
drivers/staging/iio/resolver/ad2s1210.h | 20 -
3 files changed, 65 insertions(+), 70 deletions(-)
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] staging: rtl8188eu: change return type of rtw_hal_xmit()
On Sat, 2018-10-27 at 15:57 -0700, Joe Perches wrote:
> On Sat, 2018-10-27 at 22:28 +0200, Michael Straube wrote:
> > The function rtw_hal_xmit() returns true or false.
> > Change the return type from s32 to bool.
> []
> > diff --git a/drivers/staging/rtl8188eu/hal/rtl8188eu_xmit.c
> > b/drivers/staging/rtl8188eu/hal/rtl8188eu_xmit.c
> []
> > @@ -598,7 +598,7 @@ bool rtl8188eu_xmitframe_complete(struct adapter *adapt,
> > * truedump packet directly
> > * false enqueue packet
> > */
> > -s32 rtw_hal_xmit(struct adapter *adapt, struct xmit_frame *pxmitframe)
> > +bool rtw_hal_xmit(struct adapter *adapt, struct xmit_frame *pxmitframe)
> > {
> > s32 res;
>
> Does "s32 res" need changing to bool too?
Perhaps all the functions regardless of types
with returns of only _SUCCESS and _FAIL could be
converted to bool.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: olpc_dcon: olpc_dcon_xo_1.c: Switch to the gpio descriptor interface
Hi Nishad,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.19 next-20181019]
[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/Nishad-Kamdar/staging-olpc_dcon-olpc_dcon_xo_1-c-Switch-to-the-gpio-descriptor-interface/20181028-124517
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/staging/olpc_dcon/olpc_dcon_xo_1.c: In function 'dcon_init_xo_1':
>> drivers/staging/olpc_dcon/olpc_dcon_xo_1.c:37:43: error: dereferencing
>> pointer to incomplete type 'struct backlight_device'
*pin->ptr = devm_gpiod_get(>bl_dev->dev, pin->name,
^~
>> drivers/staging/olpc_dcon/olpc_dcon_xo_1.c:41:4: error: implicit declaration
>> of function 'dev_err'; did you mean 'pr_err'?
>> [-Werror=implicit-function-declaration]
dev_err(>bl_dev->dev,
^~~
pr_err
>> drivers/staging/olpc_dcon/olpc_dcon_xo_1.c:89:3: error: label 'err_req_irq'
>> used but not defined
goto err_req_irq;
^~~~
cc1: some warnings being treated as errors
vim +37 drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
19
20 static int dcon_init_xo_1(struct dcon_priv *dcon)
21 {
22 unsigned char lob;
23 int ret, i;
24 struct dcon_gpio *pin;
25 unsigned long flags = GPIOD_ASIS;
26
27 struct dcon_gpio gpios[] = {
28 { .ptr = _stat0, .name = "dcon_stat0", .flags =
flags },
29 { .ptr = _stat1, .name = "dcon_stat1", .flags =
flags },
30 { .ptr = _irq, .name = "dcon_irq", .flags = flags
},
31 { .ptr = _load, .name = "dcon_load", .flags =
flags },
32 { .ptr = _blank, .name = "dcon_blank", .flags =
flags },
33 };
34
35 for (i = 0; i < ARRAY_SIZE(gpios); i++) {
36 pin = [i];
> 37 *pin->ptr = devm_gpiod_get(>bl_dev->dev,
pin->name,
38 pin->flags);
39 if (IS_ERR(*pin->ptr)) {
40 ret = PTR_ERR(*pin->ptr);
> 41 dev_err(>bl_dev->dev,
42 "failed to request %s GPIO: %d\n",
43 pin->name, ret);
44 return ret;
45 }
46 }
47
48 /* Turn off the event enable for GPIO7 just to be safe */
49 cs5535_gpio_clear(OLPC_GPIO_DCON_IRQ, GPIO_EVENTS_ENABLE);
50
51 /*
52 * Determine the current state by reading the GPIO bit; earlier
53 * stages of the boot process have established the state.
54 *
55 * Note that we read GPIO_OUTPUT_VAL rather than GPIO_READ_BACK
here;
56 * this is because OFW will disable input for the pin and set a
value..
57 * READ_BACK will only contain a valid value if input is
enabled and
58 * then a value is set. So, future readings of the pin can use
59 * READ_BACK, but the first one cannot. Awesome, huh?
60 */
61 dcon->curr_src = cs5535_gpio_isset(OLPC_GPIO_DCON_LOAD,
GPIO_OUTPUT_VAL)
62 ? DCON_SOURCE_CPU
63 : DCON_SOURCE_DCON;
64 dcon->pending_src = dcon->curr_src;
65
66 /* Set the directions for the GPIO pins */
67 gpiod_direction_input(dcon_stat0);
68 gpiod_direction_input(dcon_stat1);
69 gpiod_direction_input(dcon_irq);
70 gpiod_direction_input(dcon_blank);
71 gpiod_direction_output(dcon_load, dcon->curr_src ==
DCON_SOURCE_CPU);
72
73 /* Set up the interrupt mappings */
74
75 /* Set the IRQ to pair 2 */
76 cs5535_gpio_setup_event(OLPC_GPIO_DCON_IRQ, 2, 0);
77
78 /* Enable group 2 to trigger the DCON interrupt */
79 cs5535_gpio_set_irq(2, DCON_IRQ);
80
81 /* Select edge level for interrupt (in PIC) */
82 lob = inb(0x4d0);
83 lob &= ~(1 << DCON_IRQ);
84 outb(lob, 0x4d0);
85
86 /* Register the interrupt handler */
87 if (request_irq(DCON_IRQ, _interrupt, 0, "DCON", dcon)) {
88 pr_err("failed t
Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.
On Sat, Oct 27, 2018 at 05:49:03PM +0200, Slawomir Stepien wrote:
> Hi
>
> On paź 26, 2018 18:55, Nishad Kamdar wrote:
> > Add device tree table for matching vendor ID
> > and support for retrieving platform data
> > from device tree.
>
> So maybe you should make 2 commits?
>
Ok. I'll do that.
> > Signed-off-by: Nishad Kamdar
> > ---
> > drivers/staging/iio/resolver/ad2s1210.c | 43 -
> > 1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index 93c3c70ce62e..9fd5461c4ed0 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state
> > *st)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_OF
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct ad2s1210_platform_data *pdata;
> > +
> > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return NULL;
> > +
> > + pdata->gpioin = of_property_read_bool(np, "adi,gpioin");
>
> Why here you are adding "adi", but you are not adding it to the gpios in prev
> commit?
>
> I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I
> do
> not understand why adding vendor id to props is needed...
>
> > +
> > + return pdata;
> > +}
> > +#else
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > +#endif
> > +
> > static int ad2s1210_probe(struct spi_device *spi)
> > {
> > struct iio_dev *indio_dev;
> > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi)
> > if (!indio_dev)
> > return -ENOMEM;
> > st = iio_priv(indio_dev);
> > - st->pdata = spi->dev.platform_data;
> > + if (spi->dev.of_node) {
> > + st->pdata = ad2s1210_parse_dt(>dev);
> > + if (!st->pdata)
> > + return -EINVAL;
> > + } else {
> > + st->pdata = spi->dev.platform_data;
> > + }
> > +
> > + if (!st->pdata) {
> > + dev_err(>dev, "ad2s1210: no platform data supplied\n");
> > + return -EINVAL;
> > + }
> > +
>
> Why not just use only device-tree here? The ad2s1210_platform_data has now
> only
> one entry... In that case remember to add "depends on OF" in Kconfig.
>
Ok. I'll do that.
> > ret = ad2s1210_setup_gpios(st);
> > if (ret < 0)
> > return ret;
> > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi)
> > return 0;
> > }
> >
> > +static const struct of_device_id ad2s1210_of_match[] = {
> > + { .compatible = "adi,ad2s1210", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
> > +
> > static const struct spi_device_id ad2s1210_id[] = {
> > { "ad2s1210" },
> > {}
> > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id);
> > static struct spi_driver ad2s1210_driver = {
> > .driver = {
> > .name = DRV_NAME,
> > + .of_match_table = of_match_ptr(ad2s1210_of_match),
> > },
> > .probe = ad2s1210_probe,
> > .remove = ad2s1210_remove,
>
> --
> Slawomir Stepien
Thanks for the review.
Regards,
Nishad
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
