[1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state
diff mbox series

Message ID 20191206140520.10457-1-kieran.bingham@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • [1/3] media: i2c: max9286: Remove redundant max9286_i2c_mux_state
Related show

Commit Message

Kieran Bingham Dec. 6, 2019, 2:05 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Dec. 6, 2019, 2:10 p.m. UTC | #1
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
Kieran Bingham Dec. 6, 2019, 2:22 p.m. UTC | #2
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
>
Kieran Bingham Dec. 10, 2019, 10:26 a.m. UTC | #3
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
>>

Patch
diff mbox series

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)