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: <1900fc73-c57b-5001-6aaa-7b85214da267@opensynergy.com>
Date:   Mon, 18 Oct 2021 09:49:32 +0300
Subject: Re: [PATCH v5 1/1] iio/scmi: Add reading "raw" attribute.
Content-Language: en-US
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     jbhayana@google.com, lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, Vasyl.Vavrychuk@opensynergy.com
References: <20211008182826.24412-1-andriy.tryshnivskyy@opensynergy.com>
 <20211008182826.24412-2-andriy.tryshnivskyy@opensynergy.com>
 <20211017125138.4f5bbc4d@jic23-huawei>
From:   Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
In-Reply-To: <20211017125138.4f5bbc4d@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Kzh4VytlWXB5MXZoVkZOcWVFYmp3aGZ3SE9GZ0hPdkh0Y0IrVHV3RW0rWll5?=
 =?utf-8?B?Vm13L1B3SFVzbmtYQ2drSmY2aEsyZWQxLy9DaWFCUVJvYmtCcHk0NVpuYWRz?=
 =?utf-8?B?ckI0YmlqbGhCTERXU2ZIZW81M0dCbStLSHZpTjhlKytwUlk0U3FKeUxQb3JR?=
 =?utf-8?B?cCtJcEg5U0JlUWVIZkxEYmt0NnZubzQyVkx3QXhoN1RFL05YdDZ0ZUU3RXN0?=
 =?utf-8?B?dUZ2cm0yMjI5eDZPQUxMVUdKaHA1OUlxL2pSZ0VFWDF1SHVPSWs4cmFCai80?=
 =?utf-8?B?dm5mZWg2NDVPdjhIYjhhcTVOT3RUSnVEcVV5MGFJR2plRTdWbTI0a1ErSDNa?=
 =?utf-8?B?alN0S2tTNHB6OHRnbTJreUF0V3RVeWVkSWFYZDF1ZGpWam93K0pEOWRJSkE5?=
 =?utf-8?B?c1pReVhHdGlyYlRjRFNoWSs2KzhpRFVuRVhabWgvdkVnNVFIU09nSU5KSkdY?=
 =?utf-8?B?UVcrbGZaSFIzTnRIS2VnU21QRERibWNUdEpkNnhTTDM0ald6NDU5aklUVStB?=
 =?utf-8?B?WFYvVkVjUXhaVzRmUFR5a21WdVBqZDVOSUR6Sjc5Z0g1b0FjOVc1S3Z3akJl?=
 =?utf-8?B?MS9lV0lWaGE3ODFRVy9OOTloQmY3VjFaQ1Fhc0Y4bjZyemc5eTlUMlBDdk00?=
 =?utf-8?B?UnhVeXdXTG44UitRL05GclU0MEJCMUlpOUwwRzF6bjFnYzAzYkNuV1VUNVNH?=
 =?utf-8?B?TFdHUjVJUUE4dk9zTWJuWG1BUjVsMlJCZlhqY2pNL1B4QTNuMERlUWo1Rmk1?=
 =?utf-8?B?NUlVTFpSUmtHVXBjdkxvM1JNa1RHMnRzbDNJZ1g1cklYMFVPZTQ3K2VDeUdv?=
 =?utf-8?B?TldoeUV0UlhSSytmTVFXS3piL1A4OGxNVUREWUE3cWVyb3hIcnk2VTRGbzRa?=
 =?utf-8?B?eUcxdUJidkUxZHFHYXhzNmx6cWdJMjdpMUx3R2lsanAvck4xOHFKMUtrNHNn?=
 =?utf-8?B?OVE2VCtFaTlQN05leGtKQ1hZY1pRYzgzdC91aGlKVy9Ea3VNbVdIckxydEVj?=
 =?utf-8?B?aWQ3KytYYXVvTGltbTFId1FOdURwNklhSEFpcTNIc0tHYXkxTTdKUVlLOWow?=
 =?utf-8?B?OGhMSGdsYWFsSlBvTjA1YmR1WkVLUnNWWnZ0WE1UWHY0VUI2Yk9TeUYybnFi?=
 =?utf-8?B?NEpFQ1ZPdS8wNlppWWVuWGN2MFMyemkxSFJpekNIbG1nUU13M3FtbkRuWjlO?=
 =?utf-8?B?cjV6ZFhvRmp2b0Y0T3NJUEV3RU43QkFQQzk1aDZzbVNYNU9rd1hDZ0Qwbk1y?=
 =?utf-8?B?eUdPQkhRekJZQ2YzMjVmQThkN3B0Wml4NWE1NUt1dmMwdTlnZnI1ZXRQZkxJ?=
 =?utf-8?B?L3dQRU15ZDFEdWJJNU91SCtJNlR4ZE55N01UZGpLOVBSY2FwN1h0Q2Y3TUo2?=
 =?utf-8?B?VzVoWmVUQkVxSEtSNnhYUlhFK1M2YmFRVTlPOTNhUUdxalVkK0lHb1JIQXl3?=
 =?utf-8?B?SlVwUTZSV01udHZxT2N1MnZZRjYzTjNUQWE4M2RPK2xvclRNU2JYL0piaFlU?=
 =?utf-8?B?VXk1SDBrYUhZY0dYbGJNWGh3RVZVaHBmeGovVWdvVVVZalNsclFzYXFVVmpp?=
 =?utf-8?B?b3h5L1QvOVI3VzhPSnNGaEl2OCtDb2lId0xZekRYSFYxd2cwYTBMTWw3bGdk?=
 =?utf-8?B?SCt5ZWdneHhodzhQT1V4WmdBUnNwTzIwV1FaNlZrRVgxWVJxMDE2U3lhQm1I?=
 =?utf-8?B?VjZWTG14c2lLNEZ0TDJOTDNrTmRtSm10QS9oZVpDQ3lKZ3F3MElwTktpeXcy?=
 =?utf-8?Q?6kaWweK4rEm4sEtHXmW6P0XMk2GN61twNTOC9O6?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b6dfb6b3-fcda-47bf-89e7-08d9920371c4
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB6359.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2021 06:49:34.2208
 (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: /kihAAQ9EQwrjkigCJzUi0/8Mtsm9Bm4A0RY/4YWxfArccs4wpuV7RoDLo4+3nsqfB00nwcvVt6Jd6ssizObzg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB4232
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 17.10.21 14:51, Jonathan Cameron 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.
>
>
> On Fri,  8 Oct 2021 21:28:26 +0300
> Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote:
>
>> Add scmi_iio_get_raw() to read "raw" attribute.
>>
>> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
>> ---
> For a single patch series, it is better to put a change log in the patch (here)
> Whilst I can see why you would use this approach rather than the read_raw callback
> there are significant disadvantages in doing so.  The channel can't be used
> by in kernel users such as iio-hwmon.   That may cause you more trouble than
> it is worth in the long run.
>
> Note that you could also define a new IIO_VAL type to still use the two
> (possibly) 32 bit values and return a 64 bit value. That way, with appropriate
> additions in the consumer drivers the channel could still be used.
>
> IIO_VAL_INT_64 perhaps with val as the lower 32 bits and val2 as the upper
> with appropriate care around the sign.

Hi Jonathan,

In the past I also was thinking about introducing a new type to return 64 bits value - IIO_VAL_INT_LONG.
However since it requires changes in

     include/linux/iio/types.h
     drivers/iio/industrialio-core.c

I thought it will be harder to get approval for this approach.
But now it seems there is no other way. So I will prepare a new patch version soon which introduces IIO_VAL_INT_LONG.

Thank you for your review!

>
>>   drivers/iio/common/scmi_sensors/scmi_iio.c | 61 ++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
>> index 7cf2bf282cef..691cbbd61e3a 100644
>> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
>> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
>> @@ -311,6 +311,62 @@ static const struct iio_info scmi_iio_info = {
>>        .write_raw = scmi_iio_write_raw,
>>   };
>>
>> +static ssize_t scmi_iio_get_raw(struct iio_dev *iio_dev, uintptr_t private,
>> +                             const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>> +     int err;
>> +     u32 sensor_config;
>> +     struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
>> +     int len = 0;
>> +
>> +     err = iio_device_claim_direct_mode(iio_dev);
>> +     if (err) {
>> +             dev_err(&iio_dev->dev,
>> +                     "Error in claiming direct mode for sensor %s err %d",
>> +                     sensor->sensor_info->name, err);
> It's not an error, it just means the device is busy, so at most dev_info()
> or just rely on userspace correctly interpreting EBUSY.

Agree. Will use dev_info().
Thank you!

>> +             goto err_release;
>> +     }
>> +
>> +     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);
>> +             goto err_release;
>> +     }
>> +
>> +     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);
>> +             goto err_release;
>> +     }
>> +
>> +     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 disabling sensor %s err %d",
>> +                     sensor->sensor_info->name, err);
>> +             goto err_release;
>> +     }
>> +
>> +     len = scnprintf(buf, PAGE_SIZE, "%lld\n",
>> +                     readings[chan->scan_index].value);
>> +
>> +err_release:
>> +     iio_device_release_direct_mode(iio_dev);
>> +
>> +     return len;
>> +}
>> +
>>   static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>>                                          uintptr_t private,
>>                                          const struct iio_chan_spec *chan,
>> @@ -355,6 +411,11 @@ static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>>   }
>>
>>   static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
>> +     {
>> +             .name = "raw",
>> +             .read = scmi_iio_get_raw,
>> +             .shared = IIO_SEPARATE,
>> +     },
>>        {
>>                .name = "raw_available",
>>                .read = scmi_iio_get_raw_available,
>