Jean Delvare wrote:
> Hi Marc,
>
> On Sat, 17 Mar 2007 12:29:24 +0000, Ralf Baechle wrote:
> > ----- Forwarded message from Marc St-Jean <stjeanma@xxxxxxxxxxxxxx>
> -----
> >
> > On Fri, Mar 16, 2007 at 03:40:41PM -0600, Marc St-Jean wrote:
> > From: Marc St-Jean <stjeanma@xxxxxxxxxxxxxx>
> > Date: Fri, 16 Mar 2007 15:40:41 -0600
> > To: akpm@xxxxxxxxxxxxxxxxxxxx
> > Subject: [PATCH 9/12] drivers: PMC MSP71xx LED driver
> > Cc: linux-mips@xxxxxxxxxxxxxx
> >
> > [PATCH 9/12] drivers: PMC MSP71xx LED driver
> >
> > Patch to add LED driver for the PMC-Sierra MSP71xx devices.
> >
> > Reposting patches as a single set at the request of akpm.
> > Only 9 of 12 will be posted at this time, 3 more to follow
> > when cleanups are complete.
> >
> > CCing linux-mips.org since minor changes have occured
> > during cleanup of driver patches for other lists.
>
> I don't have the time for a complete review, sorry, somebody else will
> have to do it. A few comments though:
>
> > diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> > index 87ee3ce..3bef46b 100644
> > --- a/drivers/i2c/chips/Kconfig
> > +++ b/drivers/i2c/chips/Kconfig
> > @@ -50,6 +50,15 @@ config SENSORS_PCF8574
> > These devices are hard to detect and rarely found on mainstream
> > hardware. If unsure, say N.
> >
> > +config SENSORS_PMCTWILED
>
> Please don't include "SENSORS" in the configuration option name, as the
> device doesn't include any sensors.
It will be in the next patch update.
> > + tristate "PMC Led-over-TWI driver"
> > + depends on I2C && PMC_MSP
> > + help
> > + The new VPE-safe backend driver for all the LEDs on the 7120
> platform.
> > +
> > + While you may build this as a module, it is recommended you
> build it
> > + into the kernel monolithic so all drivers may access it at
> all times.
>
> > diff --git a/drivers/i2c/chips/pmctwiled.c
> b/drivers/i2c/chips/pmctwiled.c
> > new file mode 100644
> > index 0000000..69845a5
> > --- /dev/null
> > +++ b/drivers/i2c/chips/pmctwiled.c
> > @@ -0,0 +1,524 @@
> > +/*
> > + * Special LED-over-TWI-PCA9554 driver for the PMC Sierra
> > + * Residential Gateway demo board (and potentially others).
> > (...)
> > +/* This function is called by i2c_probe */
> > +static int pmctwiled_detect(struct i2c_adapter *adapter,
> > + int address, int kind)
> > +{
> > + struct i2c_client *new_client = NULL; /* client structure */
>
> Please rename to just "client".
Ditto.
> > + struct pmctwiled_data *data = NULL; /* local data structure */
> > + int err = 0;
> > + int dev_id = address - PMCTWILED_BASEADDRESS;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > + goto exit;
> > +
> > + /*
> > + * For now, we presume we have a valid client. We now create the
> > + * client structure, even though we cannot fill it completely yet.
> > + */
> > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + if (!data) {
> > + err = -ENOMEM;
> > + goto exit;
> > + }
> > +
> > + new_client = &data->client;
> > + i2c_set_clientdata(new_client, data);
> > + new_client->addr = address;
> > + new_client->adapter = adapter;
> > + new_client->driver = &pmctwiled_driver;
> > + new_client->flags = 0;
>
> Not needed, kzalloc above did it for you.
Ditto.
> > +
> > + /*
> > + * Detection:
> > + * The pca9554 only has 4 registers (0-3).
> > + * All other reads should fail.
> > + */
> > + if (i2c_smbus_read_byte_data(new_client, 3) < 0 ||
> > + i2c_smbus_read_byte_data(new_client, 4) >= 0)
> > + goto exit_kfree;
> > +
> > + /* Found PCA9554 (probably) */
> > + strlcpy(new_client->name, "pca9554", I2C_NAME_SIZE);
> > + printk(KERN_WARNING
> > + "Detected PCA9554 I/O chip (device %d) at 0x%02x\n",
> > + dev_id, address);
>
> This is confusing. First you write a dedicated driver, then you use the
> generic name for the device name. This raises a question:
>
> Would it make sense to have generic PCA9554 driver, possibly
> implementing the new GPIO infrastructure, and have dedicated drivers
> such as this one build on top of that?
>
> Either way you have to be consistent, if you go with dedicated code,
> the i2c client name should not be generic.
I have renamed the driver "pmctwiled" and the client "pmctwiled_pca9554"
to help avoid confusion.
> > +
> > + /* Tell the I2C layer a new client has arrived */
> > + err = i2c_attach_client(new_client);
> > + if (err)
> > + goto exit_kfree;
> > +
> > + /*
> > + * Register this in the list of available devices, and set up the
> > + * initial state
> > + */
> > + i2c_smbus_write_byte_data(new_client, PCA9554_OUTPUT,
> > + i2c_smbus_read_byte_data(new_client,
> PCA9554_INPUT));
> > + i2c_smbus_write_byte_data(new_client, PCA9554_DIRECTION,
> > + msp_led_initial_input_state[dev_id]);
> > + pmctwiled_device[dev_id] = new_client;
>
> Here you assume that the PCA9554 devices can only live on one I2C bus,
> otherwise you could have two devices with the same address. But you do
> not check that this is actually the case anywhere in your code.
I've added code to verify it is our SoC the adapter in
pmctwiled_attach_adapter() before calling i2c_probe().
> This driver appears to be a good candidate to become a new-style i2c
> driver, where devices are instantiated explicitely by the platform code
> rather than probed for afterwards. The i2c-core changes allowing that
> will be in the next -mm kernel and will be merged in 2.6.22-rc1.
OK, I will look at it when it reaches l-m.o. Although the probe still
allows us to support several demo boards on the same device family
which could have a different number of clients.
Thanks,
Marc
> > +
> > + return 0;
> > +
> > +exit_kfree:
> > + kfree(data);
> > +exit:
> > + return err;
> > +}
>
> --
> Jean Delvare
>
