Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 6 Jul 2020 15:27:43 +0300 | Subject | Re: [PATCH v5 1/2] gpio: add GPO driver for PCA9570 |
| |
On Sun, Jul 5, 2020 at 4:31 PM Sungbo Eo <mans0n@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 <mans0n@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
| |