* Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger
From: Andreas Klinger @ 2017-01-14 17:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ktsai-GubuWUlQtMwciDkP5Hr2oA,
wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, trivial-DgEjT+Ai2ygdnm+yROfE0A,
mranostay-Re5JQEeQqe8AvxtiuMwx3w,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4099feac-959c-2b5b-a21a-3b111098af39-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hi Jonathan,
see comments below.
Andreas
Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> schrieb am Sat, 14. Jan 12:17:
> On 10/01/17 18:48, Andreas Klinger wrote:
> > This is the IIO driver for devantech srf08 ultrasonic ranger which can be
> > used to measure the distances to an object.
> >
> > The sensor supports I2C with some registers.
> >
> > Supported Features include:
> > - read the distance in ranging mode in centimeters
> > - output of the driver is directly the read value
> > - together with the scale the driver delivers the distance in meters
> > - only the first echo of the nearest object is delivered
> > - set max gain register; userspace enters analogue value
> > - set range registers; userspace enters range in millimeters in 43 mm steps
> >
> > Features not supported by this driver:
> > - ranging mode in inches or in microseconds
> > - ANN mode
> > - change I2C address through this driver
> > - light sensor
> >
> > The driver was added in the directory "proximity" of the iio subsystem
> > in absence of another directory named "distance".
> > There is also a new submenu "distance"
> Hi Andreas,
>
> Sorry it took me a while to get to this!
>
> I'd not bother with the new submenu. Perhaps we should rename the
> proximity menu to proximity/distance.
>
> We already the lightening detector in there which is definitely not
> measuring proximity in the convetional sense!
>
> Anyhow, the actual code is fine, but we need to think about how the
> userspace ABI fits within the wider IIO ABI. Naming and approaches
> that make sense in a single class of drivers can end up meaining
> very different things for other drivers. Various suggestions inline.
>
> Jonathan
> >
> > Signed-off-by: Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> > ---
> > drivers/iio/proximity/Kconfig | 15 ++
> > drivers/iio/proximity/Makefile | 1 +
> > drivers/iio/proximity/srf08.c | 362 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 378 insertions(+)
> > create mode 100644 drivers/iio/proximity/srf08.c
> >
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index ef4c73db5b53..7b10a137702b 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -46,3 +46,18 @@ config SX9500
> > module will be called sx9500.
> >
> > endmenu
> > +
> > +menu "Distance sensors"
> > +
> > +config SRF08
> > + tristate "Devantech SRF08 ultrasonic ranger sensor"
> > + depends on I2C
> > + help
> > + Say Y here to build a driver for Devantech SRF08 ultrasonic
> > + ranger sensor. This driver can be used to measure the distance
> > + of objects.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called srf08.
> > +
> > +endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 9aadd9a8ee99..e914c2a5dd49 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,4 +5,5 @@
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AS3935) += as3935.o
> > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> > +obj-$(CONFIG_SRF08) += srf08.o
> > obj-$(CONFIG_SX9500) += sx9500.o
> > diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
> > new file mode 100644
> > index 000000000000..f38c74ed0933
> > --- /dev/null
> > +++ b/drivers/iio/proximity/srf08.c
> > @@ -0,0 +1,362 @@
> > +/*
> > + * srf08.c - Support for Devantech SRF08 ultrasonic ranger
> > + *
> > + * Copyright (c) 2016 Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> > + *
> > + * This file is subject to the terms and conditions of version 2 of
> > + * the GNU General Public License. See the file COPYING in the main
> > + * directory of this archive for more details.
> > + *
> > + * For details about the device see:
> > + * http://www.robot-electronics.co.uk/htm/srf08tech.html
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/bitops.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +/* registers of SRF08 device */
> > +#define SRF08_WRITE_COMMAND 0x00 /* Command Register */
> > +#define SRF08_WRITE_MAX_GAIN 0x01 /* Max Gain Register: 0 .. 31 */
> > +#define SRF08_WRITE_RANGE 0x02 /* Range Register: 0 .. 255 */
> > +#define SRF08_READ_SW_REVISION 0x00 /* Software Revision */
> > +#define SRF08_READ_LIGHT 0x01 /* Light Sensor during last echo */
> > +#define SRF08_READ_ECHO_1_HIGH 0x02 /* Range of first echo received */
> > +#define SRF08_READ_ECHO_1_LOW 0x03 /* Range of first echo received */
> > +
> > +#define SRF08_CMD_RANGING_CM 0x51 /* Ranging Mode - Result in cm */
> > +
> > +#define SRF08_DEFAULT_GAIN 1025 /* max. analogue value of Gain */
> > +#define SRF08_DEFAULT_RANGE 11008 /* max. value of Range in mm */
> > +
> > +struct srf08_data {
> > + struct i2c_client *client;
> > + int gain; /* Max Gain */
> > + int range_mm; /* Range in mm */
> > + struct mutex lock;
> > +};
> > +
> > +static const int srf08_gain[] = {
> > + 94, 97, 100, 103, 107, 110, 114, 118,
> > + 123, 128, 133, 139, 145, 152, 159, 168,
> > + 177, 187, 199, 212, 227, 245, 265, 288,
> > + 317, 352, 395, 450, 524, 626, 777, 1025 };
> > +
> > +static int srf08_read_ranging(struct srf08_data *data)
> > +{
> > + struct i2c_client *client = data->client;
> > + int ret, i;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_COMMAND, SRF08_CMD_RANGING_CM);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write command - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + /*
> > + * normally after 65 ms the device should have the read value
> > + * we round it up to 100 ms
> I'd suggest this should be adapted so that it takes advantage of knowing
> roughly how long it is going to take as the 'range' maximum is changed.
> So perhaps in the basic case, sleep for 65 msecs, then poll at 5msec
> intervals. If we know it's going to be a lot faster, then poll it from
> an earlier time.
> > + *
> > + * we read here until a correct version number shows up as
> > + * suggested by the documentation
> > + */
> > + for (i = 0; i < 5; i++) {
> > + ret = i2c_smbus_read_byte_data(data->client,
> > + SRF08_READ_SW_REVISION);
> > +
> > + /* check if a valid version number is read */
> > + if (ret < 255 && ret > 0)
> > + break;
> > + msleep(20);
> > + }
> > +
> > + if (ret >= 255 || ret <= 0) {
> > + dev_err(&client->dev, "device not ready\n");
> > + mutex_unlock(&data->lock);
> > + return -EIO;
> > + }
> > +
> > + ret = i2c_smbus_read_word_swapped(data->client,
> > + SRF08_READ_ECHO_1_HIGH);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "cannot read distance: ret=%d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int srf08_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (channel->type != IIO_DISTANCE)
> > + return -EINVAL;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = srf08_read_ranging(data);
> > + if (ret < 0)
> > + return ret;
> > + *val = ret;
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /* 1 LSB is 1 cm */
> > + *val = 0;
> > + *val2 = 10000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t srf08_show_range_mm_available(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int i, len = 0;
> > +
> > + for (i = 0; i < 256; i++)
> > + len += scnprintf(buf + len, PAGE_SIZE - len,
> > + "%d ", (i + 1) * 43);
> > +
> > + buf[len - 1] = '\n';
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(range_mm_available, S_IRUGO,
> > + srf08_show_range_mm_available, NULL, 0);
> > +
> > +static ssize_t srf08_show_range_mm(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > +
> > + return sprintf(buf, "%d\n", data->range_mm);
> > +}
> > +
> > +/*
> > + * set the range of the sensor to an even multiple of 43 mm
> > + * which corresponds to 1 LSB in the register
> > + *
> > + * register value corresponding range
> > + * 0x00 43 mm
> > + * 0x01 86 mm
> > + * 0x02 129 mm
> > + * ...
> > + * 0xFF 11008 mm
> > + */
> > +static ssize_t srf08_write_range_mm(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + int ret;
> > + unsigned int val, mod;
> > + u8 regval;
> > +
> > + ret = kstrtouint(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = val / 43 - 1;
> > + mod = val % 43;
> > +
> > + if (mod || (ret < 0) || (ret > 255))
> > + return -EINVAL;
> > +
> > + regval = ret;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_RANGE, regval);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write_range - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + data->range_mm = val;
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(range_mm, S_IRUGO | S_IWUSR,
> > + srf08_show_range_mm, srf08_write_range_mm, 0);
> > +
> > +static ssize_t srf08_show_gain_available(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int i, len = 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> > + len += sprintf(buf + len, "%d ", srf08_gain[i]);
> > +
> > + len += sprintf(buf + len, "\n");
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(gain_available, S_IRUGO,
> > + srf08_show_gain_available, NULL, 0);
> > +
> > +static ssize_t srf08_show_gain(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + int len;
> > +
> > + len = sprintf(buf, "%d\n", data->gain);
> > +
> > + return len;
> > +}
> > +
> > +static ssize_t srf08_write_gain(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + int ret, i;
> > + unsigned int val;
> > + u8 regval;
> > +
> > + ret = kstrtouint(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> > + if (val == srf08_gain[i]) {
> > + regval = i;
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(srf08_gain))
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_MAX_GAIN, regval);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write_gain - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + data->gain = val;
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
> > + srf08_show_gain, srf08_write_gain, 0);
> > +
> > +static struct attribute *srf08_attributes[] = {
> > + &iio_dev_attr_range_mm.dev_attr.attr,
> > + &iio_dev_attr_range_mm_available.dev_attr.attr,
> > + &iio_dev_attr_gain.dev_attr.attr,
> > + &iio_dev_attr_gain_available.dev_attr.attr,
> Hmm. Custom attributes always give us issues. The primary point of IIO
> is to enforce (more or less) standard interfaces.
>
> If you do need to add something new then that is fine (and I do think
> you need to here!).
>
> They need to be formally proposed as an addition to the ABI with
> docs in /Documentation/ABI/testing/sysfs-bus-iio*
>
> Once we take one driver using it it becomes part of our ABI that
> userspace will need to handle, hence we consider these very
> carefully.
>
> My gut feeling would be that gain needs to be more specific as it's
> a term that can mean very different things.. Here we are talking
> about an amplifier on a signal that we are then looking at the timing
> of. It might otherwise be interpretted as another term for what
> we term 'scale' in IIO.
>
> So what to call it... Perhaps afegain for Analog front end gain?
> We might want to add this to the core supported attrs, but lets
> not do so until we see if we have this on a number of devices.
>
In /Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 there is also a
gain used in a similar situation and it's called there "sensor_sensitivity"
What it we also use this name here?
> The description would need to make it explicit that this gain is
> for cases where we aren't measuring the magnitude of what is
> being amplified.
>
> For the range, it's an interesting one. Again the term range could
> mean too many things within the wider ABI. We need to make it more
> specific.
>
> Actually reading the datasheet, I think this is fundamentally about the
> maximum sampling frequency rather than directly about the range.
> The only reason you'd reduce the range is to speed that up. It doesn't
> improve the resolution, the device simply answers quicker.
>
> So I'd support this as sampling_frequency. You could then use
> the the iio_info_mask_*_available and relevant callback to provide
> info on what it then restricts the possible output values to
> (rather than controlling it directly).
>
By changing the range one cannot influence the sampling frequency directly. I
have seen on the oszilloscope that the telegrams arrive almost at the same time
with different settings of range and the same gain.
Only if the gain is also adjusted the sensor works faster and a higher frequency
can be used. But the gain is also used to adjust the sensitivity of the sensor.
What about calling it "sensor_domain" or "sensor_max_range"?
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group srf08_attribute_group = {
> > + .attrs = srf08_attributes,
> > +};
> > +
> > +static const struct iio_chan_spec srf08_channels[] = {
> > + {
> > + .type = IIO_DISTANCE,
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + },
> > +};
> > +
> > +static const struct iio_info srf08_info = {
> > + .read_raw = srf08_read_raw,
> > + .attrs = &srf08_attribute_group,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int srf08_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct srf08_data *data;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > + return -ENODEV;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + data->client = client;
> > +
> > + /*
> > + * set default values of device here
> > + * these values are already set on the hardware after power on
> > + */
> > + data->gain = SRF08_DEFAULT_GAIN;
> > + data->range_mm = SRF08_DEFAULT_RANGE;
> We should be a little careful with assumptions about the device having
> just been powered on. The driver might simply have been removed and
> reprobed. So I'd sugest rewriting them whatever to be sure we have
> what we expect. Either that or if they can be read back, then just
> always retrieve them from the device.
You are right.
Then i'll set the default value at the sensor, because it cannot be read.
> > +
> > + indio_dev->name = dev_name(&client->dev);
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &srf08_info;
> > + indio_dev->channels = srf08_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(srf08_channels);
> > +
> > + mutex_init(&data->lock);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct i2c_device_id srf08_id[] = {
> > + { "srf08", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, srf08_id);
> > +
> > +static struct i2c_driver srf08_driver = {
> > + .driver = {
> > + .name = "srf08",
> > + },
> > + .probe = srf08_probe,
> > + .id_table = srf08_id,
> > +};
> > +module_i2c_driver(srf08_driver);
> > +
> > +MODULE_AUTHOR("Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>");
> > +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> > +MODULE_LICENSE("GPL");
> >
>
--
--
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
* [PATCH] i2c: busses: constify dev_pm_ops structures
From: Bhumika Goyal @ 2017-01-15 9:59 UTC (permalink / raw)
To: julia.lawall, tony, wsa, linux-omap, linux-i2c, linux-kernel
Cc: Bhumika Goyal
Declare dev_pm_ops structures as const as they are only stored in the pm
field of a device_driver structure. This field is of type const, so
dev_pm_ops structures having similar properties can be declared const
too.
File size before: drivers/i2c/busses/i2c-omap.o
text data bss dec hex filename
6814 584 0 7398 1ce6 drivers/i2c/busses/i2c-omap.o
File size after: drivers/i2c/busses/i2c-omap.o
text data bss dec hex filename
7006 392 0 7398 1ce6 drivers/i2c/busses/i2c-omap.o
Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
drivers/i2c/busses/i2c-omap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c7da0c4..1ebb5e9 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1504,7 +1504,7 @@ static int omap_i2c_runtime_resume(struct device *dev)
return 0;
}
-static struct dev_pm_ops omap_i2c_pm_ops = {
+static const struct dev_pm_ops omap_i2c_pm_ops = {
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
--
1.9.1
^ permalink raw reply related
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-15 11:10 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Jarkko Nikula, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx, tagorereddy
In-Reply-To: <20170113163005.GK31595@intel.com>
Hi,
On 13-01-17 17:30, Ville Syrjälä wrote:
> On Fri, Jan 13, 2017 at 05:06:52PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/13/2017 10:26 AM, Ville Syrjälä wrote:
>>> On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 02-01-17 15:12, Ville Syrjälä wrote:
>>>>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
>>>>>> The punit on baytrail / cherrytrail systems is not only accessed through
>>>>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
>>>>>> the punit against simultaneous accesses and 2 functions to lock / unlock
>>>>>> this mutex.
>>>>>
>>>>> I'm not sure which part of punit you're actually trying to protect
>>>>> here. Some specific registers?
>>>>
>>>> The theory I'm going by is that for certain actions / certain requests
>>>> we send to the punit, the punit needs to access the (axp288) pmic, to
>>>> change (or enable / disable) certain voltages.
>>>
>>> At least for cpu/display/gt voltages that shouldn't really be the case.
>>> The vcc/vnn/vgg rails are controlled via svid, not i2c.
>>
>> Are you sure? The ax288 pmic does not have a svid interface, only
>> an i2c interface, and AFAICT its buck DCDC converters are used to
>> feed all of these.
>
> Yes, looks like you're right. I guess someone didn't want to spend three
> pins for svid.
>
>>
>>> It also feels quite hand wavy since the punit could do whatever at
>>> any time AFAIK. Eg. if there's some thermal event or something the
>>> punit might kick into action. So trying to protect this from the OS
>>> side might not be able to avoid these problems entirely. It feels like
>>> there really should be some kind of shared hardware/firmware mutex
>>> with the punit to arbitrate access to the i2c bus.
>>
>> Right, and there is such a mutex (which only gets used on systems
>> with an axp288 pmic...) and we are taking this mutex before starting
>> an i2c transaction on the pmic i2c bus. But this does not seem to be
>> enough. It seems the the punit does not check the mutex before
>> certain OS / host triggered actions. I guess it expects the host to
>> do this itself.
>>
>> Please see my new (non RFC) version of this series I've posted.
>>
>> There are at least 2 problems when relying solely on the punit
>> pmic i2c bus sempaphore:
>>
>> 1) CPU C1 <-> C6 transations happening while the pmic i2c bus
>> is being accessed by the host cause the system to hang
>> 2) i915 (runtime) suspend resume fails every other attempt
>> with timeouts when trying to get a forcewake lock inn i915,
>> often followed by a system freeze shortly after this.
>
> Hmm. But forcewake works at other times?
It depends on the workload, I believe the forcewake timeouts are
caused by e.g. the axp288 fuel-gauge driver directly accessing
the pmic i2c bus at the same time as the i915 driver is doing a
forcewake. So in essence this is race and as such not 100%
reproducible. With my workload (Fedora 25 with gnome3) full suspend
+ resume is a good way to reproduce. The bug reporter
(tagorereddy) in:
https://bugzilla.kernel.org/show_bug.cgi?id=155241
Is seeing this during normal use when using a kde / plasma desktop.
Some history, this problem started surfacing when I fixed the
i2c punit semaphore code in i2c-designware-baytrail.c to actually
work on cht, before that systems with an axp288 any attempt to
access the i2c bus by e.g. the axp288_fuel_gauge driver would result
in -ETIMEOUT as the code would fail to acquire the punit i2c bus
semaphore, this i2c-designware-baytrail.c cht bug has so far protected
users against the described race (*).
tagorereddy then tried my patches to get battery monitoring working
on his cht device. Then he reported back in the above bug that he
was getting forcewake timeouts + system hangs. I only noticed I could
reproduce them myself on resume later (which was quite useful in
actually developing the proposed fix).
> That seems quite strange.
> Runtime suspend itself shouldn't really do much, and if we're still
> poking at the the hw then we haven't really even suspended anything
> yet, so having failing forcewake doesn't sounds at all good.
Sorry, I'm actually seeing these on a (full not runtime) resume,
not suspend, it seems that at resume my setup has the ideal
circumstances to hit the race.
Regards,
Hans
*) Note as described in the cover letter of the non RFC version of
this patch-set:
https://www.spinics.net/lists/dri-devel/msg128896.html
Disabling access to the pmic i2c bus (as the fixed bug does) is
not a workable solution:
"Unfortunately that will cause some major issues on affected devices:
-No battery monitoring
-No "AC" plugged in monitoring
-If booted with a normal USB-A -> micro-USB cable, or no cable, plugged
in and then the user replaces the cable with an otg USB-host cable /
adapter, the id-pin shorting will enable a 5v boost convertor, but we
need to disable the pmic's USB-Vbus path otherwise it will start drawing
current from the boost convertor, leading to aprox 300mA of extra
battery drain, this is done by the axp288_charger driver, which needs
direct i2c access to the pmic bus"
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-15 11:21 UTC (permalink / raw)
To: Wolfram Sang
Cc: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Len Brown, Andy Shevchenko, intel-gfx, dri-devel,
Mika Westerberg, Takashi Iwai, russianneuromancer @ ya . ru,
linux-i2c
In-Reply-To: <20170112184545.sejbzawnesu6zj5i@ninjato>
Hi,
On 12-01-17 19:45, Wolfram Sang wrote:
> On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
>> Take the punit lock to stop others from accessing the punit while the
>> pmic i2c bus is in use. This is necessary because accessing the punit
>> from the kernel may result in the punit trying to access the pmic i2c
>> bus, which results in a hang when it happens while we own the pmic i2c
>> bus semaphore.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>
> I don't think the I2C patches need to go via I2C tree, so:
>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
Note that as mentioned in the cover-letter, these 2 i2c patches depend
on these:
https://patchwork.ozlabs.org/patch/710067/
https://patchwork.ozlabs.org/patch/710068/
https://patchwork.ozlabs.org/patch/710069/
https://patchwork.ozlabs.org/patch/710070/
https://patchwork.ozlabs.org/patch/710071/
https://patchwork.ozlabs.org/patch/710072/
Which all have many reviews / acks. Given the race between i915
and designware-i2c opened up by the last patch in this series
these not having been merged yet is a good thing, but patch 1-5
are ready for merging.
So Wolfram, what is the plan with these ? As said they are necessary
for the 2 i2c patches in this patch-set, so do you want the
entire set of 8 i2c patches to go through an other tree to avoid
inter tree dependencies ?
And if so, can we then have you're Acked-by for the 6 above too ?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Wolfram Sang @ 2017-01-15 11:45 UTC (permalink / raw)
To: Hans de Goede
Cc: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Len Brown, Andy Shevchenko, intel-gfx, dri-devel,
Mika Westerberg, Takashi Iwai, russianneuromancer @ ya . ru,
linux-i2c
In-Reply-To: <5aa8a3ab-a527-1f3b-0cf6-92fd4691ddc8@redhat.com>
Hi Hans,
> So Wolfram, what is the plan with these ? As said they are necessary
> for the 2 i2c patches in this patch-set, so do you want the
> entire set of 8 i2c patches to go through an other tree to avoid
> inter tree dependencies ?
Thanks for the heads up. So, my plan was that I send a pull request for
4.10 any minute now, and start pulling in patches for 4.11 based on
shiny new rc4 from tomorrow on. And your series would have been one of
the fist to get merged because I think it is ready.
Reading this though, my feeling is now that it should be merged via some
other tree so you can get these nasty issues fixed without too many
dependencies. An immutable branch for me to pull in would be great,
though.
Makes sense?
Regards,
Wolfram
^ permalink raw reply
* [PULL REQUEST] i2c for 4.10
From: Wolfram Sang @ 2017-01-15 12:19 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-i2c, linux-kernel
Linus,
here is a pull request with bugfixes for I2C. Mostly core this time
which is a bit unusual but nothing really scary in there.
Also, this is the first public test run of my git-request-pull
enhancement to give credits to people taking part in quality assurance
(like reviewing and testing). I think we are lacking in this department
and this is my take on fixing it. I'll post about it on LKML seperately
later today.
Please pull.
Thanks,
Wolfram
The following changes since commit a121103c922847ba5010819a3f250f1f7fc84ab8:
Linux 4.10-rc3 (2017-01-08 14:18:17 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-current
for you to fetch changes up to 701dc207bf551d9fe6defa36e84a911e880398c3:
i2c: piix4: Avoid race conditions with IMC (2017-01-12 20:52:12 +0100)
----------------------------------------------------------------
Colin Ian King (1):
i2c: fix spelling mistake: "insufficent" -> "insufficient"
Dmitry Torokhov (1):
i2c: do not enable fall back to Host Notify by default
John Garry (1):
i2c: print correct device invalid address
Ricardo Ribalda Delgado (1):
i2c: piix4: Avoid race conditions with IMC
Vlad Tsyrklevich (1):
i2c: fix kernel memory disclosure in dev interface
with much appreciated quality assurance from
----------------------------------------------------------------
Andy Shevchenko (1):
(Rev.) i2c: piix4: Avoid race conditions with IMC
Benjamin Tissoires (1):
(Test) i2c: do not enable fall back to Host Notify by default
Vladimir Zapolskiy (1):
(Rev.) i2c: print correct device invalid address
Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
drivers/i2c/busses/i2c-piix4.c | 22 ++++++++++++++++++++++
drivers/i2c/i2c-core.c | 21 ++++++++++-----------
drivers/i2c/i2c-dev.c | 2 +-
include/linux/i2c.h | 1 +
5 files changed, 42 insertions(+), 12 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger
From: Lars-Peter Clausen @ 2017-01-15 12:24 UTC (permalink / raw)
To: Andreas Klinger, jic23-DgEjT+Ai2ygdnm+yROfE0A,
knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ktsai-GubuWUlQtMwciDkP5Hr2oA,
wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, trivial-DgEjT+Ai2ygdnm+yROfE0A,
mranostay-Re5JQEeQqe8AvxtiuMwx3w,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170110184815.GA15532@andreas>
On 01/10/2017 07:48 PM, Andreas Klinger wrote:
[...]
> + indio_dev->name = dev_name(&client->dev);
The name is supposed to be the type of the device, e.g. part name, not the
name of parent device instance. E.g. "srf08" in this case.
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger
From: Jonathan Cameron @ 2017-01-15 13:12 UTC (permalink / raw)
To: Andreas Klinger
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ktsai-GubuWUlQtMwciDkP5Hr2oA,
wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, trivial-DgEjT+Ai2ygdnm+yROfE0A,
mranostay-Re5JQEeQqe8AvxtiuMwx3w,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170114174856.GA2351@andreas>
On 14/01/17 17:48, Andreas Klinger wrote:
> Hi Jonathan,
>
> see comments below.
>
> Andreas
>
>
> Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> schrieb am Sat, 14. Jan 12:17:
>> On 10/01/17 18:48, Andreas Klinger wrote:
>>> This is the IIO driver for devantech srf08 ultrasonic ranger which can be
>>> used to measure the distances to an object.
>>>
>>> The sensor supports I2C with some registers.
>>>
>>> Supported Features include:
>>> - read the distance in ranging mode in centimeters
>>> - output of the driver is directly the read value
>>> - together with the scale the driver delivers the distance in meters
>>> - only the first echo of the nearest object is delivered
>>> - set max gain register; userspace enters analogue value
>>> - set range registers; userspace enters range in millimeters in 43 mm steps
>>>
>>> Features not supported by this driver:
>>> - ranging mode in inches or in microseconds
>>> - ANN mode
>>> - change I2C address through this driver
>>> - light sensor
>>>
>>> The driver was added in the directory "proximity" of the iio subsystem
>>> in absence of another directory named "distance".
>>> There is also a new submenu "distance"
>> Hi Andreas,
>>
>> Sorry it took me a while to get to this!
>>
>> I'd not bother with the new submenu. Perhaps we should rename the
>> proximity menu to proximity/distance.
>>
>> We already the lightening detector in there which is definitely not
>> measuring proximity in the convetional sense!
>>
>> Anyhow, the actual code is fine, but we need to think about how the
>> userspace ABI fits within the wider IIO ABI. Naming and approaches
>> that make sense in a single class of drivers can end up meaining
>> very different things for other drivers. Various suggestions inline.
>>
>> Jonathan
>>>
>>> Signed-off-by: Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
>>> ---
>>> drivers/iio/proximity/Kconfig | 15 ++
>>> drivers/iio/proximity/Makefile | 1 +
>>> drivers/iio/proximity/srf08.c | 362 +++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 378 insertions(+)
>>> create mode 100644 drivers/iio/proximity/srf08.c
>>>
>>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
>>> index ef4c73db5b53..7b10a137702b 100644
>>> --- a/drivers/iio/proximity/Kconfig
>>> +++ b/drivers/iio/proximity/Kconfig
>>> @@ -46,3 +46,18 @@ config SX9500
>>> module will be called sx9500.
>>>
>>> endmenu
>>> +
>>> +menu "Distance sensors"
>>> +
>>> +config SRF08
>>> + tristate "Devantech SRF08 ultrasonic ranger sensor"
>>> + depends on I2C
>>> + help
>>> + Say Y here to build a driver for Devantech SRF08 ultrasonic
>>> + ranger sensor. This driver can be used to measure the distance
>>> + of objects.
>>> +
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called srf08.
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
>>> index 9aadd9a8ee99..e914c2a5dd49 100644
>>> --- a/drivers/iio/proximity/Makefile
>>> +++ b/drivers/iio/proximity/Makefile
>>> @@ -5,4 +5,5 @@
>>> # When adding new entries keep the list in alphabetical order
>>> obj-$(CONFIG_AS3935) += as3935.o
>>> obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
>>> +obj-$(CONFIG_SRF08) += srf08.o
>>> obj-$(CONFIG_SX9500) += sx9500.o
>>> diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
>>> new file mode 100644
>>> index 000000000000..f38c74ed0933
>>> --- /dev/null
>>> +++ b/drivers/iio/proximity/srf08.c
>>> @@ -0,0 +1,362 @@
>>> +/*
>>> + * srf08.c - Support for Devantech SRF08 ultrasonic ranger
>>> + *
>>> + * Copyright (c) 2016 Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
>>> + *
>>> + * This file is subject to the terms and conditions of version 2 of
>>> + * the GNU General Public License. See the file COPYING in the main
>>> + * directory of this archive for more details.
>>> + *
>>> + * For details about the device see:
>>> + * http://www.robot-electronics.co.uk/htm/srf08tech.html
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +/* registers of SRF08 device */
>>> +#define SRF08_WRITE_COMMAND 0x00 /* Command Register */
>>> +#define SRF08_WRITE_MAX_GAIN 0x01 /* Max Gain Register: 0 .. 31 */
>>> +#define SRF08_WRITE_RANGE 0x02 /* Range Register: 0 .. 255 */
>>> +#define SRF08_READ_SW_REVISION 0x00 /* Software Revision */
>>> +#define SRF08_READ_LIGHT 0x01 /* Light Sensor during last echo */
>>> +#define SRF08_READ_ECHO_1_HIGH 0x02 /* Range of first echo received */
>>> +#define SRF08_READ_ECHO_1_LOW 0x03 /* Range of first echo received */
>>> +
>>> +#define SRF08_CMD_RANGING_CM 0x51 /* Ranging Mode - Result in cm */
>>> +
>>> +#define SRF08_DEFAULT_GAIN 1025 /* max. analogue value of Gain */
>>> +#define SRF08_DEFAULT_RANGE 11008 /* max. value of Range in mm */
>>> +
>>> +struct srf08_data {
>>> + struct i2c_client *client;
>>> + int gain; /* Max Gain */
>>> + int range_mm; /* Range in mm */
>>> + struct mutex lock;
>>> +};
>>> +
>>> +static const int srf08_gain[] = {
>>> + 94, 97, 100, 103, 107, 110, 114, 118,
>>> + 123, 128, 133, 139, 145, 152, 159, 168,
>>> + 177, 187, 199, 212, 227, 245, 265, 288,
>>> + 317, 352, 395, 450, 524, 626, 777, 1025 };
>>> +
>>> +static int srf08_read_ranging(struct srf08_data *data)
>>> +{
>>> + struct i2c_client *client = data->client;
>>> + int ret, i;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_COMMAND, SRF08_CMD_RANGING_CM);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write command - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * normally after 65 ms the device should have the read value
>>> + * we round it up to 100 ms
>> I'd suggest this should be adapted so that it takes advantage of knowing
>> roughly how long it is going to take as the 'range' maximum is changed.
>> So perhaps in the basic case, sleep for 65 msecs, then poll at 5msec
>> intervals. If we know it's going to be a lot faster, then poll it from
>> an earlier time.
>>> + *
>>> + * we read here until a correct version number shows up as
>>> + * suggested by the documentation
>>> + */
>>> + for (i = 0; i < 5; i++) {
>>> + ret = i2c_smbus_read_byte_data(data->client,
>>> + SRF08_READ_SW_REVISION);
>>> +
>>> + /* check if a valid version number is read */
>>> + if (ret < 255 && ret > 0)
>>> + break;
>>> + msleep(20);
>>> + }
>>> +
>>> + if (ret >= 255 || ret <= 0) {
>>> + dev_err(&client->dev, "device not ready\n");
>>> + mutex_unlock(&data->lock);
>>> + return -EIO;
>>> + }
>>> +
>>> + ret = i2c_smbus_read_word_swapped(data->client,
>>> + SRF08_READ_ECHO_1_HIGH);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "cannot read distance: ret=%d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int srf08_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *channel, int *val,
>>> + int *val2, long mask)
>>> +{
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (channel->type != IIO_DISTANCE)
>>> + return -EINVAL;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = srf08_read_ranging(data);
>>> + if (ret < 0)
>>> + return ret;
>>> + *val = ret;
>>> + return IIO_VAL_INT;
>>> + case IIO_CHAN_INFO_SCALE:
>>> + /* 1 LSB is 1 cm */
>>> + *val = 0;
>>> + *val2 = 10000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static ssize_t srf08_show_range_mm_available(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + int i, len = 0;
>>> +
>>> + for (i = 0; i < 256; i++)
>>> + len += scnprintf(buf + len, PAGE_SIZE - len,
>>> + "%d ", (i + 1) * 43);
>>> +
>>> + buf[len - 1] = '\n';
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(range_mm_available, S_IRUGO,
>>> + srf08_show_range_mm_available, NULL, 0);
>>> +
>>> +static ssize_t srf08_show_range_mm(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> +
>>> + return sprintf(buf, "%d\n", data->range_mm);
>>> +}
>>> +
>>> +/*
>>> + * set the range of the sensor to an even multiple of 43 mm
>>> + * which corresponds to 1 LSB in the register
>>> + *
>>> + * register value corresponding range
>>> + * 0x00 43 mm
>>> + * 0x01 86 mm
>>> + * 0x02 129 mm
>>> + * ...
>>> + * 0xFF 11008 mm
>>> + */
>>> +static ssize_t srf08_write_range_mm(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret;
>>> + unsigned int val, mod;
>>> + u8 regval;
>>> +
>>> + ret = kstrtouint(buf, 10, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = val / 43 - 1;
>>> + mod = val % 43;
>>> +
>>> + if (mod || (ret < 0) || (ret > 255))
>>> + return -EINVAL;
>>> +
>>> + regval = ret;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_RANGE, regval);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write_range - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + data->range_mm = val;
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(range_mm, S_IRUGO | S_IWUSR,
>>> + srf08_show_range_mm, srf08_write_range_mm, 0);
>>> +
>>> +static ssize_t srf08_show_gain_available(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + int i, len = 0;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
>>> + len += sprintf(buf + len, "%d ", srf08_gain[i]);
>>> +
>>> + len += sprintf(buf + len, "\n");
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(gain_available, S_IRUGO,
>>> + srf08_show_gain_available, NULL, 0);
>>> +
>>> +static ssize_t srf08_show_gain(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + int len;
>>> +
>>> + len = sprintf(buf, "%d\n", data->gain);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t srf08_write_gain(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret, i;
>>> + unsigned int val;
>>> + u8 regval;
>>> +
>>> + ret = kstrtouint(buf, 10, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
>>> + if (val == srf08_gain[i]) {
>>> + regval = i;
>>> + break;
>>> + }
>>> +
>>> + if (i >= ARRAY_SIZE(srf08_gain))
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_MAX_GAIN, regval);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write_gain - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + data->gain = val;
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
>>> + srf08_show_gain, srf08_write_gain, 0);
>>> +
>>> +static struct attribute *srf08_attributes[] = {
>>> + &iio_dev_attr_range_mm.dev_attr.attr,
>>> + &iio_dev_attr_range_mm_available.dev_attr.attr,
>>> + &iio_dev_attr_gain.dev_attr.attr,
>>> + &iio_dev_attr_gain_available.dev_attr.attr,
>> Hmm. Custom attributes always give us issues. The primary point of IIO
>> is to enforce (more or less) standard interfaces.
>>
>> If you do need to add something new then that is fine (and I do think
>> you need to here!).
>>
>> They need to be formally proposed as an addition to the ABI with
>> docs in /Documentation/ABI/testing/sysfs-bus-iio*
>>
>> Once we take one driver using it it becomes part of our ABI that
>> userspace will need to handle, hence we consider these very
>> carefully.
>>
>> My gut feeling would be that gain needs to be more specific as it's
>> a term that can mean very different things.. Here we are talking
>> about an amplifier on a signal that we are then looking at the timing
>> of. It might otherwise be interpretted as another term for what
>> we term 'scale' in IIO.
>>
>> So what to call it... Perhaps afegain for Analog front end gain?
>> We might want to add this to the core supported attrs, but lets
>> not do so until we see if we have this on a number of devices.
>>
>
> In /Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 there is also a
> gain used in a similar situation and it's called there "sensor_sensitivity"
>
> What it we also use this name here?
It's not ideal as it's not linked to a particular channel or anything,
but lets go with that as at least we will be consistent between drivers.
>
>> The description would need to make it explicit that this gain is
>> for cases where we aren't measuring the magnitude of what is
>> being amplified.
>>
>> For the range, it's an interesting one. Again the term range could
>> mean too many things within the wider ABI. We need to make it more
>> specific.
>>
>> Actually reading the datasheet, I think this is fundamentally about the
>> maximum sampling frequency rather than directly about the range.
>> The only reason you'd reduce the range is to speed that up. It doesn't
>> improve the resolution, the device simply answers quicker.
>>
>> So I'd support this as sampling_frequency. You could then use
>> the the iio_info_mask_*_available and relevant callback to provide
>> info on what it then restricts the possible output values to
>> (rather than controlling it directly).
>>
>
> By changing the range one cannot influence the sampling frequency directly. I
> have seen on the oszilloscope that the telegrams arrive almost at the same time
> with different settings of range and the same gain.
>
> Only if the gain is also adjusted the sensor works faster and a higher frequency
> can be used. But the gain is also used to adjust the sensitivity of the sensor.
That's rather weird and not what the datasheet suggests. Ah well.
>
> What about calling it "sensor_domain" or "sensor_max_range"?
hmm. Not sure - propose that with appropriate Docs and we can think more on it.
>
>
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group srf08_attribute_group = {
>>> + .attrs = srf08_attributes,
>>> +};
>>> +
>>> +static const struct iio_chan_spec srf08_channels[] = {
>>> + {
>>> + .type = IIO_DISTANCE,
>>> + .info_mask_separate =
>>> + BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + },
>>> +};
>>> +
>>> +static const struct iio_info srf08_info = {
>>> + .read_raw = srf08_read_raw,
>>> + .attrs = &srf08_attribute_group,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int srf08_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct iio_dev *indio_dev;
>>> + struct srf08_data *data;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + data = iio_priv(indio_dev);
>>> + i2c_set_clientdata(client, indio_dev);
>>> + data->client = client;
>>> +
>>> + /*
>>> + * set default values of device here
>>> + * these values are already set on the hardware after power on
>>> + */
>>> + data->gain = SRF08_DEFAULT_GAIN;
>>> + data->range_mm = SRF08_DEFAULT_RANGE;
>> We should be a little careful with assumptions about the device having
>> just been powered on. The driver might simply have been removed and
>> reprobed. So I'd sugest rewriting them whatever to be sure we have
>> what we expect. Either that or if they can be read back, then just
>> always retrieve them from the device.
>
> You are right.
> Then i'll set the default value at the sensor, because it cannot be read.
>
>>> +
>>> + indio_dev->name = dev_name(&client->dev);
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &srf08_info;
>>> + indio_dev->channels = srf08_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(srf08_channels);
>>> +
>>> + mutex_init(&data->lock);
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id srf08_id[] = {
>>> + { "srf08", 0 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, srf08_id);
>>> +
>>> +static struct i2c_driver srf08_driver = {
>>> + .driver = {
>>> + .name = "srf08",
>>> + },
>>> + .probe = srf08_probe,
>>> + .id_table = srf08_id,
>>> +};
>>> +module_i2c_driver(srf08_driver);
>>> +
>>> +MODULE_AUTHOR("Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-15 15:11 UTC (permalink / raw)
To: Wolfram Sang
Cc: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Len Brown, Andy Shevchenko, intel-gfx, dri-devel,
Mika Westerberg, Takashi Iwai, russianneuromancer @ ya . ru,
linux-i2c
In-Reply-To: <20170115114529.jxwcadse23gdykx5@ninjato>
Hi,
On 15-01-17 12:45, Wolfram Sang wrote:
> Hi Hans,
>
>> So Wolfram, what is the plan with these ? As said they are necessary
>> for the 2 i2c patches in this patch-set, so do you want the
>> entire set of 8 i2c patches to go through an other tree to avoid
>> inter tree dependencies ?
>
> Thanks for the heads up. So, my plan was that I send a pull request for
> 4.10 any minute now, and start pulling in patches for 4.11 based on
> shiny new rc4 from tomorrow on. And your series would have been one of
> the fist to get merged because I think it is ready.
>
> Reading this though, my feeling is now that it should be merged via some
> other tree so you can get these nasty issues fixed without too many
> dependencies. An immutable branch for me to pull in would be great,
> though.
>
> Makes sense?
Sounds fine to me, step one is to get consensus on how to deal with
coordinating the kernel directly accessing to the pmic-i2c-bus vs punit
accesses which also end up needing to use the same i2c-bus from the
punit side.
Once I've a patch-set everyone likes I will start talking to people
to coordinate the merging, I believe it is probably best for all this
to be merged through the drm-intel tree. But we'll see about that
when the patch-set is ready.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Wolfram Sang @ 2017-01-15 15:17 UTC (permalink / raw)
To: Hans de Goede
Cc: russianneuromancer @ ya . ru, intel-gfx, linux-i2c, Jarkko Nikula,
dri-devel, Daniel Vetter, Andy Shevchenko, Mika Westerberg,
Len Brown
In-Reply-To: <84cabe1f-1129-19b9-f0ed-6ec5f189bb29@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 252 bytes --]
> Once I've a patch-set everyone likes I will start talking to people
> to coordinate the merging, I believe it is probably best for all this
> to be merged through the drm-intel tree. But we'll see about that
> when the patch-set is ready.
Agreed.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v5 1/6] i2c: designware: Rename accessor_flags to flags
From: Wolfram Sang @ 2017-01-15 15:18 UTC (permalink / raw)
To: Hans de Goede
Cc: Takashi Iwai, russianneuromancer @ ya . ru, intel-gfx,
Jarkko Nikula, linux-i2c, Andy Shevchenko, Mika Westerberg,
Len Brown
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 552 bytes --]
On Sun, Jan 01, 2017 at 09:15:16PM +0100, Hans de Goede wrote:
> Rename accessor_flags to flags, so that we can use the field for
> other flags too. This is a preparation patch for adding cherrytrail
> support to the punit semaphore code.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Tested-by: Takashi Iwai <tiwai@suse.de>
This and the whole series is:
Acked-by: Wolfram Sang <wsa@the-dreams.de>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH linux v2 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: Guenter Roeck @ 2017-01-15 18:15 UTC (permalink / raw)
To: eajames.ibm
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-6-git-send-email-eajames.ibm@gmail.com>
On 01/11/2017 10:10 AM, eajames.ibm@gmail.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Documentation/devicetree/bindings/hwmon/occ.txt | 13 +++
> drivers/hwmon/occ/Kconfig | 14 +++
> drivers/hwmon/occ/Makefile | 1 +
> drivers/hwmon/occ/p8_occ_i2c.c | 123 ++++++++++++++++++++++++
> 4 files changed, 151 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
> create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
> new file mode 100644
> index 0000000..b0d2b36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/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..4515c68
> --- /dev/null
> +++ b/drivers/hwmon/occ/p8_occ_i2c.c
> @@ -0,0 +1,123 @@
> +/*
> + * 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>
For all patches: alphabetic order, please.
> +
> +#include "scom.h"
> +#include "occ_scom_i2c.h"
> +#include "occ_p8.h"
> +#include "occ_sysfs.h"
> +
> +#define P8_OCC_I2C_NAME "p8-occ-i2c"
> +
> +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 int p8_occ_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct occ *occ;
> + struct occ_sysfs *hwmon;
> + const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
> +
> + 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, sensor_hwmon_configs,
> + P8_OCC_I2C_NAME);
> + if (IS_ERR(hwmon))
> + return PTR_ERR(hwmon);
> +
> + i2c_set_clientdata(client, occ);
This maps to
dev_set_drvdata(&client->dev, occ);
and code in occ_sysfs_start() does the same, writing a reference to the hwmon device.
> +
> + return 0;
> +}
> +
> +static int p8_occ_remove(struct i2c_client *client)
> +{
> + struct occ *occ = i2c_get_clientdata(client);
> +
> + return p8_occ_stop(occ);
> +}
> +
> +/* 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@us.ibm.com>");
> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply
* Re: [PATCH linux v2 2/6] hwmon: occ: Add sysfs interface
From: Guenter Roeck @ 2017-01-15 18:18 UTC (permalink / raw)
To: eajames.ibm
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-3-git-send-email-eajames.ibm@gmail.com>
On 01/11/2017 10:10 AM, 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>
> ---
> Documentation/hwmon/occ | 62 ++++++++++
> drivers/hwmon/occ/Makefile | 2 +-
> drivers/hwmon/occ/occ_sysfs.c | 274 ++++++++++++++++++++++++++++++++++++++++++
> drivers/hwmon/occ/occ_sysfs.h | 44 +++++++
> 4 files changed, 381 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..d0bdf06 100644
> --- a/Documentation/hwmon/occ
> +++ b/Documentation/hwmon/occ
> @@ -25,6 +25,68 @@ 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 a number of sysfs files for each type of sensor. The
> +sensor-specific files vary depending on the processor type, though many of the
> +attributes are common for both the POWER8 and POWER9.
> +
> +The hwmon interface cannot define every type of sensor that may be used.
> +Therefore, the frequency sensor on the OCC uses the "input" type sensor defined
> +by the hwmon interface, rather than defining a new type of custom sensor.
> +
> +Below are detailed the names and meaning of each sensor file for both types of
> +processors. All sensors are read-only unless otherwise specified. <x> indicates
> +the hwmon index. sensor id indicates the unique internal OCC identifer. Please
> +see the POWER OCC specification for details on all these sensor values.
> +
> +frequency:
> + all processors:
> + in<x>_input - frequency value
> + in<x>_label - sensor id
> +temperature:
> + POWER8:
> + temp<x>_input - temperature value
> + temp<x>_label - sensor id
> + POWER9 (in addition to above):
> + temp<x>_type - FRU type
> +
> +power:
> + POWER8:
> + power<x>_input - power value
> + power<x>_label - sensor id
> + power<x>_average - accumulator
> + power<x>_average_interval - update tag (number of samples in
> + accumulator)
> + POWER9:
> + power<x>_input - power value
> + power<x>_label - sensor id
> + power<x>_average_min - accumulator[0]
> + power<x>_average_max - accumulator[1] (64 bits total)
> + power<x>_average_interval - update tag
> + power<x>_reset_history - (function_id | (apss_channel << 8)
> +
> +caps:
> + POWER8:
> + power<x>_cap - current powercap
> + power<x>_cap_max - max powercap
> + power<x>_cap_min - min powercap
> + power<x>_max - normal powercap
> + power<x>_alarm - user powercap, r/w
> + POWER9:
> + power<x>_cap_alarm - user powercap source
> +
> +The driver also provides two sysfs entries through hwmon to better
> +control the driver and monitor the master OCC. Though there may be multiple
> +OCCs present on the system, these two files are only present for the "master"
> +OCC.
> + name - read the name of the driver
> + update_interval - read or write the minimum interval for polling the
> + OCC.
> +
> 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..e846b0c
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.c
> @@ -0,0 +1,274 @@
> +/*
> + * 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>
> +
Again, for all patches: alphabetic order, please.
> +#include "occ.h"
> +#include "occ_sysfs.h"
> +
> +#define RESP_RETURN_CMD_INVAL 0x13
> +
> +static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + int rc = 0;
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + struct occ *occ = driver->occ;
> +
> + switch (type) {
> + case hwmon_in:
> + rc = occ_get_sensor_value(occ, FREQ, channel, attr, val);
> + break;
> + case hwmon_temp:
> + rc = occ_get_sensor_value(occ, TEMP, channel, attr, val);
> + break;
> + case hwmon_power:
> + rc = occ_get_sensor_value(occ, POWER, channel, attr, val);
> + break;
> + default:
> + rc = -EOPNOTSUPP;
> + }
> +
> + return rc;
> +}
> +
> +static int occ_hwmon_read_string(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, char **str)
> +{
> + int rc;
> + unsigned long val = 0;
> +
> + if (!((type == hwmon_in && attr == hwmon_in_label) ||
> + (type == hwmon_temp && attr == hwmon_temp_label) ||
> + (type == hwmon_power && attr == hwmon_power_label)))
> + return -EOPNOTSUPP;
> +
> + rc = occ_hwmon_read(dev, type, attr, channel, &val);
> + if (rc < 0)
> + return rc;
> +
> + rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);
> + if (rc > 0)
> + rc = 0;
> +
> + return rc;
> +}
> +
> +static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + int rc = 0;
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> + occ_set_update_interval(driver->occ, val);
> + return 0;
> + } else if (type == hwmon_power && attr == hwmon_power_alarm) {
> + rc = occ_set_user_powercap(driver->occ, val);
> + if (rc) {
> + if (rc == RESP_RETURN_CMD_INVAL) {
> + dev_err(dev,
> + "set invalid powercap value: %ld\n",
> + val);
> + return -EINVAL;
> + }
> +
> + dev_err(dev, "set user powercap failed: 0x:%x\n", rc);
> + return rc;
> + }
> +
> + driver->user_powercap = val;
> +
> + return rc;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + const struct occ_sysfs *driver = data;
> +
> + switch (type) {
> + case hwmon_chip:
> + if (attr == hwmon_chip_update_interval)
> + return S_IRUGO | S_IWUSR;
> + break;
> + case hwmon_in:
> + if (BIT(attr) & driver->sensor_hwmon_configs[0])
> + return S_IRUGO;
> + break;
> + case hwmon_temp:
> + if (BIT(attr) & driver->sensor_hwmon_configs[1])
> + return S_IRUGO;
> + break;
> + case hwmon_power:
> + /* user power limit */
> + if (attr == hwmon_power_alarm)
> + return S_IRUGO | S_IWUSR;
> + else if ((BIT(attr) & driver->sensor_hwmon_configs[2]) ||
> + (BIT(attr) & driver->sensor_hwmon_configs[3]))
> + return S_IRUGO;
> + break;
> + default:
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_ops occ_hwmon_ops = {
> + .is_visible = occ_is_visible,
> + .read = occ_hwmon_read,
> + .read_string = occ_hwmon_read_string,
> + .write = occ_hwmon_write,
> +};
> +
> +static const u32 occ_chip_config[] = {
> + HWMON_C_UPDATE_INTERVAL,
> + 0
> +};
> +
> +static const struct hwmon_channel_info occ_chip = {
> + .type = hwmon_chip,
> + .config = occ_chip_config
> +};
> +
> +static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
> + hwmon_in,
> + hwmon_temp,
> + hwmon_power,
> + hwmon_power
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + const u32 *sensor_hwmon_configs,
> + const char *name)
> +{
> + bool master_occ = false;
> + int rc, i, j, sensor_num, index = 0, id;
> + char *brk;
> + struct occ_blocks *resp = NULL;
> + u32 *sensor_config;
> + struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> + GFP_KERNEL);
> + if (!hwmon)
> + return ERR_PTR(-ENOMEM);
> +
> + /* need space for null-termination and occ chip */
> + hwmon->occ_sensors =
> + devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
> + (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
> + if (!hwmon->occ_sensors)
> + return ERR_PTR(-ENOMEM);
> +
> + hwmon->occ = occ;
> + hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;
> + hwmon->occ_info.ops = &occ_hwmon_ops;
> + hwmon->occ_info.info =
> + (const struct hwmon_channel_info **)hwmon->occ_sensors;
> +
> + dev_set_drvdata(dev, hwmon);
> +
Overwritten in the calling code. It is actually not needed here because dev_get_drvdata()
in the access functions gets the value from the hwmon device, and that is set with
the 3rd parameter of devm_hwmon_device_register_with_info().
> + occ_get_response_blocks(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(occ);
> + if (rc) {
> + dev_err(dev, "cannot get occ sensor data: %d\n", rc);
> + return ERR_PTR(rc);
> + }
> + if (!resp->blocks)
> + return ERR_PTR(-ENOMEM);
> +
> + master_occ = resp->sensor_block_id[CAPS] >= 0;
> +
> + 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;
> + /* need null-termination */
> + sensor_config = devm_kzalloc(dev,
> + sizeof(u32) * (sensor_num + 1),
> + GFP_KERNEL);
> + if (!sensor_config)
> + return ERR_PTR(-ENOMEM);
> +
> + for (j = 0; j < sensor_num; j++)
> + sensor_config[j] = sensor_hwmon_configs[i];
> +
> + hwmon->occ_sensors[index] =
> + devm_kzalloc(dev, sizeof(struct hwmon_channel_info),
> + GFP_KERNEL);
> + if (!hwmon->occ_sensors[index])
> + return ERR_PTR(-ENOMEM);
> +
> + hwmon->occ_sensors[index]->type = occ_sensor_types[i];
> + hwmon->occ_sensors[index]->config = sensor_config;
> + index++;
> + }
> +
> + /* only need one of these for any number of occs */
> + if (master_occ)
> + hwmon->occ_sensors[index] =
> + (struct hwmon_channel_info *)&occ_chip;
> +
> + /* search for bad chars */
> + strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
> + brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
> + while (brk) {
> + *brk = '_';
> + brk = strpbrk(brk, "-* \t\n");
> + }
> +
> + hwmon->dev = devm_hwmon_device_register_with_info(dev,
> + hwmon->hwmon_name,
> + hwmon,
> + &hwmon->occ_info,
> + NULL);
> + if (IS_ERR(hwmon->dev)) {
> + dev_err(dev, "cannot register hwmon device %s: %ld\n",
> + hwmon->hwmon_name, PTR_ERR(hwmon->dev));
> + return ERR_PTR(PTR_ERR(hwmon->dev));
> + }
> +
> + return hwmon;
> +}
> +EXPORT_SYMBOL(occ_sysfs_start);
> +
> +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..7de92e7
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.h
> @@ -0,0 +1,44 @@
> +/*
> + * 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 <linux/hwmon.h>
> +
> +struct occ;
> +struct device;
> +
> +#define OCC_HWMON_NAME_LENGTH 32
> +
> +struct occ_sysfs {
> + struct device *dev;
> + struct occ *occ;
> +
> + char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
> + u32 *sensor_hwmon_configs;
> + struct hwmon_channel_info **occ_sensors;
> + struct hwmon_chip_info occ_info;
> + u16 user_powercap;
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + const u32 *sensor_hwmon_configs,
> + const char *name);
> +#endif /* __OCC_SYSFS_H__ */
>
^ permalink raw reply
* Re: [PATCH linux v2 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: Guenter Roeck @ 2017-01-15 18:34 UTC (permalink / raw)
To: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
corbet-T1hC0tSOHrs, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, wsa-z923LK4zBo2bacvFa/9K2g,
andrew-zrmu5oMJ5Fs, joel-U3u1mxZcP9KHXe+LvDLADg,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, Edward A. James
In-Reply-To: <1484158237-10014-6-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 01/11/2017 10:10 AM, eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> 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>
> ---
> Documentation/devicetree/bindings/hwmon/occ.txt | 13 +++
> drivers/hwmon/occ/Kconfig | 14 +++
> drivers/hwmon/occ/Makefile | 1 +
> drivers/hwmon/occ/p8_occ_i2c.c | 123 ++++++++++++++++++++++++
> 4 files changed, 151 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
> create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
> new file mode 100644
> index 0000000..b0d2b36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/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..4515c68
> --- /dev/null
> +++ b/drivers/hwmon/occ/p8_occ_i2c.c
> @@ -0,0 +1,123 @@
> +/*
> + * 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"
> +
> +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 int p8_occ_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct occ *occ;
> + struct occ_sysfs *hwmon;
> + const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
> +
> + 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, sensor_hwmon_configs,
> + P8_OCC_I2C_NAME);
> + if (IS_ERR(hwmon))
> + return PTR_ERR(hwmon);
> +
> + i2c_set_clientdata(client, occ);
> +
You might do the set_clientdata() call before the registration, and use
return PTR_ERR_OR_ZERO(hwmon). You'll need to drop the call to dev_set_drvdata()
in occ_sysfs_start() for that to work.
> + return 0;
> +}
> +
> +static int p8_occ_remove(struct i2c_client *client)
> +{
> + struct occ *occ = i2c_get_clientdata(client);
> +
> + return p8_occ_stop(occ);
That stops occ first, then removes the hwmon driver registration. It also leaves a hole:
if occ_sysfs_start() failed, occ_stop() won't be called.
But wait ... all occ_stop does is to call devm_kfree(), which is wrong, because it
should never be necessary to call it, but even more so since the data structure
is still in use after the call to p8_occ_stop(). Just drop that function.
> +}
> +
> +/* 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 };
> +
... but only if there is a detect function, and trying to detect any chip
on the common eeprom addresses is not a good idea anyway (it would be really
easy to spoof). Please drop.
> +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");
>
--
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
* Re: [PATCH linux v2 0/6] drivers: hwmon: Add On-Chip Controller driver
From: Guenter Roeck @ 2017-01-15 18:35 UTC (permalink / raw)
To: eajames.ibm
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
On 01/11/2017 10:10 AM, eajames.ibm@gmail.com wrote:
> 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.
>
Overall looks pretty good. There is the 0day hiccup (was that solved ?) and
a few comments I made separately. It might make sense to add someone from IBM
as maintainer.
Thanks,
Guenter
> 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
>
> Documentation/devicetree/bindings/hwmon/occ.txt | 13 +
> Documentation/hwmon/occ | 114 +++++
> drivers/hwmon/Kconfig | 2 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/occ/Kconfig | 29 ++
> drivers/hwmon/occ/Makefile | 2 +
> drivers/hwmon/occ/occ.c | 533 ++++++++++++++++++++++++
> drivers/hwmon/occ/occ.h | 83 ++++
> drivers/hwmon/occ/occ_p8.c | 254 +++++++++++
> drivers/hwmon/occ/occ_p8.h | 31 ++
> drivers/hwmon/occ/occ_p9.c | 314 ++++++++++++++
> drivers/hwmon/occ/occ_p9.h | 31 ++
> drivers/hwmon/occ/occ_scom_i2c.c | 73 ++++
> drivers/hwmon/occ/occ_scom_i2c.h | 26 ++
> drivers/hwmon/occ/occ_sysfs.c | 274 ++++++++++++
> drivers/hwmon/occ/occ_sysfs.h | 44 ++
> drivers/hwmon/occ/p8_occ_i2c.c | 123 ++++++
> drivers/hwmon/occ/scom.h | 47 +++
> 18 files changed, 1994 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/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_p9.c
> create mode 100644 drivers/hwmon/occ/occ_p9.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/scom.h
>
^ permalink raw reply
* [PATCH v4 0/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latenct can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.
In addition an additional enable mask is added to work around devices
that assert irq immediately before being setup buy disabling the irq
from the mux until all devices are registered.
Changes from v3:
- p3: Add spin lock to irq mask / unmask.
- p4: Add Rob's ack.
Changes from v2:
- p1: Added Acked-by
- p5: fixup 2 typos
Changes from v1:
- Update for new ACPI table
- Fix typo in documentation
- Fix typo in function names
- Fix typo in irq name
- Added spaces around '+' / '='
- Change goto label names
- Change property name from i2c-mux-irq-mask-en to nxp,irq-mask-enable
- Change variable name irq_mask_en to irq_mask_enable
- Add commentt about irq_mask_enable
- Added Acked-By's
Phil Reid (5):
i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
dt: bindings: i2c-mux-pca954x: Add documentation for interrupt
controller
i2c: mux: pca954x: Add interrupt controller support
dt: bindings: i2c-mux-pca954x: Add documentation for
i2c-mux-irq-mask-en
i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
.../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 17 ++-
drivers/i2c/muxes/i2c-mux-pca954x.c | 170 ++++++++++++++++++++-
2 files changed, 182 insertions(+), 5 deletions(-)
--
1.8.3.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
* [PATCH v4 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484536275-75995-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Various muxes can aggregate multiple irq lines and provide a control
register to determine the active line. Add bindings for interrupt
controller support.
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index cf53d5f..aa09704 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,7 +19,14 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
-
+ - interrupt-parent: Phandle for the interrupt controller that services
+ interrupts for this device.
+ - interrupts: Interrupt mapping for IRQ.
+ - interrupt-controller: Marks the device node as an interrupt controller.
+ - #interrupt-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify flags.
+ See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Example:
@@ -29,6 +36,11 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ interrupt-parent = <&ipic>;
+ interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
i2c@2 {
#address-cells = <1>;
#size-cells = <0>;
--
1.8.3.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 v4 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484536275-75995-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latency can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.
As there is no irq masking on the mux irq are disabled until irq_unmask is
called at least once.
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 141 +++++++++++++++++++++++++++++++++++-
1 file changed, 139 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index bbf088e..f55da88 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,14 +41,20 @@
#include <linux/i2c.h>
#include <linux/i2c-mux.h>
#include <linux/i2c/pca954x.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <linux/pm.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#define PCA954X_MAX_NCHANS 8
+#define PCA954X_IRQ_OFFSET 4
+
enum pca_type {
pca_9540,
pca_9542,
@@ -63,6 +69,7 @@ enum pca_type {
struct chip_desc {
u8 nchans;
u8 enable; /* used for muxes only */
+ u8 has_irq;
enum muxtype {
pca954x_ismux = 0,
pca954x_isswi
@@ -75,6 +82,10 @@ struct pca954x {
u8 last_chan; /* last register value */
u8 deselect;
struct i2c_client *client;
+
+ struct irq_domain *irq;
+ unsigned int irq_mask;
+ spinlock_t lock;
};
/* Provide specs for the PCA954x types we know about */
@@ -87,19 +98,23 @@ struct pca954x {
[pca_9542] = {
.nchans = 2,
.enable = 0x4,
+ .has_irq = 1,
.muxtype = pca954x_ismux,
},
[pca_9543] = {
.nchans = 2,
+ .has_irq = 1,
.muxtype = pca954x_isswi,
},
[pca_9544] = {
.nchans = 4,
.enable = 0x4,
+ .has_irq = 1,
.muxtype = pca954x_ismux,
},
[pca_9545] = {
.nchans = 4,
+ .has_irq = 1,
.muxtype = pca954x_isswi,
},
[pca_9547] = {
@@ -222,6 +237,114 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
return pca954x_reg_write(muxc->parent, client, data->last_chan);
}
+static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
+{
+ struct pca954x *data = dev_id;
+ unsigned int child_irq;
+ int ret, i, handled;
+
+ ret = i2c_smbus_read_byte(data->client);
+ if (ret < 0)
+ return IRQ_NONE;
+
+ for (i = 0; i < data->chip->nchans; i++) {
+ if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
+ child_irq = irq_linear_revmap(data->irq, i);
+ handle_nested_irq(child_irq);
+ handled++;
+ }
+ }
+ return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void pca954x_irq_mask(struct irq_data *idata)
+{
+ struct pca954x *data = irq_data_get_irq_chip_data(idata);
+ unsigned int pos = idata->hwirq;
+ unsigned long flags;
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ data->irq_mask &= ~BIT(pos);
+ if (!data->irq_mask)
+ disable_irq(data->client->irq);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void pca954x_irq_unmask(struct irq_data *idata)
+{
+ struct pca954x *data = irq_data_get_irq_chip_data(idata);
+ unsigned int pos = idata->hwirq;
+ unsigned long flags;
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ if (!data->irq_mask)
+ enable_irq(data->client->irq);
+ data->irq_mask |= BIT(pos);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+{
+ if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
+ return -EINVAL;
+ return 0;
+}
+
+static struct irq_chip pca954x_irq_chip = {
+ .name = "i2c-mux-pca954x",
+ .irq_mask = pca954x_irq_mask,
+ .irq_unmask = pca954x_irq_unmask,
+ .irq_set_type = pca954x_irq_set_type,
+};
+
+static int pca954x_irq_setup(struct i2c_mux_core *muxc)
+{
+ struct pca954x *data = i2c_mux_priv(muxc);
+ struct i2c_client *client = data->client;
+ int c, err, irq;
+
+ if (!data->chip->has_irq || client->irq <= 0)
+ return 0;
+
+ spin_lock_init(&data->lock);
+
+ data->irq = irq_domain_add_linear(client->dev.of_node,
+ data->chip->nchans,
+ &irq_domain_simple_ops, data);
+ if (!data->irq)
+ return -ENODEV;
+
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_create_mapping(data->irq, c);
+ irq_set_chip_data(irq, data);
+ irq_set_chip_and_handler(irq, &pca954x_irq_chip,
+ handle_simple_irq);
+ }
+
+ err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
+ pca954x_irq_handler,
+ IRQF_ONESHOT | IRQF_SHARED,
+ "pca954x", data);
+ if (err)
+ goto err_req_irq;
+
+ disable_irq(data->client->irq);
+
+ return 0;
+err_req_irq:
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_find_mapping(data->irq, c);
+ irq_dispose_mapping(irq);
+ }
+ irq_domain_remove(data->irq);
+
+ return err;
+}
+
/*
* I2C init/probing/exit functions
*/
@@ -286,6 +409,10 @@ static int pca954x_probe(struct i2c_client *client,
idle_disconnect_dt = of_node &&
of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+ ret = pca954x_irq_setup(muxc);
+ if (ret)
+ goto fail_del_adapters;
+
/* Now create an adapter for each channel */
for (num = 0; num < data->chip->nchans; num++) {
bool idle_disconnect_pd = false;
@@ -311,7 +438,7 @@ static int pca954x_probe(struct i2c_client *client,
dev_err(&client->dev,
"failed to register multiplexed adapter"
" %d as bus %d\n", num, force);
- goto virt_reg_failed;
+ goto fail_del_adapters;
}
}
@@ -322,7 +449,7 @@ static int pca954x_probe(struct i2c_client *client,
return 0;
-virt_reg_failed:
+fail_del_adapters:
i2c_mux_del_adapters(muxc);
return ret;
}
@@ -330,6 +457,16 @@ static int pca954x_probe(struct i2c_client *client,
static int pca954x_remove(struct i2c_client *client)
{
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+ struct pca954x *data = i2c_mux_priv(muxc);
+ int c, irq;
+
+ if (data->irq) {
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_find_mapping(data->irq, c);
+ irq_dispose_mapping(irq);
+ }
+ irq_domain_remove(data->irq);
+ }
i2c_mux_del_adapters(muxc);
return 0;
--
1.8.3.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 v4 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484536275-75995-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index f55da88..66f7ed8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -76,6 +76,19 @@ struct chip_desc {
} muxtype;
};
+/*
+ * irq_mask_enable: Provides a mechanism to work around hardware that asserts
+ * their irq immediately on power on. It allows the enabling of the irq to be
+ * delayed until the corresponding bits in the the irq_mask are set thru
+ * irq_unmask.
+ * For example the ltc1760 assert its SMBALERT irq immediately on power on.
+ * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
+ * device is registered irq are enabled and fire continuously as the second
+ * device driver has not yet loaded. Setting this parameter to 0x3 while
+ * delay the irq being enabled until both devices are ready.
+ * This workaround will not work if two devices share an interrupt on the
+ * same bus segment.
+ */
struct pca954x {
const struct chip_desc *chip;
@@ -84,6 +97,7 @@ struct pca954x {
struct i2c_client *client;
struct irq_domain *irq;
+ unsigned int irq_mask_enable;
unsigned int irq_mask;
spinlock_t lock;
};
@@ -280,9 +294,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
spin_lock_irqsave(&data->lock, flags);
- if (!data->irq_mask)
+ if (!data->irq_mask_enable && !data->irq_mask)
enable_irq(data->client->irq);
data->irq_mask |= BIT(pos);
+ if (data->irq_mask_enable &&
+ (data->irq_mask & data->irq_mask) == data->irq_mask_enable)
+ enable_irq(data->client->irq);
spin_unlock_irqrestore(&data->lock, flags);
}
@@ -409,6 +426,9 @@ static int pca954x_probe(struct i2c_client *client,
idle_disconnect_dt = of_node &&
of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+ of_property_read_u32(of_node, "nxp,irq-mask-enable",
+ &data->irq_mask_enable);
+
ret = pca954x_irq_setup(muxc);
if (ret)
goto fail_del_adapters;
--
1.8.3.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 v4 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1484536275-75995-1-git-send-email-preid@electromag.com.au>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Acked-by: Peter Rosin <peda@axentia.se>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa09704..6de1e8e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,6 +19,8 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
+ - nxp,irq-mask-enable: BitMask; Defines a mask for which irq lines need to be
+ unmasked before the parent irq line in enabled.
- interrupt-parent: Phandle for the interrupt controller that services
interrupts for this device.
- interrupts: Interrupt mapping for IRQ.
@@ -36,6 +38,7 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ nxp,irq-mask-enable = <0x3>;
interrupt-parent = <&ipic>;
interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v4 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
From: Phil Reid @ 2017-01-16 3:11 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1484536275-75995-1-git-send-email-preid@electromag.com.au>
The spec for the pca954x was missing. This chip is the same as the pca9540
except that it has interrupt lines. While the i2c_device_id table mapped
the pca9542 to the pca9540 definition the compatible table did not. In
preparation for irq support add the pca9542 definition.
Acked-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index dd18b9c..bbf088e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -84,6 +84,11 @@ struct pca954x {
.enable = 0x4,
.muxtype = pca954x_ismux,
},
+ [pca_9542] = {
+ .nchans = 2,
+ .enable = 0x4,
+ .muxtype = pca954x_ismux,
+ },
[pca_9543] = {
.nchans = 2,
.muxtype = pca954x_isswi,
@@ -110,7 +115,7 @@ struct pca954x {
static const struct i2c_device_id pca954x_id[] = {
{ "pca9540", pca_9540 },
- { "pca9542", pca_9540 },
+ { "pca9542", pca_9542 },
{ "pca9543", pca_9543 },
{ "pca9544", pca_9544 },
{ "pca9545", pca_9545 },
@@ -124,7 +129,7 @@ struct pca954x {
#ifdef CONFIG_ACPI
static const struct acpi_device_id pca954x_acpi_ids[] = {
{ .id = "PCA9540", .driver_data = pca_9540 },
- { .id = "PCA9542", .driver_data = pca_9540 },
+ { .id = "PCA9542", .driver_data = pca_9542 },
{ .id = "PCA9543", .driver_data = pca_9543 },
{ .id = "PCA9544", .driver_data = pca_9544 },
{ .id = "PCA9545", .driver_data = pca_9545 },
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] i2c: core: helper function to detect slave mode
From: Luis Oliveira @ 2017-01-16 10:32 UTC (permalink / raw)
To: Andy Shevchenko, Vladimir Zapolskiy, Andy Shevchenko
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Mika Westerberg, linux-i2c, devicetree,
linux-kernel@vger.kernel.org, Ramiro.Oliveira, Joao Pinto,
CARLOS.PALMINHA
In-Reply-To: <1484240482.2133.92.camel@linux.intel.com>
On 12-Jan-17 17:01, Andy Shevchenko wrote:
> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
>>> wrote:
>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>
>>>>> + }
>>>>>>> + } else if (IS_BUILTIN(CONFIG_ACPI) &&
>>>>>>> ACPI_HANDLE(dev)) {
>>>>>>> + dev_dbg(dev, "ACPI slave is not supported
>>>>>>> yet\n");
>>>>>>> + }
>>>>>>
>>>>>> If so, then it might be better to drop else-if stub for now.
>>>>>
>>>>> Please, don't.
>>>>>
>>>>
>>>> Why do you ask for this stub to be added?
>>>
>>> 1. Exactly the reason you asked above. Here is the code which has
>>> built differently on different platforms. x86 usually is not using
>>> CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
>>> existing examples.
>>
>> From the context by the stub I mean dev_dbg() in
>> i2c_slave_mode_detect()
>> function, I don't see a connection to GPIO library, please clarify.
>
> I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.
I can prepare a V3 and remove it if that's the decision.
>
>>> 2. We might add that support later, but here is again, just no-op.
>>>
>>> So, what is your strong argument here against that?
>>
>> When the support is ready for ACPI case, you'll remove the added
>> dev_dbg(), and I don't see a good point by adding it temporarily.
>
> It would remind me to look at it at some point.
>
>> What is wrong with the approach of adding the ACPI case handling
>> branch when it is ready and remove any kind of stubs right now?
>
> I will not object. Here is maintainer, let him speak.
>
>> On ACPI platforms the function returns 'false' always, will the
>> function work correctly (= corresponding to its description) as is?
>
> Yes.
>
^ permalink raw reply
* Re: [PATCH v4 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en
From: Peter Rosin @ 2017-01-16 11:40 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484536275-75995-5-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-16 04:11, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
>
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
>
> Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hmm, I see that this is already acked by some people :-) ...
... but. I just thought a bit more about it and going forward it might
make sense to make this not a bitmask that only support 0 or 1 irq clients
for each mux segment, and instead make it one u32 for each segment. Then
the binding would extend to also cover cases where several i2c clients
register for interrupts and the shared interrupt needs to be masked until
the last client is registered.
Or is that idea over-engineered?
I'd be satisfied if the implementation simply mapped the u32 array to
a bitmask, as long as it failed noisily when finding anything bigger
than 1 in the array.
Cheers,
peda
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa09704..6de1e8e 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -19,6 +19,8 @@ Optional Properties:
> - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
> children in idle state. This is necessary for example, if there are several
> multiplexers on the bus and the devices behind them use same I2C addresses.
> + - nxp,irq-mask-enable: BitMask; Defines a mask for which irq lines need to be
> + unmasked before the parent irq line in enabled.
> - interrupt-parent: Phandle for the interrupt controller that services
> interrupts for this device.
> - interrupts: Interrupt mapping for IRQ.
> @@ -36,6 +38,7 @@ Example:
> #size-cells = <0>;
> reg = <0x74>;
>
> + nxp,irq-mask-enable = <0x3>;
> interrupt-parent = <&ipic>;
> interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
>
--
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
* Re: [PATCH v4 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Peter Rosin @ 2017-01-16 12:08 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1484536275-75995-6-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-16 04:11, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
>
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
>
> Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index f55da88..66f7ed8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -76,6 +76,19 @@ struct chip_desc {
> } muxtype;
> };
>
> +/*
> + * irq_mask_enable: Provides a mechanism to work around hardware that asserts
> + * their irq immediately on power on. It allows the enabling of the irq to be
> + * delayed until the corresponding bits in the the irq_mask are set thru
> + * irq_unmask.
> + * For example the ltc1760 assert its SMBALERT irq immediately on power on.
> + * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> + * device is registered irq are enabled and fire continuously as the second
> + * device driver has not yet loaded. Setting this parameter to 0x3 while
> + * delay the irq being enabled until both devices are ready.
> + * This workaround will not work if two devices share an interrupt on the
> + * same bus segment.
> + */
> struct pca954x {
> const struct chip_desc *chip;
>
> @@ -84,6 +97,7 @@ struct pca954x {
> struct i2c_client *client;
>
> struct irq_domain *irq;
> + unsigned int irq_mask_enable;
> unsigned int irq_mask;
> spinlock_t lock;
> };
> @@ -280,9 +294,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
>
> spin_lock_irqsave(&data->lock, flags);
>
> - if (!data->irq_mask)
> + if (!data->irq_mask_enable && !data->irq_mask)
> enable_irq(data->client->irq);
> data->irq_mask |= BIT(pos);
> + if (data->irq_mask_enable &&
> + (data->irq_mask & data->irq_mask) == data->irq_mask_enable)
Hmm, I see that some apparently incompetent person :-) already acked this,
but the (data->irq_mask & data->irq_mask) part doesn't make sense at all.
> + enable_irq(data->client->irq);
>
Hmm2, if you have a problematic device (like the ltc1760) on mux segment 0
and sane devices on other segments I'd be inclined to specify irq-mask-enable
as 0x1. But then this is possible:
1. ltc1760 registers its irq
2. enable_irq(data->client->irq) is called because irq_mask_enable is "fulfilled"
3. a sane irq register an irq on some other segment
4. enable_irq(...) is called again (which the code appears to try to avoid)
As I read the code, there will be problems with specifying irq-mask-enable
whenever there are more than one irq-client on a mux segment.
So, I'm removing my ack until the above is resolved...
Cheers,
peda
> spin_unlock_irqrestore(&data->lock, flags);
> }
> @@ -409,6 +426,9 @@ static int pca954x_probe(struct i2c_client *client,
> idle_disconnect_dt = of_node &&
> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>
> + of_property_read_u32(of_node, "nxp,irq-mask-enable",
> + &data->irq_mask_enable);
> +
> ret = pca954x_irq_setup(muxc);
> if (ret)
> goto fail_del_adapters;
>
--
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
* [PATCH 2/2] i2c: pca9541: Export OF device ID table as module aliases
From: Javier Martinez Canillas @ 2017-01-16 13:54 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, linux-i2c, Guenter Roeck, Peter Rosin,
Wolfram Sang
In-Reply-To: <1484574895-952-1-git-send-email-javier@osg.samsung.com>
The I2C core always reports a MODALIAS of the form i2c:<foo> even if the
device was registered via OF, this means that exporting the OF device ID
table device aliases in the module is not needed. But in order to change
how the core reports modaliases to user-space, it's better to export it.
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
drivers/i2c/muxes/i2c-mux-pca9541.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 4ea7e691afc7..77840f7845a1 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -90,6 +90,7 @@ static const struct of_device_id pca9541_of_match[] = {
{ .compatible = "nxp,pca9541" },
{}
};
+MODULE_DEVICE_TABLE(of, pca9541_of_match);
#endif
/*
--
2.7.4
^ permalink raw reply related
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