* Re: [PATCH linux 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: kbuild test robot @ 2017-01-01 0:25 UTC (permalink / raw)
To: eajames.ibm
Cc: kbuild-all, linux, jdelvare, corbet, mark.rutland, robh+dt, wsa,
andrew, joel, devicetree, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, Edward A. James
In-Reply-To: <1483120568-21082-6-git-send-email-eajames.ibm@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]
Hi Edward,
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.10-rc1 next-20161224]
[cannot apply to linux/master]
[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/eajames-ibm-gmail-com/drivers-hwmon-Add-On-Chip-Controller-driver/20161231-021324
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-n0-01010656 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "occ_sysfs_start" [drivers/hwmon/occ/p8_occ_i2c.ko] undefined!
>> ERROR: "occ_sysfs_stop" [drivers/hwmon/occ/p8_occ_i2c.ko] undefined!
>> ERROR: "occ_stop" [drivers/hwmon/occ/occ_p8.ko] undefined!
>> ERROR: "occ_start" [drivers/hwmon/occ/occ_p8.ko] undefined!
>> ERROR: "occ_get_sensor" [drivers/hwmon/occ/occ_p8.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21665 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/7] i2c: designware: add SLAVE mode functions
From: kbuild test robot @ 2016-12-31 23:45 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <15538bd29b3ab62608a4e9153af6ee5d4bdc79e2.1482934380.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]
Hi Luis,
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.10-rc1 next-20161224]
[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/Luis-Oliveira/i2c-designware-Cleaning-and-comment-style-fixes/20161229-010120
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-c0-01010626 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/i2c/busses/i2c-designware-slave.c: In function 'i2c_dw_irq_handler_slave':
>> drivers/i2c/busses/i2c-designware-slave.c:293:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
^
>> drivers/i2c/busses/i2c-designware-slave.c:293:31: error: 'I2C_SLAVE_WRITE_REQUESTED' undeclared (first use in this function)
i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
^
drivers/i2c/busses/i2c-designware-slave.c:293:31: note: each undeclared identifier is reported only once for each function it appears in
In file included from include/linux/err.h:4:0,
from drivers/i2c/busses/i2c-designware-slave.c:23:
>> drivers/i2c/busses/i2c-designware-slave.c:300:6: error: 'I2C_SLAVE_WRITE_RECEIVED' undeclared (first use in this function)
I2C_SLAVE_WRITE_RECEIVED, &val)) {
^
include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
drivers/i2c/busses/i2c-designware-slave.c:299:5: note: in expansion of macro 'if'
if (!i2c_slave_event(dev->slave,
^
>> drivers/i2c/busses/i2c-designware-slave.c:312:7: error: 'I2C_SLAVE_READ_REQUESTED' undeclared (first use in this function)
I2C_SLAVE_READ_REQUESTED, &val))
^
include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
drivers/i2c/busses/i2c-designware-slave.c:311:4: note: in expansion of macro 'if'
if (!i2c_slave_event(dev->slave,
^
>> drivers/i2c/busses/i2c-designware-slave.c:318:36: error: 'I2C_SLAVE_READ_PROCESSED' undeclared (first use in this function)
if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
^
include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
drivers/i2c/busses/i2c-designware-slave.c:318:3: note: in expansion of macro 'if'
if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
^
>> drivers/i2c/busses/i2c-designware-slave.c:322:31: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
^
drivers/i2c/busses/i2c-designware-slave.c: At top level:
>> drivers/i2c/busses/i2c-designware-slave.c:359:2: error: unknown field 'reg_slave' specified in initializer
.reg_slave = i2c_dw_reg_slave,
^
drivers/i2c/busses/i2c-designware-slave.c:359:2: warning: excess elements in struct initializer
drivers/i2c/busses/i2c-designware-slave.c:359:2: warning: (near initialization for 'i2c_dw_algo')
>> drivers/i2c/busses/i2c-designware-slave.c:360:2: error: unknown field 'unreg_slave' specified in initializer
.unreg_slave = i2c_dw_unreg_slave,
^
drivers/i2c/busses/i2c-designware-slave.c:360:2: warning: excess elements in struct initializer
drivers/i2c/busses/i2c-designware-slave.c:360:2: warning: (near initialization for 'i2c_dw_algo')
cc1: some warnings being treated as errors
vim +/i2c_slave_event +293 drivers/i2c/busses/i2c-designware-slave.c
287 dw_readl(dev, DW_IC_CLR_START_DET);
288 if (stat & DW_IC_INTR_ACTIVITY)
289 dw_readl(dev, DW_IC_CLR_ACTIVITY);
290 if (stat & DW_IC_INTR_RX_OVER)
291 dw_readl(dev, DW_IC_CLR_RX_OVER);
292 if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
> 293 i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
294
295 if (slave_activity) {
296 if (stat & DW_IC_INTR_RD_REQ) {
297 if (stat & DW_IC_INTR_RX_FULL) {
298 val = dw_readl(dev, DW_IC_DATA_CMD);
299 if (!i2c_slave_event(dev->slave,
> 300 I2C_SLAVE_WRITE_RECEIVED, &val)) {
301 dev_dbg(dev->dev, "Byte %X acked!",
302 val);
303 }
304 dw_readl(dev, DW_IC_CLR_RD_REQ);
305 stat = i2c_dw_read_clear_intrbits_slave(dev);
306 } else {
307 dw_readl(dev, DW_IC_CLR_RD_REQ);
308 dw_readl(dev, DW_IC_CLR_RX_UNDER);
309 stat = i2c_dw_read_clear_intrbits_slave(dev);
310 }
311 if (!i2c_slave_event(dev->slave,
> 312 I2C_SLAVE_READ_REQUESTED, &val))
313 dw_writel(dev, val, DW_IC_DATA_CMD);
314 }
315 }
316
317 if (stat & DW_IC_INTR_RX_DONE) {
> 318 if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
319 &val))
320 dw_readl(dev, DW_IC_CLR_RX_DONE);
321
> 322 i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
323 stat = i2c_dw_read_clear_intrbits_slave(dev);
324 return true;
325 }
326
327 if (stat & DW_IC_INTR_RX_FULL) {
328 val = dw_readl(dev, DW_IC_DATA_CMD);
329 if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
330 &val))
331 dev_dbg(dev->dev, "Byte %X acked!", val);
332 } else {
333 i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
334 stat = i2c_dw_read_clear_intrbits_slave(dev);
335 }
336
337 if (stat & DW_IC_INTR_TX_OVER)
338 dw_readl(dev, DW_IC_CLR_TX_OVER);
339
340 return 1;
341 }
342
343 static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
344 {
345 struct dw_i2c_dev *dev = dev_id;
346 int ret;
347
348 i2c_dw_read_clear_intrbits_slave(dev);
349 ret = i2c_dw_irq_handler_slave(dev);
350
351 if (ret > 0)
352 complete(&dev->cmd_complete);
353
354 return IRQ_RETVAL(ret);
355 }
356
357 static struct i2c_algorithm i2c_dw_algo = {
358 .functionality = i2c_dw_func,
> 359 .reg_slave = i2c_dw_reg_slave,
> 360 .unreg_slave = i2c_dw_unreg_slave,
361 };
362
363 void i2c_dw_disable_slave(struct dw_i2c_dev *dev)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27840 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore
From: Hans de Goede @ 2016-12-31 21:29 UTC (permalink / raw)
To: Len Brown, jani.nikula
Cc: Hans de Goede, Wolfram Sang, Takashi Iwai,
russianneuromancer @ ya . ru, intel-gfx, tagore.chandan,
Vincent Gerris, Jarkko Nikula, linux-i2c, Andy Shevchenko,
Mika Westerberg
In-Reply-To: <e415e26c-8781-6ce1-c0f9-a68249e542d3@redhat.com>
Hi,
On 26-12-16 12:07, Hans de Goede wrote:
> Hi,
>
> On 25-12-16 19:31, Len Brown wrote:
>> Is there a simple way to run a test to keep deep C-states
>> and instead disable part or all of i2c on this platform,
>> to see how much stability separating the two will buy us?
>
> This should do the trick to completely disable i2c from the kernel
> to the pmic, leaving the bus fully free for the punit:
>
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 1590ad0..fe73271 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -140,6 +140,7 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
>
> if (shared_host) {
> dev_info(dev->dev, "I2C bus managed by PUNIT\n");
> + return -ENODEV;
> dev->acquire_lock = baytrail_i2c_acquire;
> dev->release_lock = baytrail_i2c_release;
> dev->pm_runtime_disabled = true;
>
> Note that my patch only disabled deep C-states while the kernel
> is accessing the pmic i2c bus, not all the time as most other
> workarounds are doing.
Ok, some more very interesting input on this, from the bug
about the PUNIT semaphore issues on cherrytrail:
https://bugzilla.kernel.org/show_bug.cgi?id=155241#c37
"My device is a laptop with no USB charging or OTG. So, I'd tried only SDIO _ADR patch, i2c and axp288_fuel_gauge patches. Everything works well for upto 3 mins after boot, then the device freezes. I hadn't tried any drm patches BTW. Here's the log:
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
clocksource: 'refined-jiffies' wd_now: 10002ee30 wd_last: 10002edb8 mask: ffffffff
clocksource: 'tsc' cs_now: 16ac2c7744a cs_last: 16a8d9bd8f2 mask: ffffffffffffffff
clocksource: Switched to clocksource refined-jiffies
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: axp288 reg read err:-110
axp288_fuel_gauge axp288_fuel_gauge: PWR STAT read failed:-110
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 0
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: IIO channel read error: fffffffb, 0
power_supply axp288_fuel_gauge: driver failed to report `voltage_now' property: -5
***SYSTEM FREEZE***
If I blacklist axp288_fuel_gauge, then there were no errors."
So it seems that not only bad things happen when the punit tries to
change the cpu C-state while we're holding the pmic i2c bus
semaphore, but that similar bad things happen when the gpu code
tries to acquire / release a forcewake lock on the GPU while
we're accessing the pmic i2c bus.
This makes sense, the iosf_mbi code which is used by the
i2c bus semaphore code has this:
arch/x86/platform/intel/iosf_mbi.c:
int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
{
u32 mcr, mcrx;
unsigned long flags;
int ret;
/* Access to the GFX unit is handled by GPU code */
if (port == BT_MBI_UNIT_GFX) {
WARN_ON(1);
return -EPERM;
}
...
}
So the i915 driver definitely is interacting with the punit
through the mailbox interface too...
I'll try to write a quick and dirty patch where the i915 code
simply calls intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
an extra time on probe, and ask the user who is seeing this to
test.
Regards,
Hans
code calls
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v6 5/9] iio: multiplexer: new iio category and iio-mux driver
From: Jonathan Cameron @ 2016-12-31 16:21 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <1480493823-21462-6-git-send-email-peda@axentia.se>
On 30/11/16 08:16, Peter Rosin wrote:
> When a multiplexer changes how an iio device behaves (for example
> by feeding different signals to an ADC), this driver can be used
> to create one virtual iio channel for each multiplexer state.
>
> Depends on the generic multiplexer subsystem.
>
> Cache any ext_info values from the parent iio channel, creating a private
> copy of the ext_info attributes for each multiplexer state/channel.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Other than binding naming, I'm happy with this - so pending that changing...
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
If it makes sense I can obviously take this via IIO once the core is in
place.
Jonathan
> ---
> MAINTAINERS | 1 +
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/multiplexer/Kconfig | 18 ++
> drivers/iio/multiplexer/Makefile | 6 +
> drivers/iio/multiplexer/iio-mux.c | 456 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 483 insertions(+)
> create mode 100644 drivers/iio/multiplexer/Kconfig
> create mode 100644 drivers/iio/multiplexer/Makefile
> create mode 100644 drivers/iio/multiplexer/iio-mux.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77045ae15865..16490fbd1721 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6239,6 +6239,7 @@ M: Peter Rosin <peda@axentia.se>
> L: linux-iio@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
> +F: drivers/iio/multiplexer/iio-mux.c
>
> IIO SUBSYSTEM AND DRIVERS
> M: Jonathan Cameron <jic23@kernel.org>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index a918270d6f54..b3c8c6ef0dff 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -83,6 +83,7 @@ source "drivers/iio/humidity/Kconfig"
> source "drivers/iio/imu/Kconfig"
> source "drivers/iio/light/Kconfig"
> source "drivers/iio/magnetometer/Kconfig"
> +source "drivers/iio/multiplexer/Kconfig"
> source "drivers/iio/orientation/Kconfig"
> if IIO_TRIGGER
> source "drivers/iio/trigger/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 33fa4026f92c..93c769cd99bf 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -28,6 +28,7 @@ obj-y += humidity/
> obj-y += imu/
> obj-y += light/
> obj-y += magnetometer/
> +obj-y += multiplexer/
> obj-y += orientation/
> obj-y += potentiometer/
> obj-y += potentiostat/
> diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
> new file mode 100644
> index 000000000000..70a044510686
> --- /dev/null
> +++ b/drivers/iio/multiplexer/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Multiplexer drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Multiplexers"
> +
> +config IIO_MUX
> + tristate "IIO multiplexer driver"
> + select MULTIPLEXER
> + depends on OF
> + help
> + Say yes here to build support for the IIO multiplexer.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called iio-mux.
> +
> +endmenu
> diff --git a/drivers/iio/multiplexer/Makefile b/drivers/iio/multiplexer/Makefile
> new file mode 100644
> index 000000000000..68be3c4abd07
> --- /dev/null
> +++ b/drivers/iio/multiplexer/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O multiplexer drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_IIO_MUX) += iio-mux.o
> diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> new file mode 100644
> index 000000000000..92dfee2dfed1
> --- /dev/null
> +++ b/drivers/iio/multiplexer/iio-mux.c
> @@ -0,0 +1,456 @@
> +/*
> + * IIO multiplexer driver
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +struct mux_ext_info_cache {
> + char *data;
> + size_t size;
> +};
> +
> +struct mux_child {
> + struct mux_ext_info_cache *ext_info_cache;
> +};
> +
> +struct mux {
> + int cached_state;
> + struct mux_control *control;
> + struct iio_channel *parent;
> + struct iio_dev *indio_dev;
> + struct iio_chan_spec *chan;
> + struct iio_chan_spec_ext_info *ext_info;
> + struct mux_child *child;
> +};
> +
> +static int iio_mux_select(struct mux *mux, int idx)
> +{
> + struct mux_child *child = &mux->child[idx];
> + struct iio_chan_spec const *chan = &mux->chan[idx];
> + int ret;
> + int i;
> +
> + ret = mux_control_select(mux->control, chan->channel);
> + if (ret < 0) {
> + mux->cached_state = -1;
> + return ret;
> + }
> +
> + if (mux->cached_state == chan->channel)
> + return 0;
> +
> + if (chan->ext_info) {
> + for (i = 0; chan->ext_info[i].name; ++i) {
> + const char *attr = chan->ext_info[i].name;
> + struct mux_ext_info_cache *cache;
> +
> + cache = &child->ext_info_cache[i];
> +
> + if (cache->size < 0)
> + continue;
> +
> + ret = iio_write_channel_ext_info(mux->parent, attr,
> + cache->data,
> + cache->size);
> +
> + if (ret < 0) {
> + mux_control_deselect(mux->control);
> + mux->cached_state = -1;
> + return ret;
> + }
> + }
> + }
> + mux->cached_state = chan->channel;
> +
> + return 0;
> +}
> +
> +static void iio_mux_deselect(struct mux *mux)
> +{
> + mux_control_deselect(mux->control);
> +}
> +
> +static int mux_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mux *mux = iio_priv(indio_dev);
> + int idx = chan - mux->chan;
> + int ret;
> +
> + ret = iio_mux_select(mux, idx);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_read_channel_raw(mux->parent, val);
> + break;
> +
> + case IIO_CHAN_INFO_SCALE:
> + ret = iio_read_channel_scale(mux->parent, val, val2);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + }
> +
> + iio_mux_deselect(mux);
> +
> + return ret;
> +}
> +
> +static int mux_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + struct mux *mux = iio_priv(indio_dev);
> + int idx = chan - mux->chan;
> + int ret;
> +
> + ret = iio_mux_select(mux, idx);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *type = IIO_VAL_INT;
> + ret = iio_read_avail_channel_raw(mux->parent, vals, length);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + }
> +
> + iio_mux_deselect(mux);
> +
> + return ret;
> +}
> +
> +static int mux_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mux *mux = iio_priv(indio_dev);
> + int idx = chan - mux->chan;
> + int ret;
> +
> + ret = iio_mux_select(mux, idx);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_write_channel_raw(mux->parent, val);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + }
> +
> + iio_mux_deselect(mux);
> +
> + return ret;
> +}
> +
> +static const struct iio_info mux_info = {
> + .read_raw = mux_read_raw,
> + .read_avail = mux_read_avail,
> + .write_raw = mux_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static ssize_t mux_read_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *chan, char *buf)
> +{
> + struct mux *mux = iio_priv(indio_dev);
> + int idx = chan - mux->chan;
> + ssize_t ret;
> +
> + ret = iio_mux_select(mux, idx);
> + if (ret < 0)
> + return ret;
> +
> + ret = iio_read_channel_ext_info(mux->parent,
> + mux->ext_info[private].name,
> + buf);
> +
> + iio_mux_deselect(mux);
> +
> + return ret;
> +}
> +
> +static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *chan,
> + const char *buf, size_t len)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct mux *mux = iio_priv(indio_dev);
> + int idx = chan - mux->chan;
> + char *new;
> + ssize_t ret;
> +
> + ret = iio_mux_select(mux, idx);
> + if (ret < 0)
> + return ret;
> +
> + new = devm_kmemdup(dev, buf, len + 1, GFP_KERNEL);
> + if (!new) {
> + iio_mux_deselect(mux);
> + return -ENOMEM;
> + }
> +
> + new[len] = 0;
> +
> + ret = iio_write_channel_ext_info(mux->parent,
> + mux->ext_info[private].name,
> + buf, len);
> + if (ret < 0) {
> + iio_mux_deselect(mux);
> + devm_kfree(dev, new);
> + return ret;
> + }
> +
> + devm_kfree(dev, mux->child[idx].ext_info_cache[private].data);
> + mux->child[idx].ext_info_cache[private].data = new;
> + mux->child[idx].ext_info_cache[private].size = len;
> +
> + iio_mux_deselect(mux);
> +
> + return ret;
> +}
> +
> +static int mux_configure_channel(struct device *dev, struct mux *mux,
> + u32 state, const char *label, int idx)
> +{
> + struct mux_child *child = &mux->child[idx];
> + struct iio_chan_spec *chan = &mux->chan[idx];
> + struct iio_chan_spec const *pchan = mux->parent->channel;
> + char *page = NULL;
> + int num_ext_info;
> + int i;
> + int ret;
> +
> + chan->indexed = 1;
> + chan->output = pchan->output;
> + chan->datasheet_name = label;
> + chan->ext_info = mux->ext_info;
> +
> + ret = iio_get_channel_type(mux->parent, &chan->type);
> + if (ret < 0) {
> + dev_err(dev, "failed to get parent channel type\n");
> + return ret;
> + }
> +
> + if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> + if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> +
> + if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> +
> + if (state >= mux->control->states) {
> + dev_err(dev, "too many channels\n");
> + return -EINVAL;
> + }
> +
> + chan->channel = state;
> +
> + num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> + if (num_ext_info) {
> + page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> + if (!page)
> + return -ENOMEM;
> + }
> + child->ext_info_cache = devm_kzalloc(dev,
> + sizeof(*child->ext_info_cache) *
> + num_ext_info, GFP_KERNEL);
> + for (i = 0; i < num_ext_info; ++i) {
> + child->ext_info_cache[i].size = -1;
> +
> + if (!pchan->ext_info[i].write)
> + continue;
> + if (!pchan->ext_info[i].read)
> + continue;
> +
> + ret = iio_read_channel_ext_info(mux->parent,
> + mux->ext_info[i].name,
> + page);
> + if (ret < 0) {
> + dev_err(dev, "failed to get ext_info '%s'\n",
> + pchan->ext_info[i].name);
> + return ret;
> + }
> + if (ret >= PAGE_SIZE) {
> + dev_err(dev, "too large ext_info '%s'\n",
> + pchan->ext_info[i].name);
> + return -EINVAL;
> + }
> +
> + child->ext_info_cache[i].data = devm_kmemdup(dev, page, ret + 1,
> + GFP_KERNEL);
> + child->ext_info_cache[i].data[ret] = 0;
> + child->ext_info_cache[i].size = ret;
> + }
> +
> + if (page)
> + devm_kfree(dev, page);
> +
> + return 0;
> +}
> +
> +/*
> + * Same as of_property_for_each_string(), but also keeps track of the
> + * index of each string.
> + */
> +#define of_property_for_each_string_index(np, propname, prop, s, i) \
> + for (prop = of_find_property(np, propname, NULL), \
> + s = of_prop_next_string(prop, NULL), \
> + i = 0; \
> + s; \
> + s = of_prop_next_string(prop, s), \
> + i++)
> +
> +static int mux_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = pdev->dev.of_node;
> + struct iio_dev *indio_dev;
> + struct iio_channel *parent;
> + struct mux *mux;
> + struct property *prop;
> + const char *label;
> + u32 state;
> + int sizeof_ext_info;
> + int children;
> + int sizeof_priv;
> + int i;
> + int ret;
> +
> + if (!np)
> + return -ENODEV;
> +
> + parent = devm_iio_channel_get(dev, "parent");
> + if (IS_ERR(parent)) {
> + if (PTR_ERR(parent) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get parent channel\n");
> + return PTR_ERR(parent);
> + }
> +
> + sizeof_ext_info = iio_get_channel_ext_info_count(parent);
> + if (sizeof_ext_info) {
> + sizeof_ext_info += 1; /* one extra entry for the sentinel */
> + sizeof_ext_info *= sizeof(*mux->ext_info);
> + }
> +
> + children = 0;
> + of_property_for_each_string(np, "channels", prop, label) {
> + if (*label)
> + children++;
> + }
> + if (children <= 0) {
> + dev_err(dev, "not even a single child\n");
> + return -EINVAL;
> + }
> +
> + sizeof_priv = sizeof(*mux);
> + sizeof_priv += sizeof(*mux->child) * children;
> + sizeof_priv += sizeof(*mux->chan) * children;
> + sizeof_priv += sizeof_ext_info;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + mux = iio_priv(indio_dev);
> + mux->child = (struct mux_child *)(mux + 1);
> + mux->chan = (struct iio_chan_spec *)(mux->child + children);
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + mux->parent = parent;
> + mux->cached_state = -1;
> +
> + indio_dev->name = dev_name(dev);
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &mux_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = mux->chan;
> + indio_dev->num_channels = children;
> + if (sizeof_ext_info) {
> + mux->ext_info = devm_kmemdup(dev,
> + parent->channel->ext_info,
> + sizeof_ext_info, GFP_KERNEL);
> + if (!mux->ext_info)
> + return -ENOMEM;
> +
> + for (i = 0; mux->ext_info[i].name; ++i) {
> + if (parent->channel->ext_info[i].read)
> + mux->ext_info[i].read = mux_read_ext_info;
> + if (parent->channel->ext_info[i].write)
> + mux->ext_info[i].write = mux_write_ext_info;
> + mux->ext_info[i].private = i;
> + }
> + }
> +
> + mux->control = devm_mux_control_get(dev, NULL);
> + if (IS_ERR(mux->control)) {
> + if (PTR_ERR(mux->control) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get control-mux\n");
> + return PTR_ERR(mux->control);
> + }
> +
> + i = 0;
> + of_property_for_each_string_index(np, "channels", prop, label, state) {
> + if (!*label)
> + continue;
> +
> + ret = mux_configure_channel(dev, mux, state, label, i++);
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret) {
> + dev_err(dev, "failed to register iio device\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mux_match[] = {
> + { .compatible = "iio-mux" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_match);
> +
> +static struct platform_driver mux_driver = {
> + .probe = mux_probe,
> + .driver = {
> + .name = "iio-mux",
> + .of_match_table = mux_match,
> + },
> +};
> +module_platform_driver(mux_driver);
> +
> +MODULE_DESCRIPTION("IIO multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v6 2/9] misc: minimal mux subsystem and gpio-based mux controller
From: Jonathan Cameron @ 2016-12-31 16:19 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <1480493823-21462-3-git-send-email-peda@axentia.se>
On 30/11/16 08:16, Peter Rosin wrote:
> Add a new minimalistic subsystem that handles multiplexer controllers.
> When multiplexers are used in various places in the kernel, and the
> same multiplexer controller can be used for several independent things,
> there should be one place to implement support for said multiplexer
> controller.
>
> A single multiplexer controller can also be used to control several
> parallel multiplexers, that are in turn used by different subsystems
> in the kernel, leading to a need to coordinate multiplexer accesses.
> The multiplexer subsystem handles this coordination.
>
> This new mux controller subsystem initially comes with a single backend
> driver that controls gpio based multiplexers. Even though not needed by
> this initial driver, the mux controller subsystem is prepared to handle
> chips with multiple (independent) mux controllers.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Few trivial bits inline + question of whether misc is the right location..
It's small, but not totally trivial as subsystems go, so perhaps it needs it's
own directory.
> ---
> Documentation/driver-model/devres.txt | 6 +-
> MAINTAINERS | 2 +
> drivers/misc/Kconfig | 30 ++++
> drivers/misc/Makefile | 2 +
> drivers/misc/mux-core.c | 311 ++++++++++++++++++++++++++++++++++
> drivers/misc/mux-gpio.c | 138 +++++++++++++++
> include/linux/mux.h | 197 +++++++++++++++++++++
> 7 files changed, 685 insertions(+), 1 deletion(-)
> create mode 100644 drivers/misc/mux-core.c
> create mode 100644 drivers/misc/mux-gpio.c
> create mode 100644 include/linux/mux.h
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index ca9d1eb46bc0..d64ede85b61b 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -330,7 +330,11 @@ MEM
> devm_kzalloc()
>
> MFD
> - devm_mfd_add_devices()
Technically should be in a separate cleanup patch...
> + devm_mfd_add_devices()
> +
> +MUX
> + devm_mux_control_get()
> + devm_mux_control_put()
>
> PER-CPU MEM
> devm_alloc_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d4d0efc2b64..dc7498682752 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8407,6 +8407,8 @@ MULTIPLEXER SUBSYSTEM
> M: Peter Rosin <peda@axentia.se>
> S: Maintained
> F: Documentation/devicetree/bindings/misc/mux-*
> +F: include/linux/mux.h
> +F: drivers/misc/mux-*
>
> MULTISOUND SOUND DRIVER
> M: Andrew Veliath <andrewtv@usa.net>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 64971baf11fa..2ce675e410c5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -766,6 +766,36 @@ config PANEL_BOOT_MESSAGE
> An empty message will only clear the display at driver init time. Any other
> printf()-formatted message is valid with newline and escape codes.
>
> +menuconfig MULTIPLEXER
> + bool "Multiplexer subsystem"
> + help
> + Multiplexer controller subsystem. Multiplexers are used in a
> + variety of settings, and this subsystem abstracts their use
> + so that the rest of the kernel sees a common interface. When
> + multiple parallel multiplexers are controlled by one single
> + multiplexer controller, this subsystem also coordinates the
> + multiplexer accesses.
> +
> + If unsure, say no.
> +
Fun question of the day. Is misc the place to put this or should it
have it's own directory. I'd go for own directory...
On the plus side, whilst it's in misc you get to pester Greg and Arnd
for review.
> +if MULTIPLEXER
> +
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB
> + help
> + GPIO-controlled Multiplexer controller.
> +
> + The driver builds a single multiplexer controller using a number
> + of gpio pins. For N pins, there will be 2^N possible multiplexer
> + states. The GPIO pins can be connected (by the hardware) to several
> + multiplexers, which in that case will be operated in parallel.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called mux-gpio.
> +
> +endif
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 31983366090a..0befa2bba762 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -53,6 +53,8 @@ obj-$(CONFIG_ECHO) += echo/
> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> obj-$(CONFIG_CXL_BASE) += cxl/
> obj-$(CONFIG_PANEL) += panel.o
> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>
> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o
> lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o
> diff --git a/drivers/misc/mux-core.c b/drivers/misc/mux-core.c
> new file mode 100644
> index 000000000000..cccaa7261a6e
> --- /dev/null
> +++ b/drivers/misc/mux-core.c
> @@ -0,0 +1,311 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};
> +
> +static int __init mux_init(void)
> +{
> + return class_register(&mux_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);
> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(&mux_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (!dev || !controllers)
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> + controllers * sizeof(*mux_chip->mux) +
> + sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;
> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = &mux_class;
> + mux_chip->dev.type = &mux_type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(&mux_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(&mux->lock);
> + mux->cached_state = -1;
> + mux->idle_state = -1;
> + }
> +
> + device_initialize(&mux_chip->dev);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? -1 : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
> +
> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return device_add(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
> +
> +void mux_chip_unregister(struct mux_chip *mux_chip)
> +{
> + device_del(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
> +
> +void mux_chip_free(struct mux_chip *mux_chip)
> +{
> + if (!mux_chip)
> + return;
I'd drop a blank line in here. Slightly nicer to read.
> + put_device(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_free);
> +
> +int mux_control_select(struct mux_control *mux, int state)
> +{
> + int ret;
> +
> + if (down_read_trylock(&mux->lock)) {
> + if (mux->cached_state == state)
> + return 0;
> +
> + /* Sigh, the mux needs updating... */
> + up_read(&mux->lock);
> + }
> +
> + /* ...or it's just contended. */
> + down_write(&mux->lock);
> +
> + if (mux->cached_state == state) {
> + /*
> + * Hmmm, someone else changed the mux to my liking.
> + * That makes me wonder how long I waited for nothing?
> + */
> + downgrade_write(&mux->lock);
> + return 0;
> + }
> +
> + ret = mux_control_set(mux, state);
> + if (ret < 0) {
> + if (mux->idle_state != -1)
> + mux_control_set(mux, mux->idle_state);
> +
> + up_write(&mux->lock);
> + return ret;
> + }
> +
> + downgrade_write(&mux->lock);
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_select);
> +
> +int mux_control_deselect(struct mux_control *mux)
> +{
> + int ret = 0;
> +
> + if (mux->idle_state != -1 && mux->cached_state != mux->idle_state)
> + ret = mux_control_set(mux, mux->idle_state);
> +
> + up_read(&mux->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_deselect);
> +
> +static int of_dev_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
> +
> + return dev ? to_mux_chip(dev) : NULL;
> +}
> +
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> + struct device_node *np = dev->of_node;
> + struct of_phandle_args args;
> + struct mux_chip *mux_chip;
> + unsigned int controller;
> + int index = 0;
> + int ret;
> +
> + if (mux_name) {
> + index = of_property_match_string(np, "mux-control-names",
> + mux_name);
> + if (index < 0)
> + return ERR_PTR(index);
> + }
> +
> + ret = of_parse_phandle_with_args(np,
> + "mux-controls", "#mux-control-cells",
> + index, &args);
> + if (ret) {
> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
> + np->full_name, mux_name ?: "", index);
> + return ERR_PTR(ret);
> + }
> +
> + mux_chip = of_find_mux_chip_by_node(args.np);
> + of_node_put(args.np);
> + if (!mux_chip)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (args.args_count > 1 ||
> + (!args.args_count && (mux_chip->controllers > 1))) {
> + dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
> + np->full_name, args.np->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + controller = 0;
> + if (args.args_count)
> + controller = args.args[0];
> +
> + if (controller >= mux_chip->controllers)
> + return ERR_PTR(-EINVAL);
> +
> + get_device(&mux_chip->dev);
> + return &mux_chip->mux[controller];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get);
> +
> +void mux_control_put(struct mux_control *mux)
> +{
> + put_device(&mux->chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_put);
> +
> +static void devm_mux_control_release(struct device *dev, void *res)
> +{
> + struct mux_control *mux = *(struct mux_control **)res;
> +
> + mux_control_put(mux);
> +}
> +
> +struct mux_control *devm_mux_control_get(struct device *dev,
> + const char *mux_name)
> +{
> + struct mux_control **ptr, *mux;
> +
> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + mux = mux_control_get(dev, mux_name);
> + if (IS_ERR(mux)) {
> + devres_free(ptr);
> + return mux;
> + }
> +
> + *ptr = mux;
> + devres_add(dev, ptr);
> +
> + return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
> +
> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_control **r = res;
> +
> + if (!r || !*r) {
> + WARN_ON(!r || !*r);
> + return 0;
> + }
> +
> + return *r == data;
> +}
> +
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
> +{
> + WARN_ON(devres_release(dev, devm_mux_control_release,
> + devm_mux_control_match, mux));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
> +
> +subsys_initcall(mux_init);
> +
> +MODULE_DESCRIPTION("Multiplexer subsystem");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/mux-gpio.c b/drivers/misc/mux-gpio.c
> new file mode 100644
> index 000000000000..a107a9b96854
> --- /dev/null
> +++ b/drivers/misc/mux-gpio.c
> @@ -0,0 +1,138 @@
> +/*
> + * GPIO-controlled multiplexer driver
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +struct mux_gpio {
> + struct gpio_descs *gpios;
> + int *val;
> +};
> +
> +static int mux_gpio_set(struct mux_control *mux, int state)
> +{
> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> + int i;
> +
> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> + mux_gpio->val[i] = (state >> i) & 1;
> +
> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> + mux_gpio->gpios->desc,
> + mux_gpio->val);
> +
> + return 0;
> +}
> +
> +static const struct mux_control_ops mux_gpio_ops = {
> + .set = mux_gpio_set,
> +};
> +
> +static const struct of_device_id mux_gpio_dt_ids[] = {
> + { .compatible = "mux-gpio", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
> +
> +static int mux_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = pdev->dev.of_node;
> + struct mux_chip *mux_chip;
> + struct mux_gpio *mux_gpio;
> + int pins;
> + u32 idle_state;
> + int ret;
> +
> + if (!np)
> + return -ENODEV;
> +
> + pins = gpiod_count(dev, "mux");
> + if (pins < 0)
> + return pins;
> +
> + mux_chip = mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
> + pins * sizeof(*mux_gpio->val));
> + if (!mux_chip)
> + return -ENOMEM;
Rather feels like mux_chip_alloc is a good candidate for a managed
allocator. Might be worth doing the register as well then the remove
below would go away. I guess perhaps not that worthwhile as not many
mux types are likely to ever exist...
> +
> + mux_gpio = mux_chip_priv(mux_chip);
> + mux_gpio->val = (int *)(mux_gpio + 1);
> + mux_chip->ops = &mux_gpio_ops;
> +
> + platform_set_drvdata(pdev, mux_chip);
> +
> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> + if (IS_ERR(mux_gpio->gpios)) {
> + ret = PTR_ERR(mux_gpio->gpios);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get gpios\n");
> + goto free_mux_chip;
> + }
> + WARN_ON(pins != mux_gpio->gpios->ndescs);
> + mux_chip->mux->states = 1 << pins;
> +
> + ret = of_property_read_u32(np, "idle-state", &idle_state);
> + if (ret >= 0) {
> + if (idle_state >= mux_chip->mux->states) {
> + dev_err(dev, "invalid idle-state %u\n", idle_state);
> + ret = -EINVAL;
> + goto free_mux_chip;
> + }
> +
> + mux_chip->mux->idle_state = idle_state;
> + }
> +
> + ret = mux_chip_register(mux_chip);
> + if (ret < 0) {
> + dev_err(dev, "failed to register mux-chip\n");
> + goto free_mux_chip;
> + }
> +
> + dev_info(dev, "%u-way mux-controller registered\n",
> + mux_chip->mux->states);
> +
> + return 0;
> +
> +free_mux_chip:
> + mux_chip_free(mux_chip);
> + return ret;
> +}
> +
> +static int mux_gpio_remove(struct platform_device *pdev)
> +{
> + struct mux_chip *mux_chip = platform_get_drvdata(pdev);
> +
> + mux_chip_unregister(mux_chip);
> + mux_chip_free(mux_chip);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mux_gpio_driver = {
> + .driver = {
> + .name = "mux-gpio",
> + .of_match_table = of_match_ptr(mux_gpio_dt_ids),
> + },
> + .probe = mux_gpio_probe,
> + .remove = mux_gpio_remove,
> +};
> +module_platform_driver(mux_gpio_driver);
> +
> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..88d607b7fde7
> --- /dev/null
> +++ b/include/linux/mux.h
> @@ -0,0 +1,197 @@
> +/*
> + * mux.h - definitions for the multiplexer interface
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * 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 _LINUX_MUX_H
> +#define _LINUX_MUX_H
> +
> +#include <linux/device.h>
> +#include <linux/rwsem.h>
> +
> +struct mux_chip;
> +struct mux_control;
> +struct platform_device;
> +
> +struct mux_control_ops {
> + int (*set)(struct mux_control *mux, int state);
> +};
> +
> +/**
> + * struct mux_control - Represents a mux controller.
> + * @lock: Protects the mux controller state.
> + * @chip: The mux chip that is handling this mux controller.
> + * @states: The number of mux controller states.
> + * @cached_state: The current mux controller state, or -1 if none.
> + * @idle_state: The mux controller state to use when inactive, or -1
> + * for none.
> + */
> +struct mux_control {
> + struct rw_semaphore lock; /* protects the state of the mux */
> +
> + struct mux_chip *chip;
> +
> + unsigned int states;
> + int cached_state;
> + int idle_state;
> +};
> +
> +/**
> + * struct mux_chip - Represents a chip holding mux controllers.
> + * @controllers: Number of mux controllers handled by the chip.
> + * @mux: Array of mux controllers that is handled.
> + * @dev: Device structure.
> + * @id: Used to identify the device internally.
> + * @ops: Mux controller operations.
> + */
> +struct mux_chip {
> + unsigned int controllers;
> + struct mux_control *mux;
> + struct device dev;
> + int id;
> +
> + const struct mux_control_ops *ops;
> +};
> +
> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
> +
> +/**
> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
> + * @mux_chip: The mux-chip to get the private memory from.
> + *
> + * Return: Pointer to the private memory reserved by the allocator.
> + */
> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
> +{
> + return &mux_chip->mux[mux_chip->controllers];
> +}
> +
> +/**
> + * mux_chip_alloc() - Allocate a mux-chip.
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv);
> +
> +/**
> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
> + * for use.
> + * @mux_chip: The mux-chip to register.
> + *
> + * Do not retry registration of the same mux-chip on failure. You should
> + * instead put it away with mux_chip_free() and allocate a new one, if you
> + * for some reason would like to retry registration.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_register(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_unregister() - Take the mux-chip off-line.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * mux_chip_unregister() reverses the effects of mux_chip_register().
> + * But not completely, you should not try to call mux_chip_register()
> + * on a mux-chip that has been registered before.
> + */
> +void mux_chip_unregister(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_free() - Free the mux-chip for good.
> + * @mux_chip: The mux-chip to free.
> + *
> + * mux_chip_free() reverses the effects of mux_chip_alloc().
> + */
> +void mux_chip_free(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_control_select() - Select the given multiplexer state.
> + * @mux: The mux-control to request a change of state from.
> + * @state: The new requested state.
> + *
> + * Make sure to call mux_control_deselect() when the operation is complete and
> + * the mux-control is free for others to use, but do not call
> + * mux_control_deselect() if mux_control_select() fails.
> + *
> + * Return: 0 if the requested state was already active, or 1 it the
> + * mux-control state was changed to the requested state. Or a negavive
> + * errno on error.
> + *
> + * Note that the difference in return value of zero or one is of
> + * questionable value; especially if the mux-control has several independent
> + * consumers, which is something the consumers should perhaps not be making
> + * assumptions about.
> + */
> +int mux_control_select(struct mux_control *mux, int state);
> +
> +/**
> + * mux_control_deselect() - Deselect the previously selected multiplexer state.
> + * @mux: The mux-control to deselect.
> + *
> + * Return: 0 on success and a negative errno on error. An error can only
> + * occur if the mux has an idle state. Note that even if an error occurs, the
> + * mux-control is unlocked for others to access.
> + */
> +int mux_control_deselect(struct mux_control *mux);
> +
> +/**
> + * mux_control_get_index() - Get the index of the given mux controller
> + * @mux: The mux-control to the the index for.
> + *
> + * Return: The index of the mux controller within the mux chip the mux
> + * controller is a part of.
> + */
> +static inline unsigned int mux_control_get_index(struct mux_control *mux)
> +{
> + return mux - mux->chip->mux;
> +}
> +
> +/**
> + * mux_control_get() - Get the mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
> +
> +/**
> + * mux_control_put() - Put away the mux-control for good.
> + * @mux: The mux-control to put away.
> + *
> + * mux_control_put() reverses the effects of mux_control_get().
> + */
> +void mux_control_put(struct mux_control *mux);
> +
> +/**
> + * devm_mux_control_get() - Get the mux-control for a device, with resource
> + * management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get(struct device *dev,
> + const char *mux_name);
> +
> +/**
> + * devm_mux_control_put() - Resource-managed version mux_control_put().
> + * @dev: The device that originally got the mux-control.
> + * @mux: The mux-control to put away.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux);
> +
> +#endif /* _LINUX_MUX_H */
>
^ permalink raw reply
* Re: [PATCH v6 1/9] dt-bindings: document devicetree bindings for mux-controllers and mux-gpio
From: Jonathan Cameron @ 2016-12-31 16:10 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <1480493823-21462-2-git-send-email-peda@axentia.se>
On 30/11/16 08:16, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda@axentia.se>
Bindings are still a bit of a black art to me ;)
Other than the naming of the iio-mux as discussed in the other patch
I'm happy with this. It feels like it has struck the right balance
between flexibility and complexity. Which probably means we'll
have an application it doesn't stretch to before the day is out...
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> .../devicetree/bindings/misc/mux-controller.txt | 127 +++++++++++++++++++++
> .../devicetree/bindings/misc/mux-gpio.txt | 68 +++++++++++
> MAINTAINERS | 5 +
> 3 files changed, 200 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/misc/mux-controller.txt
> create mode 100644 Documentation/devicetree/bindings/misc/mux-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/misc/mux-controller.txt b/Documentation/devicetree/bindings/misc/mux-controller.txt
> new file mode 100644
> index 000000000000..19c36b73173e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/mux-controller.txt
> @@ -0,0 +1,127 @@
> +Common multiplexer controller bindings
> +======================================
> +
> +A multiplexer (or mux) controller will have one, or several, consumer devices
> +that uses the mux controller. Thus, a mux controller can possibly control
> +several parallel multiplexers, presumably there will be at least one
> +multiplexer needed by each consumer..
> +
> +A mux controller provides a number of states to its consumers, and the state
> +space is a simple zero-based enumeration. I.e. 0-1 for a 2-way multiplexer,
> +0-7 for an 8-way multiplexer, etc.
> +
> +
> +Consumers
> +---------
> +
> +Mux controller consumers should specify a list of mux controllers that they
> +want to use with a property containing a 'mux-ctrl-list':
> +
> + mux-ctrl-list ::= <single-mux-ctrl> [mux-ctrl-list]
> + single-mux-ctrl ::= <mux-ctrl-phandle> [mux-ctrl-specifier]
> + mux-ctrl-phandle : phandle to mux controller node
> + mux-ctrl-specifier : array of #mux-control-cells specifying the
> + given mux controller (controller specific)
> +
> +Mux controller properties should be named "mux-controls". The exact meaning of
> +each mux controller property must be documented in the device tree binding for
> +each consumer. An optional property "mux-control-names" may contain a list of
> +strings to label each of the mux controllers listed in the "mux-controls"
> +property.
> +
> +Drivers for devices that use more than a single mux controller can use the
> +"mux-control-names" property to map the name of the mux controller requested by
> +the mux_control_get() call to an index into the list given by the
> +"mux-controls" property.
> +
> +mux-ctrl-specifier typically encodes the chip-relative mux controller number.
> +If the mux controller chip only provides a single mux controller, the
> +mux-ctrl-specifier can typically be left out.
> +
> +Example:
> +
> + /* One consumer of a 2-way mux controller (one GPIO-line) */
> + mux: mux-controller {
> + compatible = "mux-gpio";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>;
> + };
> +
> + adc-mux {
> + compatible = "iio-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> + mux-controls = <&mux>;
> + mux-control-names = "adc";
> +
> + channels = "sync", "in";
> + };
> +
> +Note that in the example above, specifying the "mux-control-names" is redundant
> +because there is only one mux controller in the list.
> +
> + /*
> + * Two consumers (one for an ADC line and one for an i2c bus) of
> + * parallel 4-way multiplexers controlled by the same two GPIO-lines.
> + */
> + mux: mux-controller {
> + compatible = "mux-gpio";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + adc-mux {
> + compatible = "iio-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> + mux-controls = <&mux>;
> +
> + channels = "sync-1", "in", "out", "sync-2";
> + };
> +
> + i2c-mux {
> + compatible = "i2c-mux-simple,mux-locked";
> + i2c-parent = <&i2c1>;
> + mux-controls = <&mux>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ssd1307: oled@3c {
> + /* ... */
> + };
> + };
> +
> + i2c@3 {
> + reg = <3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pca9555: pca9555@20 {
> + /* ... */
> + };
> + };
> + };
> +
> +
> +Mux controller nodes
> +--------------------
> +
> +Mux controller nodes must specify the number of cells used for the
> +specifier using the '#mux-control-cells' property.
> +
> +An example mux controller might look like this:
> +
> + mux: adg792a@50 {
> + compatible = "adi,adg792a";
> + reg = <0x50>;
> + #mux-control-cells = <1>;
> + };
> diff --git a/Documentation/devicetree/bindings/misc/mux-gpio.txt b/Documentation/devicetree/bindings/misc/mux-gpio.txt
> new file mode 100644
> index 000000000000..2ff814f082c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/mux-gpio.txt
> @@ -0,0 +1,68 @@
> +GPIO-based multiplexer controller bindings
> +
> +Define what GPIO pins are used to control a multiplexer. Or several
> +multiplexers, if the same pins control more than one multiplexer.
> +
> +Required properties:
> +- compatible : "mux-gpio"
> +- mux-gpios : list of gpios used to control the multiplexer, least
> + significant bit first.
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle.
> +
> +The multiplexer state is defined as the number represented by the
> +multiplexer GPIO pins, where the first pin is the least significant
> +bit. An active pin is a binary 1, an inactive pin is a binary 0.
> +
> +Example:
> +
> + mux: mux-controller {
> + compatible = "mux-gpio";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + adc-mux {
> + compatible = "iio-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&mux>;
> +
> + channels = "sync-1", "in", "out", "sync-2";
> + };
> +
> + i2c-mux {
> + compatible = "i2c-mux-simple,mux-locked";
> + i2c-parent = <&i2c1>;
> +
> + mux-controls = <&mux>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ssd1307: oled@3c {
> + /* ... */
> + };
> + };
> +
> + i2c@3 {
> + reg = <3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pca9555: pca9555@20 {
> + /* ... */
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d8eb3843dbd4..3d4d0efc2b64 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8403,6 +8403,11 @@ S: Orphan
> F: drivers/mmc/host/mmc_spi.c
> F: include/linux/spi/mmc_spi.h
>
> +MULTIPLEXER SUBSYSTEM
> +M: Peter Rosin <peda@axentia.se>
> +S: Maintained
> +F: Documentation/devicetree/bindings/misc/mux-*
> +
> MULTISOUND SOUND DRIVER
> M: Andrew Veliath <andrewtv@usa.net>
> S: Maintained
>
^ permalink raw reply
* Re: [PATCH v6 4/9] dt-bindings: iio: iio-mux: document iio-mux bindings
From: Jonathan Cameron @ 2016-12-31 16:01 UTC (permalink / raw)
To: Peter Rosin, Rob Herring
Cc: linux-kernel, Wolfram Sang, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <2275a7ff-6a21-dd99-ab7d-b213d7fcd6e5@axentia.se>
On 12/12/16 12:18, Peter Rosin wrote:
> On 2016-12-10 19:21, Jonathan Cameron wrote:
>> On 06/12/16 09:18, Peter Rosin wrote:
>>> On 2016-12-06 00:26, Rob Herring wrote:
>>>> On Wed, Nov 30, 2016 at 09:16:58AM +0100, Peter Rosin wrote:
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>> .../bindings/iio/multiplexer/iio-mux.txt | 40 ++++++++++++++++++++++
>>>>> MAINTAINERS | 6 ++++
>>>>> 2 files changed, 46 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>>
>>>> I'm still not convinced about this binding, but don't really have more
>>>> comments ATM. Sending 6 versions in 2 weeks or so doesn't really help
>>>> either.
>>>
>>> Sorry about the noise, I'll try to be more careful going forward. On
>>> the flip side, I haven't touched the code since v6.
>>>
>>> I don't see how bindings that are as flexible as the current (and
>>> original) phandle link between the mux consumer and the mux controller
>>> would look, and at the same time be simpler to understand. You need
>>> to be able to refer to a mux controller from several mux consumers, and
>>> you need to support several mux controllers in one node (the ADG792A
>>> case). And, AFAICT, the complex case wasn't really the problem, it was
>>> that it is overly complex to describe the simple case of one mux
>>> consumer and one mux controller. But in your comment for v2 [1] you
>>> said that I was working around limitations with shared GPIO pins. But
>>> solving that in the GPIO subsystem would not solve all that the
>>> phandle approach is solving, since you would not have support for
>>> ADG792A (or other non-GPIO controlled muxes). So, I think listing
>>> the gpio pins inside the mux consumer node is a non-starter, the mux
>>> controller has to live in its own node with its own compatible.
>>>
>>> Would you be happier if I managed to marry the phandle approach with
>>> the option of having the mux controller as a child node of the mux
>>> consumer for the simple case?
>>>
>>> I added an example at the end of this message (the same as the first
>>> example in v4 [2], at least in principle) for easy comparison between
>>> the phandle and the controller-in-child-node approaches. I can't say
>>> that I personally find the difference all that significant, and do not
>>> think it is worth it. As I see it, the "simple option" would just muddy
>>> the waters...
>>>
>>> [1] http://marc.info/?l=linux-kernel&m=147948334204795&w=2
>>> [2] http://marc.info/?l=linux-kernel&m=148001364904240&w=2
>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt b/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>>> new file mode 100644
>>>>> index 000000000000..8080cf790d82
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>>> @@ -0,0 +1,40 @@
>>>>> +IIO multiplexer bindings
>>>>> +
>>>>> +If a multiplexer is used to select which hardware signal is fed to
>>>>> +e.g. an ADC channel, these bindings describe that situation.
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : "iio-mux"
>>>>
>>>> This is a Linuxism. perhaps "adc-mux".
>>>
>>> No, that's not general enough, it could just as well be used to mux a
>>> temperature sensor. Or whatever. Hmmm, given that "iio-mux" is bad, perhaps
>>> "io-channel-mux" is better? That matches the io-channels property used to
>>> refer to the parent channel.
>> analog-mux maybe? Makes more sense out of context (though with io-channels defined on
>> the next line you have plenty of context here ;)
>
> Not that I care all that much about the name, but that doesn't really
> fit if you take e.g. an IIO_INDEX channel. That sounds entirely non-analog
> to me, but what do I know? Maybe that example doesn't make sense for some
> reason, but I can't help but think that there will be some non-analog
> channel in the future, if there isn't one already.
>
> So, my preference is io-channel-mux, as that matches the previous dt
> naming for what is muxed. But that's just my opinion, if I'm told that
> it should be something else, then that's ok.
io-channel-mux works fine for me. It's some sort of input / output channel
and we are muxing it ;)
>
> I'm more worried about other aspects, such as how to get reviewers and who
> is going to take the core mux patches and what tree they are going to be
> merged into etc. That is, if this series is going anywhere at all or if
> someone is going to put up a road-block for some reason...
Whilst it is meeting some resistance, I'm not seeing any absolute blockers
(people tend to be rather explicit about that). The binding is still causing
the most friction I think and it may be that it just needs some more time for
Rob to mull it over. It's a fiddly thing to describe, so was never going
to drop straight in!
The core mux patches probably need to go one of a few possible routes.
1. Directly as a pull to linus with a good collection of Acks.
2. Via Greg KH perhaps as generic driver infrastructure, or Andrew Morton
as being in the category no one else will take.
3. Via either me or Wolfram (as a separate immutable branch) on the basis it's
core stuff but the users currently are IIO and I2C.
In any case, this needs at the very least Acks from Rob, Wolfram and myself.
Others would be most welcome, Arnd and/or Greg might be persuaded to take
a look for example...
Happy New Year,
Jonathan
>
> Cheers,
> peda
>
^ permalink raw reply
* Re: [PATCH v6 3/9] iio: inkern: api for manipulating ext_info of iio channels
From: Jonathan Cameron @ 2016-12-31 15:51 UTC (permalink / raw)
To: Peter Rosin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480493823-21462-4-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 30/11/16 08:16, Peter Rosin wrote:
> Extend the inkern api with functions for reading and writing ext_info
> of iio channels.
>
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
It may make more sense to take this particular patch separately via
IIO, but as the churn on this file is fairly low I think it is probably
going to be easier to take it with the rest of the series if / when that
heads upstream.
Jonathan
> ---
> drivers/iio/inkern.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/iio/consumer.h | 37 +++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b0f4630a163f..4848b8129e6c 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -863,3 +863,63 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
> return ret;
> }
> EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> +
> +unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> +{
> + const struct iio_chan_spec_ext_info *ext_info;
> + unsigned int i = 0;
> +
> + if (!chan->channel->ext_info)
> + return i;
> +
> + for (ext_info = chan->channel->ext_info; ext_info->name; ext_info++)
> + ++i;
> +
> + return i;
> +}
> +EXPORT_SYMBOL_GPL(iio_get_channel_ext_info_count);
> +
> +static const struct iio_chan_spec_ext_info *iio_lookup_ext_info(
> + const struct iio_channel *chan,
> + const char *attr)
> +{
> + const struct iio_chan_spec_ext_info *ext_info;
> +
> + if (!chan->channel->ext_info)
> + return NULL;
> +
> + for (ext_info = chan->channel->ext_info; ext_info->name; ++ext_info) {
> + if (!strcmp(attr, ext_info->name))
> + return ext_info;
> + }
> +
> + return NULL;
> +}
> +
> +ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
> + const char *attr, char *buf)
> +{
> + const struct iio_chan_spec_ext_info *ext_info;
> +
> + ext_info = iio_lookup_ext_info(chan, attr);
> + if (!ext_info)
> + return -EINVAL;
> +
> + return ext_info->read(chan->indio_dev, ext_info->private,
> + chan->channel, buf);
> +}
> +EXPORT_SYMBOL_GPL(iio_read_channel_ext_info);
> +
> +ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
> + const char *buf, size_t len)
> +{
> + const struct iio_chan_spec_ext_info *ext_info;
> +
> + ext_info = iio_lookup_ext_info(chan, attr);
> + if (!ext_info)
> + return -EINVAL;
> +
> + return ext_info->write(chan->indio_dev, ext_info->private,
> + chan->channel, buf, len);
> +}
> +EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 47eeec3218b5..5e347a9805fd 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -312,4 +312,41 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
> int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> int *processed, unsigned int scale);
>
> +/**
> + * iio_get_channel_ext_info_count() - get number of ext_info attributes
> + * connected to the channel.
> + * @chan: The channel being queried
> + *
> + * Returns the number of ext_info attributes
> + */
> +unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan);
> +
> +/**
> + * iio_read_channel_ext_info() - read ext_info attribute from a given channel
> + * @chan: The channel being queried.
> + * @attr: The ext_info attribute to read.
> + * @buf: Where to store the attribute value. Assumed to hold
> + * at least PAGE_SIZE bytes.
> + *
> + * Returns the number of bytes written to buf (perhaps w/o zero termination;
> + * it need not even be a string), or an error code.
> + */
> +ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
> + const char *attr, char *buf);
> +
> +/**
> + * iio_write_channel_ext_info() - write ext_info attribute from a given channel
> + * @chan: The channel being queried.
> + * @attr: The ext_info attribute to read.
> + * @buf: The new attribute value. Strings needs to be zero-
> + * terminated, but the terminator should not be included
> + * in the below len.
> + * @len: The size of the new attribute value.
> + *
> + * Returns the number of accepted bytes, which should be the same as len.
> + * An error code can also be returned.
> + */
> +ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
> + const char *buf, size_t len);
> +
> #endif
>
^ permalink raw reply
* Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface
From: Guenter Roeck @ 2016-12-30 19:34 UTC (permalink / raw)
To: eajames.ibm
Cc: jdelvare, corbet, mark.rutland, robh+dt, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
In-Reply-To: <1483120568-21082-3-git-send-email-eajames.ibm@gmail.com>
On Fri, Dec 30, 2016 at 11:56:04AM -0600, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add a generic mechanism to expose the sensors provided by the OCC in
> sysfs.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Documentation/hwmon/occ | 48 +++++
> drivers/hwmon/occ/Makefile | 2 +-
> drivers/hwmon/occ/occ_sysfs.c | 492 ++++++++++++++++++++++++++++++++++++++++++
> drivers/hwmon/occ/occ_sysfs.h | 52 +++++
> 4 files changed, 593 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/occ/occ_sysfs.c
> create mode 100644 drivers/hwmon/occ/occ_sysfs.h
>
> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
> index 79d1642..1ee8689 100644
> --- a/Documentation/hwmon/occ
> +++ b/Documentation/hwmon/occ
> @@ -25,6 +25,54 @@ Currently, all versions of the OCC support four types of sensor data: power,
> temperature, frequency, and "caps," which indicate limits and thresholds used
> internally on the OCC.
>
> +sysfs Entries
> +-------------
> +
> +The OCC driver uses the hwmon sysfs framework to provide data to userspace.
> +
> +The driver exports two sysfs files for each frequency, temperature, and power
> +sensor. These are "input" and "label". The input file contains the value of the
> +sensor. The label file contains the sensor id. The sensor id is the unique
> +internal OCC identifier. Sensor ids may be provided by the OCC specification.
> +The names of these files will be in the following format:
> + <sensor type><sensor index>_input
> + <sensor type><sensor index>_label
> +Sensor types will be one of "temp", "freq", or "power". The sensor index is
> +an index to differentiate different sensor files. For example, a single
> +temperature sensor will have two sysfs files: temp1_input and temp1_label.
> +
> +Caps sensors are exported differently. For each caps sensor, the driver will
> +export 6 entries:
> + curr_powercap - current power cap in watts
> + curr_powerreading - current power output in watts
> + norm_powercap - power cap without redundant power
> + max_powercap - maximum power cap that can be set in watts
> + min_powercap - minimum power cap that can be set in watts
> + user_powerlimit - power limit specified by the user in watts
> +In addition, the OCC driver for P9 will export a 7th entry:
> + user_powerlimit_source - can be one of two values depending on who set
> + the user_powerlimit. 0x1 - out of band from BMC or host. 0x2 -
> + in band from other source.
> +The format for these files is caps<sensor index>_<entry type>. For example,
> +caps1_curr_powercap.
> +
> +The driver also provides a number of sysfs entries through hwmon to better
> +control the driver and monitor the OCC.
> + powercap - read or write the OCC user power limit in watts.
> + name - read the name of the driver
> + update_interval - read or write the minimum interval for polling the
> + OCC.
> +
> +The driver also exports a single sysfs file through the communication protocol
> +device (see BMC - Host Communications). The filename is "online" and represents
> +the status of the OCC with respect to the driver. The OCC can be in one of two
> +states: OCC polling enabled or OCC polling disabled. The purpose of this file
> +is to control the behavior of the driver and it's hwmon sysfs entries, not to
> +infer any information about the state of the physical OCC. Reading the file
> +returns either a 0 (polling disabled) or 1 (polling enabled). Writing 1 to the
> +file enables OCC polling in the driver if communications can be established
> +with the OCC. Writing a 0 to the driver disables OCC polling.
> +
> BMC - Host Communications
> -------------------------
>
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index 93cb52f..a6881f9 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
> new file mode 100644
> index 0000000..b0e063da
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.c
> @@ -0,0 +1,492 @@
> +/*
> + * occ_sysfs.c - OCC sysfs interface
> + *
> + * This file contains the methods and data structures for implementing the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +#include "occ_sysfs.h"
> +
> +#define MAX_SENSOR_ATTR_LEN 32
> +
> +#define RESP_RETURN_CMD_INVAL 0x13
> +
> +struct sensor_attr_data {
> + enum sensor_type type;
> + u32 hwmon_index;
> + u32 attr_id;
> + char name[MAX_SENSOR_ATTR_LEN];
> + struct device_attribute dev_attr;
> +};
> +
> +static ssize_t show_input(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + val = occ_get_sensor_value(driver->occ, sdata->type,
> + sdata->hwmon_index - 1);
> + if (sdata->type == TEMP)
> + val *= 1000; /* in millidegree Celsius */
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +/* show_label provides the OCC sensor id. The sensor id will be either a
> + * 2-byte (for P8) or 4-byte (for P9) value. The sensor id is a way to
> + * identify what each sensor represents, according to the OCC specification.
> + */
> +static ssize_t show_label(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + val = occ_get_sensor_id(driver->occ, sdata->type,
> + sdata->hwmon_index - 1);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static ssize_t show_caps(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct caps_sensor *sensor;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + sensor = occ_get_sensor(driver->occ, CAPS);
> + if (!sensor) {
> + val = -1;
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> + }
> +
> + val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
> + sdata->attr_id);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static ssize_t show_update_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%lu\n", driver->update_interval);
> +}
> +
> +static ssize_t store_update_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + unsigned long val;
> + int rc;
> +
> + rc = kstrtoul(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + driver->update_interval = val;
> + occ_set_update_interval(driver->occ, val);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_update_interval,
> + store_update_interval);
> +
> +static ssize_t show_name(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE - 1, "occ\n");
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
> +static ssize_t show_user_powercap(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->user_powercap);
> +}
> +
> +static ssize_t store_user_powercap(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + u16 val;
> + int rc;
> +
> + rc = kstrtou16(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + dev_dbg(dev, "set user powercap to: %d\n", val);
> + rc = occ_set_user_powercap(driver->occ, val);
> + if (rc) {
> + dev_err(dev, "set user powercap failed: 0x%x\n", rc);
> + if (rc == RESP_RETURN_CMD_INVAL) {
> + dev_err(dev, "set invalid powercap value: %d\n", val);
> + return -EINVAL;
> + }
> +
> + return rc;
> + }
> +
> + driver->user_powercap = val;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(user_powercap, S_IWUSR | S_IRUGO, show_user_powercap,
> + store_user_powercap);
> +
> +static void deinit_sensor_groups(struct device *dev,
> + struct sensor_group *sensor_groups)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> + if (sensor_groups[i].group.attrs)
> + devm_kfree(dev, sensor_groups[i].group.attrs);
> + if (sensor_groups[i].sattr)
> + devm_kfree(dev, sensor_groups[i].sattr);
> + sensor_groups[i].group.attrs = NULL;
> + sensor_groups[i].sattr = NULL;
> + }
> +}
> +
> +static void sensor_attr_init(struct sensor_attr_data *sdata,
> + char *sensor_group_name,
> + char *attr_name,
> + ssize_t (*show)(struct device *dev,
> + struct device_attribute *attr,
> + char *buf))
> +{
> + sysfs_attr_init(&sdata->dev_attr.attr);
> +
> + snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
> + sensor_group_name, sdata->hwmon_index, attr_name);
> + sdata->dev_attr.attr.name = sdata->name;
> + sdata->dev_attr.attr.mode = S_IRUGO;
> + sdata->dev_attr.show = show;
> +}
> +
> +static int create_sensor_group(struct occ_sysfs *driver,
> + enum sensor_type type, int sensor_num)
> +{
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + struct sensor_attr_data *sdata;
> + int rc, i;
> +
> + /* each sensor has 'label' and 'input' attributes */
> + sensor_groups[type].group.attrs =
> + devm_kzalloc(dev, sizeof(struct attribute *) *
> + sensor_num * 2 + 1, GFP_KERNEL);
> + if (!sensor_groups[type].group.attrs) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + sensor_groups[type].sattr =
> + devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
> + sensor_num * 2, GFP_KERNEL);
> + if (!sensor_groups[type].sattr) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + for (i = 0; i < sensor_num; i++) {
> + sdata = &sensor_groups[type].sattr[i];
> + /* hwmon attributes index starts from 1 */
> + sdata->hwmon_index = i + 1;
> + sdata->type = type;
> + sensor_attr_init(sdata, sensor_groups[type].name, "input",
> + show_input);
> + sensor_groups[type].group.attrs[i] = &sdata->dev_attr.attr;
> +
> + sdata = &sensor_groups[type].sattr[i + sensor_num];
> + sdata->hwmon_index = i + 1;
> + sdata->type = type;
> + sensor_attr_init(sdata, sensor_groups[type].name, "label",
> + show_label);
> + sensor_groups[type].group.attrs[i + sensor_num] =
> + &sdata->dev_attr.attr;
> + }
> +
> + rc = sysfs_create_group(&dev->kobj, &sensor_groups[type].group);
> + if (rc)
> + goto err;
> +
> + return 0;
> +err:
> + deinit_sensor_groups(dev, sensor_groups);
> + return rc;
> +}
> +
> +static void caps_sensor_attr_init(struct sensor_attr_data *sdata,
> + char *attr_name, uint32_t hwmon_index,
> + uint32_t attr_id)
> +{
> + sdata->type = CAPS;
> + sdata->hwmon_index = hwmon_index;
> + sdata->attr_id = attr_id;
> +
> + snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
> + "caps", sdata->hwmon_index, attr_name);
> +
> + sysfs_attr_init(&sdata->dev_attr.attr);
> + sdata->dev_attr.attr.name = sdata->name;
> + sdata->dev_attr.attr.mode = S_IRUGO;
> + sdata->dev_attr.show = show_caps;
> +}
> +
> +static int create_caps_sensor_group(struct occ_sysfs *driver, int sensor_num)
> +{
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + int field_num = driver->num_caps_fields;
> + struct sensor_attr_data *sdata;
> + int i, j, rc;
> +
> + sensor_groups[CAPS].group.attrs =
> + devm_kzalloc(dev, sizeof(struct attribute *) * sensor_num *
> + field_num + 1, GFP_KERNEL);
> + if (!sensor_groups[CAPS].group.attrs) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + sensor_groups[CAPS].sattr =
> + devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
> + sensor_num * field_num, GFP_KERNEL);
> + if (!sensor_groups[CAPS].sattr) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + for (j = 0; j < sensor_num; ++j) {
> + for (i = 0; i < field_num; ++i) {
> + sdata = &sensor_groups[CAPS].sattr[j * field_num + i];
> + caps_sensor_attr_init(sdata,
> + driver->caps_names[i], j + 1, i);
> + sensor_groups[CAPS].group.attrs[j * field_num + i] =
> + &sdata->dev_attr.attr;
> + }
> + }
> +
> + rc = sysfs_create_group(&dev->kobj, &sensor_groups[CAPS].group);
> + if (rc)
> + goto err;
> +
> + return rc;
> +err:
> + deinit_sensor_groups(dev, sensor_groups);
> + return rc;
> +}
> +
> +static void occ_remove_hwmon_attrs(struct occ_sysfs *driver)
> +{
> + struct device *dev = driver->dev;
> +
> + device_remove_file(dev, &dev_attr_user_powercap);
> + device_remove_file(dev, &dev_attr_update_interval);
> + device_remove_file(dev, &dev_attr_name);
> +}
> +
> +static int occ_create_hwmon_attrs(struct occ_sysfs *driver)
> +{
> + int i, rc, id, sensor_num;
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + struct occ_blocks *resp = NULL;
> +
> + occ_get_response_blocks(driver->occ, &resp);
> +
> + for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> + resp->sensor_block_id[i] = -1;
> +
> + /* read sensor data from occ */
> + rc = occ_update_device(driver->occ);
> + if (rc) {
> + dev_err(dev, "cannot get occ sensor data: %d\n", rc);
> + return rc;
> + }
> + if (!resp->blocks)
> + return -ENOMEM;
> +
> + rc = device_create_file(dev, &dev_attr_name);
> + if (rc)
> + goto error;
> +
> + rc = device_create_file(dev, &dev_attr_update_interval);
> + if (rc)
> + goto error;
> +
> + if (resp->sensor_block_id[CAPS] >= 0) {
> + /* user powercap: only for master OCC */
> + rc = device_create_file(dev, &dev_attr_user_powercap);
> + if (rc)
> + goto error;
> + }
> +
> + sensor_groups[FREQ].name = "freq";
> + sensor_groups[TEMP].name = "temp";
> + sensor_groups[POWER].name = "power";
> + sensor_groups[CAPS].name = "caps";
> +
> + for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> + id = resp->sensor_block_id[i];
> + if (id < 0)
> + continue;
> +
> + sensor_num = resp->blocks[id].header.sensor_num;
> + if (i == CAPS)
> + rc = create_caps_sensor_group(driver, sensor_num);
> + else
> + rc = create_sensor_group(driver, i, sensor_num);
> + if (rc)
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + dev_err(dev, "cannot create hwmon attributes: %d\n", rc);
> + occ_remove_hwmon_attrs(driver);
> + return rc;
> +}
> +
> +static ssize_t show_occ_online(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->occ_online);
> +}
> +
> +static ssize_t store_occ_online(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + unsigned long val;
> + int rc;
> +
> + rc = kstrtoul(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + if (val == 1) {
> + if (driver->occ_online)
> + return count;
> +
> + driver->dev = hwmon_device_register(dev);
hwmon_device_register() is deprecated. Please consider using
devm_hwmon_device_register_with_info() or at least
hwmon_device_register_with_info().
Uuh ... and registering a hwmon device based on writing into a sysfs attribute
is completely out of the question.
Thanks,
Guenter
> + if (IS_ERR(driver->dev))
> + return PTR_ERR(driver->dev);
> +
> + dev_set_drvdata(driver->dev, driver);
> +
> + rc = occ_create_hwmon_attrs(driver);
> + if (rc) {
> + hwmon_device_unregister(driver->dev);
> + driver->dev = NULL;
> + return rc;
> + }
> + } else if (val == 0) {
> + if (!driver->occ_online)
> + return count;
> +
> + occ_remove_hwmon_attrs(driver);
> + hwmon_device_unregister(driver->dev);
> + driver->dev = NULL;
> + } else
> + return -EINVAL;
> +
> + driver->occ_online = val;
> + return count;
> +}
> +
> +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
> + store_occ_online);
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + struct occ_sysfs_config *config)
> +{
> + struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> + GFP_KERNEL);
> + int rc;
> +
> + if (!hwmon)
> + return ERR_PTR(-ENOMEM);
> +
> + hwmon->occ = occ;
> + hwmon->num_caps_fields = config->num_caps_fields;
> + hwmon->caps_names = config->caps_names;
> +
> + dev_set_drvdata(dev, hwmon);
> +
> + rc = device_create_file(dev, &dev_attr_online);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + return hwmon;
> +}
> +EXPORT_SYMBOL(occ_sysfs_start);
> +
> +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
> +{
> + if (driver->dev) {
> + occ_remove_hwmon_attrs(driver);
> + hwmon_device_unregister(driver->dev);
> + }
> +
> + device_remove_file(driver->dev, &dev_attr_online);
> +
> + devm_kfree(dev, driver);
Thw point of using devm_ functions is not to require remove/free functions.
Something is completely wrong here if you need that call.
Overall, this is architectually completely wrong. One does not register
or instantiate drivers based on writing into sysfs attributes. Please
reconsider your approach.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(occ_sysfs_stop);
> +
> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
> +MODULE_DESCRIPTION("OCC sysfs driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
> new file mode 100644
> index 0000000..2a8044f
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.h
> @@ -0,0 +1,52 @@
> +/*
> + * occ_sysfs.h - OCC sysfs interface
> + *
> + * This file contains the data structures and function prototypes for the OCC
> + * hwmon sysfs entries.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#ifndef __OCC_SYSFS_H__
> +#define __OCC_SYSFS_H__
> +
> +#include "occ.h"
> +
> +struct sensor_group {
> + char *name;
> + struct sensor_attr_data *sattr;
> + struct attribute_group group;
> +};
> +
> +struct occ_sysfs_config {
> + unsigned int num_caps_fields;
> + char **caps_names;
> +};
> +
> +struct occ_sysfs {
> + struct device *dev;
> + struct occ *occ;
> +
> + u16 user_powercap;
> + bool occ_online;
> + struct sensor_group sensor_groups[MAX_OCC_SENSOR_TYPE];
> + unsigned long update_interval;
> + unsigned int num_caps_fields;
> + char **caps_names;
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + struct occ_sysfs_config *config);
> +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver);
> +
> +#endif /* __OCC_SYSFS_H__ */
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH linux 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
From: kbuild test robot @ 2016-12-30 19:18 UTC (permalink / raw)
To: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w
Cc: kbuild-all-JC7UmRfGjtg, linux-0h96xk9xTtrk1uMJSBkQmQ,
jdelvare-IBi9RG/b67k, corbet-T1hC0tSOHrs,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wsa-z923LK4zBo2bacvFa/9K2g, andrew-zrmu5oMJ5Fs,
joel-U3u1mxZcP9KHXe+LvDLADg, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Edward A. James
In-Reply-To: <1483120568-21082-2-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]
Hi Edward,
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v4.10-rc1 next-20161224]
[cannot apply to linux/master]
[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/eajames-ibm-gmail-com/drivers-hwmon-Add-On-Chip-Controller-driver/20161231-021324
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All warnings (new ones prefixed by >>):
drivers/hwmon/occ/occ.c: In function 'occ_get_all':
>> drivers/hwmon/occ/occ.c:390:44: warning: passing argument 5 of 'occ_send_cmd' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
^
drivers/hwmon/occ/occ.c:281:11: note: expected 'u8 * {aka unsigned char *}' but argument is of type 'const u8 * {aka const unsigned char *}'
static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
^~~~~~~~~~~~
vim +390 drivers/hwmon/occ/occ.c
374 return rc;
375 }
376
377 static int occ_get_all(struct occ *driver)
378 {
379 int i = 0, rc;
380 u8 *occ_data;
381 u16 num_bytes;
382 const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
383 struct device *dev = driver->dev;
384 struct occ_response *resp = &driver->response;
385
386 occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
387 if (!occ_data)
388 return -ENOMEM;
389
> 390 rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
391 if (rc) {
392 dev_err(dev, "OCC poll failed: %d\n", rc);
393 goto out;
394 }
395
396 num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
397 num_bytes = be16_to_cpu(num_bytes);
398 dev_dbg(dev, "OCC data length: %d\n", num_bytes);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45853 bytes --]
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Jacob Pan @ 2016-12-30 18:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Chen-Yu Tsai, Lars-Peter Clausen,
Peter Meerwald-Stadler, russianneuromancer @ ya . ru,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <1f31b2e7-90fa-0fb8-5f6e-a8ee2ddf69f7-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Fri, 30 Dec 2016 16:46:03 +0000
Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On 14/12/16 13:55, Hans de Goede wrote:
> > For some reason the axp288_adc driver was modifying the
> > AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
> > whether the GP_ADC channel or another channel was written.
> >
> > These bits control when a bias current is send to the TS_PIN, the
> > GP_ADC has its own pin and a separate bit in another register to
> > control the bias current.
> >
It has been a while. Just looked at the datasheet, reg 84h is for ADC
and TS pin control. IIRC, we had to set the TS bits in order to make
ADC read to work. What is the other register?
> > Not only does changing when to enable the TS_PIN bias current
> > (always or only when sampling) when reading the GP_ADC make no sense
> > at all, the code is modifying these bits is writing the entire
> > register, assuming that all the other bits have their default value.
> >
Agreed, it would be better to do read-modify-write and not to
touch the other bits.
> > So if the firmware has configured a different bias-current for
> > either pin, then that change gets clobbered by the write, likewise
> > if the firmware has set bit 2 to indicate that the battery has no
> > thermal sensor, this will get clobbered by the write.
> >
> > This commit fixes all this, by simply removing all writes to the
> > AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
> > GP_ADC pin, and can actually be harmful.
> >
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> I guess you probably have more up to date contact details than I do,
> but seems worth trying to cc Jacob Pan on this to see if we can find
> out what the original reasoning behind this was. Seems a very odd
> thing to do with no purpose!
>
> If Jacob isn't contactable we'll fall back to guessing it was just
> an oddity of driver evolution.
>
> Jonathan
> > ---
> > drivers/iio/adc/axp288_adc.c | 32 +-------------------------------
> > 1 file changed, 1 insertion(+), 31 deletions(-)
> >
> > diff --git a/drivers/iio/adc/axp288_adc.c
> > b/drivers/iio/adc/axp288_adc.c index 7fd2494..64799ad 100644
> > --- a/drivers/iio/adc/axp288_adc.c
> > +++ b/drivers/iio/adc/axp288_adc.c
> > @@ -28,8 +28,6 @@
> > #include <linux/iio/driver.h>
> >
> > #define AXP288_ADC_EN_MASK 0xF1
> > -#define AXP288_ADC_TS_PIN_GPADC 0xF2
> > -#define AXP288_ADC_TS_PIN_ON 0xF3
> >
> > enum axp288_adc_id {
> > AXP288_ADC_TS,
> > @@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val,
> > unsigned long address, return IIO_VAL_INT;
> > }
> >
> > -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int
> > mode,
> > - unsigned long address)
> > -{
> > - /* channels other than GPADC do not need to switch TS pin
> > */
> > - if (address != AXP288_GP_ADC_H)
> > - return 0;
> > -
> > - return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
> > -}
> > -
> > static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int *val, int *val2, long mask)
> > @@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev
> > *indio_dev, mutex_lock(&indio_dev->mlock);
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > - if (axp288_adc_set_ts(info->regmap,
> > AXP288_ADC_TS_PIN_GPADC,
> > - chan->address)) {
> > - dev_err(&indio_dev->dev, "GPADC mode\n");
> > - ret = -EINVAL;
> > - break;
> > - }
> > ret = axp288_adc_read_channel(val, chan->address,
> > info->regmap);
> > - if (axp288_adc_set_ts(info->regmap,
> > AXP288_ADC_TS_PIN_ON,
> > - chan->address))
> > - dev_err(&indio_dev->dev, "TS pin
> > restore\n"); break;
> > default:
> > ret = -EINVAL;
> > @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev
> > *indio_dev, return ret;
> > }
> >
> > -static int axp288_adc_set_state(struct regmap *regmap)
> > -{
> > - /* ADC should be always enabled for internal FG to
> > function */
> > - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
> > AXP288_ADC_TS_PIN_ON))
> > - return -EIO;
> > -
> > - return regmap_write(regmap, AXP20X_ADC_EN1,
> > AXP288_ADC_EN_MASK); -}
> > -
> > static const struct iio_info axp288_adc_iio_info = {
> > .read_raw = &axp288_adc_read_raw,
> > .driver_module = THIS_MODULE,
> > @@ -199,7 +169,7 @@ static int axp288_adc_probe(struct
> > platform_device *pdev)
> > * Set ADC to enabled state at all time, including system
> > suspend.
> > * otherwise internal fuel gauge functionality may be
> > affected. */
> > - ret = axp288_adc_set_state(axp20x->regmap);
> > + ret = regmap_write(info->regmap, AXP20X_ADC_EN1,
> > AXP288_ADC_EN_MASK); if (ret) {
> > dev_err(&pdev->dev, "unable to enable ADC
> > device\n"); return ret;
> >
>
[Jacob Pan]
^ permalink raw reply
* [PATCH linux 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures
From: eajames.ibm @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux
Cc: jdelvare, corbet, mark.rutland, robh+dt, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add functions to parse the data structures that are specific to the OCC on
the POWER9 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 3 +
drivers/hwmon/occ/p9_occ.c | 243 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/p9_occ.h | 30 ++++++
3 files changed, 276 insertions(+)
create mode 100644 drivers/hwmon/occ/p9_occ.c
create mode 100644 drivers/hwmon/occ/p9_occ.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index a6b3dd6..0f17c2f 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -34,6 +34,9 @@ number of data structures, such as command format, response headers, and the
like, are also defined in this specification, and are common to both POWER8 and
POWER9 OCCs.
+There is currently no public P9 OCC specification, and the data structures
+defined in the POWER9 OCC driver are subject to change.
+
sysfs Entries
-------------
diff --git a/drivers/hwmon/occ/p9_occ.c b/drivers/hwmon/occ/p9_occ.c
new file mode 100644
index 0000000..f69b469
--- /dev/null
+++ b/drivers/hwmon/occ/p9_occ.c
@@ -0,0 +1,243 @@
+/*
+ * p9.c - OCC hwmon driver
+ *
+ * This file contains the Power9-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <asm/unaligned.h>
+
+#include "occ.h"
+#include "occ_p9.h"
+
+/* P9 OCC sensor data format */
+struct p9_temp_sensor {
+ u32 sensor_id;
+ u8 fru_type;
+ u8 value;
+};
+
+struct p9_freq_sensor {
+ u32 sensor_id;
+ u16 value;
+};
+
+struct p9_power_sensor {
+ u32 sensor_id;
+ u8 function_id;
+ u8 apss_channel;
+ u16 reserved;
+ u32 update_tag;
+ u64 accumulator;
+ u16 value;
+};
+
+struct p9_caps_sensor {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+ u8 user_powerlimit_source;
+};
+
+void p9_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+ int snum)
+{
+ switch (sensor_type) {
+ case FREQ:
+ {
+ struct p9_freq_sensor *fs =
+ &(((struct p9_freq_sensor *)sensor)[snum]);
+
+ fs->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ fs->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ }
+ break;
+ case TEMP:
+ {
+ struct p9_temp_sensor *ts =
+ &(((struct p9_temp_sensor *)sensor)[snum]);
+
+ ts->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ fs->fru_type = data[off + 4];
+ fs->value = data[off + 5];
+ }
+ break;
+ case POWER:
+ {
+ struct p9_power_sensor *ps =
+ &(((struct p9_power_sensor *)sensor)[snum]);
+
+ ps->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ ps->function_id = data[off + 4];
+ ps->apss_channel = data[off + 5];
+ ps->update_tag =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 8]));
+ ps->accumulator =
+ be64_to_cpu(get_unaligned((u64 *)&data[off + 12]));
+ ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 20]));
+ }
+ break;
+ case CAPS:
+ {
+ struct p9_caps_sensor *cs =
+ &(((struct p9_caps_sensor *)sensor)[snum]);
+
+ cs->curr_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ cs->curr_powerreading =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ cs->norm_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ cs->max_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+ cs->min_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+ cs->user_powerlimit =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ cs->user_powerlimit_source = data[off + 12];
+ }
+ break;
+ };
+}
+
+void *p9_alloc_sensor(int sensor_type, int num_sensors)
+{
+ switch (sensor_type) {
+ case FREQ:
+ return kcalloc(num_sensors, sizeof(struct p9_freq_sensor),
+ GFP_KERNEL);
+ case TEMP:
+ return kcalloc(num_sensors, sizeof(struct p9_temp_sensor),
+ GFP_KERNEL);
+ case POWER:
+ return kcalloc(num_sensors, sizeof(struct p9_power_sensor),
+ GFP_KERNEL);
+ case CAPS:
+ return kcalloc(num_sensors, sizeof(struct p9_caps_sensor),
+ GFP_KERNEL);
+ default:
+ return NULL;
+ }
+}
+
+int p9_get_sensor_value(struct occ *driver, int sensor_type, int snum)
+{
+ void *sensor;
+
+ if (sensor_type == CAPS)
+ return -EINVAL;
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ return ((struct p9_freq_sensor *)sensor)[snum].value;
+ case TEMP:
+ return ((struct p9_temp_sensor *)sensor)[snum].value;
+ case POWER:
+ return ((struct p9_power_sensor *)sensor)[snum].value;
+ default:
+ return -EINVAL;
+ }
+}
+
+int p9_get_sensor_id(struct occ *driver, int sensor_type, int snum)
+{
+ void *sensor;
+
+ if (sensor_type == CAPS)
+ return -EINVAL;
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ return ((struct p9_freq_sensor *)sensor)[snum].sensor_id;
+ case TEMP:
+ return ((struct p9_temp_sensor *)sensor)[snum].sensor_id;
+ case POWER:
+ return ((struct p9_power_sensor *)sensor)[snum].sensor_id;
+ default:
+ return -EINVAL;
+ }
+}
+
+int p9_get_caps_value(void *sensor, int snum, int caps_field)
+{
+ struct p9_caps_sensor *caps_sensor = sensor;
+
+ switch (caps_field) {
+ case 0:
+ return caps_sensor[snum].curr_powercap;
+ case 1:
+ return caps_sensor[snum].curr_powerreading;
+ case 2:
+ return caps_sensor[snum].norm_powercap;
+ case 3:
+ return caps_sensor[snum].max_powercap;
+ case 4:
+ return caps_sensor[snum].min_powercap;
+ case 5:
+ return caps_sensor[snum].user_powerlimit;
+ case 6:
+ return caps_sensor[snum].user_powerlimit_source;
+ default:
+ return -EINVAL;
+ }
+}
+static const struct occ_ops p9_ops = {
+ .parse_sensor = p9_parse_sensor,
+ .alloc_sensor = p9_alloc_sensor,
+ .get_sensor_value = p9_get_sensor_value,
+ .get_sensor_id = p9_get_sensor_id,
+ .get_caps_value = p9_get_caps_value,
+};
+
+static const struct occ_config p9_config = {
+ .command_addr = 0xFFFBE000,
+ .response_addr = 0xFFFBF000,
+};
+
+struct occ *p9_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops)
+{
+ return occ_start(dev, bus, bus_ops, &p9_ops, &p9_config);
+}
+EXPORT_SYMBOL(occ_p9_start);
+
+int p9_occ_stop(struct occ *occ)
+{
+ return occ_stop(occ);
+}
+EXPORT_SYMBOL(occ_p9_stop);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("P9 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/p9_occ.h b/drivers/hwmon/occ/p9_occ.h
new file mode 100644
index 0000000..8130873
--- /dev/null
+++ b/drivers/hwmon/occ/p9_occ.h
@@ -0,0 +1,30 @@
+/*
+ * occ_p9.h - OCC hwmon driver
+ *
+ * This file contains Power9-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_P9_H__
+#define __OCC_P9_H__
+
+#include "scom.h"
+
+struct device;
+
+struct occ *p9_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops);
+int p9_occ_stop(struct occ *occ);
+
+#endif /* __OCC_P9_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux-0h96xk9xTtrk1uMJSBkQmQ
Cc: jdelvare-IBi9RG/b67k, corbet-T1hC0tSOHrs,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wsa-z923LK4zBo2bacvFa/9K2g, andrew-zrmu5oMJ5Fs,
joel-U3u1mxZcP9KHXe+LvDLADg, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
well as probe the entire driver from the I2C bus. I2C is the communication
method between the BMC and the P8 OCC.
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
Reviewed-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
---
.../devicetree/bindings/i2c/i2c-ibm-occ.txt | 13 ++
drivers/hwmon/occ/Kconfig | 14 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/p8_occ_i2c.c | 141 +++++++++++++++++++++
4 files changed, 169 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
diff --git a/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt b/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
new file mode 100644
index 0000000..b0d2b36
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
@@ -0,0 +1,13 @@
+HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
+
+Required properties:
+ - compatible: must be "ibm,p8-occ-i2c"
+ - reg: physical address
+
+Example:
+i2c3: i2c-bus@100 {
+ occ@50 {
+ compatible = "ibm,p8-occ-i2c";
+ reg = <0x50>;
+ };
+};
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
index cdb64a7..3a5188f 100644
--- a/drivers/hwmon/occ/Kconfig
+++ b/drivers/hwmon/occ/Kconfig
@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
This driver can also be built as a module. If so, the module
will be called occ.
+
+if SENSORS_PPC_OCC
+
+config SENSORS_PPC_OCC_P8_I2C
+ tristate "POWER8 OCC hwmon support"
+ depends on I2C
+ help
+ Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
+ exposing temperature, frequency and power measurements.
+
+ This driver can also be built as a module. If so, the module will be
+ called p8-occ-i2c.
+
+endif
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index a6881f9..9294b58 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
+obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
new file mode 100644
index 0000000..0c65894
--- /dev/null
+++ b/drivers/hwmon/occ/p8_occ_i2c.c
@@ -0,0 +1,141 @@
+/*
+ * p8_occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+#include "occ_p8.h"
+#include "occ_sysfs.h"
+
+#define P8_OCC_I2C_NAME "p8-occ-i2c"
+
+static char *caps_sensor_names[] = {
+ "curr_powercap",
+ "curr_powerreading",
+ "norm_powercap",
+ "max_powercap",
+ "min_powercap",
+ "user_powerlimit"
+};
+
+int p8_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+ /* P8 i2c slave requires address to be shifted by 1 */
+ address = address << 1;
+
+ return occ_i2c_getscom(bus, address, data);
+}
+
+int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+ /* P8 i2c slave requires address to be shifted by 1 */
+ address = address << 1;
+
+ return occ_i2c_putscom(bus, address, data0, data1);
+}
+
+static struct occ_bus_ops p8_bus_ops = {
+ .getscom = p8_i2c_getscom,
+ .putscom = p8_i2c_putscom,
+};
+
+static struct occ_sysfs_config p8_sysfs_config = {
+ .num_caps_fields = ARRAY_SIZE(caps_sensor_names),
+ .caps_names = caps_sensor_names,
+};
+
+static int p8_occ_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct occ *occ;
+ struct occ_sysfs *hwmon;
+
+ occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
+ if (IS_ERR(occ))
+ return PTR_ERR(occ);
+
+ hwmon = occ_sysfs_start(&client->dev, occ, &p8_sysfs_config);
+ if (IS_ERR(hwmon))
+ return PTR_ERR(hwmon);
+
+ i2c_set_clientdata(client, hwmon);
+
+ return 0;
+}
+
+static int p8_occ_remove(struct i2c_client *client)
+{
+ int rc;
+ struct occ_sysfs *hwmon = i2c_get_clientdata(client);
+ struct occ *occ = hwmon->occ;
+
+ rc = occ_sysfs_stop(&client->dev, hwmon);
+
+ rc = rc || p8_occ_stop(occ);
+
+ return rc;
+}
+
+/* used by old-style board info. */
+static const struct i2c_device_id occ_ids[] = {
+ { P8_OCC_I2C_NAME, 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, occ_ids);
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+ { .compatible = "ibm,p8-occ-i2c" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+/*
+ * i2c-core uses i2c-detect() to detect device in below address list.
+ * If exists, address will be assigned to client.
+ * It is also possible to read address from device table.
+ */
+static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
+
+static struct i2c_driver p8_occ_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = P8_OCC_I2C_NAME,
+ .pm = NULL,
+ .of_match_table = occ_of_match,
+ },
+ .probe = p8_occ_probe,
+ .remove = p8_occ_remove,
+ .id_table = occ_ids,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(p8_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH linux 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
From: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux-0h96xk9xTtrk1uMJSBkQmQ
Cc: jdelvare-IBi9RG/b67k, corbet-T1hC0tSOHrs,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wsa-z923LK4zBo2bacvFa/9K2g, andrew-zrmu5oMJ5Fs,
joel-U3u1mxZcP9KHXe+LvDLADg, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Add functions to parse the data structures that are specific to the OCC on
the POWER8 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
Reviewed-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
---
Documentation/hwmon/occ | 9 ++
drivers/hwmon/occ/occ_p8.c | 217 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_p8.h | 30 +++++++
3 files changed, 256 insertions(+)
create mode 100644 drivers/hwmon/occ/occ_p8.c
create mode 100644 drivers/hwmon/occ/occ_p8.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 1ee8689..a6b3dd6 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,15 @@ Currently, all versions of the OCC support four types of sensor data: power,
temperature, frequency, and "caps," which indicate limits and thresholds used
internally on the OCC.
+The format for the POWER8 OCC sensor data can be found in the P8 OCC
+specification:
+github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
+This document provides the details of the OCC sensors: power, frequency,
+temperature, and caps. These sensor formats are specific to the POWER8 OCC. A
+number of data structures, such as command format, response headers, and the
+like, are also defined in this specification, and are common to both POWER8 and
+POWER9 OCCs.
+
sysfs Entries
-------------
diff --git a/drivers/hwmon/occ/occ_p8.c b/drivers/hwmon/occ/occ_p8.c
new file mode 100644
index 0000000..812df16
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.c
@@ -0,0 +1,217 @@
+/*
+ * occ_p8.c - OCC hwmon driver
+ *
+ * This file contains the Power8-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <asm/unaligned.h>
+
+#include "occ.h"
+#include "occ_p8.h"
+
+/* P8 OCC sensor data format */
+struct p8_occ_sensor {
+ u16 sensor_id;
+ u16 value;
+};
+
+struct p8_power_sensor {
+ u16 sensor_id;
+ u32 update_tag;
+ u32 accumulator;
+ u16 value;
+};
+
+struct p8_caps_sensor {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+};
+
+void p8_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+ int snum)
+{
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ {
+ struct p8_occ_sensor *os =
+ &(((struct p8_occ_sensor *)sensor)[snum]);
+
+ os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ }
+ break;
+ case POWER:
+ {
+ struct p8_power_sensor *ps =
+ &(((struct p8_power_sensor *)sensor)[snum]);
+
+ ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ ps->update_tag =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));
+ ps->accumulator =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 6]));
+ ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ }
+ break;
+ case CAPS:
+ {
+ struct p8_caps_sensor *cs =
+ &(((struct p8_caps_sensor *)sensor)[snum]);
+
+ cs->curr_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ cs->curr_powerreading =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ cs->norm_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ cs->max_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+ cs->min_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+ cs->user_powerlimit =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ }
+ break;
+ };
+}
+
+void *p8_alloc_sensor(int sensor_type, int num_sensors)
+{
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ return kcalloc(num_sensors, sizeof(struct p8_occ_sensor),
+ GFP_KERNEL);
+ case POWER:
+ return kcalloc(num_sensors, sizeof(struct p8_power_sensor),
+ GFP_KERNEL);
+ case CAPS:
+ return kcalloc(num_sensors, sizeof(struct p8_caps_sensor),
+ GFP_KERNEL);
+ default:
+ return NULL;
+ }
+}
+
+int p8_get_sensor_value(struct occ *driver, int sensor_type, int snum)
+{
+ void *sensor;
+
+ if (sensor_type == CAPS)
+ return -EINVAL;
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ return ((struct p8_occ_sensor *)sensor)[snum].value;
+ case POWER:
+ return ((struct p8_power_sensor *)sensor)[snum].value;
+ default:
+ return -EINVAL;
+ }
+}
+
+int p8_get_sensor_id(struct occ *driver, int sensor_type, int snum)
+{
+ void *sensor;
+ int i = snum;
+
+ if (sensor_type == CAPS)
+ return -EINVAL;
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ return ((struct p8_occ_sensor *)sensor)[i].sensor_id;
+ case POWER:
+ return ((struct p8_power_sensor *)sensor)[i].sensor_id;
+ default:
+ return -EINVAL;
+ }
+}
+
+int p8_get_caps_value(void *sensor, int snum, int caps_field)
+{
+ struct p8_caps_sensor *caps_sensor = sensor;
+
+ switch (caps_field) {
+ case 0:
+ return caps_sensor[snum].curr_powercap;
+ case 1:
+ return caps_sensor[snum].curr_powerreading;
+ case 2:
+ return caps_sensor[snum].norm_powercap;
+ case 3:
+ return caps_sensor[snum].max_powercap;
+ case 4:
+ return caps_sensor[snum].min_powercap;
+ case 5:
+ return caps_sensor[snum].user_powerlimit;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct occ_ops p8_ops = {
+ .parse_sensor = p8_parse_sensor,
+ .alloc_sensor = p8_alloc_sensor,
+ .get_sensor_value = p8_get_sensor_value,
+ .get_sensor_id = p8_get_sensor_id,
+ .get_caps_value = p8_get_caps_value,
+};
+
+static const struct occ_config p8_config = {
+ .command_addr = 0xFFFF6000,
+ .response_addr = 0xFFFF7000,
+};
+
+struct occ *p8_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops)
+{
+ return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config);
+}
+EXPORT_SYMBOL(p8_occ_start);
+
+int p8_occ_stop(struct occ *occ)
+{
+ return occ_stop(occ);
+}
+EXPORT_SYMBOL(p8_occ_stop);
+
+MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("P8 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_p8.h b/drivers/hwmon/occ/occ_p8.h
new file mode 100644
index 0000000..b44cdd4
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.h
@@ -0,0 +1,30 @@
+/*
+ * occ_p8.h - OCC hwmon driver
+ *
+ * This file contains Power8-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_P8_H__
+#define __OCC_P8_H__
+
+#include "scom.h"
+
+struct device;
+
+struct occ *p8_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops);
+int p8_occ_stop(struct occ *occ);
+
+#endif /* __OCC_P8_H__ */
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH linux 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations
From: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux-0h96xk9xTtrk1uMJSBkQmQ
Cc: jdelvare-IBi9RG/b67k, corbet-T1hC0tSOHrs,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wsa-z923LK4zBo2bacvFa/9K2g, andrew-zrmu5oMJ5Fs,
joel-U3u1mxZcP9KHXe+LvDLADg, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Add functions to send SCOM operations over I2C bus. The BMC can
communicate with the Power8 host processor over I2C, but needs to use SCOM
operations in order to access the OCC register space.
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
---
drivers/hwmon/occ/occ_scom_i2c.c | 73 ++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++
2 files changed, 99 insertions(+)
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
new file mode 100644
index 0000000..a922f83
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.c
@@ -0,0 +1,73 @@
+/*
+ * occ_scom_i2c.c - hwmon OCC driver
+ *
+ * This file contains the functions for performing SCOM operations over I2C bus
+ * to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+ ssize_t rc;
+ u64 buf;
+ struct i2c_client *client = bus;
+
+ rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u32))
+ return -EIO;
+
+ rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u64))
+ return -EIO;
+
+ *data = be64_to_cpu(buf);
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+ u32 buf[3];
+ ssize_t rc;
+ struct i2c_client *client = bus;
+
+ buf[0] = address;
+ buf[1] = data1;
+ buf[2] = data0;
+
+ rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u32) * 3)
+ return -EIO;
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+
+MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
new file mode 100644
index 0000000..945739c
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.h
@@ -0,0 +1,26 @@
+/*
+ * occ_scom_i2c.h - hwmon OCC driver
+ *
+ * This file contains function protoypes for peforming SCOM operations over I2C
+ * bus to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_SCOM_I2C_H__
+#define __OCC_SCOM_I2C_H__
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
+
+#endif /* __OCC_SCOM_I2C_H__ */
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH linux 2/6] hwmon: occ: Add sysfs interface
From: eajames.ibm @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux
Cc: jdelvare, corbet, mark.rutland, robh+dt, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add a generic mechanism to expose the sensors provided by the OCC in
sysfs.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 48 +++++
drivers/hwmon/occ/Makefile | 2 +-
drivers/hwmon/occ/occ_sysfs.c | 492 ++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_sysfs.h | 52 +++++
4 files changed, 593 insertions(+), 1 deletion(-)
create mode 100644 drivers/hwmon/occ/occ_sysfs.c
create mode 100644 drivers/hwmon/occ/occ_sysfs.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 79d1642..1ee8689 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,54 @@ Currently, all versions of the OCC support four types of sensor data: power,
temperature, frequency, and "caps," which indicate limits and thresholds used
internally on the OCC.
+sysfs Entries
+-------------
+
+The OCC driver uses the hwmon sysfs framework to provide data to userspace.
+
+The driver exports two sysfs files for each frequency, temperature, and power
+sensor. These are "input" and "label". The input file contains the value of the
+sensor. The label file contains the sensor id. The sensor id is the unique
+internal OCC identifier. Sensor ids may be provided by the OCC specification.
+The names of these files will be in the following format:
+ <sensor type><sensor index>_input
+ <sensor type><sensor index>_label
+Sensor types will be one of "temp", "freq", or "power". The sensor index is
+an index to differentiate different sensor files. For example, a single
+temperature sensor will have two sysfs files: temp1_input and temp1_label.
+
+Caps sensors are exported differently. For each caps sensor, the driver will
+export 6 entries:
+ curr_powercap - current power cap in watts
+ curr_powerreading - current power output in watts
+ norm_powercap - power cap without redundant power
+ max_powercap - maximum power cap that can be set in watts
+ min_powercap - minimum power cap that can be set in watts
+ user_powerlimit - power limit specified by the user in watts
+In addition, the OCC driver for P9 will export a 7th entry:
+ user_powerlimit_source - can be one of two values depending on who set
+ the user_powerlimit. 0x1 - out of band from BMC or host. 0x2 -
+ in band from other source.
+The format for these files is caps<sensor index>_<entry type>. For example,
+caps1_curr_powercap.
+
+The driver also provides a number of sysfs entries through hwmon to better
+control the driver and monitor the OCC.
+ powercap - read or write the OCC user power limit in watts.
+ name - read the name of the driver
+ update_interval - read or write the minimum interval for polling the
+ OCC.
+
+The driver also exports a single sysfs file through the communication protocol
+device (see BMC - Host Communications). The filename is "online" and represents
+the status of the OCC with respect to the driver. The OCC can be in one of two
+states: OCC polling enabled or OCC polling disabled. The purpose of this file
+is to control the behavior of the driver and it's hwmon sysfs entries, not to
+infer any information about the state of the physical OCC. Reading the file
+returns either a 0 (polling disabled) or 1 (polling enabled). Writing 1 to the
+file enables OCC polling in the driver if communications can be established
+with the OCC. Writing a 0 to the driver disables OCC polling.
+
BMC - Host Communications
-------------------------
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index 93cb52f..a6881f9 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
new file mode 100644
index 0000000..b0e063da
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.c
@@ -0,0 +1,492 @@
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for implementing the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "occ_sysfs.h"
+
+#define MAX_SENSOR_ATTR_LEN 32
+
+#define RESP_RETURN_CMD_INVAL 0x13
+
+struct sensor_attr_data {
+ enum sensor_type type;
+ u32 hwmon_index;
+ u32 attr_id;
+ char name[MAX_SENSOR_ATTR_LEN];
+ struct device_attribute dev_attr;
+};
+
+static ssize_t show_input(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct sensor_attr_data *sdata = container_of(attr,
+ struct sensor_attr_data,
+ dev_attr);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ val = occ_get_sensor_value(driver->occ, sdata->type,
+ sdata->hwmon_index - 1);
+ if (sdata->type == TEMP)
+ val *= 1000; /* in millidegree Celsius */
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+/* show_label provides the OCC sensor id. The sensor id will be either a
+ * 2-byte (for P8) or 4-byte (for P9) value. The sensor id is a way to
+ * identify what each sensor represents, according to the OCC specification.
+ */
+static ssize_t show_label(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct sensor_attr_data *sdata = container_of(attr,
+ struct sensor_attr_data,
+ dev_attr);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ val = occ_get_sensor_id(driver->occ, sdata->type,
+ sdata->hwmon_index - 1);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_caps(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct caps_sensor *sensor;
+ struct sensor_attr_data *sdata = container_of(attr,
+ struct sensor_attr_data,
+ dev_attr);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ sensor = occ_get_sensor(driver->occ, CAPS);
+ if (!sensor) {
+ val = -1;
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+ }
+
+ val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
+ sdata->attr_id);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_update_interval(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%lu\n", driver->update_interval);
+}
+
+static ssize_t store_update_interval(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+ unsigned long val;
+ int rc;
+
+ rc = kstrtoul(buf, 10, &val);
+ if (rc)
+ return rc;
+
+ driver->update_interval = val;
+ occ_set_update_interval(driver->occ, val);
+
+ return count;
+}
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_update_interval,
+ store_update_interval);
+
+static ssize_t show_name(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE - 1, "occ\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static ssize_t show_user_powercap(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->user_powercap);
+}
+
+static ssize_t store_user_powercap(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+ u16 val;
+ int rc;
+
+ rc = kstrtou16(buf, 10, &val);
+ if (rc)
+ return rc;
+
+ dev_dbg(dev, "set user powercap to: %d\n", val);
+ rc = occ_set_user_powercap(driver->occ, val);
+ if (rc) {
+ dev_err(dev, "set user powercap failed: 0x%x\n", rc);
+ if (rc == RESP_RETURN_CMD_INVAL) {
+ dev_err(dev, "set invalid powercap value: %d\n", val);
+ return -EINVAL;
+ }
+
+ return rc;
+ }
+
+ driver->user_powercap = val;
+
+ return count;
+}
+
+static DEVICE_ATTR(user_powercap, S_IWUSR | S_IRUGO, show_user_powercap,
+ store_user_powercap);
+
+static void deinit_sensor_groups(struct device *dev,
+ struct sensor_group *sensor_groups)
+{
+ int i;
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+ if (sensor_groups[i].group.attrs)
+ devm_kfree(dev, sensor_groups[i].group.attrs);
+ if (sensor_groups[i].sattr)
+ devm_kfree(dev, sensor_groups[i].sattr);
+ sensor_groups[i].group.attrs = NULL;
+ sensor_groups[i].sattr = NULL;
+ }
+}
+
+static void sensor_attr_init(struct sensor_attr_data *sdata,
+ char *sensor_group_name,
+ char *attr_name,
+ ssize_t (*show)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf))
+{
+ sysfs_attr_init(&sdata->dev_attr.attr);
+
+ snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+ sensor_group_name, sdata->hwmon_index, attr_name);
+ sdata->dev_attr.attr.name = sdata->name;
+ sdata->dev_attr.attr.mode = S_IRUGO;
+ sdata->dev_attr.show = show;
+}
+
+static int create_sensor_group(struct occ_sysfs *driver,
+ enum sensor_type type, int sensor_num)
+{
+ struct device *dev = driver->dev;
+ struct sensor_group *sensor_groups = driver->sensor_groups;
+ struct sensor_attr_data *sdata;
+ int rc, i;
+
+ /* each sensor has 'label' and 'input' attributes */
+ sensor_groups[type].group.attrs =
+ devm_kzalloc(dev, sizeof(struct attribute *) *
+ sensor_num * 2 + 1, GFP_KERNEL);
+ if (!sensor_groups[type].group.attrs) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ sensor_groups[type].sattr =
+ devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+ sensor_num * 2, GFP_KERNEL);
+ if (!sensor_groups[type].sattr) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ for (i = 0; i < sensor_num; i++) {
+ sdata = &sensor_groups[type].sattr[i];
+ /* hwmon attributes index starts from 1 */
+ sdata->hwmon_index = i + 1;
+ sdata->type = type;
+ sensor_attr_init(sdata, sensor_groups[type].name, "input",
+ show_input);
+ sensor_groups[type].group.attrs[i] = &sdata->dev_attr.attr;
+
+ sdata = &sensor_groups[type].sattr[i + sensor_num];
+ sdata->hwmon_index = i + 1;
+ sdata->type = type;
+ sensor_attr_init(sdata, sensor_groups[type].name, "label",
+ show_label);
+ sensor_groups[type].group.attrs[i + sensor_num] =
+ &sdata->dev_attr.attr;
+ }
+
+ rc = sysfs_create_group(&dev->kobj, &sensor_groups[type].group);
+ if (rc)
+ goto err;
+
+ return 0;
+err:
+ deinit_sensor_groups(dev, sensor_groups);
+ return rc;
+}
+
+static void caps_sensor_attr_init(struct sensor_attr_data *sdata,
+ char *attr_name, uint32_t hwmon_index,
+ uint32_t attr_id)
+{
+ sdata->type = CAPS;
+ sdata->hwmon_index = hwmon_index;
+ sdata->attr_id = attr_id;
+
+ snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+ "caps", sdata->hwmon_index, attr_name);
+
+ sysfs_attr_init(&sdata->dev_attr.attr);
+ sdata->dev_attr.attr.name = sdata->name;
+ sdata->dev_attr.attr.mode = S_IRUGO;
+ sdata->dev_attr.show = show_caps;
+}
+
+static int create_caps_sensor_group(struct occ_sysfs *driver, int sensor_num)
+{
+ struct device *dev = driver->dev;
+ struct sensor_group *sensor_groups = driver->sensor_groups;
+ int field_num = driver->num_caps_fields;
+ struct sensor_attr_data *sdata;
+ int i, j, rc;
+
+ sensor_groups[CAPS].group.attrs =
+ devm_kzalloc(dev, sizeof(struct attribute *) * sensor_num *
+ field_num + 1, GFP_KERNEL);
+ if (!sensor_groups[CAPS].group.attrs) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ sensor_groups[CAPS].sattr =
+ devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+ sensor_num * field_num, GFP_KERNEL);
+ if (!sensor_groups[CAPS].sattr) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ for (j = 0; j < sensor_num; ++j) {
+ for (i = 0; i < field_num; ++i) {
+ sdata = &sensor_groups[CAPS].sattr[j * field_num + i];
+ caps_sensor_attr_init(sdata,
+ driver->caps_names[i], j + 1, i);
+ sensor_groups[CAPS].group.attrs[j * field_num + i] =
+ &sdata->dev_attr.attr;
+ }
+ }
+
+ rc = sysfs_create_group(&dev->kobj, &sensor_groups[CAPS].group);
+ if (rc)
+ goto err;
+
+ return rc;
+err:
+ deinit_sensor_groups(dev, sensor_groups);
+ return rc;
+}
+
+static void occ_remove_hwmon_attrs(struct occ_sysfs *driver)
+{
+ struct device *dev = driver->dev;
+
+ device_remove_file(dev, &dev_attr_user_powercap);
+ device_remove_file(dev, &dev_attr_update_interval);
+ device_remove_file(dev, &dev_attr_name);
+}
+
+static int occ_create_hwmon_attrs(struct occ_sysfs *driver)
+{
+ int i, rc, id, sensor_num;
+ struct device *dev = driver->dev;
+ struct sensor_group *sensor_groups = driver->sensor_groups;
+ struct occ_blocks *resp = NULL;
+
+ occ_get_response_blocks(driver->occ, &resp);
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+ resp->sensor_block_id[i] = -1;
+
+ /* read sensor data from occ */
+ rc = occ_update_device(driver->occ);
+ if (rc) {
+ dev_err(dev, "cannot get occ sensor data: %d\n", rc);
+ return rc;
+ }
+ if (!resp->blocks)
+ return -ENOMEM;
+
+ rc = device_create_file(dev, &dev_attr_name);
+ if (rc)
+ goto error;
+
+ rc = device_create_file(dev, &dev_attr_update_interval);
+ if (rc)
+ goto error;
+
+ if (resp->sensor_block_id[CAPS] >= 0) {
+ /* user powercap: only for master OCC */
+ rc = device_create_file(dev, &dev_attr_user_powercap);
+ if (rc)
+ goto error;
+ }
+
+ sensor_groups[FREQ].name = "freq";
+ sensor_groups[TEMP].name = "temp";
+ sensor_groups[POWER].name = "power";
+ sensor_groups[CAPS].name = "caps";
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+ id = resp->sensor_block_id[i];
+ if (id < 0)
+ continue;
+
+ sensor_num = resp->blocks[id].header.sensor_num;
+ if (i == CAPS)
+ rc = create_caps_sensor_group(driver, sensor_num);
+ else
+ rc = create_sensor_group(driver, i, sensor_num);
+ if (rc)
+ goto error;
+ }
+
+ return 0;
+
+error:
+ dev_err(dev, "cannot create hwmon attributes: %d\n", rc);
+ occ_remove_hwmon_attrs(driver);
+ return rc;
+}
+
+static ssize_t show_occ_online(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->occ_online);
+}
+
+static ssize_t store_occ_online(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+ unsigned long val;
+ int rc;
+
+ rc = kstrtoul(buf, 10, &val);
+ if (rc)
+ return rc;
+
+ if (val == 1) {
+ if (driver->occ_online)
+ return count;
+
+ driver->dev = hwmon_device_register(dev);
+ if (IS_ERR(driver->dev))
+ return PTR_ERR(driver->dev);
+
+ dev_set_drvdata(driver->dev, driver);
+
+ rc = occ_create_hwmon_attrs(driver);
+ if (rc) {
+ hwmon_device_unregister(driver->dev);
+ driver->dev = NULL;
+ return rc;
+ }
+ } else if (val == 0) {
+ if (!driver->occ_online)
+ return count;
+
+ occ_remove_hwmon_attrs(driver);
+ hwmon_device_unregister(driver->dev);
+ driver->dev = NULL;
+ } else
+ return -EINVAL;
+
+ driver->occ_online = val;
+ return count;
+}
+
+static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
+ store_occ_online);
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+ struct occ_sysfs_config *config)
+{
+ struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
+ GFP_KERNEL);
+ int rc;
+
+ if (!hwmon)
+ return ERR_PTR(-ENOMEM);
+
+ hwmon->occ = occ;
+ hwmon->num_caps_fields = config->num_caps_fields;
+ hwmon->caps_names = config->caps_names;
+
+ dev_set_drvdata(dev, hwmon);
+
+ rc = device_create_file(dev, &dev_attr_online);
+ if (rc)
+ return ERR_PTR(rc);
+
+ return hwmon;
+}
+EXPORT_SYMBOL(occ_sysfs_start);
+
+int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
+{
+ if (driver->dev) {
+ occ_remove_hwmon_attrs(driver);
+ hwmon_device_unregister(driver->dev);
+ }
+
+ device_remove_file(driver->dev, &dev_attr_online);
+
+ devm_kfree(dev, driver);
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_sysfs_stop);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("OCC sysfs driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
new file mode 100644
index 0000000..2a8044f
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.h
@@ -0,0 +1,52 @@
+/*
+ * occ_sysfs.h - OCC sysfs interface
+ *
+ * This file contains the data structures and function prototypes for the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_SYSFS_H__
+#define __OCC_SYSFS_H__
+
+#include "occ.h"
+
+struct sensor_group {
+ char *name;
+ struct sensor_attr_data *sattr;
+ struct attribute_group group;
+};
+
+struct occ_sysfs_config {
+ unsigned int num_caps_fields;
+ char **caps_names;
+};
+
+struct occ_sysfs {
+ struct device *dev;
+ struct occ *occ;
+
+ u16 user_powercap;
+ bool occ_online;
+ struct sensor_group sensor_groups[MAX_OCC_SENSOR_TYPE];
+ unsigned long update_interval;
+ unsigned int num_caps_fields;
+ char **caps_names;
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+ struct occ_sysfs_config *config);
+int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver);
+
+#endif /* __OCC_SYSFS_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
From: eajames.ibm @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux
Cc: jdelvare, corbet, mark.rutland, robh+dt, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
In-Reply-To: <1483120568-21082-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add core support for polling the OCC for it's sensor data and parsing that
data into sensor-specific information.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 40 ++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 15 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/occ.c | 544 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ.h | 86 +++++++
drivers/hwmon/occ/scom.h | 47 ++++
8 files changed, 736 insertions(+)
create mode 100644 Documentation/hwmon/occ
create mode 100644 drivers/hwmon/occ/Kconfig
create mode 100644 drivers/hwmon/occ/Makefile
create mode 100644 drivers/hwmon/occ/occ.c
create mode 100644 drivers/hwmon/occ/occ.h
create mode 100644 drivers/hwmon/occ/scom.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
new file mode 100644
index 0000000..79d1642
--- /dev/null
+++ b/Documentation/hwmon/occ
@@ -0,0 +1,40 @@
+Kernel driver occ
+=================
+
+Supported chips:
+ * ASPEED AST2400
+ * ASPEED AST2500
+
+Please note that the chip must be connected to a POWER8 or POWER9 processor
+(see the BMC - Host Communications section).
+
+Author: Eddie James <eajames@us.ibm.com>
+
+Description
+-----------
+
+This driver implements support for the OCC (On-Chip Controller) on the IBM
+POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
+OCC is an embedded processor that provides real time power and thermal
+monitoring.
+
+This driver provides an interface on a BMC to poll OCC sensor data, set user
+power caps, and perform some basic OCC error handling.
+
+Currently, all versions of the OCC support four types of sensor data: power,
+temperature, frequency, and "caps," which indicate limits and thresholds used
+internally on the OCC.
+
+BMC - Host Communications
+-------------------------
+
+For the POWER8 application, the BMC can communicate with the P8 over I2C bus.
+However, to access the OCC register space, any data transfer must use a SCOM
+operation. SCOM is a procedure to initiate a data transfer, typically of 8
+bytes. SCOMs consist of writing a 32-bit command register and then
+reading/writing two 32-bit data registers. This driver implements these
+SCOM operations over I2C bus in order to communicate with the OCC.
+
+For the POWER9 application, the BMC can communicate with the P9 over FSI bus
+and SBE engine. Once again, SCOM operations are required. This driver will
+implement SCOM ops over FSI/SBE. This will require the FSI driver.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 190d270..e80ca81 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1240,6 +1240,8 @@ config SENSORS_NSA320
This driver can also be built as a module. If so, the module
will be called nsa320-hwmon.
+source drivers/hwmon/occ/Kconfig
+
config SENSORS_PCF8591
tristate "Philips PCF8591 ADC/DAC"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d2cb7e8..c7ec5d4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
new file mode 100644
index 0000000..cdb64a7
--- /dev/null
+++ b/drivers/hwmon/occ/Kconfig
@@ -0,0 +1,15 @@
+#
+# On Chip Controller configuration
+#
+
+menuconfig SENSORS_PPC_OCC
+ bool "PPC On-Chip Controller"
+ help
+ If you say yes here you get support to monitor Power CPU
+ sensors via the On-Chip Controller (OCC).
+
+ Generally this is used by management controllers such as a BMC
+ on an OpenPower system.
+
+ This driver can also be built as a module. If so, the module
+ will be called occ.
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
new file mode 100644
index 0000000..93cb52f
--- /dev/null
+++ b/drivers/hwmon/occ/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
new file mode 100644
index 0000000..9884ddd
--- /dev/null
+++ b/drivers/hwmon/occ/occ.c
@@ -0,0 +1,544 @@
+/*
+ * occ.c - OCC hwmon driver
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <asm/unaligned.h>
+
+#include "occ.h"
+
+#define OCC_DATA_MAX 4096
+#define OCC_BMC_TIMEOUT_MS 20000
+
+/* To generate attn to OCC */
+#define ATTN_DATA 0x0006B035
+
+/* For BMC to read/write SRAM */
+#define OCB_ADDRESS 0x0006B070
+#define OCB_DATA 0x0006B075
+#define OCB_STATUS_CONTROL_AND 0x0006B072
+#define OCB_STATUS_CONTROL_OR 0x0006B073
+
+/* To init OCB */
+#define OCB_AND_INIT0 0xFBFFFFFF
+#define OCB_AND_INIT1 0xFFFFFFFF
+#define OCB_OR_INIT0 0x08000000
+#define OCB_OR_INIT1 0x00000000
+
+/* To generate attention on OCC */
+#define ATTN0 0x01010000
+#define ATTN1 0x00000000
+
+/* OCC return status */
+#define RESP_RETURN_CMD_IN_PRG 0xFF
+#define RESP_RETURN_SUCCESS 0
+#define RESP_RETURN_CMD_INVAL 0x11
+#define RESP_RETURN_CMD_LEN 0x12
+#define RESP_RETURN_DATA_INVAL 0x13
+#define RESP_RETURN_CHKSUM 0x14
+#define RESP_RETURN_OCC_ERR 0x15
+#define RESP_RETURN_STATE 0x16
+
+/* time interval to retry on "command in progress" return status */
+#define CMD_IN_PRG_INT_MS 100
+#define CMD_IN_PRG_RETRIES (OCC_BMC_TIMEOUT_MS / CMD_IN_PRG_INT_MS)
+
+/* OCC command definitions */
+#define OCC_POLL 0
+#define OCC_SET_USER_POWR_CAP 0x22
+
+/* OCC poll command data */
+#define OCC_POLL_STAT_SENSOR 0x10
+
+/* OCC response data offsets */
+#define RESP_RETURN_STATUS 2
+#define RESP_DATA_LENGTH 3
+#define RESP_HEADER_OFFSET 5
+#define SENSOR_STR_OFFSET 37
+#define SENSOR_BLOCK_NUM_OFFSET 43
+#define SENSOR_BLOCK_OFFSET 45
+
+/* occ_poll_header
+ * structure to match the raw occ poll response data
+ */
+struct occ_poll_header {
+ u8 status;
+ u8 ext_status;
+ u8 occs_present;
+ u8 config;
+ u8 occ_state;
+ u8 mode;
+ u8 ips_status;
+ u8 error_log_id;
+ u32 error_log_addr_start;
+ u16 error_log_length;
+ u8 reserved2;
+ u8 reserved3;
+ u8 occ_code_level[16];
+ u8 sensor_eye_catcher[6];
+ u8 sensor_block_num;
+ u8 sensor_data_version;
+} __attribute__((packed, aligned(4)));
+
+struct occ_response {
+ struct occ_poll_header header;
+ struct occ_blocks data;
+};
+
+struct occ {
+ struct device *dev;
+ void *bus;
+ struct occ_bus_ops bus_ops;
+ struct occ_ops ops;
+ struct occ_config config;
+ unsigned long update_interval;
+ unsigned long last_updated;
+ struct mutex update_lock;
+ struct occ_response response;
+ bool valid;
+};
+
+static void deinit_occ_resp_buf(struct occ_response *resp)
+{
+ int i;
+
+ if (!resp)
+ return;
+
+ if (!resp->data.blocks)
+ return;
+
+ for (i = 0; i < resp->header.sensor_block_num; ++i)
+ kfree(resp->data.blocks[i].sensors);
+
+ kfree(resp->data.blocks);
+
+ memset(resp, 0, sizeof(struct occ_response));
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+ resp->data.sensor_block_id[i] = -1;
+}
+
+static void *occ_get_sensor_by_type(struct occ_response *resp,
+ enum sensor_type t)
+{
+ if (!resp->data.blocks)
+ return NULL;
+
+ if (resp->data.sensor_block_id[t] == -1)
+ return NULL;
+
+ return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;
+}
+
+static int occ_check_sensor(struct occ *driver, u8 sensor_length,
+ u8 sensor_num, enum sensor_type t, int block)
+{
+ void *sensor;
+ int type_block_id;
+ struct occ_response *resp = &driver->response;
+
+ sensor = occ_get_sensor_by_type(resp, t);
+
+ /* empty sensor block, release older sensor data */
+ if (sensor_num == 0 || sensor_length == 0) {
+ kfree(sensor);
+ dev_err(driver->dev, "no sensor blocks available\n");
+ return -ENODATA;
+ }
+
+ type_block_id = resp->data.sensor_block_id[t];
+ if (!sensor || sensor_num !=
+ resp->data.blocks[type_block_id].header.sensor_num) {
+ kfree(sensor);
+ resp->data.blocks[block].sensors =
+ driver->ops.alloc_sensor(t, sensor_num);
+ if (!resp->data.blocks[block].sensors)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int parse_occ_response(struct occ *driver, u8 *data,
+ struct occ_response *resp)
+{
+ int b;
+ int s;
+ int rc;
+ int offset = SENSOR_BLOCK_OFFSET;
+ int sensor_type;
+ u8 sensor_block_num;
+ char sensor_type_string[5] = { 0 };
+ struct sensor_data_block_header *block;
+ struct device *dev = driver->dev;
+
+ /* check if the data is valid */
+ if (strncmp(&data[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
+ dev_err(dev, "no SENSOR string in response\n");
+ rc = -ENODATA;
+ goto err;
+ }
+
+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET];
+ if (sensor_block_num == 0) {
+ dev_err(dev, "no sensor blocks available\n");
+ rc = -ENODATA;
+ goto err;
+ }
+
+ /* if number of sensor block has changed, re-malloc */
+ if (sensor_block_num != resp->header.sensor_block_num) {
+ deinit_occ_resp_buf(resp);
+ resp->data.blocks = kcalloc(sensor_block_num,
+ sizeof(struct sensor_data_block),
+ GFP_KERNEL);
+ if (!resp->data.blocks)
+ return -ENOMEM;
+ }
+
+ memcpy(&resp->header, &data[RESP_HEADER_OFFSET],
+ sizeof(struct occ_poll_header));
+ resp->header.error_log_addr_start =
+ be32_to_cpu(resp->header.error_log_addr_start);
+ resp->header.error_log_length =
+ be16_to_cpu(resp->header.error_log_length);
+
+ dev_dbg(dev, "Reading %d sensor blocks\n",
+ resp->header.sensor_block_num);
+ for (b = 0; b < sensor_block_num; b++) {
+ block = (struct sensor_data_block_header *)&data[offset];
+ /* copy to a null terminated string */
+ strncpy(sensor_type_string, block->sensor_type, 4);
+ offset += 8;
+
+ dev_dbg(dev, "sensor block[%d]: type: %s, sensor_num: %d\n", b,
+ sensor_type_string, block->sensor_num);
+
+ if (strncmp(block->sensor_type, "FREQ", 4) == 0)
+ sensor_type = FREQ;
+ else if (strncmp(block->sensor_type, "TEMP", 4) == 0)
+ sensor_type = TEMP;
+ else if (strncmp(block->sensor_type, "POWR", 4) == 0)
+ sensor_type = POWER;
+ else if (strncmp(block->sensor_type, "CAPS", 4) == 0)
+ sensor_type = CAPS;
+ else {
+ dev_err(dev, "sensor type not supported %s\n",
+ sensor_type_string);
+ continue;
+ }
+
+ rc = occ_check_sensor(driver, block->sensor_length,
+ block->sensor_num, sensor_type, b);
+ if (rc == -ENOMEM)
+ goto err;
+ else if (rc)
+ continue;
+
+ resp->data.sensor_block_id[sensor_type] = b;
+ for (s = 0; s < block->sensor_num; s++) {
+ driver->ops.parse_sensor(data,
+ resp->data.blocks[b].sensors,
+ sensor_type, offset, s);
+ offset += block->sensor_length;
+ }
+
+ /* copy block data over to response pointer */
+ resp->data.blocks[b].header = *block;
+ }
+
+ return 0;
+err:
+ deinit_occ_resp_buf(resp);
+ return rc;
+}
+
+static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
+ u8 *data, u8 *resp)
+{
+ u32 cmd1, cmd2;
+ u16 checksum = 0;
+ u16 length_le = cpu_to_le16(length);
+ bool retry = 0;
+ int i, rc, tries = 0;
+
+ cmd1 = (seq << 24) | (type << 16) | length_le;
+ memcpy(&cmd2, data, length);
+ cmd2 <<= ((4 - length) * 8);
+
+ /* checksum: sum of every bytes of cmd1, cmd2 */
+ for (i = 0; i < 4; i++) {
+ checksum += (cmd1 >> (i * 8)) & 0xFF;
+ checksum += (cmd2 >> (i * 8)) & 0xFF;
+ }
+
+ cmd2 |= checksum << ((2 - length) * 8);
+
+ /* Init OCB */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
+ OCB_OR_INIT0, OCB_OR_INIT1);
+ if (rc)
+ goto err;
+
+ rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_AND,
+ OCB_AND_INIT0, OCB_AND_INIT1);
+ if (rc)
+ goto err;
+
+ /* Send command, 2nd half of the 64-bit addr is unused (write 0) */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+ driver->config.command_addr, 0);
+ if (rc)
+ goto err;
+
+ rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
+ if (rc)
+ goto err;
+
+ /* Trigger attention */
+ rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0, ATTN1);
+ if (rc)
+ goto err;
+
+ /* Get response data */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+ driver->config.response_addr, 0);
+ if (rc)
+ goto err;
+
+ do {
+ if (retry) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
+ }
+
+ rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+ (u64 *)resp);
+ if (rc)
+ goto err;
+
+ /* retry if we get "command in progress" return status */
+ retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) &&
+ (tries++ < CMD_IN_PRG_RETRIES);
+ } while (retry);
+
+ switch (resp[RESP_RETURN_STATUS]) {
+ case RESP_RETURN_CMD_IN_PRG:
+ rc = -EALREADY;
+ break;
+ case RESP_RETURN_SUCCESS:
+ rc = 0;
+ break;
+ case RESP_RETURN_CMD_INVAL:
+ case RESP_RETURN_CMD_LEN:
+ case RESP_RETURN_DATA_INVAL:
+ case RESP_RETURN_CHKSUM:
+ rc = -EINVAL;
+ break;
+ case RESP_RETURN_OCC_ERR:
+ rc = -EREMOTE;
+ break;
+ default:
+ rc = -EFAULT;
+ }
+
+ return rc;
+
+err:
+ dev_err(driver->dev, "scom op failed rc:%d\n", rc);
+ return rc;
+}
+
+static int occ_get_all(struct occ *driver)
+{
+ int i = 0, rc;
+ u8 *occ_data;
+ u16 num_bytes;
+ const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
+ struct device *dev = driver->dev;
+ struct occ_response *resp = &driver->response;
+
+ occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
+ if (!occ_data)
+ return -ENOMEM;
+
+ rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
+ if (rc) {
+ dev_err(dev, "OCC poll failed: %d\n", rc);
+ goto out;
+ }
+
+ num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
+ num_bytes = be16_to_cpu(num_bytes);
+ dev_dbg(dev, "OCC data length: %d\n", num_bytes);
+
+ if (num_bytes > OCC_DATA_MAX) {
+ dev_err(dev, "OCC data length must be < 4KB\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ if (num_bytes <= 0) {
+ dev_err(dev, "OCC data length is zero\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ /* read remaining data */
+ for (i = 8; i < num_bytes + 8; i += 8) {
+ rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+ (u64 *)&occ_data[i]);
+ if (rc) {
+ dev_err(dev, "scom op failed rc:%d\n", rc);
+ goto out;
+ }
+ }
+
+ /* don't need more sanity checks; buffer is alloc'd for max response
+ * size so we just check for valid data in parse_occ_response
+ */
+ rc = parse_occ_response(driver, occ_data, resp);
+
+out:
+ devm_kfree(dev, occ_data);
+ return rc;
+}
+
+int occ_update_device(struct occ *driver)
+{
+ int rc = 0;
+
+ mutex_lock(&driver->update_lock);
+
+ if (time_after(jiffies, driver->last_updated + driver->update_interval)
+ || !driver->valid) {
+ driver->valid = 1;
+
+ rc = occ_get_all(driver);
+ if (rc)
+ driver->valid = 0;
+
+ driver->last_updated = jiffies;
+ }
+
+ mutex_unlock(&driver->update_lock);
+
+ return rc;
+}
+EXPORT_SYMBOL(occ_update_device);
+
+void *occ_get_sensor(struct occ *driver, int sensor_type)
+{
+ int rc;
+
+ /* occ_update_device locks the update lock */
+ rc = occ_update_device(driver);
+ if (rc != 0) {
+ dev_err(driver->dev, "cannot get occ sensor data: %d\n",
+ rc);
+ return NULL;
+ }
+
+ return occ_get_sensor_by_type(&driver->response, sensor_type);
+}
+EXPORT_SYMBOL(occ_get_sensor);
+
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int snum)
+{
+ return occ->ops.get_sensor_value(occ, sensor_type, snum);
+}
+EXPORT_SYMBOL(occ_get_sensor_value);
+
+int occ_get_sensor_id(struct occ *occ, int sensor_type, int snum)
+{
+ return occ->ops.get_sensor_id(occ, sensor_type, snum);
+}
+EXPORT_SYMBOL(occ_get_sensor_id);
+
+int occ_get_caps_value(struct occ *occ, void *sensor, int snum, int caps_field)
+{
+ return occ->ops.get_caps_value(sensor, snum, caps_field);
+}
+EXPORT_SYMBOL(occ_get_caps_value);
+
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks)
+{
+ *blocks = &occ->response.data;
+}
+EXPORT_SYMBOL(occ_get_response_blocks);
+
+void occ_set_update_interval(struct occ *occ, unsigned long interval)
+{
+ occ->update_interval = msecs_to_jiffies(interval);
+}
+EXPORT_SYMBOL(occ_set_update_interval);
+
+int occ_set_user_powercap(struct occ *occ, u16 cap)
+{
+ u8 resp[8];
+
+ cap = cpu_to_be16(cap);
+
+ return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (u8 *)&cap,
+ resp);
+}
+EXPORT_SYMBOL(occ_set_user_powercap);
+
+struct occ *occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+ const struct occ_config *config)
+{
+ struct occ *driver = devm_kzalloc(dev, sizeof(struct occ), GFP_KERNEL);
+
+ if (!driver)
+ return ERR_PTR(-ENOMEM);
+
+ driver->dev = dev;
+ driver->bus = bus;
+ driver->bus_ops = *bus_ops;
+ driver->ops = *ops;
+ driver->config = *config;
+
+ driver->update_interval = HZ;
+ mutex_init(&driver->update_lock);
+
+ return driver;
+}
+EXPORT_SYMBOL(occ_start);
+
+int occ_stop(struct occ *occ)
+{
+ devm_kfree(occ->dev, occ);
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_stop);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("OCC hwmon core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ.h b/drivers/hwmon/occ/occ.h
new file mode 100644
index 0000000..8c08607
--- /dev/null
+++ b/drivers/hwmon/occ/occ.h
@@ -0,0 +1,86 @@
+/*
+ * occ.h - hwmon OCC driver
+ *
+ * This file contains data structures and function prototypes for common access
+ * between different bus protocols and host systems.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __OCC_H__
+#define __OCC_H__
+
+#include "scom.h"
+
+struct device;
+struct occ;
+
+/* sensor_data_block_header
+ * structure to match the raw occ sensor block header
+ */
+struct sensor_data_block_header {
+ u8 sensor_type[4];
+ u8 reserved0;
+ u8 sensor_format;
+ u8 sensor_length;
+ u8 sensor_num;
+} __attribute__((packed, aligned(4)));
+
+struct sensor_data_block {
+ struct sensor_data_block_header header;
+ void *sensors;
+};
+
+enum sensor_type {
+ FREQ = 0,
+ TEMP,
+ POWER,
+ CAPS,
+ MAX_OCC_SENSOR_TYPE
+};
+
+struct occ_ops {
+ void (*parse_sensor)(u8 *data, void *sensor, int sensor_type, int off,
+ int snum);
+ void *(*alloc_sensor)(int sensor_type, int num_sensors);
+ int (*get_sensor_value)(struct occ *driver, int sensor_type, int snum);
+ int (*get_sensor_id)(struct occ *driver, int sensor_type, int snum);
+ int (*get_caps_value)(void *sensor, int snum, int caps_field);
+};
+
+struct occ_config {
+ u32 command_addr;
+ u32 response_addr;
+};
+
+struct occ_blocks {
+ int sensor_block_id[MAX_OCC_SENSOR_TYPE];
+ struct sensor_data_block *blocks;
+};
+
+struct occ *occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+ const struct occ_config *config);
+int occ_stop(struct occ *occ);
+
+void *occ_get_sensor(struct occ *occ, int sensor_type);
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int snum);
+int occ_get_sensor_id(struct occ *occ, int sensor_type, int snum);
+int occ_get_caps_value(struct occ *occ, void *sensor, int snum,
+ int caps_field);
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);
+int occ_update_device(struct occ *driver);
+void occ_set_update_interval(struct occ *occ, unsigned long interval);
+int occ_set_user_powercap(struct occ *occ, u16 cap);
+
+#endif /* __OCC_H__ */
diff --git a/drivers/hwmon/occ/scom.h b/drivers/hwmon/occ/scom.h
new file mode 100644
index 0000000..c1da645
--- /dev/null
+++ b/drivers/hwmon/occ/scom.h
@@ -0,0 +1,47 @@
+/*
+ * scom.h - hwmon OCC driver
+ *
+ * This file contains data structures for scom operations to the OCC
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __SCOM_H__
+#define __SCOM_H__
+
+/*
+ * occ_bus_ops - represent the low-level transfer methods to communicate with
+ * the OCC.
+ *
+ * getscom - OCC scom read
+ * @bus: handle to slave device
+ * @address: address
+ * @data: where to store data read from slave; buffer size must be at least
+ * eight bytes.
+ *
+ * Returns 0 on success or a negative errno on error
+ *
+ * putscom - OCC scom write
+ * @bus: handle to slave device
+ * @address: address
+ * @data0: first data byte to write
+ * @data1: second data byte to write
+ *
+ * Returns 0 on success or a negative errno on error
+ */
+struct occ_bus_ops {
+ int (*getscom)(void *bus, u32 address, u64 *data);
+ int (*putscom)(void *bus, u32 address, u32 data0, u32 data1);
+};
+
+#endif /* __SCOM_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux 0/6] drivers: hwmon: Add On-Chip Controller driver
From: eajames.ibm @ 2016-12-30 17:56 UTC (permalink / raw)
To: linux
Cc: jdelvare, corbet, mark.rutland, robh+dt, wsa, andrew, joel,
devicetree, linux-doc, linux-hwmon, linux-i2c, linux-kernel,
Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
This patchset adds a hwmon driver to support the OCC (On-Chip Controller)
on the IBM POWER8 and POWER9 processors, from a BMC (Baseboard Management
Controller). The OCC is an embedded processor that provides real time
power and thermal monitoring.
The driver provides an interface on a BMC to poll OCC sensor data, set
user power caps, and perform some basic OCC error handling. It interfaces
with userspace through hwmon.
The driver is currently functional only for the OCC on POWER8 chips.
Communicating with the POWER9 OCC requries FSI support.
Edward A. James (6):
hwmon: Add core On-Chip Controller support for POWER CPUs
hwmon: occ: Add sysfs interface
hwmon: occ: Add I2C transport implementation for SCOM operations
hwmon: occ: Add callbacks for parsing P8 OCC datastructures
hwmon: occ: Add hwmon implementation for the P8 OCC
hwmon: occ: Add callbacks for parsing P9 OCC datastructures
.../devicetree/bindings/i2c/i2c-ibm-occ.txt | 13 +
Documentation/hwmon/occ | 100 ++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 29 ++
drivers/hwmon/occ/Makefile | 2 +
drivers/hwmon/occ/occ.c | 544 +++++++++++++++++++++
drivers/hwmon/occ/occ.h | 86 ++++
drivers/hwmon/occ/occ_p8.c | 217 ++++++++
drivers/hwmon/occ/occ_p8.h | 30 ++
drivers/hwmon/occ/occ_scom_i2c.c | 73 +++
drivers/hwmon/occ/occ_scom_i2c.h | 26 +
drivers/hwmon/occ/occ_sysfs.c | 492 +++++++++++++++++++
drivers/hwmon/occ/occ_sysfs.h | 52 ++
drivers/hwmon/occ/p8_occ_i2c.c | 141 ++++++
drivers/hwmon/occ/p9_occ.c | 243 +++++++++
drivers/hwmon/occ/p9_occ.h | 30 ++
drivers/hwmon/occ/scom.h | 47 ++
18 files changed, 2128 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
create mode 100644 Documentation/hwmon/occ
create mode 100644 drivers/hwmon/occ/Kconfig
create mode 100644 drivers/hwmon/occ/Makefile
create mode 100644 drivers/hwmon/occ/occ.c
create mode 100644 drivers/hwmon/occ/occ.h
create mode 100644 drivers/hwmon/occ/occ_p8.c
create mode 100644 drivers/hwmon/occ/occ_p8.h
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
create mode 100644 drivers/hwmon/occ/occ_sysfs.c
create mode 100644 drivers/hwmon/occ/occ_sysfs.h
create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
create mode 100644 drivers/hwmon/occ/p9_occ.c
create mode 100644 drivers/hwmon/occ/p9_occ.h
create mode 100644 drivers/hwmon/occ/scom.h
--
1.9.1
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Jonathan Cameron @ 2016-12-30 16:46 UTC (permalink / raw)
To: Hans de Goede, Chen-Yu Tsai
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio, linux-i2c, Jacob Pan
In-Reply-To: <20161214135525.16477-1-hdegoede@redhat.com>
On 14/12/16 13:55, Hans de Goede wrote:
> For some reason the axp288_adc driver was modifying the
> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
> whether the GP_ADC channel or another channel was written.
>
> These bits control when a bias current is send to the TS_PIN, the
> GP_ADC has its own pin and a separate bit in another register to
> control the bias current.
>
> Not only does changing when to enable the TS_PIN bias current
> (always or only when sampling) when reading the GP_ADC make no sense
> at all, the code is modifying these bits is writing the entire register,
> assuming that all the other bits have their default value.
>
> So if the firmware has configured a different bias-current for either
> pin, then that change gets clobbered by the write, likewise if the
> firmware has set bit 2 to indicate that the battery has no thermal sensor,
> this will get clobbered by the write.
>
> This commit fixes all this, by simply removing all writes to the
> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
> GP_ADC pin, and can actually be harmful.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
I guess you probably have more up to date contact details than I do,
but seems worth trying to cc Jacob Pan on this to see if we can find
out what the original reasoning behind this was. Seems a very odd
thing to do with no purpose!
If Jacob isn't contactable we'll fall back to guessing it was just
an oddity of driver evolution.
Jonathan
> ---
> drivers/iio/adc/axp288_adc.c | 32 +-------------------------------
> 1 file changed, 1 insertion(+), 31 deletions(-)
>
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 7fd2494..64799ad 100644
> --- a/drivers/iio/adc/axp288_adc.c
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -28,8 +28,6 @@
> #include <linux/iio/driver.h>
>
> #define AXP288_ADC_EN_MASK 0xF1
> -#define AXP288_ADC_TS_PIN_GPADC 0xF2
> -#define AXP288_ADC_TS_PIN_ON 0xF3
>
> enum axp288_adc_id {
> AXP288_ADC_TS,
> @@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val, unsigned long address,
> return IIO_VAL_INT;
> }
>
> -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int mode,
> - unsigned long address)
> -{
> - /* channels other than GPADC do not need to switch TS pin */
> - if (address != AXP288_GP_ADC_H)
> - return 0;
> -
> - return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
> -}
> -
> static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> mutex_lock(&indio_dev->mlock);
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_GPADC,
> - chan->address)) {
> - dev_err(&indio_dev->dev, "GPADC mode\n");
> - ret = -EINVAL;
> - break;
> - }
> ret = axp288_adc_read_channel(val, chan->address, info->regmap);
> - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_ON,
> - chan->address))
> - dev_err(&indio_dev->dev, "TS pin restore\n");
> break;
> default:
> ret = -EINVAL;
> @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> -static int axp288_adc_set_state(struct regmap *regmap)
> -{
> - /* ADC should be always enabled for internal FG to function */
> - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON))
> - return -EIO;
> -
> - return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
> -}
> -
> static const struct iio_info axp288_adc_iio_info = {
> .read_raw = &axp288_adc_read_raw,
> .driver_module = THIS_MODULE,
> @@ -199,7 +169,7 @@ static int axp288_adc_probe(struct platform_device *pdev)
> * Set ADC to enabled state at all time, including system suspend.
> * otherwise internal fuel gauge functionality may be affected.
> */
> - ret = axp288_adc_set_state(axp20x->regmap);
> + ret = regmap_write(info->regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
> if (ret) {
> dev_err(&pdev->dev, "unable to enable ADC device\n");
> return ret;
>
^ permalink raw reply
* Re: [PATCH v7 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: M'boumba Cedric Madianga @ 2016-12-30 14:36 UTC (permalink / raw)
To: Linus Walleij
Cc: Uwe Kleine-König, Wolfram Sang, Rob Herring, Maxime Coquelin,
Alexandre Torgue, Patrice Chotard, Russell King,
linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdaNyL2iV+risLudW=O55w81-AuEZhMwRu9NLdXpnC2r1w@mail.gmail.com>
Hi Linus,
2016-12-30 10:07 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Dec 23, 2016 at 2:09 PM, M'boumba Cedric Madianga
> <cedric.madianga@gmail.com> wrote:
>> 2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>>> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>>>> @@ -337,6 +350,16 @@
>>>> slew-rate = <2>;
>>>> };
>>>> };
>>>> +
>>>> + i2c1_pins_b: i2c1@0 {
>>>> + pins1 {
>>>> + pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
>>>> + drive-open-drain;
>>>> + };
>>>> + pins2 {
>>>> + pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
>>>> + };
>>>
>>> the second doesn't need the open-drain property? Why?
>>
>> I thought that open-drain was only needed for SDA line.
>> But after double-checking I2C specification, it seems that SDA and SCL
>> lines need open-drain or open-collector to perform the wired-AND
>> function.
>
> I think I2C SDA/SCL must be open drain by definition.
>
> It also requires pull-up resistors, I guess you have these mounted on the board
> so you do not need pull-up from the pin controller?
Yes, I have 1 pull-up resistor of 1,5K ohm for each line (SDA & SCL)
on the board
>
> Yours,
> Linus Walleij
^ permalink raw reply
* Re: [PATCH v7 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: Linus Walleij @ 2016-12-30 9:07 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: Uwe Kleine-König, Wolfram Sang, Rob Herring, Maxime Coquelin,
Alexandre Torgue, Patrice Chotard, Russell King,
linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAOAejn1zHMFNiGc-wVbbkC1-JFzG3sjnnUJogtCp9EeAkWsV5A@mail.gmail.com>
On Fri, Dec 23, 2016 at 2:09 PM, M'boumba Cedric Madianga
<cedric.madianga@gmail.com> wrote:
> 2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>>> @@ -337,6 +350,16 @@
>>> slew-rate = <2>;
>>> };
>>> };
>>> +
>>> + i2c1_pins_b: i2c1@0 {
>>> + pins1 {
>>> + pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
>>> + drive-open-drain;
>>> + };
>>> + pins2 {
>>> + pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
>>> + };
>>
>> the second doesn't need the open-drain property? Why?
>
> I thought that open-drain was only needed for SDA line.
> But after double-checking I2C specification, it seems that SDA and SCL
> lines need open-drain or open-collector to perform the wired-AND
> function.
I think I2C SDA/SCL must be open drain by definition.
It also requires pull-up resistors, I guess you have these mounted on the board
so you do not need pull-up from the pin controller?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] i2c: fix spelling mistake: "insufficent" -> "insufficient"
From: Colin King @ 2016-12-29 22:27 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c; +Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in WARN message, insufficient has
an insufficient number of i's in the spelling.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/i2c/i2c-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3de95a2..c299428 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3633,7 +3633,7 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
int ret;
if (!client || !slave_cb) {
- WARN(1, "insufficent data\n");
+ WARN(1, "insufficient data\n");
return -EINVAL;
}
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-29 21:28 UTC (permalink / raw)
To: Michał Kępień
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, Benjamin Tissoires,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20161229210932.GA1254@kmp-mobile.hq.kempniu.pl>
[-- Attachment #1: Type: Text/Plain, Size: 13187 bytes --]
On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > > Latitude machines have ST microelectronics accelerometer at
> > > > > > i2c address 0x29. That i2c address is not specified in DMI
> > > > > > or ACPI, so runtime detection without whitelist which is
> > > > > > below is not possible.
> > > > > >
> > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > represent that accelerometer. Unfortunately without i2c
> > > > > > address.
> > > > >
> > > > > This part of the commit message sounded a bit confusing to me
> > > > > at first because there is already an ACPI driver which
> > > > > handles SMO88xx
> > > > >
> > > > > devices (dell-smo8800). My understanding is that:
> > > > > * the purpose of this patch is to expose a richer interface
> > > > > (as
> > > > >
> > > > > provided by lis3lv02d) to these devices on some machines,
> > > > >
> > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d can
> > > > > work
> > > > >
> > > > > simultaneously (even though dell-smo8800 effectively
> > > > > duplicates the work that lis3lv02d does).
> > > >
> > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > /dev/freefall device which notify userspace about falls.
> > > > lis3lv02d is i2c driver which exports axes of accelerometer.
> > > > Additionaly lis3lv02d can export also /dev/freefall if
> > > > registerer of i2c device provides irq number -- which is not
> > > > case of this patch.
> > > >
> > > > So both drivers are doing different things and both are useful.
> > > >
> > > > IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > (that ST microelectronics accelerometer) but due to
> > > > complicated HW abstraction and layers on Dell laptops it is
> > > > handled by two drivers, one ACPI and one i2c.
> > > >
> > > > Yes, in ideal world irq number should be passed to lis3lv02d
> > > > driver and that would export whole device (with /dev/freefall
> > > > too), but due to HW abstraction it is too much complicated...
> > >
> > > Why? AFAICT, all that is required to pass that IRQ number all
> > > the way down to lis3lv02d is to set the irq field of the struct
> > > i2c_board_info you are passing to i2c_new_device(). And you can
> > > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > > However, you would then need to make sure dell-smo8800 does not
> > > attempt to request the same IRQ on whitelisted machines. This
> > > got me thinking about a way to somehow incorporate your changes
> > > into dell-smo8800 using Wolfram's bus_notifier suggestion, but I
> > > do not have a working solution for now. What is tempting about
> > > this approach is that you would not have to scan the ACPI
> > > namespace in search of SMO88xx devices, because smo8800_add() is
> > > automatically called for them. However, I fear that the
> > > resulting solution may be more complicated than the one you
> > > submitted.
> >
> > Then we need to deal with lot of problems. Order of loading .ko
> > modules is undefined. Binding devices to drivers registered by .ko
> > module is also in "random" order. At any time any of those .ko
> > module can be unloaded or at least device unbind (via sysfs) from
> > driver... And there can be some pathological situation (thanks to
> > adding ACPI layer as Andy pointed) that there will be more SMO88xx
> > devices in ACPI. Plus you can compile kernel with and without
> > those modules and also you can blacklist loading them (so compile
> > time check is not enough). And still some correct message notifier
> > must be used.
> >
> > I think such solution is much much more complicated, there are lot
> > of combinations of kernel configuration and available dell
> > devices...
>
> I tried a few more things, but ultimately failed to find a nice way
> to implement this.
>
> Another issue popped up, though. Linus' master branch contains a
> recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use
> an IRQ to report Host Notify events, not alert") which breaks your
> patch. The reason for that is that lis3lv02d relies on the i2c
> client's IRQ being 0 to detect that it should not create
> /dev/freefall. Benjamin's patch causes the Host Notify IRQ to be
> assigned to the i2c client your patch creates, thus causing
> lis3lv02d to create /dev/freefall, which in turn conflicts with
> dell-smo8800 which is trying to create /dev/freefall itself.
So 4d5538f5882a is breaking lis3lv02d driver...
> Also, just to make sure we do not overthink this, I understand that
> not every unit of the models from the whitelist has an
> accelerometer, correct? In other words, could we perhaps skip the
> part where we are making sure the SMO88xx ACPI device is there?
Good question... At least for E6440 I'm did not thing it was possible to
configure notebook without "3 axes free fall sensor".
But! In BIOS SETUP it is possible to disable free fall sensor. I will
try to disable it there and will check what happen. My guess is that it
will be disabled in ACPI.
> > > > > If I got something wrong, please correct me. If I got it
> > > > > right, it might make sense to rephrase the commit message a
> > > > > bit so that the first bullet point above is immediately
> > > > > clear to the reader.
> > > > >
> > > > > > This patch registers lis3lv02d device at i2c address 0x29
> > > > > > if is detected.
> > > > > >
> > > > > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI
> > > > > > SystemIO OpRegion to conflict with PCI BAR") allowed to
> > > > > > use i2c-i801 driver on Dell machines so lis3lv02d
> > > > > > correctly initialize accelerometer.
> > > > > >
> > > > > > Tested on Dell Latitude E6440.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > drivers/i2c/busses/i2c-i801.c | 98
> > > > > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > > > > 98 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4
> > > > > > 100644 --- a/drivers/i2c/busses/i2c-i801.c
> > > > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > > > @@ -1118,6 +1118,101 @@ static void
> > > > > > dmi_check_onboard_devices(const struct dmi_header *dm, void
> > > > > > *adap)
> > > > > >
> > > > > > }
> > > > > >
> > > > > > }
> > > > > >
> > > > > > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > > > > > obj_handle, + u32 nesting_level,
> > > > > > + void *context,
> > > > > > + void **return_value)
> > > > > > +{
> > > > > > + struct acpi_device_info *info;
> > > > > > + acpi_status status;
> > > > > > + char *hid;
> > > > > > +
> > > > > > + status = acpi_get_object_info(obj_handle, &info);
> > > > >
> > > > > acpi_get_object_info() allocates the returned buffer, which
> > > > > the caller has to free.
> > > >
> > > > Ok, I will fix it in next patch iteration.
> > > >
> > > > > > + if (!ACPI_SUCCESS(status) || !(info->valid &
> > > > > > ACPI_VALID_HID)) + return AE_OK;
> > > > > > +
> > > > > > + hid = info->hardware_id.string;
> > > > > > + if (!hid)
> > > > > > + return AE_OK;
> > > > > > +
> > > > > > + if (strlen(hid) < 7)
> > > > > > + return AE_OK;
> > > > > > +
> > > > > > + if (memcmp(hid, "SMO88", 5) != 0)
> > > > > > + return AE_OK;
> > > > > > +
> > > > > > + *((bool *)return_value) = true;
> > > > > > + return AE_CTRL_TERMINATE;
> > > > > > +}
> > > > > > +
> > > > > > +static bool is_dell_system_with_lis3lv02d(void)
> > > > > > +{
> > > > > > + bool found;
> > > > > > + acpi_status status;
> > > > > > + const char *vendor;
> > > > > > +
> > > > > > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > > > > > + if (strcmp(vendor, "Dell Inc.") != 0)
> > > > > > + return false;
> > > > > > +
> > > > > > + /*
> > > > > > + * Check if ACPI device SMO88xx exists and if is enabled.
> > > > > > That ACPI + * device represent our ST microelectronics
> > > > > > lis3lv02d accelerometer but + * unfortunately without any
> > > > > > other additional information. + */
> > > > > > + found = false;
> > > > > > + status = acpi_get_devices(NULL,
> > > > > > check_acpi_smo88xx_device, NULL, + (void
> > > > > > **)&found);
> > > > > > + if (!ACPI_SUCCESS(status) || !found)
> > > > > > + return false;
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * Dell platform team told us that these Latitude devices
> > > > > > have + * ST microelectronics accelerometer at i2c address
> > > > > > 0x29. + * That i2c address is not specified in DMI or
> > > > > > ACPI, so runtime + * detection without whitelist which is
> > > > > > below is not possible. + */
> > > > > > +static const char * const dmi_dell_product_names[] = {
> > > > > > + "Latitude E5250",
> > > > > > + "Latitude E5450",
> > > > > > + "Latitude E5550",
> > > > > > + "Latitude E6440",
> > > > > > + "Latitude E6440 ATG",
> > > > > > + "Latitude E6540",
> > > > > > +};
> > > > > > +
> > > > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > > > i801_priv *priv) +{
> > > > > > + struct i2c_board_info info;
> > > > > > + const char *product_name;
> > > > > > + bool known_i2c_address;
> > > > > > + int i;
> > > > > > +
> > > > > > + known_i2c_address = false;
> > > > > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > > > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i)
> > > > > > { + if (strcmp(product_name, dmi_dell_product_names[i])
> > > > > > == 0) {
> > > > > > + known_i2c_address = true;
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + if (!known_i2c_address) {
> > > > > > + dev_warn(&priv->pci_dev->dev,
> > > > > > + "Accelerometer lis3lv02d i2c device is present "
> > > > > > + "but its i2c address is unknown, skipping ...
> > > > > > \n");
> > > > >
> > > > > You are probably well aware of this, but checkpatch prefers
> > > > > keeping long log messages in one line. I am pointing it out
> > > > > just in case.
> > > >
> > > > Yes, but I do not know how to fix it. Splitting message into
> > > > two lines generates warning. Having long line generates
> > > > warning too.
> > >
> > > Weird, checkpatch does not protest on my machine when the log
> > > message is written on a single line...
> >
> > I hope that i2c maintainers decide how to format that line.
> >
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + memset(&info, 0, sizeof(struct i2c_board_info));
> > > > >
> > > > > How about just doing "struct i2c_board_info info = { 0 };"
> > > > > instead?
> > > >
> > > > Ok.
> > > >
> > > > > > + info.addr = 0x29;
> > > > > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > > > > > + i2c_new_device(&priv->adapter, &info);
> > > > > > +}
> > > > > > +
> > > > > >
> > > > > > /* Register optional slaves */
> > > > > > static void i801_probe_optional_slaves(struct i801_priv
> > > > > > *priv) {
> > > > > >
> > > > > > @@ -1136,6 +1231,9 @@ static void
> > > > > > i801_probe_optional_slaves(struct i801_priv *priv)
> > > > > >
> > > > > > if (dmi_name_in_vendors("FUJITSU"))
> > > > > >
> > > > > > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> > > > > >
> > > > > > +
> > > > > > + if (is_dell_system_with_lis3lv02d())
> > > > > > + register_dell_lis3lv02d_i2c_device(priv);
> > > > > >
> > > > > > }
> > > > > > #else
> > > > > > static void __init input_apanel_init(void) {}
> > > > >
> > > > > I tested this patch on a Vostro V131, which is not on the
> > > > > whitelist, so all I got was the warning message, but to this
> > > > > extent, it works for me.
> > > >
> > > > Hm... That means your notebook has ST microelectronics
> > > > accelerometer too. You could try to find it on i2c-i801 bus
> > > > with userspace i2cdetect program (part of i2c-tools) and get
> > > > i2c address.
> > >
> > > Bingo, it is at 0x1d. I modified your patch to set the i2c
> > > address to 0x1d and at least free fall detection seems to be
> > > working correctly.
> >
> > lis3lv02d exports input device, you should find its number lsinput.
> > You can then test accelerometer with e.g. program input-events.
> >
> > If it is working fine, I can add your machine to whitelist with i2c
> > address 0x1d.
>
> I did some tests with evtest and it seems that axis values are
> consistent with laptop's movements, so I think it is safe to
> whitelist Vostro V131 with i2c address 0x1d.
Ok.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Michał Kępień @ 2016-12-29 21:09 UTC (permalink / raw)
To: Pali Rohár
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, Benjamin Tissoires,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201612291517.37474@pali>
> On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > Latitude machines have ST microelectronics accelerometer at i2c
> > > > > address 0x29. That i2c address is not specified in DMI or ACPI,
> > > > > so runtime detection without whitelist which is below is not
> > > > > possible.
> > > > >
> > > > > Presence of that ST microelectronics accelerometer is verified
> > > > > by existence of SMO88xx ACPI device which represent that
> > > > > accelerometer. Unfortunately without i2c address.
> > > >
> > > > This part of the commit message sounded a bit confusing to me at
> > > > first because there is already an ACPI driver which handles
> > > > SMO88xx
> > > >
> > > > devices (dell-smo8800). My understanding is that:
> > > > * the purpose of this patch is to expose a richer interface (as
> > > >
> > > > provided by lis3lv02d) to these devices on some machines,
> > > >
> > > > * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > > >
> > > > simultaneously (even though dell-smo8800 effectively
> > > > duplicates the work that lis3lv02d does).
> > >
> > > No. dell-smo8800 reads from ACPI irq number and exports
> > > /dev/freefall device which notify userspace about falls. lis3lv02d
> > > is i2c driver which exports axes of accelerometer. Additionaly
> > > lis3lv02d can export also /dev/freefall if registerer of i2c
> > > device provides irq number -- which is not case of this patch.
> > >
> > > So both drivers are doing different things and both are useful.
> > >
> > > IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > > ST microelectronics accelerometer) but due to complicated HW
> > > abstraction and layers on Dell laptops it is handled by two
> > > drivers, one ACPI and one i2c.
> > >
> > > Yes, in ideal world irq number should be passed to lis3lv02d driver
> > > and that would export whole device (with /dev/freefall too), but
> > > due to HW abstraction it is too much complicated...
> >
> > Why? AFAICT, all that is required to pass that IRQ number all the
> > way down to lis3lv02d is to set the irq field of the struct
> > i2c_board_info you are passing to i2c_new_device(). And you can
> > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > However, you would then need to make sure dell-smo8800 does not
> > attempt to request the same IRQ on whitelisted machines. This got
> > me thinking about a way to somehow incorporate your changes into
> > dell-smo8800 using Wolfram's bus_notifier suggestion, but I do not
> > have a working solution for now. What is tempting about this
> > approach is that you would not have to scan the ACPI namespace in
> > search of SMO88xx devices, because smo8800_add() is automatically
> > called for them. However, I fear that the resulting solution may be
> > more complicated than the one you submitted.
>
> Then we need to deal with lot of problems. Order of loading .ko modules
> is undefined. Binding devices to drivers registered by .ko module is
> also in "random" order. At any time any of those .ko module can be
> unloaded or at least device unbind (via sysfs) from driver... And there
> can be some pathological situation (thanks to adding ACPI layer as Andy
> pointed) that there will be more SMO88xx devices in ACPI. Plus you can
> compile kernel with and without those modules and also you can blacklist
> loading them (so compile time check is not enough). And still some
> correct message notifier must be used.
>
> I think such solution is much much more complicated, there are lot of
> combinations of kernel configuration and available dell devices...
I tried a few more things, but ultimately failed to find a nice way to
implement this.
Another issue popped up, though. Linus' master branch contains a recent
commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use an IRQ to
report Host Notify events, not alert") which breaks your patch. The
reason for that is that lis3lv02d relies on the i2c client's IRQ being 0
to detect that it should not create /dev/freefall. Benjamin's patch
causes the Host Notify IRQ to be assigned to the i2c client your patch
creates, thus causing lis3lv02d to create /dev/freefall, which in turn
conflicts with dell-smo8800 which is trying to create /dev/freefall
itself.
Also, just to make sure we do not overthink this, I understand that not
every unit of the models from the whitelist has an accelerometer,
correct? In other words, could we perhaps skip the part where we are
making sure the SMO88xx ACPI device is there?
>
> > > > If I got something wrong, please correct me. If I got it right,
> > > > it might make sense to rephrase the commit message a bit so that
> > > > the first bullet point above is immediately clear to the reader.
> > > >
> > > > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > > > detected.
> > > > >
> > > > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO
> > > > > OpRegion to conflict with PCI BAR") allowed to use i2c-i801
> > > > > driver on Dell machines so lis3lv02d correctly initialize
> > > > > accelerometer.
> > > > >
> > > > > Tested on Dell Latitude E6440.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > ---
> > > > >
> > > > > drivers/i2c/busses/i2c-i801.c | 98
> > > > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > > > insertions(+)
> > > > >
> > > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > > > --- a/drivers/i2c/busses/i2c-i801.c
> > > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > > @@ -1118,6 +1118,101 @@ static void
> > > > > dmi_check_onboard_devices(const struct dmi_header *dm, void
> > > > > *adap)
> > > > >
> > > > > }
> > > > >
> > > > > }
> > > > >
> > > > > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > > > > obj_handle, + u32 nesting_level,
> > > > > + void *context,
> > > > > + void **return_value)
> > > > > +{
> > > > > + struct acpi_device_info *info;
> > > > > + acpi_status status;
> > > > > + char *hid;
> > > > > +
> > > > > + status = acpi_get_object_info(obj_handle, &info);
> > > >
> > > > acpi_get_object_info() allocates the returned buffer, which the
> > > > caller has to free.
> > >
> > > Ok, I will fix it in next patch iteration.
> > >
> > > > > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > > > > + return AE_OK;
> > > > > +
> > > > > + hid = info->hardware_id.string;
> > > > > + if (!hid)
> > > > > + return AE_OK;
> > > > > +
> > > > > + if (strlen(hid) < 7)
> > > > > + return AE_OK;
> > > > > +
> > > > > + if (memcmp(hid, "SMO88", 5) != 0)
> > > > > + return AE_OK;
> > > > > +
> > > > > + *((bool *)return_value) = true;
> > > > > + return AE_CTRL_TERMINATE;
> > > > > +}
> > > > > +
> > > > > +static bool is_dell_system_with_lis3lv02d(void)
> > > > > +{
> > > > > + bool found;
> > > > > + acpi_status status;
> > > > > + const char *vendor;
> > > > > +
> > > > > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > > > > + if (strcmp(vendor, "Dell Inc.") != 0)
> > > > > + return false;
> > > > > +
> > > > > + /*
> > > > > + * Check if ACPI device SMO88xx exists and if is enabled.
> > > > > That ACPI + * device represent our ST microelectronics
> > > > > lis3lv02d accelerometer but + * unfortunately without any
> > > > > other additional information. + */
> > > > > + found = false;
> > > > > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device,
> > > > > NULL, + (void **)&found);
> > > > > + if (!ACPI_SUCCESS(status) || !found)
> > > > > + return false;
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Dell platform team told us that these Latitude devices have
> > > > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > > > + * That i2c address is not specified in DMI or ACPI, so
> > > > > runtime + * detection without whitelist which is below is not
> > > > > possible. + */
> > > > > +static const char * const dmi_dell_product_names[] = {
> > > > > + "Latitude E5250",
> > > > > + "Latitude E5450",
> > > > > + "Latitude E5550",
> > > > > + "Latitude E6440",
> > > > > + "Latitude E6440 ATG",
> > > > > + "Latitude E6540",
> > > > > +};
> > > > > +
> > > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > > i801_priv *priv) +{
> > > > > + struct i2c_board_info info;
> > > > > + const char *product_name;
> > > > > + bool known_i2c_address;
> > > > > + int i;
> > > > > +
> > > > > + known_i2c_address = false;
> > > > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > > > + if (strcmp(product_name, dmi_dell_product_names[i]) == 0)
> > > > > {
> > > > > + known_i2c_address = true;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (!known_i2c_address) {
> > > > > + dev_warn(&priv->pci_dev->dev,
> > > > > + "Accelerometer lis3lv02d i2c device is present "
> > > > > + "but its i2c address is unknown, skipping ...\n");
> > > >
> > > > You are probably well aware of this, but checkpatch prefers
> > > > keeping long log messages in one line. I am pointing it out
> > > > just in case.
> > >
> > > Yes, but I do not know how to fix it. Splitting message into two
> > > lines generates warning. Having long line generates warning too.
> >
> > Weird, checkpatch does not protest on my machine when the log message
> > is written on a single line...
>
> I hope that i2c maintainers decide how to format that line.
>
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + memset(&info, 0, sizeof(struct i2c_board_info));
> > > >
> > > > How about just doing "struct i2c_board_info info = { 0 };"
> > > > instead?
> > >
> > > Ok.
> > >
> > > > > + info.addr = 0x29;
> > > > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > > > > + i2c_new_device(&priv->adapter, &info);
> > > > > +}
> > > > > +
> > > > >
> > > > > /* Register optional slaves */
> > > > > static void i801_probe_optional_slaves(struct i801_priv *priv)
> > > > > {
> > > > >
> > > > > @@ -1136,6 +1231,9 @@ static void
> > > > > i801_probe_optional_slaves(struct i801_priv *priv)
> > > > >
> > > > > if (dmi_name_in_vendors("FUJITSU"))
> > > > >
> > > > > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> > > > >
> > > > > +
> > > > > + if (is_dell_system_with_lis3lv02d())
> > > > > + register_dell_lis3lv02d_i2c_device(priv);
> > > > >
> > > > > }
> > > > > #else
> > > > > static void __init input_apanel_init(void) {}
> > > >
> > > > I tested this patch on a Vostro V131, which is not on the
> > > > whitelist, so all I got was the warning message, but to this
> > > > extent, it works for me.
> > >
> > > Hm... That means your notebook has ST microelectronics
> > > accelerometer too. You could try to find it on i2c-i801 bus with
> > > userspace i2cdetect program (part of i2c-tools) and get i2c
> > > address.
> >
> > Bingo, it is at 0x1d. I modified your patch to set the i2c address
> > to 0x1d and at least free fall detection seems to be working
> > correctly.
>
> lis3lv02d exports input device, you should find its number lsinput. You
> can then test accelerometer with e.g. program input-events.
>
> If it is working fine, I can add your machine to whitelist with i2c
> address 0x1d.
I did some tests with evtest and it seems that axis values are
consistent with laptop's movements, so I think it is safe to whitelist
Vostro V131 with i2c address 0x1d.
--
Best regards,
Michał Kępień
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-29 14:17 UTC (permalink / raw)
To: Michał Kępień
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <20161229134719.GA941@ozzy.nask.waw.pl>
[-- Attachment #1: Type: Text/Plain, Size: 10463 bytes --]
On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > Latitude machines have ST microelectronics accelerometer at i2c
> > > > address 0x29. That i2c address is not specified in DMI or ACPI,
> > > > so runtime detection without whitelist which is below is not
> > > > possible.
> > > >
> > > > Presence of that ST microelectronics accelerometer is verified
> > > > by existence of SMO88xx ACPI device which represent that
> > > > accelerometer. Unfortunately without i2c address.
> > >
> > > This part of the commit message sounded a bit confusing to me at
> > > first because there is already an ACPI driver which handles
> > > SMO88xx
> > >
> > > devices (dell-smo8800). My understanding is that:
> > > * the purpose of this patch is to expose a richer interface (as
> > >
> > > provided by lis3lv02d) to these devices on some machines,
> > >
> > > * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > >
> > > simultaneously (even though dell-smo8800 effectively
> > > duplicates the work that lis3lv02d does).
> >
> > No. dell-smo8800 reads from ACPI irq number and exports
> > /dev/freefall device which notify userspace about falls. lis3lv02d
> > is i2c driver which exports axes of accelerometer. Additionaly
> > lis3lv02d can export also /dev/freefall if registerer of i2c
> > device provides irq number -- which is not case of this patch.
> >
> > So both drivers are doing different things and both are useful.
> >
> > IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > ST microelectronics accelerometer) but due to complicated HW
> > abstraction and layers on Dell laptops it is handled by two
> > drivers, one ACPI and one i2c.
> >
> > Yes, in ideal world irq number should be passed to lis3lv02d driver
> > and that would export whole device (with /dev/freefall too), but
> > due to HW abstraction it is too much complicated...
>
> Why? AFAICT, all that is required to pass that IRQ number all the
> way down to lis3lv02d is to set the irq field of the struct
> i2c_board_info you are passing to i2c_new_device(). And you can
> extract that IRQ number e.g. in check_acpi_smo88xx_device().
> However, you would then need to make sure dell-smo8800 does not
> attempt to request the same IRQ on whitelisted machines. This got
> me thinking about a way to somehow incorporate your changes into
> dell-smo8800 using Wolfram's bus_notifier suggestion, but I do not
> have a working solution for now. What is tempting about this
> approach is that you would not have to scan the ACPI namespace in
> search of SMO88xx devices, because smo8800_add() is automatically
> called for them. However, I fear that the resulting solution may be
> more complicated than the one you submitted.
Then we need to deal with lot of problems. Order of loading .ko modules
is undefined. Binding devices to drivers registered by .ko module is
also in "random" order. At any time any of those .ko module can be
unloaded or at least device unbind (via sysfs) from driver... And there
can be some pathological situation (thanks to adding ACPI layer as Andy
pointed) that there will be more SMO88xx devices in ACPI. Plus you can
compile kernel with and without those modules and also you can blacklist
loading them (so compile time check is not enough). And still some
correct message notifier must be used.
I think such solution is much much more complicated, there are lot of
combinations of kernel configuration and available dell devices...
> > > If I got something wrong, please correct me. If I got it right,
> > > it might make sense to rephrase the commit message a bit so that
> > > the first bullet point above is immediately clear to the reader.
> > >
> > > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > > detected.
> > > >
> > > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO
> > > > OpRegion to conflict with PCI BAR") allowed to use i2c-i801
> > > > driver on Dell machines so lis3lv02d correctly initialize
> > > > accelerometer.
> > > >
> > > > Tested on Dell Latitude E6440.
> > > >
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > >
> > > > drivers/i2c/busses/i2c-i801.c | 98
> > > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > > insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > > --- a/drivers/i2c/busses/i2c-i801.c
> > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > @@ -1118,6 +1118,101 @@ static void
> > > > dmi_check_onboard_devices(const struct dmi_header *dm, void
> > > > *adap)
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > > > obj_handle, + u32 nesting_level,
> > > > + void *context,
> > > > + void **return_value)
> > > > +{
> > > > + struct acpi_device_info *info;
> > > > + acpi_status status;
> > > > + char *hid;
> > > > +
> > > > + status = acpi_get_object_info(obj_handle, &info);
> > >
> > > acpi_get_object_info() allocates the returned buffer, which the
> > > caller has to free.
> >
> > Ok, I will fix it in next patch iteration.
> >
> > > > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > > > + return AE_OK;
> > > > +
> > > > + hid = info->hardware_id.string;
> > > > + if (!hid)
> > > > + return AE_OK;
> > > > +
> > > > + if (strlen(hid) < 7)
> > > > + return AE_OK;
> > > > +
> > > > + if (memcmp(hid, "SMO88", 5) != 0)
> > > > + return AE_OK;
> > > > +
> > > > + *((bool *)return_value) = true;
> > > > + return AE_CTRL_TERMINATE;
> > > > +}
> > > > +
> > > > +static bool is_dell_system_with_lis3lv02d(void)
> > > > +{
> > > > + bool found;
> > > > + acpi_status status;
> > > > + const char *vendor;
> > > > +
> > > > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > > > + if (strcmp(vendor, "Dell Inc.") != 0)
> > > > + return false;
> > > > +
> > > > + /*
> > > > + * Check if ACPI device SMO88xx exists and if is enabled.
> > > > That ACPI + * device represent our ST microelectronics
> > > > lis3lv02d accelerometer but + * unfortunately without any
> > > > other additional information. + */
> > > > + found = false;
> > > > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device,
> > > > NULL, + (void **)&found);
> > > > + if (!ACPI_SUCCESS(status) || !found)
> > > > + return false;
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Dell platform team told us that these Latitude devices have
> > > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > > + * That i2c address is not specified in DMI or ACPI, so
> > > > runtime + * detection without whitelist which is below is not
> > > > possible. + */
> > > > +static const char * const dmi_dell_product_names[] = {
> > > > + "Latitude E5250",
> > > > + "Latitude E5450",
> > > > + "Latitude E5550",
> > > > + "Latitude E6440",
> > > > + "Latitude E6440 ATG",
> > > > + "Latitude E6540",
> > > > +};
> > > > +
> > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > i801_priv *priv) +{
> > > > + struct i2c_board_info info;
> > > > + const char *product_name;
> > > > + bool known_i2c_address;
> > > > + int i;
> > > > +
> > > > + known_i2c_address = false;
> > > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > > + if (strcmp(product_name, dmi_dell_product_names[i]) == 0)
> > > > {
> > > > + known_i2c_address = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!known_i2c_address) {
> > > > + dev_warn(&priv->pci_dev->dev,
> > > > + "Accelerometer lis3lv02d i2c device is present "
> > > > + "but its i2c address is unknown, skipping ...\n");
> > >
> > > You are probably well aware of this, but checkpatch prefers
> > > keeping long log messages in one line. I am pointing it out
> > > just in case.
> >
> > Yes, but I do not know how to fix it. Splitting message into two
> > lines generates warning. Having long line generates warning too.
>
> Weird, checkpatch does not protest on my machine when the log message
> is written on a single line...
I hope that i2c maintainers decide how to format that line.
> > > > + return;
> > > > + }
> > > > +
> > > > + memset(&info, 0, sizeof(struct i2c_board_info));
> > >
> > > How about just doing "struct i2c_board_info info = { 0 };"
> > > instead?
> >
> > Ok.
> >
> > > > + info.addr = 0x29;
> > > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > > > + i2c_new_device(&priv->adapter, &info);
> > > > +}
> > > > +
> > > >
> > > > /* Register optional slaves */
> > > > static void i801_probe_optional_slaves(struct i801_priv *priv)
> > > > {
> > > >
> > > > @@ -1136,6 +1231,9 @@ static void
> > > > i801_probe_optional_slaves(struct i801_priv *priv)
> > > >
> > > > if (dmi_name_in_vendors("FUJITSU"))
> > > >
> > > > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> > > >
> > > > +
> > > > + if (is_dell_system_with_lis3lv02d())
> > > > + register_dell_lis3lv02d_i2c_device(priv);
> > > >
> > > > }
> > > > #else
> > > > static void __init input_apanel_init(void) {}
> > >
> > > I tested this patch on a Vostro V131, which is not on the
> > > whitelist, so all I got was the warning message, but to this
> > > extent, it works for me.
> >
> > Hm... That means your notebook has ST microelectronics
> > accelerometer too. You could try to find it on i2c-i801 bus with
> > userspace i2cdetect program (part of i2c-tools) and get i2c
> > address.
>
> Bingo, it is at 0x1d. I modified your patch to set the i2c address
> to 0x1d and at least free fall detection seems to be working
> correctly.
lis3lv02d exports input device, you should find its number lsinput. You
can then test accelerometer with e.g. program input-events.
If it is working fine, I can add your machine to whitelist with i2c
address 0x1d.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox