Thank you Alexander for taking time, Well I'm not an expert, so correct me If I'm wrong: but if I'm using the clock node with fixed-clock property, therefore this function will be called : ret = clk_prepare_enable(s->clk) That the part that I don't like, or I don't need. I don't want the processor to create a clock and use ressources for an unused clock. The clock node is good when you want your processor to generate the clock, but in my case, I don't need it. "Are you sure max310xs[k] will be NULL after remove driver?" I will work on it. I was inspired by this : http://lxr.free-electrons.com/source/drivers/tty/serial/max3100.c?v=4.1 , but yes module_init will be better. Do you know how can I create one patch that look at the difference between the master branch and 4 commit behind ? Micka, Le jeu. 7 avr. 2016 à 21:54, Alexander Shiyan <shc_work@xxxxxxx> a écrit : > > On Thu, 7 Apr 2016 15:44:35 +0200 > Micka <mickamusset@xxxxxxxxx> wrote: > > Hello. > > > I can't config the mail server .... on git, I don't have any mail > > server .... i'm using gmail. > > > > I prefer to keep the property clock-frequency: Because that way i > > don't need a node clock and I don't need to initialize a clock in the > > processor ( ret = clk_prepare_enable(s->clk) ). > > I still can not understand why use a separate property to specify the frequency > Why you can not use "fixed-clock" node and just specify the type "osc" or "xtal"? > > > I added the property no-gpio in case u don't want to use the gpio on > > the max310x. > > You can re-use existing property "gpio-controller" for this, like as: > if (!of_find_property(np, "gpio-controller", NULL)) > ... > > > Now, I'm concerned about two max310x working at the same time. What > > will happen ? There is no mutex to protect that in the driver. I hope > > that the spi driver will handle that problem. What do you think > > Alexander. > > This is a work for SPI core. > > ... > > diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c > > index 182549f..328bbe0 100644 > > --- a/drivers/tty/serial/max310x.c > > +++ b/drivers/tty/serial/max310x.c > ... > > struct max310x_port { > > - struct uart_driver uart; > > + int nr; > > + int index; > > +// struct uart_driver uart; > > Linux style for comments is the C89 "/* ... */" style. > > ... > > static u8 max310x_port_read(struct uart_port *port, u8 reg) > > { > > struct max310x_port *s = dev_get_drvdata(port->dev); > > unsigned int val = 0; > > - > > regmap_read(s->regmap, port->iobase + reg, &val); > > ??? > > ... > > @@ -1081,46 +1094,87 @@ static int > > max310x_gpio_direction_output(struct gpio_chip *chip, > > static int max310x_probe(struct device *dev, struct max310x_devtype *devtype, > > struct regmap *regmap, int irq, unsigned long flags) > > { > > - int i, ret, fmin, fmax, freq, uartclk; > > - struct clk *clk_osc, *clk_xtal; > > - struct max310x_port *s; > > - bool xtal = false; > > + int j,k; > > + int i, ret, fmin, fmax, freq=0, uartclk; > > + struct clk *clk_osc, *clk_xtal; > > + struct max310x_port *s; > > + bool xtal = false; > > + int retval = 0; > > > > if (IS_ERR(regmap)) > > return PTR_ERR(regmap); > > + // did we registered the driver ? > > + if (!uart_driver_registered) { > > + // no, let's do it : > > + // set the flag to 1 for the next time. > > + uart_driver_registered = 1; > > + // register the driver : > > + retval = uart_register_driver(&max310x_uart_driver); > > + if (retval) { > > + // something goes wrong : > > + return retval; > > + } > > + number_of_uarts=0; > > + } > > + // find a free spot in the array : > > + for (k = 0; k < MAX_MAX310X; k++) > > + if (!max310xs[k]) > > + break; > > Are you sure max310xs[k] will be NULL after remove driver? > > > + // test if we reach the end of the array > > + if (k == MAX_MAX310X) { > > + // no more space available > > + dev_warn(dev, "too many MAX310X chips\n"); > > + return -ENOMEM; > > + } > > Hmm... I think moving uart_register_driver in the "module_init" initcall > will be more elegant, like: > static int __init max310x_init(void) > { > ret = uart_register_driver(&max310x_uart_driver); > if (!ret) { > ret = spi_register_driver(&max310x_spi_driver); > ... > } > > return ret; > } > module_init(max310x_init); > > ... > > - clk_osc = devm_clk_get(dev, "osc"); > > - clk_xtal = devm_clk_get(dev, "xtal"); > > - if (!IS_ERR(clk_osc)) { > > - s->clk = clk_osc; > > - fmin = 500000; > > - fmax = 35000000; > > - } else if (!IS_ERR(clk_xtal)) { > > - s->clk = clk_xtal; > > - fmin = 1000000; > > - fmax = 4000000; > > - xtal = true; > > - } else if (PTR_ERR(clk_osc) == -EPROBE_DEFER || > > - PTR_ERR(clk_xtal) == -EPROBE_DEFER) { > > - return -EPROBE_DEFER; > > - } else { > > - dev_err(dev, "Cannot get clock\n"); > > - return -EINVAL; > > + // save the index of the array > > + s->index=k; > > + // test if we can get the frequency of the clock from the property : > > + if (of_find_property(dev->of_node, "clock-frequency", NULL)){ > > + // cool, let's get it : > > + of_property_read_u32(dev->of_node, "clock-frequency", &freq); > > + } > > + // check if the frequency is indicated : > > + if(freq!=0){ > > + // yes ! we don't need to build a clock in the processor : > > + fmin = 1000000; > > + fmax = 4000000; > > + xtal = 0; > > + }else{ > > + // we need an internal clock > > + clk_osc = devm_clk_get(dev, "osc"); > > + clk_xtal = devm_clk_get(dev, "xtal"); > > + if (!IS_ERR(clk_osc)) { > > + s->clk = clk_osc; > > + fmin = 500000; > > + fmax = 35000000; > > + } else if (!IS_ERR(clk_xtal)) { > > + s->clk = clk_xtal; > > + fmin = 1000000; > > + fmax = 4000000; > > + xtal = true; > > + } else if (PTR_ERR(clk_osc) == -EPROBE_DEFER || > > + PTR_ERR(clk_xtal) == -EPROBE_DEFER) { > > + return -EPROBE_DEFER; > > + } else { > > + dev_err(dev, "Cannot get clock\n"); > > + return -EINVAL; > > + } > > + ret = clk_prepare_enable(s->clk); > > + if (ret) > > + return ret; > > + freq = clk_get_rate(s->clk); > > } > > - > > - ret = clk_prepare_enable(s->clk); > > - if (ret) > > - return ret; > > - > > - freq = clk_get_rate(s->clk); > > /* Check frequency limits */ > > if (freq < fmin || freq > fmax) { > > ret = -ERANGE; > > In any case please make clock & gpio changes as a separate part of patch. > > ... > > @@ -1133,23 +1187,27 @@ static int max310x_probe(struct device *dev, > > struct max310x_devtype *devtype, > > > > /* Check device to ensure we are talking to what we expect */ > > ret = devtype->detect(dev); > > - if (ret) > > + if (ret){ > > goto out_clk; > > - > > + } > > ?? > > > for (i = 0; i < devtype->nr; i++) { > > unsigned int offs = i << 5; > > > > - /* Reset port */ > > - regmap_write(s->regmap, MAX310X_MODE2_REG + offs, > > - MAX310X_MODE2_RST_BIT); > > - /* Clear port reset */ > > - regmap_write(s->regmap, MAX310X_MODE2_REG + offs, 0); > > - > > /* Wait for port startup */ > > - do { > > - regmap_read(s->regmap, > > - MAX310X_BRGDIVLSB_REG + offs, &ret); > > - } while (ret != 0x01); > > + do{ > > + /* Reset port */ > > + regmap_write(s->regmap, MAX310X_MODE2_REG + offs, > > MAX310X_MODE2_RST_BIT); > > + /* Clear port reset */ > > + regmap_write(s->regmap, MAX310X_MODE2_REG + offs, 0); > > + > > + j=0; > > + ret=0; > > + for(j=0;(j<5 && ret!=0x01);j++){ > > + regmap_read(s->regmap, > > + MAX310X_BRGDIVLSB_REG + offs, &ret); > > + } > > + // did the port start ? if not, restart the process ... maybe we > > need to stop at some point the wait .... > > + }while(ret!=0x01); > > for(j=0;(j<5... Not a good idea. > If you want to make an infinite loop protect, let's use time_is_after_jiffies(). > > ... > > Thanks. > > -- > Alexander Shiyan <shc_work@xxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
