> -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx] > Sent: 29 March 2017 05:07 > To: Han, Nandor (GE Healthcare) <nandor.han@xxxxxx> > Cc: Alexandre Courbot <gnurou@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Mark Rutland > <mark.rutland@xxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > Malinen, Semi (GE Healthcare) <semi.malinen@xxxxxx> > Subject: EXT: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver > > On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han@xxxxxx> wrote: > > > This is a simple driver that provides a /sys/class/gpio > > interface for controlling and configuring the GPIO lines. > > Use the gpio tools in tools/gpio, use the characcter device. > Do not use sysfs. Change this to reference the tools. > > > It does not provide support for chip select or interrupts. > > > > Signed-off-by: Nandor Han <nandor.han@xxxxxx> > > Signed-off-by: Semi Malinen <semi.malinen@xxxxxx> > (...) > > +exar Exar Corporation > > Send this as a separate patch to the DT bindings maintainer > (Rob Herring.) > OK. I will create a separate patch with this one. I guess is not an issue to send all the patches to Rob as well. <snip> > > + > > + ret = xra1403_get_byte(xra, addr + (bit > 7)); > > + if (ret < 0) > > + return ret; > > + > > + return !!(ret & BIT(bit % 8)); > > +} > > This looks like it can use regmap-spi right off, do you agree? > Yes. Using regmap-spi will definitely improve the code readability and reduce boilerplate. Done. > git grep devm_regmap_init_spi > should give you some examples of how to use it. > > If it's not off-the shelf regmap drivers like > drivers/iio/pressure/mpl115_spi.c > give examples of how to make more elaborate custom > SPI transfers with regmap. > Thanks, I did check other drivers as examples. Not that I needed for this driver, but ...mpl115_spi.c doesn't seem to use regmap (checked on next-20170327) <snip> > > + > > + if (value != ret) { > > + tx[0] = addr << 1; > > + tx[1] = value; > > + ret = spi_write(xra->spi, tx, sizeof(tx)); > > + } else { > > + ret = 0; > > + } > > + > > +out_unlock: > > + mutex_unlock(&xra->lock); > > + > > + return ret; > > +} > > Classical mask-and-set implementation right? > With regmap this becomes simply regmap_update_bits(map, addr, mask, set) > True. :) <snip> > > + /* bring the chip out of reset */ > > + reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(reset_gpio)) > > + dev_warn(&spi->dev, "could not get reset-gpios\n"); > > + else if (reset_gpio) > > + gpiod_put(reset_gpio); > > I don't think you should put it, other than in the remove() > function and in that case you need to have it in the > state container. Can you please be more explicit here. Currently I'm trying to bring the device out from reset in case reset GPIO is provided. I don't see how this could be done in remove() :) > > > + mutex_init(&xra->lock); > > + > > + xra->chip.direction_input = xra1403_direction_input; > > + xra->chip.direction_output = xra1403_direction_output; > > Please implement .get_direction(). This is very nice to have. > Done > > +static int xra1403_remove(struct spi_device *spi) > > +{ > > + struct xra1403 *xra = spi_get_drvdata(spi); > > + > > + gpiochip_remove(&xra->chip); > > Use devm_gpiochip_add_data() and this remove is not > needed at all. > True. Done <snip> > > +subsys_initcall(xra1403_init); > > + > > +static void __exit xra1403_exit(void) > > +{ > > + spi_unregister_driver(&xra1403_driver); > > +} > > +module_exit(xra1403_exit); > > This seems like tricksy. Just module_spi_driver() > should be fine don't you think? Yeah. TBH I don't have a strong reason why module_spi_driver init level shouldn't be enough. Done. Regards, Nandor ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
