Messages in this thread | | | Date | Tue, 3 Aug 2010 20:02:17 +0300 | Subject | Re: [PATCH] mfd: add TPS6586x driver | From | Mike Rapoport <> |
| |
Hi Samuel,
On Mon, Aug 2, 2010 at 1:30 AM, Samuel Ortiz <sameo@linux.intel.com> wrote: > Hi Mike, > > A few comments on this driver: > > On Tue, Jul 27, 2010 at 01:52:52PM +0300, Mike Rapoport wrote: > >> +static int tps6586x_gpio_get(struct gpio_chip *gc, unsigned offset) >> +{ >> + struct tps6586x *tps6586x = container_of(gc, struct tps6586x, gpio); >> + uint8_t val; >> + int ret; >> + >> + ret = __tps6586x_read(tps6586x->client, TPS6586X_GPIOSET2, &val); >> + if (ret) >> + return ret; >> + >> + return !!(val & (1 << offset)); >> +} >> + >> + >> +static void tps6586x_gpio_set(struct gpio_chip *chip, unsigned offset, >> + int value) >> +{ >> + struct tps6586x *tps6586x = container_of(chip, struct tps6586x, gpio); >> + >> + __tps6586x_write(tps6586x->client, TPS6586X_GPIOSET2, >> + value << offset); >> +} >> + >> +static int tps6586x_gpio_output(struct gpio_chip *gc, unsigned offset, >> + int value) >> +{ >> + struct tps6586x *tps6586x = container_of(gc, struct tps6586x, gpio); >> + uint8_t val, mask; >> + >> + tps6586x_gpio_set(gc, offset, value); >> + >> + val = 0x1 << (offset * 2); >> + mask = 0x3 << (offset * 2); >> + >> + return tps6586x_update(tps6586x->dev, TPS6586X_GPIOSET1, val, mask); >> +} >> + >> +static void tps6586x_gpio_init(struct tps6586x *tps6586x, int gpio_base) >> +{ >> + int ret; >> + >> + if (!gpio_base) >> + return; >> + >> + tps6586x->gpio.owner = THIS_MODULE; >> + tps6586x->gpio.label = tps6586x->client->name; >> + tps6586x->gpio.dev = tps6586x->dev; >> + tps6586x->gpio.base = gpio_base; >> + tps6586x->gpio.ngpio = 4; >> + tps6586x->gpio.can_sleep = 1; >> + >> + /* FIXME: add handling of GPIOs as dedicated inputs */ >> + tps6586x->gpio.direction_output = tps6586x_gpio_output; >> + tps6586x->gpio.set = tps6586x_gpio_set; >> + tps6586x->gpio.get = tps6586x_gpio_get; >> + >> + ret = gpiochip_add(&tps6586x->gpio); >> + if (ret) >> + dev_warn(tps6586x->dev, "GPIO registration failed: %d\n", ret); >> +} > All the gpio bits here should be part of a separate patch adding an entry to > drivers/gpio/, with a Kconfig dependency on this MFD driver.
I remember David's comment about his preference to keep GPIO part of MFD devices in the drivers/mfd rather than drivers/gpio, that's why I've bundled the GPIO implementation with the MFD core driver.
>> +static int __devinit tps6586x_add_subdevs(struct tps6586x *tps6586x, >> + struct tps6586x_platform_data *pdata) >> +{ >> + struct tps6586x_subdev_info *subdev; >> + struct platform_device *pdev; >> + int i, ret = 0; >> + >> + for (i = 0; i < pdata->num_subdevs; i++) { >> + subdev = &pdata->subdevs[i]; >> + >> + pdev = platform_device_alloc(subdev->name, subdev->id); >> + >> + pdev->dev.parent = tps6586x->dev; >> + pdev->dev.platform_data = subdev->platform_data; >> + >> + ret = platform_device_add(pdev); >> + if (ret) >> + goto failed; >> + } >> + return 0; >> + >> +failed: >> + tps6586x_remove_subdevs(tps6586x); >> + return ret; >> +} > Although this one is correct, I'm wondering if you really have platforms > adding different subdevs for this chipset ?
For sure there's a platform using regulators and led drivers. I'm not sure about the other kind of sub-devices.
> >> +static int __devinit tps6586x_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) > For consistency sake, could you please rename this one to > tps6586x_i2c_probe() please ?
Ok, no problem.
> Cheers, > Samuel. > > -- > Intel Open Source Technology Centre > http://oss.intel.com/ > -- > 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/ >
-- Sincerely Yours, Mike. -- 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/
| |