Messages in this thread | | | Date | Wed, 23 Sep 2015 15:28:22 +0200 | From | Lars-Peter Clausen <> | Subject | Re: [PATCH v5 2/2] iio: mcp4531: Driver for Microchip digital potentiometers |
| |
On 09/23/2015 01:57 PM, Peter Rosin wrote: [...]
Looks pretty good.
One thing I'm concerned about is that this array
> +static const struct mcp4531_cfg mcp4531_cfg[] = { > + { /* MCP4531-502 */ .wipers = 1, .max_pos = 128, .kohms = 5, }, > + { /* MCP4531-103 */ .wipers = 1, .max_pos = 128, .kohms = 10, }, > + { /* MCP4531-503 */ .wipers = 1, .max_pos = 128, .kohms = 50, }, > + { /* MCP4531-104 */ .wipers = 1, .max_pos = 128, .kohms = 100, }, [...] > +}; > +
And the indices into the array defined here need to stay in sync. It is really easy to mess this up by accident when a new entry is added. And it will probably take a while to notice when this happens.
> +static const struct i2c_device_id mcp4531_id[] = { > + { "mcp4531-502", 0 }, > + { "mcp4531-103", 1 }, > + { "mcp4531-503", 2 }, > + { "mcp4531-104", 3 }, [...] > + {} > +};
I see two options to solve this. Either define a enum with all the different devices and use a symbolic name for the indices.
E.g.
enum mcp4531_type { MCP4531_502, MCP4531_103, ... };
static const struct mcp5431_cfg mcp5431_cfg[] = { [MCP4531_502] = {.wipers = 1, .max_pos = 128, .kohms = 5, }, [MCP4531_103] = {.wipers = 1, .max_pos = 128, .kohms = 10, }, ... };
static const struct i2c_device_id mcp4531_id[] = { { "mcp4531-502", MCP4531_502 }, { "mcp4531-103", MCP4531_103 }, ... };
Or the second option is to directly assign the cfg struct as the driver data in the device ID table.
E.g.
#define MCP4531_CFG(_wipers, _max_pos, _kohms) \ (kernel_ulong_t)&(struct mcp4531_cfg) { \ .wipers = _wipers, .max_pos = _max_pos, .kohms = _kohms }
static const struct i2c_device_id mcp4531_id[] = { { "mcp4531-502", MCP4531_CFG(1, 128, 5) }, { "mcp4531-103", MCP4531_CFG(1, 128, 10) }, ... };
| |