Hi Bartosz, thanks for your review! On Thu, Jan 31, 2019 at 9:28 AM Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> wrote: > a couple remarks below. Fixed most! One comment: > > +static int gw_pld_input8(struct gpio_chip *gc, unsigned offset) > > +{ > > + struct gw_pld *gw = gpiochip_get_data(gc); > > + > > + gw->out |= (1 << offset); > > + > > + return i2c_smbus_write_byte(gw->client, gw->out); > > May I suggest using the i2c regmap for that? You wouldn't even need to > keep track of the out variable - you could use the > regmap_update_bits() helper. I can't (AFAICT) beacuse, i2c regmap is for registers, which have register offsets. It accesses the devices with two standard transfers, one telling the register, and the second doing the actual byte read or write. This thing has only one read and one write register so it doesn't send any register offset, just the value. So I doubt I can make it any simpler. I realized I forgot to use the BIT() macros that I usually insist on myself though. Yours, Linus Walleij