>>>>> "Jean" == Jean Delvare <khali@xxxxxxxxxxxx> writes:
Hi,
Jean> I've fixed two remaining minor things myself, and your patch is
Jean> ready to be applied now, except for one issue I just noticed. See
Jean> below.
Great.
>> +gpio-i2cmux uses the platform bus, so you need to provide a struct
>> +platform_device with the platform_data pointing to a struct
>> +gpio_i2cmux_platform_data with the I2C adapter number of the master
>> +bus, the number bus segments to create and the GPIO pins used
Jean> "of" got lost in the battle, I've added it back.
Ahh yes, thanks.
>> + for (i = 0; i < pdata->n_gpios; i++) {
>> + ret = gpio_request(pdata->gpios[i], "gpio-i2cmux");
>> + if (ret)
>> + goto err_request_gpio;
>> + gpio_direction_output(pdata->gpios[i], pdata->idle & (1 << i));
Jean> This looks wrong if pdata->idle == GPIO_I2CMUX_NO_IDLE. I think we want
Jean> something along the lines of:
Jean> unsigned initial_state;
Jean> initial_state = pdata->idle == GPIO_I2CMUX_NO_IDLE ?
Jean> pdata->values[0] : pdata->idle;
Jean> (...)
Jean> gpio_direction_output(pdata->gpios[i], initial_state & (1 << i));
Jean> What do you think? An alternative is to leave the direction
Jean> uninitialized and hope it's already OK, but I'm not sure how realistic
Jean> this is.
Yeah, going for values[0] is probably the most sensible thing to do if
we don't have an idle state. Relying on the firmware to setup the right
direction in advance is imho not that nice.
Will you do the change yourself, or should I resend the patch?
>> + if (pdata->idle != GPIO_I2CMUX_NO_IDLE)
>> + deselect = gpiomux_deselect;
We could probably move this up and add the assignment of init_state to
this contional, instead of testing against GPIO_I2CMUX_NO_IDLE twice.
--
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
