From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933421AbdDENZT (ORCPT ); Wed, 5 Apr 2017 09:25:19 -0400 Received: from mx0b-00176a03.pphosted.com ([67.231.157.48]:50930 "EHLO mx0a-00176a03.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933359AbdDENZK (ORCPT ); Wed, 5 Apr 2017 09:25:10 -0400 From: "Han, Nandor (GE Healthcare)" To: Linus Walleij CC: Alexandre Courbot , Rob Herring , Mark Rutland , "linux-gpio@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Malinen, Semi (GE Healthcare)" Thread-Topic: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver Thread-Index: AQHSpsKisO+GNwMHKEiVTGcNOcZLiaGrFNgAgAuzFXA= Date: Wed, 5 Apr 2017 13:24:49 +0000 Message-ID: References: <6bf04ba1761f0692cb461558f0c8836f0d1f7ad8.1490595641.git.nandor.han@ge.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: linaro.org; dkim=none (message not signed) header.d=none;linaro.org; dmarc=none action=none header.from=ge.com; x-originating-ip: [205.173.90.80] x-microsoft-exchange-diagnostics: 1;HE1P101MB0188;7:X4vPl/HtRo2nRwU7+tFer7+X/gXgWQsijfb7iyZoOn1DDHp2qzOs4xPc0dJdgVXYGTITBKEPdiWXvMkrEUwHU5QNUJBKgIQEeIwX7d744dieUsUQFNzvxrYmyceAK7A6lddaqM7iwMTHKnq5aLLE+qrTudjPUQFuIMmUcrpqe7XSglpNvssV4AS9j5xpzZc6vpSnAqX++I6sCWQHhQIDltpWau6ocXQg/m2lYdO4f6v3CP9Bg1EF9Sejtfuv3hRRQqWGipiU0JFtZdaZ8DcLRTZiUKkEPaRtaiBg7gmMsowATheYMxdsZVYw0HwG4wJyqG/gj5jl74/hOgechJcJPw== x-ld-processed: 15ccb6d1-d335-4996-b6f9-7b6925f08121,ExtAddr x-ms-office365-filtering-correlation-id: c17ffb9a-37c3-4cf5-b512-08d47c272315 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254075)(201703131423075)(201703031133081)(201702281549075);SRVR:HE1P101MB0188; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(9452136761055)(106981052589767); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040450)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(6041248)(20161123560025)(20161123555025)(201703131423075)(201702281528075)(201703061421075)(20161123564025)(20161123562025)(6072148);SRVR:HE1P101MB0188;BCL:0;PCL:0;RULEID:;SRVR:HE1P101MB0188; x-forefront-prvs: 0268246AE7 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(39400400002)(39410400002)(39850400002)(39860400002)(39450400003)(39840400002)(24454002)(377454003)(13464003)(3660700001)(8936002)(39060400002)(6506006)(38730400002)(3280700002)(6436002)(102836003)(86362001)(55016002)(81166006)(8676002)(7696004)(3846002)(229853002)(110136004)(9686003)(53936002)(33656002)(4326008)(6916009)(66066001)(53546009)(76176999)(2906002)(2950100002)(54356999)(25786009)(6246003)(50986999)(189998001)(305945005)(6116002)(54906002)(7736002)(2900100001)(74316002)(5660300001)(122556002)(217873001);DIR:OUT;SFP:1102;SCL:1;SRVR:HE1P101MB0188;H:HE1P101MB0188.NAMP101.PROD.OUTLOOK.COM;FPR:;SPF:None;MLV:ovrnspm;PTR:InfoNoRecords;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Apr 2017 13:24:49.6838 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 15ccb6d1-d335-4996-b6f9-7b6925f08121 X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1P101MB0188 X-OriginatorOrg: ge.com Subject: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-05_11:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1704050118 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v35DPTmq007147 > -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: 29 March 2017 05:07 > To: Han, Nandor (GE Healthcare) > Cc: Alexandre Courbot ; Rob Herring ; Mark Rutland > ; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > Malinen, Semi (GE Healthcare) > 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 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 > > Signed-off-by: Semi Malinen > (...) > > +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. > > + > > + 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) > > + > > + 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. :) > > + /* 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 > > +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