| Message ID | 20191206140520.10457-1-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state | expand |
Hi Kieran, On Fri, Dec 6, 2019 at 3:05 PM Kieran Bingham <kieran.bingham@ideasonboard.com> 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@ideasonboard.com> 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. Gr{oetje,eeting}s, Geert
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@ideasonboard.com> 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@ideasonboard.com> > > 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 >
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@ideasonboard.com> 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@ideasonboard.com> 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 >>
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index e5b3f78318db..6ea08fd87811 100644 --- 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; struct v4l2_ctrl_handler ctrls; @@ -221,11 +221,6 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val) * I2C Multiplexer */ -enum max9286_i2c_mux_state { - MAX9286_MUX_CLOSED = 0, - MAX9286_MUX_OPEN, -}; - static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf) { max9286_write(priv, 0x0a, conf); @@ -242,7 +237,7 @@ static void max9286_i2c_mux_open(struct max9286_priv *priv) /* Open all channels on the MAX9286 */ max9286_i2c_mux_configure(priv, 0xff); - priv->mux_state = MAX9286_MUX_OPEN; + priv->mux_open = true; } static void max9286_i2c_mux_close(struct max9286_priv *priv) @@ -254,7 +249,7 @@ static void max9286_i2c_mux_close(struct max9286_priv *priv) */ max9286_i2c_mux_configure(priv, 0x00); - priv->mux_state = MAX9286_MUX_CLOSED; + priv->mux_open = false; priv->mux_channel = -1; } @@ -263,7 +258,7 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) struct max9286_priv *priv = i2c_mux_priv(muxc); /* Channel select is disabled when configured in the opened state. */ - if (priv->mux_state == MAX9286_MUX_OPEN) + if (priv->mux_open) return 0; if (priv->mux_channel == chan)
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@ideasonboard.com> --- drivers/media/i2c/max9286.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)