On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
> control transceiver. We'll read it from internal Flash with address
> 0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
> MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
> M1, M2 as 1, 1, 0.
>
> Register mapping for output value:
> Port 0:
> M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
> Port 1:
> M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
> Port 2:
> M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
> Port 3:
> M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1
>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[email protected]>
> ---
> drivers/usb/serial/f81534.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index b2d10309c335..30b966d71ae8 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -56,6 +56,7 @@
> #define F81534_CUSTOM_NO_CUSTOM_DATA 0xff
> #define F81534_CUSTOM_VALID_TOKEN 0xf0
> #define F81534_CONF_OFFSET 1
> +#define F81534_CONF_GPIO_OFFSET 4
>
> #define F81534_MAX_DATA_BLOCK 64
> #define F81534_MAX_BUS_RETRY 20
> @@ -166,6 +167,23 @@ struct f81534_port_private {
> u8 phy_num;
> };
>
> +struct f81534_pin_data {
> + const u16 reg_addr;
> + const u16 reg_mask;
Should the mask not be u8?
> +};
> +
> +struct f81534_port_out_pin {
> + struct f81534_pin_data pin[3];
> +};
> +
> +/* Pin output value for M2/M1/M0(SD) */
> +static const struct f81534_port_out_pin f81534_port_out_pins[] = {
> + {{{0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
> + {{{0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
> + {{{0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
> + {{{0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },
Please use a space after { and before } consistently.
> +};
> +
> static int f81534_logic_to_phy_port(struct usb_serial *serial,
> struct usb_serial_port *port)
> {
> @@ -271,6 +289,22 @@ static int f81534_get_register(struct usb_serial *serial, u16 reg, u8 *data)
> return status;
> }
>
> +static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
> + u8 mask, u8 data)
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81534_get_register(serial, reg, &tmp);
> + if (status)
> + return status;
> +
> + tmp &= ~mask;
> + tmp |= (mask & data);
> +
> + return f81534_set_register(serial, reg, tmp);
> +}
> +
> static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
> u8 data)
> {
> @@ -1299,6 +1333,37 @@ static void f81534_lsr_worker(struct work_struct *work)
> dev_warn(&port->dev, "read LSR failed: %d\n", status);
> }
>
> +static int f81534_set_port_output_pin(struct usb_serial_port *port)
> +{
> + struct f81534_serial_private *serial_priv;
> + struct f81534_port_private *port_priv;
> + struct usb_serial *serial;
> + const struct f81534_port_out_pin *pins;
> + int status;
> + int i;
> + u8 value;
> + u8 idx;
> +
> + serial = port->serial;
> + serial_priv = usb_get_serial_data(serial);
> + port_priv = usb_get_serial_port_data(port);
> +
> + idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
> + value = serial_priv->conf_data[idx];
> + pins = &f81534_port_out_pins[port_priv->phy_num];
> +
> + for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
> + status = f81534_set_mask_register(serial,
> + pins->pin[i].reg_addr, pins->pin[i].reg_mask,
> + value & BIT(i) ? pins->pin[i].reg_mask : 0);
> + if (status)
> + return status;
> + }
You're using 24 (get or set) accesses to update these three registers
here. Why not read them out (if necessary), determine their new values
and then write them back when done instead?
> +
> + dev_info(&port->dev, "Output pin (M0/M1/M2): %d\n", value);
Use dev_dbg here.
Johan
Hi Johan,
Johan Hovold 於 2017/12/19 上午 12:06 寫道:
> On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
>> +static int f81534_set_port_output_pin(struct usb_serial_port *port)
>> +{
>> + struct f81534_serial_private *serial_priv;
>> + struct f81534_port_private *port_priv;
>> + struct usb_serial *serial;
>> + const struct f81534_port_out_pin *pins;
>> + int status;
>> + int i;
>> + u8 value;
>> + u8 idx;
>> +
>> + serial = port->serial;
>> + serial_priv = usb_get_serial_data(serial);
>> + port_priv = usb_get_serial_port_data(port);
>> +
>> + idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
>> + value = serial_priv->conf_data[idx];
>> + pins = &f81534_port_out_pins[port_priv->phy_num];
>> +
>> + for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
>> + status = f81534_set_mask_register(serial,
>> + pins->pin[i].reg_addr, pins->pin[i].reg_mask,
>> + value & BIT(i) ? pins->pin[i].reg_mask : 0);
>> + if (status)
>> + return status;
>> + }
>
> You're using 24 (get or set) accesses to update these three registers
> here. Why not read them out (if necessary), determine their new values
> and then write them back when done instead?
>
In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
but some register will read/write more than once. Should I change the
code from port_probe() to attach() and re-write it as:
1: read the 3 register
2: change them will 12 pin desire value
3: write it back
Is it ok?
Thanks
--
With Best Regards,
Peter Hong
On Thu, Dec 21, 2017 at 05:49:45PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
>
> Johan Hovold 於 2017/12/19 上午 12:06 寫道:
> > On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> >> +static int f81534_set_port_output_pin(struct usb_serial_port *port)
> >> +{
> >> + struct f81534_serial_private *serial_priv;
> >> + struct f81534_port_private *port_priv;
> >> + struct usb_serial *serial;
> >> + const struct f81534_port_out_pin *pins;
> >> + int status;
> >> + int i;
> >> + u8 value;
> >> + u8 idx;
> >> +
> >> + serial = port->serial;
> >> + serial_priv = usb_get_serial_data(serial);
> >> + port_priv = usb_get_serial_port_data(port);
> >> +
> >> + idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
> >> + value = serial_priv->conf_data[idx];
> >> + pins = &f81534_port_out_pins[port_priv->phy_num];
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
> >> + status = f81534_set_mask_register(serial,
> >> + pins->pin[i].reg_addr, pins->pin[i].reg_mask,
> >> + value & BIT(i) ? pins->pin[i].reg_mask : 0);
> >> + if (status)
> >> + return status;
> >> + }
> >
> > You're using 24 (get or set) accesses to update these three registers
> > here. Why not read them out (if necessary), determine their new values
> > and then write them back when done instead?
> >
>
> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
> but some register will read/write more than once. Should I change the
> code from port_probe() to attach() and re-write it as:
> 1: read the 3 register
> 2: change them will 12 pin desire value
> 3: write it back
> Is it ok?
Do you expect these pins to ever be changed after probe? If not, then
perhaps it can be moved to attach(), but otherwise I guess they should
be set at port_probe(). By using shadow registers, you should be able to
reduce the number of device accesses, but perhaps it's not worth the
complexity.
Do you have a rough idea about how long these register updates take? I
was just worried that these changes will add up to really long probe
times.
Thanks,
Johan
Hi Johan,
>> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
>> but some register will read/write more than once. Should I change the
>> code from port_probe() to attach() and re-write it as:
>> 1: read the 3 register
>> 2: change them will 12 pin desire value
>> 3: write it back
>> Is it ok?
>
> Do you expect these pins to ever be changed after probe? If not, then
> perhaps it can be moved to attach(), but otherwise I guess they should
> be set at port_probe(). By using shadow registers, you should be able to
> reduce the number of device accesses, but perhaps it's not worth the
> complexity.
>
> Do you have a rough idea about how long these register updates take? I
> was just worried that these changes will add up to really long probe
> times.
>
I had measured the time of the loop in f81534_set_port_output_pin() via
getnstimeofday() with 685.410 ~ 3681.682us per port, but normally with
600~800us per port. So I prefer remain the current method of
f81534_set_port_output_pin(). Is it ok?
Thanks
--
With Best Regards,
Peter Hong
On Tue, Jan 02, 2018 at 11:24:26AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
>
> >> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
> >> but some register will read/write more than once. Should I change the
> >> code from port_probe() to attach() and re-write it as:
> >> 1: read the 3 register
> >> 2: change them will 12 pin desire value
> >> 3: write it back
> >> Is it ok?
> >
> > Do you expect these pins to ever be changed after probe? If not, then
> > perhaps it can be moved to attach(), but otherwise I guess they should
> > be set at port_probe(). By using shadow registers, you should be able to
> > reduce the number of device accesses, but perhaps it's not worth the
> > complexity.
> >
> > Do you have a rough idea about how long these register updates take? I
> > was just worried that these changes will add up to really long probe
> > times.
> >
>
> I had measured the time of the loop in f81534_set_port_output_pin() via
> getnstimeofday() with 685.410 ~ 3681.682us per port, but normally with
> 600~800us per port. So I prefer remain the current method of
> f81534_set_port_output_pin(). Is it ok?
That should be fine. Thanks for verifying.
Johan