On Mon, May 08, 2017 at 12:31:52PM +0100, Chris Dew wrote:
> First, apologies, this is my first Linux kernel patch in at least 15 years.
> While I have spent a few hours reading various pieces of documentation about
> the Linux kernel development processes, I have probably missed some critical
> files entirely.
>
> Also, there are probably a few coding issues in this patch.
>
> This driver has only been tested to work on our custom ARM board, where we
> give it the following device tree info:
>
> arch/arm/boot/dts/imx6qdl-smarcfimx6.dtsi:
>
> ...
> &i2c1 {
> ...
> pca9570: pca9570@24 {
> compatible = "pca9570";
> reg = <0x24>;
> gpio-controller;
> };
> };
>
> I have some questions:
>
> - Is the "value", with which struct gpio_chip.set is called, guaranteed to
> be 0 or 1?
>
> - Do I need to implement any form of locking around this driver, or is that
> done for me in layers above?
>
> - The pca9571 is an 8-bit version of this chip. Is leaving pca9571 support
> to a later patch the best idea? (I do not have one to test, nor any way to
> test one, at the moment.)
These are all good questions to ask on the gpio mailing list. Use
scripts/get_maintainer.pl to determine what people, and lists, to cc:
for a patch. That will get you to the right people.
> - This code was written and tested against a vendor-modified 4.1.15 kernel
> tree. This patch is made against the stable 4.9.9. To make it compile
> against the newer kernel I had to replace references to struct gpio_chip.dev
> with struct gpio_chip.parent, as that struct member had been renamed between
> kernel versions.
4.9.9 is really old as well, you need to make a patch against Linus's
tree at the least, and linux-next is usually much better. We can't go
back in time and add new drivers to old kernels.
> - Should this driver be in the staging directory?
Why would it belong there? What needs to be done to it to get it out of
staging? Hint, you need a TODO file for a staging driver listing that
type of thing...
Minor comments on your patch:
You need a proper changelog and signed-off-by: line, please read
Documentation/SubmittingPatches for all of the info you need.
>
>
> ---
> drivers/gpio/Kconfig | 7 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-pca9570.c | 253
> ++++++++++++++++++++++++++++++++++++++++++++
Your patch is line-wapped :(
> include/linux/i2c/pca9570.h | 22 ++++
> 4 files changed, 283 insertions(+)
> create mode 100644 drivers/gpio/gpio-pca9570.c
> create mode 100644 include/linux/i2c/pca9570.h
Why do you need a .h file?
> +/*
> + * Driver for pca9570 I2C GPO expanders
> + * Note: 9570 is at 0x24 - these value must be set
> + * in device tree.
> + *
> + * Copyright (C) 2017 Chris Dew, Thorcom Systems Ltd.
> + *
> + * Derived from drivers/gpio/gpio-max732x.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
Do you really mean "any later version"?
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
Always run scripts/checkpatch.pl on your patch, so you don't get grumpyu
maintainers telling you to run scripts/checkpatch.pl on your patch...
> +out_failed:
> + if (chip->client)
> + i2c_unregister_device(chip->client);
> +
> + return -1;
Don't make up random error values, use proper -E codes instead.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
