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;
From:   "Sa, Nuno" <Nuno.Sa@analog.com>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "jic23@kernel.org" <jic23@kernel.org>,
        "lars@metafoo.de" <lars@metafoo.de>,
        "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Subject: RE: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
 basis
Thread-Topic: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
 basis
Thread-Index: AQHWGffHKP4cv1C0/ES0EynNzW/LxqiH4ogg
Date:   Fri, 24 Apr 2020 07:51:05 +0000
Message-ID: <CY4PR03MB33506FD8C2BF3921FE9BA2DD99D00@CY4PR03MB3350.namprd03.prod.outlook.com>
References: <20200424051818.6408-1-alexandru.ardelean@analog.com>
 <20200424051818.6408-5-alexandru.ardelean@analog.com>
In-Reply-To: <20200424051818.6408-5-alexandru.ardelean@analog.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: analog.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b5101931-f7a8-4f1f-c6f5-08d7e8243e75
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Apr 2020 07:51:05.8558
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: eN7Hg32RWwy/MMutyjlgI/xx7XTH1Boi9lIeXvqhsCjbtzH0DVtzBcD4Mv6soFAl8U5Qr87Pr24mmvckK2LRnw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR03MB2485
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.676
 definitions=2020-04-24_02:2020-04-23,2020-04-24 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxlogscore=999 adultscore=0
 phishscore=0 clxscore=1015 impostorscore=0 lowpriorityscore=0
 malwarescore=0 priorityscore=1501 suspectscore=0 bulkscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2003020000 definitions=main-2004240059
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On Behalf Of Alexandru Ardelean
> Sent: Freitag, 24. April 2020 07:18
> To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: jic23@kernel.org; lars@metafoo.de; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>
> Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel bas=
is
>=20
> From: Lars-Peter Clausen <lars@metafoo.de>
>=20
> Now that we support multiple channels with the same scan index we can no
> longer use the scan mask to track which channels have been enabled.
> Otherwise it is not possible to enable channels with the same scan index
> independently.
>=20
> Introduce a new channel mask which is used instead of the scan mask to
> track which channels are enabled. Whenever the channel mask is changed a
> new scan mask is computed based on it.
>=20
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
>  drivers/iio/inkern.c              | 19 +++++++++-
>  include/linux/iio/buffer_impl.h   |  3 ++
>  include/linux/iio/consumer.h      |  2 +
>  4 files changed, 64 insertions(+), 22 deletions(-)
>=20
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio=
-buffer.c
> index c06691281287..1821a3e32fb3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> *buffer,
>  	if (buffer->scan_mask =3D=3D NULL)
>  		return -ENOMEM;
>=20
> +	buffer->channel_mask =3D bitmap_zalloc(indio_dev->num_channels,
> +					     GFP_KERNEL);
> +	if (buffer->channel_mask =3D=3D NULL) {
> +		bitmap_free(buffer->scan_mask);
> +		return -ENOMEM;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
>=20
>  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
>  {
> +	bitmap_free(buffer->channel_mask);
>  	bitmap_free(buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
>=20
>  	/* Ensure ret is 0 or 1. */
>  	ret =3D !!test_bit(to_iio_dev_attr(attr)->address,
> -		       indio_dev->buffer->scan_mask);
> +		       indio_dev->buffer->channel_mask);
>=20
>  	return sprintf(buf, "%d\n", ret);
>  }
> @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev
> *indio_dev,
>   * buffers might request, hence this code only verifies that the
>   * individual buffers request is plausible.
>   */
> -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> -		      struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> +				struct iio_buffer *buffer, int bit)
>  {
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
> +	unsigned int ch;
>=20
>  	trialmask =3D bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
>  	if (trialmask =3D=3D NULL)
> @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> *indio_dev,
>  		WARN(1, "Trying to set scanmask prior to registering
> buffer\n");
>  		goto err_invalid_mask;
>  	}
> -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-
> >masklength);
> -	set_bit(bit, trialmask);
> +
> +	set_bit(bit, buffer->channel_mask);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-
> >num_channels)
> +		set_bit(indio_dev->channels[ch].scan_index, trialmask);

So, here if the channels all have the same scan_index, we will end up with =
a scan_mask which is
different that channel_mask, right? I saw that in our internal driver's we =
then just access the
channel_mask field directly to know what pieces/channels do we need to enab=
le prior to
buffering, which implies including buffer_impl.h.

So, for me it would make sense to compute scan_mask so that it will be the =
same as channel_mask
(hmm but that would be a problem when computing the buffer size...) and dri=
vers can correctly use
` validate_scan_mask ()` cb. Alternatively, we need to expose channel_mask =
either on a new cb or
change the ` validate_scan_mask ()` footprint.=20

- Nuno S=E1