Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <93323bab-c000-4a2b-ecad-9f3f6dc72c69@opensynergy.com>
Date:   Fri, 8 Oct 2021 14:32:27 +0300
From:   Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
Subject: Re: [PATCH v3 0/1] iio/scmi: Add reading "raw" attribute.
To:     Jyoti Bhayana <jbhayana@google.com>,
        Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>
References: <20210928161957.24628-1-andriy.tryshnivskyy@opensynergy.com>
 <20210930174043.0845f6ed@jic23-huawei>
 <CA+=V6c34+VtYbUDG=NuY6YeyML23Tth2+fTjhvXpwMvrtkM5_Q@mail.gmail.com>
 <650ec3b8-c970-604c-9be6-577c46d3e762@opensynergy.com>
 <CA+=V6c10xE-1uUzGWusBiGVMV2Fwy93dGGHA9FH0PC4tYggVUQ@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CA+=V6c10xE-1uUzGWusBiGVMV2Fwy93dGGHA9FH0PC4tYggVUQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WXlZVTV4KzNPV3Eveno2aitwdXBnRTUveHpMWVV2S1lHSnFPUGFhUkszclEz?=
 =?utf-8?B?WVN6cnlhcU9GUHE5Z1JjdVNmcUJzRVRlSkJsWk9oMkkwOGVPUlA4L0VsNWxp?=
 =?utf-8?B?WHE1YVdNRlpRZytDaFUvc2RIWXZSaVNCbExDT2NrMDBUL3hUWW1PM05FSU1w?=
 =?utf-8?B?R0ltNlBxampEYXVHYUl1c25OY3o1M0dBSElDVndDYk9yYW50cGFhNkk2dGdy?=
 =?utf-8?B?UHFxZ1cwajJJOWhycEFIU3gxOW5zbW5yVTlqVzdUMWVPZmlJWVE3Qi9VZGxm?=
 =?utf-8?B?VTNFTmhIRnVmNTJqaVRGanMvaEFtdFo1VTZEZ0VCRkk2aFJ0U2ZaVnVleDlY?=
 =?utf-8?B?cDF1aUgyeDh2MWo4M0pVd3V1QnkzaHQzQUtUc0dYZDIzZnpTLzhyZVBobmZO?=
 =?utf-8?B?QkFscFkxc2lObHdtNFRSNmdrekVtZ1pRMjJITDBLb2hPb01jUzVXQXo2blg4?=
 =?utf-8?B?Tk1BVk5KYzkremlVVk9xbXNkY3FVMzNabHA5SzVJcm16V1NJQTBVS1ZVRzA1?=
 =?utf-8?B?OXJjTzRjakNUOTFSNlZYUlJKc1BLTGxaQmJkV0p6cUlqZWxSTldRaGd0ZjAw?=
 =?utf-8?B?VmNUcGlRcWd0SW9GUEVOSkJPNHhzNis3WU8xVHdkM2dMblZGTTVsYmpyMDdJ?=
 =?utf-8?B?TmgrR0xHejJkdGRHSndMWmFZRjlBdzltM0cxV3hleExIa0Q0Qkl4WFA3Tm9E?=
 =?utf-8?B?ditjZWRmMTRsS25ya3dCRUpuU2UzWWo1WkllZ0RrTW5sS0QrTWZZVEk1TFFF?=
 =?utf-8?B?Smp6SVZEa1g3Z01SN3hXNXNUNGJsdnhZZVF6VEdNeG5aTklJUlAzdGRNS2dH?=
 =?utf-8?B?RU5zcEF5NFBoeFljOE5sWkQxR3FjbkxveDg2RmJTR3pQUW1JeUNTN1JjUkxV?=
 =?utf-8?B?UVRjZXRMVlIyVVc3b2xyOHd6YytlcjFxZ0FSSTZ5WUplUDlGRXhrWHNnS3VP?=
 =?utf-8?B?bkltL3p5NEtDWHBiWngzUnFLcXFVNksvUXhvR0RyT2ppLzlwV1JOSUNiRHFX?=
 =?utf-8?B?bVhJZnRQYjlPRW5jNVJEVmVFRU91UXVQeisrT2xtMXBSZGJEWVFNdS9rQm9N?=
 =?utf-8?B?OGhpaElUcGRrNHY5bnZwL0JTTW5GdTl0UkhUUHFnYTEycmdReXg5NkRabVdt?=
 =?utf-8?B?ZTE0a2NZOTFXQ2QzaHIyanRHajNSU3BwenY4NEJvVENicExZbGFqS0hOaTRa?=
 =?utf-8?B?cnJCbk5Nc3RGYnRnZXNXUlRYWVY3N1I4VXFCbjZtQkhhdDd0a1dBTm1wRXpo?=
 =?utf-8?B?Z0V6dUNMWi9tOUZXV3lGbVRKWExLNGx0NXU0VU8zR1VQRE50QklUTmtQNmpI?=
 =?utf-8?B?SUhwRmh0eGIyVE4zcDBvcDlrM094Y3JiTnVobWtUZGtvQ29RSXE5SWEvYXo4?=
 =?utf-8?B?U0FsYk41Z0M0ZGhadXJmV3VZVFdZSWJVNnB6TzRWeEFvQ1RJcUVTLzBCdzJo?=
 =?utf-8?B?dklYNElmbTdhNmtGdjlLWFhCd1E2R1FDek5qdDA3QUJoSDZoeUR5ekhFSDFm?=
 =?utf-8?B?NjVlRWZHZDRqUmFZSmUyYTJYL1N2V2hiWWJjV25sRU5RRDJDa1JXOVAwSzl3?=
 =?utf-8?B?Z2g3NWJ6Z2RRMHRSeHVHcVJSL0FGa0JZZ1NFSUFibllNQmtLd0RYU1JrTEov?=
 =?utf-8?B?cWhaQXg1eEZENkswMzRIWWlubkhRbGUrZ2xrNFZrNFB2NFVZdEk5YnhvbkJD?=
 =?utf-8?B?U0FqR09RU2xma2p5amF5ekg2SmM5Wlk5Qm9kRFlYOHBWdlpLVUNubWR4a0tZ?=
 =?utf-8?Q?+mf70soNDWTKkEX6HLeQnQxni+fUsKxEju+9y4W?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1d4c604d-f624-4562-3dda-08d98a4f4f9c
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB6359.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Oct 2021 11:32:29.3054
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 800fae25-9b1b-4edc-993d-c939c4e84a64
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3siluktW7Zx1YhXvHhza56Pth3G2NlDTwBPn4+Kq9ZJyfBVBOW8CBLg3Mqf/nV3dn//TQQQgruwAbLHmN6pYSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6952
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jyoti and Vasyl,

Thanks for your review.
I will provide new patch version soon.

Thanks,
Andriy

On 06.10.21 03:16, Jyoti Bhayana wrote:

> CAUTION: This email originated from outside of the organization.
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi Vasyl,
>
> Regarding below question, yes reading raw attribute should be blocked
> if buffer is enabled for that sensor.
>
>> 1. Should we block reading raw attribute and IIO buffer enabled, for for
>> SCMI sensor it can coexist?
>
> PLease see  https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L667
> as well. It has
>
> case IIO_CHAN_INFO_RAW:
> ret = iio_device_claim_direct_mode(indio_dev);
> if (ret)
> return ret;
> mutex_lock(&st->lock);
> ret = inv_mpu6050_read_channel_data(indio_dev, chan, val);
> mutex_unlock(&st->lock);
> iio_device_release_direct_mode(indio_dev);
> return ret;
>
> Regarding the question below, the answer is yes.
>
>> 2. Should we wrap reading raw attribute implementation in iio_dev->mlock
>> mutex?
>
> Thanks, Jyoti
>
>
>
>
>
> On Tue, Oct 5, 2021 at 5:52 AM Vasyl Vavrychuk
> <vasyl.vavrychuk@opensynergy.com> wrote:
>> Hi, Jyoti,
>>
>>> In the code below, why is the logic of enabling and disabling the
>>> sensor in this function?  Generally the function to read the sensor
>>> value is just used for the code to read the sensor values ? and not
>>> enable/disable the sensor
>> But to read sensor value we have to enable it first. Other way to enable
>> sensor we found is, for example:
>>
>>     echo 1 > /sys/bus/iio/devices/.../scan_elements/in_anglvel_x_en
>>
>> But, this command is related to IIO buffers use.
>>
>> Other sensors drivers enable/disable sensor in read raw too, for
>> example, drivers/iio/accel/kxcjk-1013.c has:
>>
>>     case IIO_CHAN_INFO_RAW:
>>             mutex_lock(&data->mutex);
>>             if (iio_buffer_enabled(indio_dev))
>>                     ret = -EBUSY;
>>             else {
>>                     ret = kxcjk1013_set_power_state(data, true);
>>                     ... reading ...
>>                     ret = kxcjk1013_set_power_state(data, false);
>>             }
>>             mutex_unlock(&data->mutex);
>>
>> But, after looking on this code I have some questions:
>>
>> 1. Should we block reading raw attribute and IIO buffer enabled, for for
>> SCMI sensor it can coexist?
>> 2. Should we wrap reading raw attribute implementation in iio_dev->mlock
>> mutex?
>>
>>>>      case IIO_CHAN_INFO_RAW:
>>>> +               sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>>>> +                                          SCMI_SENS_CFG_SENSOR_ENABLE);
>>>> +               err = sensor->sensor_ops->config_set(
>>>> +                       sensor->ph, sensor->sensor_info->id, sensor_config);
>>>> +               if (err) {
>>>> +                       dev_err(&iio_dev->dev,
>>>> +                               "Error in enabling sensor %s err %d",
>>>> +                               sensor->sensor_info->name, err);
>>>> +                       return err;
>>>> +               }
>>>> +
>>>> +               err = sensor->sensor_ops->reading_get_timestamped(
>>>> +                       sensor->ph, sensor->sensor_info->id,
>>>> +                       sensor->sensor_info->num_axis, readings);
>>>> +               if (err) {
>>>> +                       dev_err(&iio_dev->dev,
>>>> +                               "Error in reading raw attribute for sensor %s err %d",
>>>> +                               sensor->sensor_info->name, err);
>>>> +                       return err;
>>>> +               }
>>>> +
>>>> +               sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>>>> +                                          SCMI_SENS_CFG_SENSOR_DISABLE);
>>>> +               err = sensor->sensor_ops->config_set(
>>>> +                       sensor->ph, sensor->sensor_info->id, sensor_config);
>>>> +               if (err) {
>>>> +                       dev_err(&iio_dev->dev,
>>>> +                               "Error in enabling sensor %s err %d",
>>>> +                               sensor->sensor_info->name, err);
>>>> +                       return err;
>>>> +               }
>>>> +               /* Check if raw value fits 32 bits */
>>>> +               if (readings[ch->scan_index].value < INT_MIN ||
>>>> +                   readings[ch->scan_index].value > INT_MAX)
>>>> +                       return -ERANGE;
>>>> +               /* Use 32-bit value, since practically there is no need in 64 bits */
>>>> +               *val = (int)readings[ch->scan_index].value;
>>>>
>>>> +               return IIO_VAL_INT;