Re: [PATCH] Add tsc2007 based touchscreen driver.

[Kwangwoo Lee]

On Wed, Dec 3, 2008 at 9:01 PM, Felipe Balbi <felipe.balbi@xxxxxxxxx> wrote:
...
>> > +
>> > +static int tsc2007_xfer(void *tsc, unsigned char cmd)
>> > +{
>> > +       struct tsc2007    *ts     = tsc;
>> > +       struct i2c_client *client = ts->client;
>> > +
>> > +       unsigned char  rbuf[2];
>> > +       unsigned short val;
>> > +       int result;
>> > +
>> > +       result = i2c_master_send(client, &cmd, 1);
>> > +       if (result != 1) {
>> > +               dev_err(&client->dev, "send failed, cmd 0x%x\n", cmd);
>> > +               goto cmd_fail;
>> > +       }
>> > +
>> > +       result = i2c_master_recv(client, rbuf, 2);
>> > +       if (result != 2) {
>> > +               dev_err(&client->dev, "recv failed, cmd 0x%x\n", cmd);
>> > +               goto cmd_fail;
>> > +       }
>> > +
>> > +       rbuf[1] = (rbuf[1] >> 4) | ((rbuf[0] & 0x0f) << 4);
>> > +       rbuf[0] = rbuf[0] >> 4;
>> > +
>> > +       val = *((unsigned short *) rbuf);
>> > +       val = be16_to_cpu(val);
>> > +
>> > +       dev_dbg(&client->dev, "cmd [0x%x], rbuf [0x%x, 0x%x] => val [%u]\n",
>> > +               cmd, rbuf[0], rbuf[1], val);
>> > +
>> > +       return (int) val;
>> > +
>> > +cmd_fail:
>> > +       return -EIO;
>> > +}
>
> so you really need to go that low level ?? Did you try
> i2c_smbus_write_*() ??
>

Because the platform which I use did not implement smbus_xfer yet,
it only implemented  master_xfer in struct i2c_algorithm.
I can add smbus_xfer for my platform. I have one question and
I know that smbus is a subset from the I2C protocol.
Is smbus_xfer method better than master_xfer or is there any
special reason to use smbus_xfer?

>> > +
>> > +static int tsc2007_probe(struct i2c_client *client,
>> > +                       const struct i2c_device_id *id)
>> > +{
>> > +       struct i2c_adapter      *adapter = to_i2c_adapter(client->dev.parent);
>
> why ??

It's for i2c_check_functionality(). This function requried struct
i2c_adapter as an argument.

>> > +       struct tsc2007          *ts;
>> > +       struct tsc2007_platform_data *pdata;
>> > +       struct input_dev        *input_dev;
>> > +       int                     err;
>> > +
>> > +       pdata = client->dev.platform_data;
>> > +       if (pdata == NULL) {
>> > +               dev_err(&client->dev, "platform data is required!\n");
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       if (!i2c_check_functionality(adapter,
>> > +                       I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
>> > +               return -EIO;
>
> should not be in this driver, I'd say.

I saw that some I2C driver uses this function, for example - max6875.c.
Is i2c_check_functionality() not necessary for a I2C client driver?

>> > +       ts->irq = client->irq;
>> > +       if (request_irq(ts->irq, tsc2007_irq, 0,
>> > +                       client->dev.driver->name, ts)) {
>> > +               dev_err(&client->dev, "irq %d busy?\n", ts->irq);
>> > +               err = -EBUSY;
>> > +               goto err_free_mem;
>
> would be better to:
>
> err = request_irq(...)
> if (err < 0) {
>        dev_err(..)
>        ...
> }
>
> Then we see the error request_irq() returned, which is more useful.

Thank you very much for your kind comments.

> Would be nice to get comments from Jean Delvare and Ben Dooks as well.

I also want to get more comments. Thank you, again.

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