Re: [PATCH v1 2/3] SFI: store GPIO table and export lookup function

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, 2013-12-05 at 15:07 -0800, David Cohen wrote:
> Hi Andy,
> 
> On 12/05/2013 08:36 AM, Andy Shevchenko wrote:
> > We have to provide a mechanism to retrive GPIO information from SFI. For this
> > we store SFI GPIO table and provide the lookup function
> > sfi_gpio_get_entry_by_name() that will be used later in GPIO framework.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/sfi/Makefile   |   2 +-
> >  drivers/sfi/sfi_core.c |   6 +++
> >  drivers/sfi/sfi_core.h |   3 ++
> >  drivers/sfi/sfi_gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sfi.h    |   8 ++++
> >  5 files changed, 141 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/sfi/sfi_gpio.c
> >
> > diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> > index 2343732..dc011db 100644
> > --- a/drivers/sfi/Makefile
> > +++ b/drivers/sfi/Makefile
> > @@ -1,3 +1,3 @@
> >  obj-y	+= sfi_acpi.o
> >  obj-y	+= sfi_core.o
> > -
> > +obj-y	+= sfi_gpio.o
> > diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> > index 296db7a..e9ff6f0 100644
> > --- a/drivers/sfi/sfi_core.c
> > +++ b/drivers/sfi/sfi_core.c
> > @@ -512,6 +512,12 @@ void __init sfi_init_late(void)
> >  	syst_va = sfi_map_memory(syst_pa, length);
> >  
> >  	sfi_acpi_init();
> > +
> > +	/*
> > +	 * Parsing GPIO table first, since the DEVS table will need this table
> > +	 * to map the pin name to the actual pin.
> > +	 */
> > +	sfi_gpio_init();
> >  }
> >  
> >  /*
> > diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> > index 1d5cfe8..18c663d 100644
> > --- a/drivers/sfi/sfi_core.h
> > +++ b/drivers/sfi/sfi_core.h
> > @@ -79,3 +79,6 @@ struct sfi_table_header *sfi_get_table(struct sfi_table_key *key);
> >  extern void sfi_put_table(struct sfi_table_header *table);
> >  extern struct sfi_table_attr __init *sfi_sysfs_install_table(u64 pa);
> >  extern void __init sfi_acpi_sysfs_init(void);
> > +
> > +/* sfi_gpio.c */
> > +int sfi_gpio_init(void);
> > diff --git a/drivers/sfi/sfi_gpio.c b/drivers/sfi/sfi_gpio.c
> > new file mode 100644
> > index 0000000..677368d
> > --- /dev/null
> > +++ b/drivers/sfi/sfi_gpio.c
> > @@ -0,0 +1,123 @@
> > +/* sfi_gpio.c Simple Firmware Interface - GPIO extensions */
> > +
> > +/*
> > +
> > +  This file is provided under a dual BSD/GPLv2 license.  When using or
> > +  redistributing this file, you may do so under either license.
> > +
> > +  GPL LICENSE SUMMARY
> > +
> > +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> > +
> > +  This program is free software; you can redistribute it and/or modify
> > +  it under the terms of version 2 of the GNU General Public License as
> > +  published by the Free Software Foundation.
> > +
> > +  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., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> > +  The full GNU General Public License is included in this distribution
> > +  in the file called LICENSE.GPL.
> > +
> > +  BSD LICENSE
> > +
> > +  Copyright(c) 2013 Intel Corporation. All rights reserved.
> > +
> > +  Redistribution and use in source and binary forms, with or without
> > +  modification, are permitted provided that the following conditions
> > +  are met:
> > +
> > +    * Redistributions of source code must retain the above copyright
> > +      notice, this list of conditions and the following disclaimer.
> > +    * Redistributions in binary form must reproduce the above copyright
> > +      notice, this list of conditions and the following disclaimer in
> > +      the documentation and/or other materials provided with the
> > +      distribution.
> > +    * Neither the name of Intel Corporation nor the names of its
> > +      contributors may be used to endorse or promote products derived
> > +      from this software without specific prior written permission.
> > +
> > +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > +  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > +  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > +  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > +  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > +  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > +  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > +  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > +  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > +
> > +*/
> > +
> > +#define KMSG_COMPONENT "SFI"
> > +#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> > +
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/export.h>
> > +#include <linux/sfi.h>
> > +
> > +#include "sfi_core.h"
> > +
> > +static struct sfi_gpio_table_entry *sfi_gpio_table;
> > +static int sfi_gpio_num_entry;
> > +
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> > +{
> > +	struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
> > +	int i;
> > +
> > +	if (!pentry)
> > +		return NULL;
> 
> If this function is called prior to sfi_gpio_table initialization, it
> will have
> same result as if the gpio name doesn't exist.
> For development sanity, how about return ERR_PTR(-EBUSY) or
> ERR_PTR(-EAGAIN) if it's too early?

Okay, I switch to error pointer.

> 
> > +
> > +	for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
> > +		if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
> > +			return pentry;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(sfi_gpio_get_entry_by_name);
> > +
> > +static int __init sfi_gpio_parse(struct sfi_table_header *table)
> > +{
> > +	struct sfi_table_simple *sb;
> > +	struct sfi_gpio_table_entry *pentry;
> > +	int num, i;
> > +
> > +	if (sfi_gpio_table)
> > +		return -EBUSY;
> 
> It can't ever happen. This is a static function called once in this file.
> IMO kernel doesn't need such kind of tests :)

Will fix it.

> 
> > +
> > +	sb = container_of(table, struct sfi_table_simple, header);
> > +
> > +	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
> > +	pentry = (struct sfi_gpio_table_entry *)sb->pentry;
> > +
> > +	sfi_gpio_table = kmemdup(pentry, num * sizeof(*pentry), GFP_KERNEL);
> > +	if (!sfi_gpio_table)
> > +		return -ENOMEM;
> > +
> > +	sfi_gpio_num_entry = num;
> > +
> > +	pr_debug("GPIO pin info:\n");
> > +	for (i = 0; i < num; i++, pentry++)
> > +		pr_debug("GPIO [%2d] chip = %16.16s, name = %16.16s, pin=%d\n",
> > +			 i, pentry->controller_name, pentry->pin_name,
> > +			 pentry->pin_no);
> > +
> > +	return 0;
> > +}
> > +
> > +int __init sfi_gpio_init(void)
> > +{
> > +	return sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_gpio_parse);
> > +}
> > diff --git a/include/linux/sfi.h b/include/linux/sfi.h
> > index d9b436f..510e74d 100644
> > --- a/include/linux/sfi.h
> > +++ b/include/linux/sfi.h
> > @@ -185,6 +185,8 @@ static inline void disable_sfi(void)
> >  	sfi_disabled = 1;
> >  }
> >  
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name);
> > +
> >  #else /* !CONFIG_SFI */
> >  
> >  static inline void sfi_init(void)
> > @@ -204,6 +206,12 @@ static inline int sfi_table_parse(char *signature, char *oem_id,
> >  	return -1;
> >  }
> >  
> > +static inline
> > +struct sfi_gpio_table_entry *sfi_gpio_get_entry_by_name(const char *name)
> > +{
> > +	return NULL;
> 
> I'd suggest something more meaningful like ERR_PTR(-ENODEV).
> Again, for development sanity.

Okay.

> 
> Br, David Cohen
> 
> > +}
> > +
> >  #endif /* !CONFIG_SFI */
> >  
> >  #endif /*_LINUX_SFI_H*/
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy