Hi All,
On 06/12/2019 14:22, Kieran Bingham wrote:
> 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>
Folding this patch into the max9286 driver without correcting for holes,
as I believe it's better to group the associated variables together, and
accept the small loss, as the structure is very large so it's a small
proportion.
--
Kieran
>>
>> 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
>>
--
Regards
--
Kieran
