Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux