lkml.org 
[lkml]   [2017]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
Subject[PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver


> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 29 March 2017 05:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: Alexandre Courbot <gnurou@gmail.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Malinen, Semi (GE Healthcare) <semi.malinen@ge.com>
> 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@ge.com> 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@ge.com>
> > Signed-off-by: Semi Malinen <semi.malinen@ge.com>
> (...)
> > +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
\
 
 \ /
  Last update: 2017-04-05 15:26    [W:0.057 / U:1.456 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site