On Sun, Jul 5, 2020 at 4:31 PM Sungbo Eo <man...@gorani.run> wrote: > > NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. > Its ports are controlled only by a data byte without register address.
Thank you for an update, my comments (besides what Bart has) below. > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf > > Signed-off-by: Sungbo Eo <man...@gorani.run> No blank line in between. ... > +#include <linux/gpio/driver.h> > +#include <linux/i2c.h> > +#include <linux/module.h> It also needs property.h. ... > +struct pca9570 { > + struct gpio_chip chip; > + struct i2c_client *client; This basically a duplication of reference to a parent device of GPIO chip. See below. > + u8 out; > +}; > + > +static int pca9570_read(struct pca9570 *gpio, u8 *value) > +{ struct i2c_client *client = to_i2c_client(gpio->chip.parent); > + int ret; > + > + ret = i2c_smbus_read_byte(gpio->client); > + if (ret < 0) > + return ret; > + > + *value = ret; > + return 0; > +} ... > +static const struct i2c_device_id pca9570_id_table[] = { > + { "pca9570", 4 }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(i2c, pca9570_id_table); > + > +static const struct of_device_id pca9570_of_match_table[] = { > + { .compatible = "nxp,pca9570", .data = (void *)4 }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pca9570_of_match_table); These structures now can be located closer to the end of the driver (exactly before first use). ... > + gpio->chip = template_chip; I'm not sure why we need an additional structure to just memcpy() to the chip. But here I have no strong opinion (perhaps the compiler even optimizes this). -- With Best Regards, Andy Shevchenko
