| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20071015183107.GA4361@loki.buserror.net>
Date: Mon, 15 Oct 2007 13:31:07 -0500
From: Scott Wood <scottwood@...escale.com>
To: Jochen Friedrich <jochen@...am.de>
Cc: "linuxppc-embedded@...abs.org" <linuxppc-embedded@...abs.org>,
linux-kernel@...r.kernel.org, i2c@...sensors.org,
Carsten Juttner <carjay@....net>
Subject: Re: [PATCH take2] [POWERPC] i2c: adds support for i2c bus on 8xx
On Mon, Oct 15, 2007 at 01:10:12PM +0200, Jochen Friedrich wrote:
> Using the port of 2.4 code from Vitaly Bordug <vitb@...nel.crashing.org>
> and the actual algorithm used by the i2c driver of the DBox code on
> cvs.tuxboc.org from Tmbinc, Gillem (htoa@....net). Renamed
> i2c-algo-8xx.c to i2c-algo-cpm.c and i2c-rpx.c to i2c-8xx.c. Added
> original i2c-rpx.c as i2c-8xx-ppc.c for pre-OF (arch ppc) devices.
Do we really need to be adding features for arch/ppc at this point? It'll
be going away in June. arch/ppc-specific things outside of arch/ppc itself
will also be more likely to be missed in the removal.
Also, please post inline rather than as an attachment; attachments are
harder to quote in a reply.
> diff --git a/arch/powerpc/boot/dts/mpc885ads.dts b/arch/powerpc/boot/dts/mpc885ads.dts
> index 8848e63..a526c02 100644
> --- a/arch/powerpc/boot/dts/mpc885ads.dts
> +++ b/arch/powerpc/boot/dts/mpc885ads.dts
> @@ -213,6 +213,15 @@
> fsl,cpm-command = <0080>;
> linux,network-index = <2>;
> };
> +
> + i2c@860 {
> + device_type = "i2c";
No device_type.
> + compatible = "fsl-i2c-cpm";
Should be fsl,cpm-i2c. Is cpm2 i2c the same? If not, it should be
fsl,cpm1-i2c. It's probably best to specify it anyway, along with
fsl,mpc885-i2c.
> diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c b/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> index 2cf1b6a..350018b 100644
> --- a/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> +++ b/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> @@ -175,6 +175,12 @@ static void __init init_ioports(void)
>
> /* Set FEC1 and FEC2 to MII mode */
> clrbits32(&mpc8xx_immr->im_cpm.cp_cptr, 0x00000180);
> +
> +#ifdef CONFIG_I2C_8XX
> + setbits32(&mpc8xx_immr->im_cpm.cp_pbpar, 0x00000030);
> + setbits32(&mpc8xx_immr->im_cpm.cp_pbdir, 0x00000030);
> + setbits16(&mpc8xx_immr->im_cpm.cp_pbodr, 0x0030);
> +#endif
Please add this to mpc885ads_pins, rather than poking the registers
directly. The relevant lines are:
{CPM_PORTB, 26, CPM_PIN_OUTPUT},
{CPM_PORTB, 27, CPM_PIN_OUTPUT},
> +static const char *i2c_regs = "regs";
> +static const char *i2c_pram = "pram";
> +static const char *i2c_irq = "interrupt";
> +
> +static int __init fsl_i2c_cpm_of_init(void)
> +{
> + struct device_node *np;
> + unsigned int i;
> + struct platform_device *i2c_dev;
> + int ret;
> +
> + for (np = NULL, i = 0;
> + (np = of_find_compatible_node(np, "i2c", "fsl-i2c-cpm")) != NULL;
> + i++) {
Why not just make an of_platform device instead of this glue code?
> diff --git a/drivers/i2c/algos/Kconfig b/drivers/i2c/algos/Kconfig
> index 014dfa5..7a8200e 100644
> --- a/drivers/i2c/algos/Kconfig
> +++ b/drivers/i2c/algos/Kconfig
> @@ -14,6 +14,18 @@ config I2C_ALGOBIT
> This support is also available as a module. If so, the module
> will be called i2c-algo-bit.
>
> +config I2C_ALGOCPM
> + tristate "I2C MPC8xx CPM and MPC8260 CPM2 interfaces"
> + depends on ( 8xx || 8260 ) && I2C
> + help
> + CPM I2C Algorithm, supports the CPM I2C interface for mpc8xx
> + and mpc8260 CPUs. Say Y if you own a CPU of this class
What if I'm just borrowing it? :-)
> +static irqreturn_t cpm_iic_interrupt(int irq, void *dev_id)
> +{
> + struct i2c_adapter *adap;
> + struct i2c_algo_cpm_data *cpm;
> + i2c8xx_t *i2c;
> + int i;
> +
> + adap = (struct i2c_adapter *) dev_id;
> + cpm = adap->algo_data;
> + i2c = cpm->i2c;
> +
> + /* Clear interrupt.
> + */
> + i = in_8(&i2c->i2c_i2cer);
> + out_8(&i2c->i2c_i2cer, i);
> +
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "Interrupt: %x\n", i);
> +
> + /* Get 'me going again.
> + */
> + wake_up_interruptible(&cpm->iic_wait);
> +
> + return IRQ_HANDLED;
> +}
Should only return IRQ_HANDLED if the event register was non-zero.
> +static int cpm_iic_init(struct i2c_adapter *adap)
> +{
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + iic_t *iip = cpm->iip;
> + i2c8xx_t *i2c = cpm->i2c;
> + unsigned char brg;
> + int ret, i;
> +
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "cpm_iic_init()\n");
Can you fold the if statement into a macro? Or just do a raw dev_dbg with
no test, like most drivers do.
> + ret = 0;
> + init_waitqueue_head(&cpm->iic_wait);
> + mutex_init(&cpm->iic_mutex);
> + /* Initialize the parameter ram.
> + * We need to make sure many things are initialized to zero,
> + * especially in the case of a microcode patch.
> + */
> + out_be32(&iip->iic_rstate, 0);
> + out_be32(&iip->iic_rdp, 0);
> + out_be16(&iip->iic_rbptr, 0);
> + out_be16(&iip->iic_rbc, 0);
> + out_be32(&iip->iic_rxtmp, 0);
> + out_be32(&iip->iic_tstate, 0);
> + out_be32(&iip->iic_tdp, 0);
> + out_be16(&iip->iic_tbptr, 0);
> + out_be16(&iip->iic_tbc, 0);
> + out_be32(&iip->iic_txtmp, 0);
This appears to be done twice, here and in cpm_reset_iic_params.
> + u16 v = mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_INIT_TRX) | CPM_CR_FLG;
Should use fsl,cpm-command rather than hardcoded constants.
> + /* Select an arbitrary address. Just make sure it is unique.
> + */
> + out_8(&i2c->i2c_i2add, 0xfe);
It's a 7-bit address... and are you sure that 0x7e is unique? Does this
driver even support slave operation?
> + /* Make clock run at 60 kHz.
> + */
> + /* brg = ppc_proc_freq / (32 * 2 * 60000) - 3; */
> + brg = 7;
> + out_8(&i2c->i2c_i2brg, brg);
Why is this hardcoded?
> + out_8(&i2c->i2c_i2mod, 0x00);
> + out_8(&i2c->i2c_i2com, 0x01); /* Master mode */
> +
> + /* Disable interrupts.
> + */
> + out_8(&i2c->i2c_i2cmr, 0);
> + out_8(&i2c->i2c_i2cer, 0xff);
> +
> + /* Allocate TX and RX buffers */
> + for (i = 0; i < CPM_MAXBD; i++) {
> + cpm->rxbuf[i] = (unsigned char *)dma_alloc_coherent(
> + NULL, CPM_MAX_READ + 1, &cpm->rxdma[i], GFP_KERNEL);
> + if (!cpm->rxbuf[i]) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + cpm->txbuf[i] = (unsigned char *)dma_alloc_coherent(
> + NULL, CPM_MAX_READ + 1, &cpm->txdma[i], GFP_KERNEL);
> + if (!cpm->txbuf[i]) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + }
> +
> + /* Install interrupt handler.
> + */
> + if (request_irq(cpm->irq, cpm_iic_interrupt, 0, "8xx_i2c", adap)) {
cpm-i2c, not 8xx.
> + ret = -EIO;
> + goto out;
> + }
> +
> + return 0;
> +out:
> + for (i = 0; i < CPM_MAXBD; i++) {
> + if (cpm->rxbuf[i]) dma_free_coherent(NULL, CPM_MAX_READ + 1,
> + cpm->rxbuf[i], cpm->rxdma[i]);
> + if (cpm->txbuf[i]) dma_free_coherent(NULL, CPM_MAX_READ + 1,
> + cpm->txbuf[i], cpm->txdma[i]);
> + }
Please put a newline between the if test and the if body.
> +static void force_close(struct i2c_adapter *adap)
> +{
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + i2c8xx_t *i2c = cpm->i2c;
> + if (cpm->reloc == 0) { /* micro code disabled */
> + cpm8xx_t *cp = cpm->cp;
> + u16 v =
> + mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_CLOSE_RXBD) | CPM_CR_FLG;
Why only if there's no microcode relocation?
> +static void cpm_parse_message(struct i2c_adapter *adap, struct i2c_msg *pmsg,
> + int num, int tx, int rx)
> +{
> + cbd_t *tbdf, *rbdf;
> + u_char addr;
> + u_char *tb;
> + u_char *rb;
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + iic_t *iip = cpm->iip;
> + int i, dscan;
> +
> + tbdf = (cbd_t *) cpm_dpram_addr(in_be16(&iip->iic_tbase));
> + rbdf = (cbd_t *) cpm_dpram_addr(in_be16(&iip->iic_rbase));
> +
> + /* This chip can't do zero lenth writes. However, the i2c core uses
s/lenth/length/
> + if (pmsg->flags & I2C_M_RD) {
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "rx sc %04x, rx sc %04x\n",
> + tbdf[tx].cbd_sc, rbdf[rx].cbd_sc);
> + if (tbdf[tx].cbd_sc & BD_SC_NAK) {
> + if (cpm_debug)
> + dev_dbg(&adap->dev, "IIC read; no ack\n");
> + if (pmsg->flags & I2C_M_IGNORE_NAK)
> + return 0;
> + else
> + return -EREMOTEIO;
s/EREMOTEIO/EIO/
> +static int cpm_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct i2c_algo_cpm_data *cpm = adap->algo_data;
> + i2c8xx_t *i2c = cpm->i2c;
> + iic_t *iip = cpm->iip;
> + struct i2c_msg *pmsg, *rmsg;
> + int ret, i;
> + int tptr;
> + int rptr;
> + cbd_t *tbdf, *rbdf;
> +
> + if (num > CPM_MAXBD)
> + return -EREMOTEIO;
> +
> + /* Check if we have any oversized READ requests */
> + for (i = 0; i < num; i++) {
> + pmsg = &msgs[i];
> + if (pmsg->len >= CPM_MAX_READ)
> + return -EREMOTEIO;
> + }
s/EREMOTEIO/EINVAL/
> +
> + mutex_lock(&cpm->iic_mutex);
> +
> + /* check for and use a microcode relocation patch */
> + if (cpm->reloc)
> + cpm_reset_iic_params(cpm);
On every transfer?
> + while (tptr < num) {
> + /* Check for outstanding messages */
> + dev_dbg(&adap->dev, "test ready.\n");
> + if (!(tbdf[tptr].cbd_sc & BD_SC_READY)) {
> + dev_dbg(&adap->dev, "ready.\n");
> + rmsg = &msgs[tptr];
> + ret = cpm_check_message(adap, rmsg, tptr, rptr);
> + tptr++;
> + if (rmsg->flags & I2C_M_RD)
> + rptr++;
> + if (ret) {
> + force_close(adap);
> + mutex_unlock(&cpm->iic_mutex);
> + return -EREMOTEIO;
s/-EREMOTEIO/ret/
> +config I2C_8XX
> + tristate "MPC8xx CPM with Open Firmware"
It's not really Open Firmware... just a flat device tree.
> diff --git a/drivers/i2c/busses/i2c-8xx.c b/drivers/i2c/busses/i2c-8xx.c
> new file mode 100644
> index 0000000..c38b52e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-8xx.c
> @@ -0,0 +1,170 @@
> +/*
> + * Embedded Planet RPX Lite MPC8xx CPM I2C interface.
> + * Copyright (c) 1999 Dan Malek (dmalek@....net).
> + *
> + * moved into proper i2c interface;
> + * Brad Parker (brad@...ltoe.com)
> + *
> + * (C) 2007 Montavista Software, Inc.
> + * Vitaly Bordug <vitb@...nel.crashing.org>
> + *
> + * RPX lite specific parts of the i2c interface
> + * Update: There actually isn't anything RPXLite-specific about this module.
> + * This should work for most any 8xx board. The console messages have been
> + * changed to eliminate RPXLite references.
So let's change the title at the top of the file...
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/stddef.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-cpm.h>
> +#include <linux/of_device.h>
Why are you including platform_device, and running glue code in fsl_soc.c,
if this is an of_platform driver?
> +#include <linux/of_platform.h>
> +#include <asm/mpc8xx.h>
> +#include <asm/commproc.h>
> +#include <asm/fs_pd.h>
> +
> +
> +struct m8xx_i2c {
> + char *base;
> + struct of_device *ofdev;
> + struct i2c_adapter adap;
> + struct i2c_algo_cpm_data *algo_8xx;
> +};
> +
> +static struct i2c_algo_cpm_data m8xx_data;
> +
> +static const struct i2c_adapter m8xx_ops = {
> + .owner = THIS_MODULE,
> + .name = "i2c-8xx",
> + .id = I2C_HW_MPC8XX_EPON,
> + .algo_data = &m8xx_data,
> + .dev.parent = &platform_bus,
> + .class = I2C_CLASS_HWMON,
> +};
It's not on the platform bus; it's on the soc of_platform bus. Why is this
embedded in the i2c_adapter struct anyway? i2c_adapter should hold a
pointer to whatever the probe function gives you.
> + data->irq = irq_of_parse_and_map(np, 0);
> + if (data->irq < 0)
> + return -EINVAL;
> +
> + if (of_address_to_resource(np, 1, &r))
> + return -EINVAL;
> +
> + data->iip = ioremap(r.start, r.end - r.start + 1);
> + if (data->iip == NULL)
> + return -EINVAL;
> +
> + /* Check for and use a microcode relocation patch.
> + */
> + data->reloc = data->iip->iic_rpbase;
> + if (data->reloc)
> + data->iip = (iic_t *)&cp->cp_dpmem[data->iip->iic_rpbase];
> +
> + if (of_address_to_resource(np, 0, &r))
> + return -EINVAL;
> +
> + data->i2c = ioremap(r.start, r.end - r.start + 1);
Use of_iomap.
> + if (data->i2c == NULL)
> + return -EINVAL;
> +
> + /* Allocate space for two transmit and two receive buffer
> + * descriptors in the DP ram.
> + */
> + data->dp_addr = cpm_dpalloc(sizeof(cbd_t) * 4, 8);
Please use the new muram_dpalloc name.
> + if (!data->dp_addr)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int i2c_8xx_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int result;
> + struct m8xx_i2c *i2c;
> +
> + i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
> +
> + i2c->ofdev = ofdev;
> + i2c->algo_8xx = &m8xx_data;
> +
> + m8xx_iic_init(i2c);
> +
> + dev_set_drvdata(&ofdev->dev, i2c);
> +
> + i2c->adap = m8xx_ops;
> + i2c_set_adapdata(&i2c->adap, i2c);
> + i2c->adap.dev.parent = &ofdev->dev;
> +
> + result = i2c_cpm_add_bus(&i2c->adap);
> + if (result < 0) {
> + printk(KERN_ERR "i2c-8xx: Unable to register with I2C\n");
> + kfree(i2c);
> + }
> +
> + return result;
> +}
> +
> +static int i2c_8xx_remove(struct of_device *ofdev)
> +{
> + struct m8xx_i2c *i2c = dev_get_drvdata(&ofdev->dev);
> +
> + i2c_cpm_del_bus(&i2c->adap);
> + dev_set_drvdata(&ofdev->dev, NULL);
> +
> + kfree(i2c);
> + return 0;
> +}
> +
> +static struct of_device_id i2c_8xx_match[] = {
> + {
> + .type = "i2c",
> + .compatible = "fsl,i2c-cpm",
> + },
> + {},
> +};
> +
Why is an 8xx driver matching all i2c cpm (i.e. what about cpm2)?
-Scot
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists