Received-SPF: pass (google.com: domain of linux-kernel+bounces-99255-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Mon, 11 Mar 2024 17:44:14 +0100
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Vasileios Amoiridis <vassilisamir@gmail.com>, jic23@kernel.org,
	lars@metafoo.de, ang.iglesiasg@gmail.com, phil@raspberrypi.com,
	579lpy@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: pressure: Fixes SPI support for BMP3xx devices
Message-ID: <20240311164414.GA1790017@vamoiridPC>
References: <20240311005432.1752853-1-vassilisamir@gmail.com>
 <Ze7XU5JS8FF5FZdg@smile.fi.intel.com>
 <20240311155829.00004478@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240311155829.00004478@Huawei.com>

On Mon, Mar 11, 2024 at 03:58:29PM +0000, Jonathan Cameron wrote:
> On Mon, 11 Mar 2024 12:05:07 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Mar 11, 2024 at 01:54:32AM +0100, Vasileios Amoiridis wrote:
> > > Bosch does not use unique BMPxxx_CHIP_ID for the different versions of
> > > the device which leads to misidentification of devices if their ID is
> > > used. Use a new value in the chip_info structure instead of the
> > > BMPxxx_CHIP_ID, in order to choose the regmap_bus to be used.  
> > 
> > ...
> > 
> > >  	const struct regmap_config *regmap_config;
> > > +	int spi_read_extra_byte;  
> > 
> > Why is it int and not boolean?
> > Also, please run `pahole` to see the best place for a new member.
> 
> Whilst that's good in general, there aren't many of these structs (4ish)
> so if the 'cheapest' positioning isn't natural or hurts readability
> ignore what you get from pahole.
> 
> Jonathan
> 

Hello Andy, hello Jonathan,

Thank you for your feedback! Andy, I already used pahole as you suggested
already in one of my previous patch series for this driver, and the
result looks like it uses only 4 byte values so adding a bool would only
create a 3 byte hole. Apart from that, I noticed that for example, the 
num_chip_ids value could have easily been a u8 but instead it's an int,
I guess to satisfy the alignment requirements. Also the id_reg could
easily be a u8 but instead it is an unsigned int. Maybe in the future, if
more values are added it will be good to have a re-organization of those
values. I can look at it, after the fix is done and it is on the mainline.
Finally, I chose this specific position because it's next to the 
regmap_config, and the new value affects the regmap_bus so in my opinion
it helps readability.

pahole --class_name=bmp280_chip_info bmp280-core.dwo
struct bmp280_chip_info {
        unsigned int               id_reg;               /*     0     4 */
        const u8  *                chip_id;              /*     4     4 */
        int                        num_chip_id;          /*     8     4 */
        const struct regmap_config  * regmap_config;     /*    12     4 */
        int                        spi_read_extra_byte;  /*    16     4 */
        const struct iio_chan_spec  * channels;          /*    20     4 */
        int                        num_channels;         /*    24     4 */
        unsigned int               start_up_time;        /*    28     4 */
        const int  *               oversampling_temp_avail; /*    32     4 */
        int                        num_oversampling_temp_avail; /*    36     4 */
        int                        oversampling_temp_default; /*    40     4 */
        const int  *               oversampling_press_avail; /*    44     4 */
        int                        num_oversampling_press_avail; /*    48     4 */
        int                        oversampling_press_default; /*    52     4 */
        const int  *               oversampling_humid_avail; /*    56     4 */
        int                        num_oversampling_humid_avail; /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        int                        oversampling_humid_default; /*    64     4 */
        const int  *               iir_filter_coeffs_avail; /*    68     4 */
        int                        num_iir_filter_coeffs_avail; /*    72     4 */
        int                        iir_filter_coeff_default; /*    76     4 */
        const int  *               sampling_freq_avail;  /*    80     4 */
        int                        num_sampling_freq_avail; /*    84     4 */
        int                        sampling_freq_default; /*    88     4 */
        int                        (*chip_config)(struct bmp280_data *); /*    92     4 */
        int                        (*read_temp)(struct bmp280_data *, int *, int *); /*    96     4 */
        int                        (*read_press)(struct bmp280_data *, int *, int *); /*   100     4 */
        int                        (*read_humid)(struct bmp280_data *, int *, int *); /*   104     4 */
        int                        (*read_calib)(struct bmp280_data *); /*   108     4 */
        int                        (*preinit)(struct bmp280_data *); /*   112     4 */

        /* size: 116, cachelines: 2, members: 29 */
        /* last cacheline: 52 bytes */
};

Thanks,
Vasilis
> > 
>