Hi Geert,
On 06/12/2019 14:10, Geert Uytterhoeven wrote:
> Hi Kieran,
>
> On Fri, Dec 6, 2019 at 3:05 PM Kieran Bingham
> <kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>> While simplifying the i2c-mux state, the states were stored in an enum
>> (initially there were three).
>>
>> This has now simplified down to 2 states, open and closed - and can be
>> represented easily in a bool.
>>
>> It 'could' also be represented within the mux_channel, but I don't want
>> to pollute that further than the '-1' value which is already stored in
>> there to represent no channel selected.
>>
>> Remove the max9286_i2c_mux_state and replace with a bool mux_open flag,
>> and move the location within the private struct to be more appropriate.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -144,10 +144,10 @@ struct max9286_priv {
>> struct media_pad pads[MAX9286_N_PADS];
>> struct regulator *regulator;
>> bool poc_enabled;
>> - int mux_state;
>>
>> struct i2c_mux_core *mux;
>> unsigned int mux_channel;
>> + bool mux_open;
>
> Please keep all booleans together, to fill up holes due to alignment
> restrictions.
I was trying to group related i2c_mux items, but I do indeed see a
strong argument there...
/me digs out pahole just to have a look :-D (but I know what the answer is)
struct max9286_priv {
struct i2c_client * client; /* 0 8 */
struct gpio_desc * gpiod_pwdn; /* 8 8 */
struct v4l2_subdev sd; /* 16 320 */
/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
struct media_pad pads[5]; /* 336 280 */
/* --- cacheline 9 boundary (576 bytes) was 40 bytes ago --- */
struct regulator * regulator; /* 616 8 */
struct dentry * dbgroot; /* 624 8 */
bool poc_enabled; /* 632 1 */
/* XXX 7 bytes hole, try to pack */
/* --- cacheline 10 boundary (640 bytes) --- */
struct gpio_chip gpio; /* 640 600 */
/* --- cacheline 19 boundary (1216 bytes) was 24 bytes ago --- */
u8 gpio_state; /* 1240 1 */
/* XXX 7 bytes hole, try to pack */
struct i2c_mux_core * mux; /* 1248 8 /
unsigned int mux_channel; /* 1256 4 */
bool mux_open; /* 1260 1 */
/* XXX 3 bytes hole, try to pack */
struct v4l2_ctrl_handler ctrls; /* 1264 296 */
/* --- cacheline 24 boundary (1536 bytes) was 24 bytes ago --- */
struct v4l2_mbus_framefmt fmt[4]; /* 1560 192 */
/* --- cacheline 27 boundary (1728 bytes) was 24 bytes ago --- */
unsigned int nsources; /* 1752 4 */
unsigned int source_mask; /* 1756 4 */
unsigned int route_mask; /* 1760 4 */
unsigned int csi2_data_lanes; /* 1764 4 */
struct max9286_source sources[4]; /* 1768 288 */
/* --- cacheline 32 boundary (2048 bytes) was 8 bytes ago --- */
struct v4l2_async_notifier notifier; /* 2056 96 */
/* size: 2152, cachelines: 34, members: 20 */
/* sum members: 2135, holes: 3, sum holes: 17 */
/* last cacheline: 40 bytes */
};
Hrm ... this one really pulls me in both directions ...
Which is the lesser evil - memory holes or ungrouped variables?
--
Kieran
> Gr{oetje,eeting}s,
>
> Geert
>
