Hi Stephen, Thanks for the review! On Saturday, June 02, 2018 08:13 AM, Stephen Boyd wrote:
Quoting Daniel Mack (2018-05-25 11:20:58)
+enum { + MAX9485_FS_12KHZ = 0 << 0, + MAX9485_FS_32KHZ = 1 << 0, + MAX9485_FS_44_1KHZ = 2 << 0, + MAX9485_FS_48KHZ = 3 << 0, +}; + +enum { + MAX9485_SCALE_256 = 0 << 2, + MAX9485_SCALE_384 = 1 << 2, + MAX9485_SCALE_768 = 2 << 2, +};Any reason to be an enum? Maybe they can just be #defines.
No particular reason, but an enum shows these values are grouped together. But I can also turn them into defines, it's just a matter of taste I guess.
+struct max9485_driver_data { + struct clk *xclk; + struct i2c_client *client; + u8 reg_value; + unsigned long clkout_rate; + struct regulator *supply; + struct gpio_desc *reset_gpio; + struct max9485_clk_hw hw[MAX9485_NUM_CLKS]; + + struct { + struct clk_hw_onecell_data data; + struct clk_hw *hw[MAX9485_NUM_CLKS];I don't quite understand this one. There's already an array of clk_hw pointers inside of clk_hw_onecell_data so just use that? But then there's also an array of max9485_clk_hw structures, so maybe roll your own clk_hw getter function?
Ah, yes. The latter makes most sense. I was somehow stuck with the idea that the onecell helper makes sense here. Thanks for the heads-up.
+static int max9485_update_bits(struct max9485_driver_data *drvdata, + u8 mask, u8 value) +{ + int ret; + + drvdata->reg_value &= ~mask; + drvdata->reg_value |= value; + + dev_dbg(&drvdata->client->dev, + "updating mask %02x value %02x -> %02x\n", + mask, value, drvdata->reg_value); + + ret = i2c_master_send(drvdata->client, + &drvdata->reg_value, + sizeof(drvdata->reg_value));Maybe use a regmap instead? Then you could use regmap_update_bits() in the places you need it.
I wish, but nope, regmap doesn't work for devices that don't actually have registers, like this one. There's no extra byte for a register address or such, just one plain byte of payload.
[...]
+ return -ENOMEM; + + drvdata->xclk = devm_clk_get(dev, "xclk"); + if (IS_ERR(drvdata->xclk)) + return PTR_ERR(drvdata->xclk); + + freq = clk_get_rate(drvdata->xclk); + if (freq != MAX9485_INPUT_FREQ) { + dev_err(dev, "Illegal xclk frequency of %ld\n", freq); + return -EINVAL; + }Is this necessary? Seems pretty useless to do everywhere to make sure that someone didn't design the hardware incorrectly. It would be better to return the rate of the parent "xclk" by not having a recalc_rate hook and relying on the parent's rate.
Hmm, but the hardware doesn't work if the clock is anything else than 27.0MHz. But I'll remove the check no problem. After all, we don't check the regulator voltages either.
I've addressed all other comments. Will resend v3. Thanks again! Daniel -- 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
