Hi Peter,
On 09/12/2016 12:17 PM, Peter Rosin wrote:
> Hi!
>
> Sorry for the delay, other stuff interfered...
Who hasn't ?
>
> As I stated previously, we have an sx1502, and this seems /almost/ good.
> Comments inline.
>
> On 2016-09-06 14:56, Neil Armstrong wrote:
[..]
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt
>> @@ -0,0 +1,67 @@
>> +SEMTECH SX150x GPIO expander bindings
>> +
>> +Please refer to ../pinctrl/pinctrl-bindings.txt, gpio.txt, and
>
> pinctrl-bindings.txt, ../gpio/gpio.txt
>
Bad copy/paste, thanks !
>> +../interrupt-controller/interrupts.txt for generic information regarding
>> +pin controller, GPIO, and interrupt bindings.
>> +
>> +Required properties:
>> +- compatible: should be "semtech,sx1506q",
>> + "semtech,sx1508q",
[...]
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -761,16 +761,12 @@ config GPIO_PCF857X
>> platform-neutral GPIO calls.
>>
>> config GPIO_SX150X
>> - bool "Semtech SX150x I2C GPIO expander"
>> - depends on I2C=y
>> - select GPIOLIB_IRQCHIP
>> + bool "Semtech SX150x I2C GPIO expander (deprecated)"
>> + select PINCTRL_SX150X
>> default n
>> help
>> Say yes here to provide support for Semtech SX150-series I2C
>
> SX150x-series ???
Same bad copy/paste
>
>> - GPIO expanders. Compatible models include:
>> -
>> - 8 bits: sx1508q
>> - 16 bits: sx1509q
>> + GPIO expanders. The GPIO driver was replaced by a Pinctrl version.
>>
>> config GPIO_TPIC2810
>> tristate "TPIC2810 8-Bit I2C GPO expander"
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 2a035ed..ffd2f4c 100644
[...]
>> +static const struct sx150x_device_data sx1506q_device_data = {
>> + .model = SX150X_456,
>> + .reg_pullup = 0x05,
>> + .reg_pulldn = 0x07,
>> + .reg_dir = 0x03,
>> + .reg_data = 0x01,
>> + .reg_irq_mask = 0x09,
>> + .reg_irq_src = 0x0f,
>> + .reg_sense = 0x0d,
>> + .pri.x456 = {
>> + .reg_pld_mode = 0x21,
>> + .reg_pld_table0 = 0x23,
>> + .reg_pld_table1 = 0x25,
>> + .reg_pld_table2 = 0x27,
>> + .reg_pld_table3 = 0x29,
>> + .reg_pld_table4 = 0x2b,
>> + .reg_advance = 0xad,
>> + },
>> + .ngpios = 16,
>> + .pins = sx150x_16_pins,
>> + .npins = ARRAY_SIZE(sx150x_16_pins),
>
> I don't think the oscio pin is available here...
Thanks for the info, I'll check all the datasheets and clarify the differences.
>> +};
>> +
>> +static const struct sx150x_device_data sx1502q_device_data = {
>> + .model = SX150X_123,
>> + .reg_pullup = 0x02,
>> + .reg_pulldn = 0x03,
>> + .reg_dir = 0x01,
>> + .reg_data = 0x00,
>> + .reg_irq_mask = 0x05,
>> + .reg_irq_src = 0x08,
>> + .reg_sense = 0x07,
>> + .pri.x123 = {
>> + .reg_pld_mode = 0x10,
>> + .reg_pld_table0 = 0x11,
>> + .reg_pld_table1 = 0x12,
>> + .reg_pld_table2 = 0x13,
>> + .reg_pld_table3 = 0x14,
>> + .reg_pld_table4 = 0x15,
>> + .reg_advance = 0xad,
>> + },
>> + .ngpios = 8,
>> + .pins = sx150x_8_pins,
>> + .npins = ARRAY_SIZE(sx150x_8_pins),
>
> ...or here, which causes a problem because the pins are named gpio503
> etc when exporting in sysfs, instead of being based at gpio504 as they
> were with the old gpio-only driver. I simply subtracted 1 from the
> npins assignment for sx1502, but I think there should be separate
> sx150x_8_pins_osc and sx150x_16_pins_osc arrays. Or sx150x_9_pins and
> sx150x_17_pins. Or something.
>
Yes, let's keep it simple.
>> +};
>> +
>> +static s32 sx150x_i2c_write(struct i2c_client *client, u8 reg, u8 val)
>> +{
>> + s32 err = i2c_smbus_write_byte_data(client, reg, val);
>> +
>> + if (err < 0)
>> + dev_warn(&client->dev,
>> + "i2c write fail: can't write %02x to %02x: %d\n",
>> + val, reg, err);
>> + return err;
>> +}
>> +
[...]
>> +static int sx150x_reset(struct sx150x_pinctrl *pctl)
>> +{
>> + int err;
>> +
>
> Perhaps
>
> if (pctl->data->model != SX150X_789)
> return -ENOTSUPP;
>
> ???
>
Aww, another crappy copy/paste, I really need to recheck what I copied verbatim from the previous driver,
>> + err = i2c_smbus_write_byte_data(pctl->client,
>> + pctl->data->pri.x789.reg_reset,
>> + 0x12);
>> + if (err < 0)
>> + return err;
>> +
>> + err = i2c_smbus_write_byte_data(pctl->client,
>> + pctl->data->pri.x789.reg_reset,
>> + 0x34);
>> + return err;
>> +}
>> +
Thanks for the comments,
I'll push a v2 asap.
Neil
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
