Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 24 Mar 2019 12:34:52 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Cristian Sicilia <sicilia.cristian@gmail.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-iio@vger.kernel.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/5] staging: iio: adc: Converted bool to bitfield
 format
Message-ID: <20190324123452.66b81183@archlinux>
In-Reply-To: <93b82557f3610f4bf4d09d0556e502b418376420.1553366098.git.sicilia.cristian@gmail.com>
References: <cover.1553335775.git.sicilia.cristian@gmail.com>
        <cover.1553366098.git.sicilia.cristian@gmail.com>
        <93b82557f3610f4bf4d09d0556e502b418376420.1553366098.git.sicilia.cristian@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, 23 Mar 2019 20:21:39 +0100
Cristian Sicilia <sicilia.cristian@gmail.com> wrote:

> Changed bool format to bitfield format to save space.
> 
> Signed-off-by: Cristian Sicilia <sicilia.cristian@gmail.com>
> 
> ---
> The strange thing is that this struct seems not populated
> using a DTS binding function.
Indeed and that should have definitely been a warning sign ;)
We are looking at traditional platform data here (pre device
tree) and generally we will want to drop it entirely for old
drivers that we are looking to move out of staging.

I don't mind improving it prior to dropping (as it avoids
setting bad precedence in the code base in the meantime) but
one comment inline...

> ---
>  drivers/staging/iio/adc/ad7192.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h
> index 7433a43..87891ba 100644
> --- a/drivers/staging/iio/adc/ad7192.h
> +++ b/drivers/staging/iio/adc/ad7192.h
> @@ -35,13 +35,13 @@ struct ad7192_platform_data {
>  	u16		vref_mv;
>  	u8		clock_source_sel;
>  	u32		ext_clk_hz;
> -	bool		refin2_en;
> -	bool		rej60_en;
> -	bool		sinc3_en;
> -	bool		chop_en;
> -	bool		buf_en;
> -	bool		unipolar_en;
> -	bool		burnout_curr_en;
> -};
> +	u8		refin2_en:1;
> +	u8		rej60_en:1;
> +	u8		sinc3_en:1;
> +	u8		chop_en:1;
> +	u8		buf_en:1;
> +	u8		unipolar_en:1;
> +	u8		burnout_curr_en:1;
> +} __attribute__((__packed__));
Please don't use packed for anything without a very very good reason.
it may result in data layouts that are much harder to read from.
That obviously doesn't matter here as I doubt it's read in a fast path.

>  
>  #endif /* IIO_ADC_AD7192_H_ */