From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 36D67C0650F for ; Mon, 5 Aug 2019 15:21:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F22BA216F4 for ; Mon, 5 Aug 2019 15:21:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1565018501; bh=HG1uUQO/leMwYCtdindsZ6+L6wwDbysuIsUhZNtigYM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=P5SaKzk3ts3tx1P9fBf6MU1Opw/GeXlNcz0D3E93BQT3okoSH8xwoB8Hfx4shgvRx UAh2tE4yJc3MhYxlU13bHdQJP2dtUVUNO9Aaf1t2hW1QCvFDsMCQRt8t3Cog5Nf9oN KsyMBNUBDam5mc8YeSz9y8FR0sHH1CkSkg+/VQnU= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728801AbfHEPVk (ORCPT ); Mon, 5 Aug 2019 11:21:40 -0400 Received: from mail.kernel.org ([198.145.29.99]:40160 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728760AbfHEPVk (ORCPT ); Mon, 5 Aug 2019 11:21:40 -0400 Received: from archlinux (cpc149474-cmbg20-2-0-cust94.5-4.cable.virginm.net [82.4.196.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id DE1CA20665; Mon, 5 Aug 2019 15:21:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1565018499; bh=HG1uUQO/leMwYCtdindsZ6+L6wwDbysuIsUhZNtigYM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=gqZVaJKgIAFx/j+z8Jf5DvJhf2IKrZGYPeiFtd8N+Opg4v/pRpujWcOYoa2Dfn7Fa yYdzzFZfhQZPOj4IZN4Sj+YPYVIAwYbjVzQbFtCd013QkD8LjF4JICgD5s9oUdpjhk di6d374c7zS+VCsdmbvZpchfwGKMisfi9HPjS1gY= Date: Mon, 5 Aug 2019 16:21:35 +0100 From: Jonathan Cameron To: "Ardelean, Alexandru" Cc: "denis.ciocca@st.com" , "linux-iio@vger.kernel.org" Subject: Re: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions Message-ID: <20190805162135.68dc97c4@archlinux> In-Reply-To: <6939dc8b7c28b1f4ed00f33b5ed2cfde17b73d41.camel@analog.com> References: <20190731215250.16403-1-denis.ciocca@st.com> <20190731215250.16403-2-denis.ciocca@st.com> <6939dc8b7c28b1f4ed00f33b5ed2cfde17b73d41.camel@analog.com> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-iio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Thu, 1 Aug 2019 08:24:10 +0000 "Ardelean, Alexandru" wrote: > On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote: > > [External] > > > > This patch is introducing preenable/postdisable in the common > > st_sensors_buffer code in order to remove the memory allocation / > > de-allocation from each single st driver. > > > > Reviewed-by: Alexandru Ardelean > > > Signed-off-by: Denis Ciocca As a rework, this is clearly reasonable, however, if we are going to touch this code at all, there are a few things I would like to tidy up about it. Firstly it's one of relatively few drivers that actually touch scan_bytes in the first place. That is supposed to be internal state to the core subsystem and not used by drivers (see INTERN marking in iio.h). It bled across the boundary in too many places where I wasn't looking. Secondly these allocations are small. You would be better off just making them part of the main state structure and not dynamically allocated at all. So move buffer_data to the end of struct st_sensor_data and make it whatever the maximum size needed is - I'm thinking probably 32 bytes but haven't checked. You call the bulk regmap API against it so you also need to ensure it's in it's own cacheline. Use the __cacheline_aligned magic to enforce that. The iio_priv region is always aligned appropriately to allow iio_priv accessed structures to pull this trick. That way we don't need to do any memory handling on demand at all. We may or many not save memory as will depend on exactly how big that structure already is and what mood the allocator is in. I don't think I'm missing a reason we can't take the approach of embedding the buffer and it definitely makes the code simpler. Thanks, Jonathan > > --- > > .../iio/common/st_sensors/st_sensors_buffer.c | 22 +++++++++++++++++++ > > include/linux/iio/common/st_sensors.h | 2 ++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c > > index eee30130ae23..9da1c8104883 100644 > > --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c > > +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c > > @@ -81,6 +81,28 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p) > > } > > EXPORT_SYMBOL(st_sensors_trigger_handler); > > > > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev) > > +{ > > + struct st_sensor_data *sdata = iio_priv(indio_dev); > > + > > + sdata->buffer_data = kmalloc(indio_dev->scan_bytes, > > + GFP_DMA | GFP_KERNEL); > > + if (!sdata->buffer_data) > > + return -ENOMEM; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(st_sensors_buffer_preenable); > > + > > +int st_sensors_buffer_postdisable(struct iio_dev *indio_dev) > > +{ > > + struct st_sensor_data *sdata = iio_priv(indio_dev); > > + > > + kfree(sdata->buffer_data); > > + return 0; > > +} > > +EXPORT_SYMBOL(st_sensors_buffer_postdisable); > > + > > MODULE_AUTHOR("Denis Ciocca "); > > MODULE_DESCRIPTION("STMicroelectronics ST-sensors buffer"); > > MODULE_LICENSE("GPL v2"); > > diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h > > index 28fc1f9fa7d5..c66ebb236a15 100644 > > --- a/include/linux/iio/common/st_sensors.h > > +++ b/include/linux/iio/common/st_sensors.h > > @@ -254,6 +254,8 @@ struct st_sensor_data { > > > > #ifdef CONFIG_IIO_BUFFER > > irqreturn_t st_sensors_trigger_handler(int irq, void *p); > > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev); > > +int st_sensors_buffer_postdisable(struct iio_dev *indio_dev); > > #endif > > > > #ifdef CONFIG_IIO_TRIGGER