Hi Andy,
On 08.08.22 13:32, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote:
>
> ..
>
>> + yas5xx->chip = id->driver_data;
>> + yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
>
> I don't see how ->chip is being used, I would expect it is the part of
> chip_info, if it's really needed. That said, please make it directly a
> pointer, so the above becomes:
>
> ... ->chip_info = (const struct ...)id->driver_data;
>
> ..
>
>> + {"yas530", yas530 },
>> + {"yas532", yas532 },
>> + {"yas533", yas533 },
>
> Read above and here:
>
> yas53... ==> (kernel_ulong_t)&...[yas53...]
>
Generally on this part, I'm quite confused about the ...
enum chip_ids {
yas530,
yas532,
yas533,
};
... at the beginning of the driver.
In my naive beginners approach I think that I have to initialize this
enum. I did this by:
struct yas5xx {
...
enum chip_ids chip;
...
};
...
yas5xx->chip = id->driver_data;
The i2c_device_id at the end of the driver initially only contained the
fist part, looking like this:
static const struct i2c_device_id yas5xx_id[] = {
{"yas530", },
{"yas532", },
{"yas533", },
{}
};
This first part is "char name[I2C_NAME_SIZE]" according to [1]. I didn't
manage to initialize the enum with "id->name"...
yas5xx->chip = id->name;
... this resulted in a compiler error stating "incompatible types when
assigning to type 'enum chip_ids' from type 'const char *'".
This made me introduce the second part of i2c_device_id...
static const struct i2c_device_id yas5xx_id[] = {
{"yas530", yas530 },
{"yas532", yas532 },
{"yas533", yas533 },
{}
};
... which is "kernel_ulong_t driver_data;". Initializing the enum by
"id->driver_data" did work:
yas5xx->chip = id->driver_data;
[1]
https://github.com/torvalds/linux/blob/v5.19/include/linux/mod_devicetable.h#L465
--------------------
I think in other drivers I've seen enums not being initialized. I don't
understand how this works. Unfortunately I can't recall specific examples.
--------------------
I now had a try with following changes...
struct yas5xx {
struct device *dev;
- enum chip_ids chip;
const struct yas5xx_chip_info *chip_info;
...
};
...
- yas5xx->chip = id->driver_data;
- yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
+ yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data];
... or summarized as:
enum chip_ids {
yas530,
yas532,
yas533,
};
...
struct yas5xx {
...
};
...
yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data];
...
static const struct i2c_device_id yas5xx_id[] = {
{"yas530", yas530 },
{"yas532", yas532 },
{"yas533", yas533 },
{}
};
This seems to work. Therefore I would it implement it that way. I hope
it works reliably, as I don't see a connection between the enum and the
chip_info.
--------------------
What I still can't manage is getting rid of the "id->driver_data" part.
When trying the above with "id->name"...
yas5xx->chip_info = &yas5xx_chip_info_tbl[id->name];
... the compiler complains about "array subscript is not an integer".
When trying to add quotation marks to the enum chip_ids content, the
compiler complains about this by "expected identifier before string
constant".
Kind regards,
Jakob
