Date: Sat, 30 Apr 2011 11:07:05 +0100
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
To: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Cc: linux-kernel@vger.kernel.org,
        Jonas Fonseca <jonas.fonseca@savoirfairelinux.com>,
        platform-driver-x86@vger.kernel.org, linux-serial@vger.kernel.org,
        lm-sensors@lm-sensors.org
Subject: Re: [RFC 1/5] platform-drivers-x86: add support for Technologic
 Systems TS-5xxx detection
Message-ID: <20110430110705.5ca3376b@lxorguk.ukuu.org.uk>
In-Reply-To: <1304115712-5299-2-git-send-email-vivien.didelot@savoirfairelinux.com>
References: <1304115712-5299-1-git-send-email-vivien.didelot@savoirfairelinux.com>
	<1304115712-5299-2-git-send-email-vivien.didelot@savoirfairelinux.com>
Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAFVBMVEWysKsSBQMIAwIZCwj///8wIhxoRDXH9QHCAAABeUlEQVQ4jaXTvW7DIBAAYCQTzz2hdq+rdg494ZmBeE5KYHZjm/d/hJ6NfzBJpp5kRb5PHJwvMPMk2L9As5Y9AmYRBL+HAyJKeOU5aHRhsAAvORQ+UEgAvgddj/lwAXndw2laEDqA4x6KEBhjYRCg9tBFCOuJFxg2OKegbWjbsRTk8PPhKPD7HcRxB7cqhgBRp9Dcqs+B8v4CQvFdqeot3Kov6hBUn0AJitrzY+sgUuiA8i0r7+B3AfqKcN6t8M6HtqQ+AOoELCikgQSbgabKaJW3kn5lBs47JSGDhhLKDUh1UMipwwinMYPTBuIBjEclSaGZUk9hDlTb5sUTYN2SFFQuPe4Gox1X0FZOufjgBiV1Vls7b+GvK3SU4wfmcGo9rPPQzgIabfj4TYQo15k3bTHX9RIw/kniir5YbtJF4jkFG+dsDK1IgE413zAthU/vR2HVMmFUPIHTvF6jWCpFaGw/A3qWgnbxpSm9MSmY5b3pM1gvNc/gQfwBsGwF0VCtxZgAAAAASUVORK5CYII=
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3598
Lines: 127

> + * ts5xxx_sbcinfo_set() - set the SBC info structure with the current SBC's info
> + * @sbcinfo:		structure containing SBC info to set.
> + */
> +void ts5xxx_sbcinfo_set(struct ts5xxx_sbcinfo *sbcinfo)
> +{
> +	memcpy(sbcinfo, &ts_sbcinfo, sizeof(*sbcinfo));
> +}
> +EXPORT_SYMBOL(ts5xxx_sbcinfo_set);

You export this but it's not clear who for or why - and there is also no
locking, so it seems to be an internal interface ?


> +/**
> + * ts_find_sbc_config() - find a SBC configuration from an id
> + * @id:			ID of the board to find.
> + */
> +static inline struct ts_sbc_config *ts_find_sbc_config(u8 id)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ts_sbcs_configs); i++)
> +		if (id == ts_sbcs_configs[i].id)
> +			return &ts_sbcs_configs[i];
> +
> +	return NULL;
> +}

gcc will figure out inlines for you in static stuff and normally very well

> +
> +/**
> + * ts_sbcinfo_detect() - detect the TS board
> + * @sbcinfo:		structure where to store the detected board's info.
> + */
> +static int ts_sbcinfo_detect(struct ts5xxx_sbcinfo *sbcinfo)
> +{
> +	u8 temp;
> +	struct ts_sbc_config *sbc;
> +	int ret = 0;
> +
> +	memset(sbcinfo, 0, sizeof(*sbcinfo));
> +
> +	if (!request_region(IOADDR_SBCID, 4, "TS-SBC"))
> +		return -EBUSY;
> +
> +	temp = inb(IOADDR_SBCID);
> +	/* If it is a 3x00 SBC only match against the first 3 bits */
> +	if (temp & 0x07)
> +		temp &= 0x07;

So if this is compiled into a kernel we blindly inb this address and some
platform just crashed on boot. Is this board like other embedded ones in
that there is some safe way to check if its such a board first ?

> +
> +	sbc = ts_find_sbc_config(temp);
> +	if (!sbc) {
> +		ret = -ENODEV;

Assuming you've indentified the board properly this case might be worth a
pr_err giving the fact it seemed to be a TS-5xxx, the config number and
that it is unknown - that will help anyone with future boards realise
their config isn't supported.


> +
> +/**
> + * ts_sbcinfo_proc_read() - function called when a read access is done on
> + *                          /proc/ts-sbcinfo
> + */
> +static int ts_sbcinfo_proc_read(char *page, char **start, off_t off,
> +				int count, int *eof, void *data)
> +{
> +	int to_copy = (procfs_buffer_size <= count) ?
> +		procfs_buffer_size - off : count;

> +
> +	if (off + to_copy >= procfs_buffer_size) {
> +		to_copy = procfs_buffer_size - off;
> +		*eof = 1;
> +	}

Suppose off is 0x7FFFFFFF and count = 100

	to_copy = 100
	off + to_copy >= procfs_buffer_size   [off_t may be 32bit]

	100 + 0x7FFFFFFF (wraps) < buffer size


> +static int __init ts5xxx_sbcinfo_init(void)
> +{
> +	int err;
> +
> +	err = ts_sbcinfo_detect(&ts_sbcinfo);
> +	if (err < 0) {
> +		printk(KERN_ERR KBUILD_MODNAME
> +		       ": Failed to get SBC information\n");
> +		return err;
> +	}

Well that will depend why and probably the caller can give the best
information including silence if the board is just not a ts-5xxx

> +	proc_entry = create_proc_read_entry(PROCFS_NAME, S_IRUGO, NULL,
> +					    ts_sbcinfo_proc_read, 0);
> +	if (proc_entry == NULL) {
> +		printk(KERN_ERR KBUILD_MODNAME
> +		       ": Failed to create proc entry\n");

pr_err

> +		return -ENOMEM;
> +	}
> +
> +	procfs_buffer_size = ts_sbcinfo_init_buffer(procfs_buffer, &ts_sbcinfo);
> +	printk(KBUILD_MODNAME ": TS SBC's info driver loaded.\n");

Printk not needed

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/