Hello,

Thank you for the review of patches.

2014-10-31 10:42 GMT+03:00 Linus Walleij [off-list ref]:

On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
[off-list ref] wrote:

quoted

LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
several design issues (special bus instead of platform bus, doesn't use
mfd-core, etc).

Implement 'core' parts of locomo support as an mfd driver.

Signed-off-by: Dmitry Eremin-Solenikov <redacted>

(...)

quoted

+/* DAC send data */
+#define        M62332_SLAVE_ADDR       0x4e    /* Slave address  */
+#define        M62332_W_BIT            0x00    /* W bit (0 only) */
+#define        M62332_SUB_ADDR         0x00    /* Sub address    */
+#define        M62332_A_BIT            0x00    /* A bit (0 only) */
+
+/* DAC setup and hold times (expressed in us) */
+#define DAC_BUS_FREE_TIME      5       /*   4.7 us */
+#define DAC_START_SETUP_TIME   5       /*   4.7 us */
+#define DAC_STOP_SETUP_TIME    4       /*   4.0 us */
+#define DAC_START_HOLD_TIME    5       /*   4.7 us */
+#define DAC_SCL_LOW_HOLD_TIME  5       /*   4.7 us */
+#define DAC_SCL_HIGH_HOLD_TIME 4       /*   4.0 us */
+#define DAC_DATA_SETUP_TIME    1       /*   250 ns */
+#define DAC_DATA_HOLD_TIME     1       /*   300 ns */
+#define DAC_LOW_SETUP_TIME     1       /*   300 ns */
+#define DAC_HIGH_SETUP_TIME    1       /*  1000 ns */

(...)

It seems some DAC handling is part of the MFD driver, and we recently
discussed that MFD should not be doing misc stuff but mainly act as
arbiter and switching station.

Can you please move the DAC parts of the driver to
drivers/iio/dac?

The IIO DAC subsystem will likely add other goodies to
the driver for free and give a nice API to consumers.

I wanted this part to be as simple as possible. I will look into IIO
DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus
controlled through a register in LoCoMo device. One channel is used
for backlight,
other will be used for volume control. So (in theory) I can add the
following device
chain:  locomo -> i2c-locomo -> m62332 -> IIO DAC client.  However isn't that
quite an overkill for just backlight & volume control? Please advice me on this.


[skipped the irqdomain part - I will use them, thanks for the suggestion.]

quoted

+       /* Longtime timer */
+       writew(0, lchip->base + LOCOMO_LTINT);
+       /* SPI */
+       writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
+
+       writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);

That's a few magic numbers and calculation don't you think?

A comment stating what's going on would be helpful.

Unfortunately little is known here - these values are c&p from old 2.4
Lineo code.
This part is related to generating synchronization pulses for accessing
touchscreen.

-- 
With best wishes
Dmitry