| Message ID | 1430146908-27919-4-git-send-email-octavian.purdila@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Since Acpi framework already exports this info to user space, Why not do this derivation in user space code ? Why do we need new ABI, if the same can be derived from existing one. > This patch derives the mounting matrix for a particular IIO device > based ont the ACPI _PLD information. Note that if mounting matrix is > defined in the device properties it overrieds the _PLD information. > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com> > --- > drivers/iio/industrialio-core.c | 51 > +++++++++++++++++++++++++++++++++++++++++ > include/linux/iio/iio.h | 46 > +++++++++++++++++++++++++++++++++++++ > 2 files changed, 97 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c > b/drivers/iio/industrialio-core.c > index 9000c53..90ee58a 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -31,6 +31,7 @@ > #include <linux/iio/sysfs.h> > #include <linux/iio/events.h> > #include <linux/iio/buffer.h> > +#include <linux/acpi.h> > > /* IDA to assign each registered device a unique id */ > static DEFINE_IDA(iio_ida); > @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev, > > static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL); > > +#if IS_ENABLED(CONFIG_ACPI) > +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) > +{ > + acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent); > + struct acpi_pld_info *info; > + > + if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) { > + IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0); > + IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0); > + IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0); > + > + /* Chip placed on the back panel ; negate x and z */ > + if (info->panel == ACPI_PLD_PANEL_BACK) { > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0); > + } > + > + switch (info->rotation) { > + case 2: > + /* 90 deg clockwise: negate y then swap x,y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); > + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); > + break; > + case 4: > + /* Upside down: negate x and y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); > + break; > + case 6: > + /* 90 deg counter clockwise: negate x then swap x,y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); > + break; > + } > + > + return true; > + } > + > + return false; > +} > +#else > +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) > +{ > + return false; > +} > +#endif > + > static bool iio_get_mounting_matrix(struct iio_dev *indio_dev) > { > int i, err; > @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev > *indio_dev) > return true; > } > > + if (ACPI_HANDLE(indio_dev->dev.parent)) > + return iio_get_mounting_matrix_acpi(indio_dev); > + > return false; > } > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index c1fa852..feb7813 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops { > > #define IIO_MM_SIZE 18 > > +enum { > + IIO_MM_XX0, > + IIO_MM_XX1, > + IIO_MM_XY0, > + IIO_MM_XY1, > + IIO_MM_XZ0, > + IIO_MM_XZ1, > + IIO_MM_YX0, > + IIO_MM_YX1, > + IIO_MM_YY0, > + IIO_MM_YY1, > + IIO_MM_YZ0, > + IIO_MM_YZ1, > + IIO_MM_ZX0, > + IIO_MM_ZX1, > + IIO_MM_ZY0, > + IIO_MM_ZY1, > + IIO_MM_ZZ0, > + IIO_MM_ZZ1, > +}; > + > +#define IIO_MM_SET(mm, a, b, val0, val1) \ > + do { \ > + mm[IIO_MM_##a##b##0] = val0; \ > + mm[IIO_MM_##a##b##1] = val1; \ > + } while (0) \ > + > +#define IIO_MM_MUL(mm, a, b, val0, val1) \ > + do { \ > + mm[IIO_MM_##a##b##0] *= val0; \ > + mm[IIO_MM_##a##b##0] *= val1; \ > + } while (0) \ > + > +#define IIO_MM_SWAP(mm, x, y) \ > + do { \ > + int tmp; \ > + \ > + tmp = mm[IIO_MM_##x##x##0]; \ > + mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0]; \ > + mm[IIO_MM_##y##y##0] = tmp; \ > + \ > + tmp = mm[IIO_MM_##x##x##1]; \ > + mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1]; \ > + mm[IIO_MM_##y##y##1] = tmp; \ > + } while (0) \ > + > /** > * struct iio_dev - industrial I/O device > * @id: [INTERN] used to identify device internally > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > Since Acpi framework already exports this info to user space, Why not do > this derivation in user space code ? Why do we need new ABI, if the same > can be derived from existing one. > The ABI was added in the previous patch so that we can present the sensor orientation information to userspace even in the case of device tree. The purpose of this patch is to provide a consistent ABI to userspace, i.e. to avoid doing one thing in the ACPI case and another thing in the case of device tree. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 04/27/2015 08:54 AM, Octavian Purdila wrote: > On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> Since Acpi framework already exports this info to user space, Why not do >> this derivation in user space code ? Why do we need new ABI, if the same >> can be derived from existing one. >> > The ABI was added in the previous patch so that we can present the > sensor orientation information to userspace even in the case of device > tree. If the main reason for implementing a new ABI is to support DT platforms, Why not implement a version of _PLD for device tree ? Don't you think it would be much better than adding a new ABI to export redundant information ? Also the location information of the device is not just specific to iio drivers. You should consider that we would have similar requirements for devices implemented as input or platform drivers. > > The purpose of this patch is to provide a consistent ABI to userspace, > i.e. to avoid doing one thing in the ACPI case and another thing in > the case of device tree. > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > Hi > > On 04/27/2015 08:54 AM, Octavian Purdila wrote: >> >> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan >> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >>> >>> Since Acpi framework already exports this info to user space, Why not do >>> this derivation in user space code ? Why do we need new ABI, if the same >>> can be derived from existing one. >>> >> The ABI was added in the previous patch so that we can present the >> sensor orientation information to userspace even in the case of device >> tree. > > If the main reason for implementing a new ABI is to support DT platforms, > Why not implement a version of _PLD for device tree ? Don't you think it > would be much better than adding a new ABI to export redundant information ? > IMO the mounting matrix is more consistent with the IIO ABIs. Although I have no issue with repicating _PLD for device tree if people agree that it is better. > Also the location information of the device is not just specific to iio > drivers. You should consider that we would have similar requirements for > devices implemented as input or platform drivers. The upstream standard for those sensors where the orientation matters (accelerometer, gyro, compass) is IIO. Granted, there are other device types for which the orientation information may be useful (e.g. camera). However the actual interpretation and action to be taken is different for each subsystem (e.g. in the camera case do the correction via V4L2_CID_HFLIP / V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem level in a way consistent with the subsystem's ABIs. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Octavian, On 04/27/2015 07:23 PM, Octavian Purdila wrote: > On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> Hi >> >> On 04/27/2015 08:54 AM, Octavian Purdila wrote: >>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan >>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >>>> Since Acpi framework already exports this info to user space, Why not do >>>> this derivation in user space code ? Why do we need new ABI, if the same >>>> can be derived from existing one. >>>> >>> The ABI was added in the previous patch so that we can present the >>> sensor orientation information to userspace even in the case of device >>> tree. >> If the main reason for implementing a new ABI is to support DT platforms, >> Why not implement a version of _PLD for device tree ? Don't you think it >> would be much better than adding a new ABI to export redundant information ? >> > IMO the mounting matrix is more consistent with the IIO ABIs. Although > I have no issue with repicating _PLD for device tree if people agree > that it is better. Since your main issue is, device tree lacking ABI to specify location information, you should consider fixing it there. Let's wait for others comment on this. If you think mounting matrix provides more information than what is supported by _PLD, then we should consider implementing another ABI. AFAIK, that is not the case here. Adding adding a new ABI to represent the information that can be derived from existing ABI does not seem to be useful. > >> Also the location information of the device is not just specific to iio >> drivers. You should consider that we would have similar requirements for >> devices implemented as input or platform drivers. > The upstream standard for those sensors where the orientation matters > (accelerometer, gyro, compass) is IIO. > > Granted, there are other device types for which the orientation > information may be useful (e.g. camera). However the actual > interpretation and action to be taken is different for each subsystem > (e.g. in the camera case do the correction via V4L2_CID_HFLIP / > V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem > level in a way consistent with the subsystem's ABIs. I agree that location information is used differently at different sub systems. But my question is why we need a new ABI ? Why not handle it in user space ?
On Mon, May 4, 2015 at 4:11 AM, Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > Hi Octavian, > > On 04/27/2015 07:23 PM, Octavian Purdila wrote: >> >> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy >> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >>> >>> Hi >>> >>> On 04/27/2015 08:54 AM, Octavian Purdila wrote: >>>> >>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan >>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >>>>> >>>>> Since Acpi framework already exports this info to user space, Why not >>>>> do >>>>> this derivation in user space code ? Why do we need new ABI, if the >>>>> same >>>>> can be derived from existing one. >>>>> >>>> The ABI was added in the previous patch so that we can present the >>>> sensor orientation information to userspace even in the case of device >>>> tree. >>> >>> If the main reason for implementing a new ABI is to support DT platforms, >>> Why not implement a version of _PLD for device tree ? Don't you think it >>> would be much better than adding a new ABI to export redundant >>> information ? >>> >> IMO the mounting matrix is more consistent with the IIO ABIs. Although >> I have no issue with repicating _PLD for device tree if people agree >> that it is better. > > Since your main issue is, device tree lacking ABI to specify location > information, you should consider fixing it there. Let's wait for others > comment on this. > > If you think mounting matrix provides more information than what is > supported > by _PLD, then we should consider implementing another ABI. AFAIK, that is > not > the case here. > > Adding adding a new ABI to represent the information that can be derived > from existing ABI does not seem to be useful. AFAICS the ACPI _PLD information is not (yet) exported to userspace. This patch: http://marc.info/?t=140795040700003&r=1&w=2 does not seem to be merged upstream. So there is no existing ABI to derive from :) >> >> >>> Also the location information of the device is not just specific to iio >>> drivers. You should consider that we would have similar requirements for >>> devices implemented as input or platform drivers. >> >> The upstream standard for those sensors where the orientation matters >> (accelerometer, gyro, compass) is IIO. >> >> Granted, there are other device types for which the orientation >> information may be useful (e.g. camera). However the actual >> interpretation and action to be taken is different for each subsystem >> (e.g. in the camera case do the correction via V4L2_CID_HFLIP / >> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem >> level in a way consistent with the subsystem's ABIs. > > I agree that location information is used differently at different > sub systems. But my question is why we need a new ABI ? > > Why not handle it in user space ? > > - -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/04/15 03:23, Octavian Purdila wrote: > On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> Hi >> >> On 04/27/2015 08:54 AM, Octavian Purdila wrote: >>> >>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan >>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >>>> >>>> Since Acpi framework already exports this info to user space, Why not do >>>> this derivation in user space code ? Why do we need new ABI, if the same >>>> can be derived from existing one. >>>> >>> The ABI was added in the previous patch so that we can present the >>> sensor orientation information to userspace even in the case of device >>> tree. >> >> If the main reason for implementing a new ABI is to support DT platforms, >> Why not implement a version of _PLD for device tree ? Don't you think it >> would be much better than adding a new ABI to export redundant information ? >> > > IMO the mounting matrix is more consistent with the IIO ABIs. Although > I have no issue with repicating _PLD for device tree if people agree > that it is better. > >> Also the location information of the device is not just specific to iio >> drivers. You should consider that we would have similar requirements for >> devices implemented as input or platform drivers. > > The upstream standard for those sensors where the orientation matters > (accelerometer, gyro, compass) is IIO. It's probably worth pull Dmitry in on this conversation as well (and maybe the input list). cc'd. For reference of those who haven't seen this before. The question here is about exposing the sensor mounting matrix ( orientation of the sensor frame of reference relative to some 'magic' orientation - probably the PCB ) Note I'd also throw in here that I'd argue for a combined R T matrix e.g. [R T] so as to cover cases where we care about the position as well as the orientation relative to some reference point. A class case in point would be rotation measurement from offset accelerometers. Could go with a full projective geometry matrix, but probably better to keep it in some base scale e.g. [R T] [000 1] but not bother exporting the 000 1 as it'll always be the same. Note I've written this email whilst without net access (free wifi at Stansted airport is rubbish!) so haven't checked out the existing ACPI ABI. Will try and remember to do this when I get a moment with working internet. Jonathan > > Granted, there are other device types for which the orientation > information may be useful (e.g. camera). However the actual > interpretation and action to be taken is different for each subsystem > (e.g. in the camera case do the correction via V4L2_CID_HFLIP / > V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem > level in a way consistent with the subsystem's ABIs. > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/04/15 16:01, Octavian Purdila wrote: > This patch derives the mounting matrix for a particular IIO device > based ont the ACPI _PLD information. Note that if mounting matrix is > defined in the device properties it overrieds the _PLD information. > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com> I have no objection to the functionality, but I'd prefer to separate it more from the core of IIO. By all means have some utility functions to help with what may be a common feature in future, but I don't want to have the data stored in the main IIO structures. It's not relevant to lots of our devices afterall! Reading this (still offline unfortunately), looks like the ACPI provided data is very very minimalist. Parts have to be on the panel and oriented at 90 degree increments? I suppose it covers a fair range of common hardware devices, but far from all of them. I'm not considerably more keen on the IIO interface as a substantial superset of what ACPI is providing. Note that we should definitely discuss with input (joysticks can have mounting matrices too!) cc'd. Sorry Dmitry, think I managed to pick up an ancient version of your email address for an earlier cc. Was being lazy and didn't check in MAINTAINERS. Jonathan > --- > drivers/iio/industrialio-core.c | 51 +++++++++++++++++++++++++++++++++++++++++ > include/linux/iio/iio.h | 46 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 97 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 9000c53..90ee58a 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -31,6 +31,7 @@ > #include <linux/iio/sysfs.h> > #include <linux/iio/events.h> > #include <linux/iio/buffer.h> > +#include <linux/acpi.h> > > /* IDA to assign each registered device a unique id */ > static DEFINE_IDA(iio_ida); > @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev, > > static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL); > > +#if IS_ENABLED(CONFIG_ACPI) > +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) > +{ > + acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent); > + struct acpi_pld_info *info; > + > + if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) { > + IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0); > + IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0); > + IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0); > + > + /* Chip placed on the back panel ; negate x and z */ > + if (info->panel == ACPI_PLD_PANEL_BACK) { > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0); > + } > + > + switch (info->rotation) { > + case 2: > + /* 90 deg clockwise: negate y then swap x,y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); > + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); > + break; > + case 4: > + /* Upside down: negate x and y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); > + break; > + case 6: > + /* 90 deg counter clockwise: negate x then swap x,y */ > + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); > + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); > + break; > + } > + > + return true; > + } > + > + return false; > +} > +#else > +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) > +{ > + return false; > +} > +#endif > + > static bool iio_get_mounting_matrix(struct iio_dev *indio_dev) > { > int i, err; > @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev *indio_dev) > return true; > } > > + if (ACPI_HANDLE(indio_dev->dev.parent)) > + return iio_get_mounting_matrix_acpi(indio_dev); > + > return false; > } > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index c1fa852..feb7813 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops { > > #define IIO_MM_SIZE 18 > > +enum { > + IIO_MM_XX0, > + IIO_MM_XX1, > + IIO_MM_XY0, > + IIO_MM_XY1, > + IIO_MM_XZ0, > + IIO_MM_XZ1, > + IIO_MM_YX0, > + IIO_MM_YX1, > + IIO_MM_YY0, > + IIO_MM_YY1, > + IIO_MM_YZ0, > + IIO_MM_YZ1, > + IIO_MM_ZX0, > + IIO_MM_ZX1, > + IIO_MM_ZY0, > + IIO_MM_ZY1, > + IIO_MM_ZZ0, > + IIO_MM_ZZ1, > +}; > + > +#define IIO_MM_SET(mm, a, b, val0, val1) \ > + do { \ > + mm[IIO_MM_##a##b##0] = val0; \ > + mm[IIO_MM_##a##b##1] = val1; \ > + } while (0) \ > + > +#define IIO_MM_MUL(mm, a, b, val0, val1) \ > + do { \ > + mm[IIO_MM_##a##b##0] *= val0; \ > + mm[IIO_MM_##a##b##0] *= val1; \ > + } while (0) \ > + > +#define IIO_MM_SWAP(mm, x, y) \ > + do { \ > + int tmp; \ > + \ > + tmp = mm[IIO_MM_##x##x##0]; \ > + mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0]; \ > + mm[IIO_MM_##y##y##0] = tmp; \ > + \ > + tmp = mm[IIO_MM_##x##x##1]; \ > + mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1]; \ > + mm[IIO_MM_##y##y##1] = tmp; \ > + } while (0) \ > + > /** > * struct iio_dev - industrial I/O device > * @id: [INTERN] used to identify device internally > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-05-04 at 11:21 +0300, Octavian Purdila wrote: > On Mon, May 4, 2015 at 4:11 AM, Sathyanarayanan Kuppuswamy > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > Hi Octavian, > > > > On 04/27/2015 07:23 PM, Octavian Purdila wrote: > >> > >> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy > >> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >>> > >>> Hi > >>> > >>> On 04/27/2015 08:54 AM, Octavian Purdila wrote: > >>>> > >>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan > >>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >>>>> > >>>>> Since Acpi framework already exports this info to user space, Why not > >>>>> do > >>>>> this derivation in user space code ? Why do we need new ABI, if the > >>>>> same > >>>>> can be derived from existing one. > >>>>> > >>>> The ABI was added in the previous patch so that we can present the > >>>> sensor orientation information to userspace even in the case of device > >>>> tree. > >>> > >>> If the main reason for implementing a new ABI is to support DT platforms, > >>> Why not implement a version of _PLD for device tree ? Don't you think it > >>> would be much better than adding a new ABI to export redundant > >>> information ? > >>> > >> IMO the mounting matrix is more consistent with the IIO ABIs. Although > >> I have no issue with repicating _PLD for device tree if people agree > >> that it is better. > > > > Since your main issue is, device tree lacking ABI to specify location > > information, you should consider fixing it there. Let's wait for others > > comment on this. > > > > If you think mounting matrix provides more information than what is > > supported > > by _PLD, then we should consider implementing another ABI. AFAIK, that is > > not > > the case here. > > > > Adding adding a new ABI to represent the information that can be derived > > from existing ABI does not seem to be useful. > > AFAICS the ACPI _PLD information is not (yet) exported to userspace. This patch: > > http://marc.info/?t=140795040700003&r=1&w=2 > > does not seem to be merged upstream. So there is no existing ABI to > derive from :) I don't think there is any major objection to this patch. The author should just try and see if he can replace to device_* calls. Thanks, Srinivas > > >> > >> > >>> Also the location information of the device is not just specific to iio > >>> drivers. You should consider that we would have similar requirements for > >>> devices implemented as input or platform drivers. > >> > >> The upstream standard for those sensors where the orientation matters > >> (accelerometer, gyro, compass) is IIO. > >> > >> Granted, there are other device types for which the orientation > >> information may be useful (e.g. camera). However the actual > >> interpretation and action to be taken is different for each subsystem > >> (e.g. in the camera case do the correction via V4L2_CID_HFLIP / > >> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem > >> level in a way consistent with the subsystem's ABIs. > > > > I agree that location information is used differently at different > > sub systems. But my question is why we need a new ABI ? > > > > Why not handle it in user space ? > > > > - -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 9000c53..90ee58a 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -31,6 +31,7 @@ #include <linux/iio/sysfs.h> #include <linux/iio/events.h> #include <linux/iio/buffer.h> +#include <linux/acpi.h> /* IDA to assign each registered device a unique id */ static DEFINE_IDA(iio_ida); @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev, static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL); +#if IS_ENABLED(CONFIG_ACPI) +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) +{ + acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent); + struct acpi_pld_info *info; + + if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) { + IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0); + IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0); + IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0); + + /* Chip placed on the back panel ; negate x and z */ + if (info->panel == ACPI_PLD_PANEL_BACK) { + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); + IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0); + } + + switch (info->rotation) { + case 2: + /* 90 deg clockwise: negate y then swap x,y */ + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); + break; + case 4: + /* Upside down: negate x and y */ + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0); + break; + case 6: + /* 90 deg counter clockwise: negate x then swap x,y */ + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0); + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y); + break; + } + + return true; + } + + return false; +} +#else +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev) +{ + return false; +} +#endif + static bool iio_get_mounting_matrix(struct iio_dev *indio_dev) { int i, err; @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev *indio_dev) return true; } + if (ACPI_HANDLE(indio_dev->dev.parent)) + return iio_get_mounting_matrix_acpi(indio_dev); + return false; } diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index c1fa852..feb7813 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops { #define IIO_MM_SIZE 18 +enum { + IIO_MM_XX0, + IIO_MM_XX1, + IIO_MM_XY0, + IIO_MM_XY1, + IIO_MM_XZ0, + IIO_MM_XZ1, + IIO_MM_YX0, + IIO_MM_YX1, + IIO_MM_YY0, + IIO_MM_YY1, + IIO_MM_YZ0, + IIO_MM_YZ1, + IIO_MM_ZX0, + IIO_MM_ZX1, + IIO_MM_ZY0, + IIO_MM_ZY1, + IIO_MM_ZZ0, + IIO_MM_ZZ1, +}; + +#define IIO_MM_SET(mm, a, b, val0, val1) \ + do { \ + mm[IIO_MM_##a##b##0] = val0; \ + mm[IIO_MM_##a##b##1] = val1; \ + } while (0) \ + +#define IIO_MM_MUL(mm, a, b, val0, val1) \ + do { \ + mm[IIO_MM_##a##b##0] *= val0; \ + mm[IIO_MM_##a##b##0] *= val1; \ + } while (0) \ + +#define IIO_MM_SWAP(mm, x, y) \ + do { \ + int tmp; \ + \ + tmp = mm[IIO_MM_##x##x##0]; \ + mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0]; \ + mm[IIO_MM_##y##y##0] = tmp; \ + \ + tmp = mm[IIO_MM_##x##x##1]; \ + mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1]; \ + mm[IIO_MM_##y##y##1] = tmp; \ + } while (0) \ + /** * struct iio_dev - industrial I/O device * @id: [INTERN] used to identify device internally
This patch derives the mounting matrix for a particular IIO device based ont the ACPI _PLD information. Note that if mounting matrix is defined in the device properties it overrieds the _PLD information. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com> --- drivers/iio/industrialio-core.c | 51 +++++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)