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: <650ec3b8-c970-604c-9be6-577c46d3e762@opensynergy.com>
Date:   Tue, 5 Oct 2021 15:51:57 +0300
Subject: Re: [PATCH v3 0/1] iio/scmi: Add reading "raw" attribute.
Content-Language: uk
To:     Jyoti Bhayana <jbhayana@google.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
        Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
References: <20210928161957.24628-1-andriy.tryshnivskyy@opensynergy.com>
 <20210930174043.0845f6ed@jic23-huawei>
 <CA+=V6c34+VtYbUDG=NuY6YeyML23Tth2+fTjhvXpwMvrtkM5_Q@mail.gmail.com>
From:   Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
In-Reply-To: <CA+=V6c34+VtYbUDG=NuY6YeyML23Tth2+fTjhvXpwMvrtkM5_Q@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?bmV5alpkMmJNV1lvaXNCY002NUN1RzUxeWxORTR1b2xmLzNYdDVQZUx4ZEZI?=
 =?utf-8?B?ejdZdXVtYkx2OEdEL1NIa2lEdVl6ZHJHVWc5UUpVOFBTSVRkMzdVVzFNMUNw?=
 =?utf-8?B?SHpyNE9EMUthc0NjdmdVdXdvbkt6THAvdHdNZkg0RFk4aHFtT0JvYUQ5OWdx?=
 =?utf-8?B?OGxPSHEwT2x3ZkdrTU0rcmYrK3YwZVJmL0hqQ09qQ3dhOXhidDB6eFFNdExC?=
 =?utf-8?B?a1VXbkp1ck9NOXE4R3ovVktBdGlacWRpTFZMOU9rcGNMdC93Wm8zTm5VRG5n?=
 =?utf-8?B?OU5MZS9PaWhtaDRraHJ0SDJtblI5aUtBQXp4cWp6MmpRbjRMVTY3eko1ZnFH?=
 =?utf-8?B?NW1uSUNzdFRaTkFZQmVZZWF6Z2JNekMrckNyMXBBeGc1c3lzK3p0d1k0N3pC?=
 =?utf-8?B?NDIvTVdwNjd6bVZwSE5RZk5DeVRlTGlFUDhZcWd2ODdkN1BmdnhlRTMySExY?=
 =?utf-8?B?TXFHTS9wS3BPK2wraHcraWpZZlJ6alN3UXNjcUFML01WSjBZNlFwM2U5NTRR?=
 =?utf-8?B?WWozWmVUbDlEMW9CakNSZUxkSER3VTNQNEJpVTBidGFwK05YK05vNnd4ZEk5?=
 =?utf-8?B?WWYwVUk3RjRwSEdxVjVFdGVGY3llRWZ6TGJiRXh0YWF5WGY5cmExOVVFNU5m?=
 =?utf-8?B?NHcydnJGZ0wvTjRTWmVtbGF3M3VhS01idVRMOUYrRUF6aFhoVTlXZlV4K0hK?=
 =?utf-8?B?VDdXdUZiU3E3OUhodTh3L09Xbm96bFJTZlViZ3ZXOG1saWszcERWY1JjYVFz?=
 =?utf-8?B?VzNRdVE2S3F3YTJMWktSa1pqRGNTLzR6eGRoWnhiamNWZEsvQUxiZzhBT1dk?=
 =?utf-8?B?dmh1anoyRC9HK1RMckRPL0JZS00vTnN4UzZlYWV2cjZjWmUyc1lhMjNzNjVi?=
 =?utf-8?B?b2tZc1RtVHlQc01XdVdZWDFOd28xWlJ2Ty9HYmFDbGcrVVQ0c2pSRjk1Z1Zo?=
 =?utf-8?B?Mm5qT2dGMmR5WGFXNWVpanFqUGVOenc0TXJYVU53a3NWemg3VlVDOXk3R1or?=
 =?utf-8?B?Q2pCMVNMbDFSUmNBa2c0YlJRaUsyU0NVRXZncHV1NE1BM1g0RjR3d3E2bjVw?=
 =?utf-8?B?SGQ5Q3JqUFRGeG1BbktOVHpEZ1hVNXpseFc4YlU3SDNSZ1ZUSlBMU0YyU3h2?=
 =?utf-8?B?UktGUVE4UFRwdWlYSUdaV3FMeUlKTWhEV1pDenY4eEk1aGd6Z3BGOEptQ0Z2?=
 =?utf-8?B?SklmQVhLUnd2c3UzUTZVeEFKZ1F2ZzVmZU9VN1QreHpiTU9LWW1UN0pVdGRx?=
 =?utf-8?B?SnhzSUNmaTNpNmhYbG15QSswL0Nhei9OQ3VBSFdndnM3WWFjcldJNURmc2xX?=
 =?utf-8?B?YWwyYkwwRlpVM1gvTGF3bXB6aHp3K2xheFloN29PWVF0b2NlUVNBM0IrR010?=
 =?utf-8?B?MlIzN0c5T0F5Z25jem82cllvWDdsMnJ6NU5vNlJ0V0JqRnVNT0dacXVrQlAv?=
 =?utf-8?B?dUN6Z21IaE4vWjZpQ2Y5dEllU054bzg2NmRKV2t0YkJ3UVV5L25wQVgrVStB?=
 =?utf-8?B?bytzUXhYQnRTR2lJL3Nxb2tYakJuYmUxemdoTE1wZ3d4TW01QjV4cGtEWU9r?=
 =?utf-8?B?cDZaTDUyTExLSlJGQ0N2NjNDMzA5M04wNHN6TUtCSlZEREdmWE04UnhJUWpF?=
 =?utf-8?B?QVB2b3F3UG13YWY2Tm9BVnRtUkFJWTd6WlE3SlNaYVdkWFRmNnN4RUxlaWFj?=
 =?utf-8?B?K1F1eWZDY1kxdHZSTTJkby8vZmZlMFVzWStOY1JkaW84THRyeGRYRGlsSXhv?=
 =?utf-8?Q?A1NhcUMJ/SN/8gerN2uDbLUpwT9qBPFKD0n7YS6?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f089fe84-eb1b-41ed-198d-08d987feeb3e
X-MS-Exchange-CrossTenant-AuthSource: DB7PR04MB4140.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2021 12:51:58.9983
 (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: 7GhOr/fQv7yTRlY21jN3Jgrux0MpTkRz6bs5Uou87UZEMxrAM6kMBON3hGu2JHf6uEGdr7rL7gucOAi7EAy4Gg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB8234
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;