Re: [PATCH v2 2/2] gpio: pca9570: add slg7xl45106 support

andy.shevchenko
2022-09-15T12:41:11Z

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