Thu, Sep 15, 2022 at 05:18:03PM +0530, Shubhrajyoti Datta kirjoitti:
> slg7xl45106 is a I2C GPO expander.
> Add a compatible string for the same. Also update the
> driver to write and read from it.
It's better, but something to improve.
...
> /**
> * struct pca9570 - GPIO driver data
> * @chip: GPIO controller chip
> @@ -25,6 +27,12 @@ struct pca9570 {
> struct gpio_chip chip;
> struct mutex lock;
> u8 out;
> + const struct pca9570_platform_data *p_data;
I would put it after 'chip' member, so it will save 4 bytes on 64-bit machines
of some architectures. Also, don't you need to add a kernel doc for a new
member?
> +};
> +struct pca9570_platform_data {
> + u16 ngpio;
> + u32 command;
> };
Strictly speaking this should be defined before struct pca9570, otherwise you
need to have a forward declaration.
> @@ -122,13 +138,28 @@ static int pca9570_probe(struct i2c_client *client)
> static const struct i2c_device_id pca9570_id_table[] = {
> { "pca9570", 4 },
> { "pca9571", 8 },
> + { "slg7xl45106", 8 },
> { /* sentinel */ }
This table should also use your new structure:
{ "slg7xl45106", (kernel_ulong_t)&slg7xl45106_gpio },
(In the similar way for the existing entries)
Taking the last into consideration I would suggest to split this to two
changes:
1) introducing a new data structure with conversion of the existing members;
2) adding support for a new chip.
> };
Othewise looks good.
--
With Best Regards,
Andy Shevchenko
