Hi,
On 5/17/21 9:46 AM, Andy Shevchenko wrote:
> On Sat, May 15, 2021 at 6:17 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>>
>> The ST accelerators support a special type of quirky
>> mounting matrix found in ACPI systems, but not a generic
>> mounting matrix such as from the device tree.
>>
>> Augment the ACPI hack to be a bit more generic and
>> accept a mounting matrix from device properties.
>>
>> This makes it possible to fix orientation on the Ux500
>> HREF device.
>
> I think this entire thread may be interesting to Hans, hence Cc him.
Thanks I'm fine with the suggested change, but I've some
review marks below (inline).
>
>
>> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> Cc: Denis Ciocca <denis.ciocca@xxxxxx>
>> Cc: Daniel Drake <drake@xxxxxxxxxxxx>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> ---
>> drivers/iio/accel/st_accel_core.c | 51 ++++++++++++++++++++-----------
>> 1 file changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
>> index 43c50167d220..cfbcf740e3cb 100644
>> --- a/drivers/iio/accel/st_accel_core.c
>> +++ b/drivers/iio/accel/st_accel_core.c
>> @@ -1069,26 +1069,25 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
>> #define ST_ACCEL_TRIGGER_OPS NULL
>> #endif
>>
>> -#ifdef CONFIG_ACPI
>> static const struct iio_mount_matrix *
>> -get_mount_matrix(const struct iio_dev *indio_dev,
>> - const struct iio_chan_spec *chan)
>> +st_accel_get_mount_matrix(const struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan)
>> {
>> struct st_sensor_data *adata = iio_priv(indio_dev);
>>
>> return adata->mount_matrix;
>> }
>>
>> -static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
>> - IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
>> +static const struct iio_chan_spec_ext_info st_accel_mount_matrix_ext_info[] = {
>> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, st_accel_get_mount_matrix),
>> { },
>> };
>>
>> +#ifdef CONFIG_ACPI
>> /* Read ST-specific _ONT orientation data from ACPI and generate an
>> * appropriate mount matrix.
>> */
>> -static int apply_acpi_orientation(struct iio_dev *indio_dev,
>> - struct iio_chan_spec *channels)
>> +static int apply_acpi_orientation(struct iio_dev *indio_dev)
>> {
>> struct st_sensor_data *adata = iio_priv(indio_dev);
>> struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
>> @@ -1207,22 +1206,20 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev,
>> }
>> }
>>
>> - /* Expose the mount matrix via ext_info */
>> - for (i = 0; i < indio_dev->num_channels; i++)
>> - channels[i].ext_info = mount_matrix_ext_info;
>> -
>> ret = 0;
>> dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n");
>>
>> out:
>> kfree(buffer.pointer);
>> + dev_warn(&indio_dev->dev,
>> + "failed to apply ACPI orientation data: %d\n", ret);
>> +
This path is also taken on a successful return, so the dev_warn needs
to be guarded by a "if (ret)" condition.
>> return ret;
>> }
>> #else /* !CONFIG_ACPI */
>> -static int apply_acpi_orientation(struct iio_dev *indio_dev,
>> - struct iio_chan_spec *channels)
>> +static int apply_acpi_orientation(struct iio_dev *indio_dev)
>> {
>> - return 0;
>> + return -EINVAL;
>> }
>> #endif
>>
>> @@ -1251,6 +1248,7 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
>> struct iio_chan_spec *channels;
>> size_t channels_size;
>> int err;
>> + int i;
>>
>> indio_dev->modes = INDIO_DIRECT_MODE;
>> indio_dev->info = &accel_info;
>> @@ -1275,9 +1273,28 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
>> goto st_accel_power_off;
>> }
>>
>> - if (apply_acpi_orientation(indio_dev, channels))
>> - dev_warn(&indio_dev->dev,
>> - "failed to apply ACPI orientation data: %d\n", err);
>> + /* First try ACPI orientation then try the generic function */
>> + err = apply_acpi_orientation(indio_dev);
>> + if (err) {
>> + adata->mount_matrix = devm_kmalloc(&indio_dev->dev,
>> + sizeof(*adata->mount_matrix),
>> + GFP_KERNEL);
>> + if (!adata->mount_matrix) {
>> + err = -ENOMEM;
>> + goto st_accel_power_off;
>> + }
So now we to this devm_kmalloc both in apply_acpi_orientation() if it succeeds
as well as here if apply_acpi_orientation() fails. It would be better to just
stop kmallocing it all together in both places and just embed the struct in adata,
iow make adata->mount_matrix be the actual iio_mount_matrix struct, rather then
a pointer to it.
This will allow removing the 2 devm_kmalloc calls + their error handling.
Regards,
Hans
>> +
>> + err = iio_read_mount_matrix(adata->dev, "mount-matrix",
>> + adata->mount_matrix);
>> + if (err)
>> + goto st_accel_power_off;
>> + }
>> + /*
>> + * We have at least an identity matrix, so expose the mount
>> + * matrix via ext_info
>> + */
>> + for (i = 0; i < indio_dev->num_channels; i++)
>> + channels[i].ext_info = st_accel_mount_matrix_ext_info;
>>
>> indio_dev->channels = channels;
>> adata->current_fullscale = &adata->sensor_settings->fs.fs_avl[0];
>> --
>> 2.31.1
>>
>
>
