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

[andy.shevchenko@gmail.com]

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