Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Jonathan Cameron]

On Wed, 30 Aug 2023 15:15:12 +0800
杨明金 <magicyangmingjin@xxxxxxxxx> wrote:

> Jonathan Cameron <jic23@xxxxxxxxxx> 于2023年8月28日周一 23:56写道：
> >
Hi,

Please crop replies to relevant part only.  Hopefully I found it!


> > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > +{
> > > +     int ret = 0;
> > > +     u32 reg_read = 0;
> > > +
> > > +     if (data->pm_data.clk_regmap) {
> > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > +                                      data->pm_data.clk_reg_mask,
> > > +                                      data->pm_data.clk_reg_mask);
> > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > +             if (ret) {
> > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > +                     return ret;
> > > +             }
> > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);  
> >
> > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > for this?  
> 
> This register is used to vote to enable/disable the pmic 26m clk which
> is provided to modules like audio, typec and adc.
> Therefore, this clk cannot be disabled or enabled directly.

clk_enable() and friends support reference counted enable and disable
so I don't understand why this needs something unusual.


> 

> > > +static int sprd_adc_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device_node *np = pdev->dev.of_node;
> > > +     struct sprd_adc_data *sprd_data;
> > > +     const struct sprd_adc_variant_data *pdata;
> > > +     struct iio_dev *indio_dev;
> > > +     int ret;
> > > +
> > > +     pdata = of_device_get_match_data(&pdev->dev);  
> >
> > device_get_match_data()
> >
> >  
> > > +     if (!pdata) {
> > > +             dev_err(&pdev->dev, "No matching driver data found\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     sprd_data = iio_priv(indio_dev);
> > > +
> > > +     sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > +     if (!sprd_data->regmap) {
> > > +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     ret = of_property_read_u32(np, "reg", &sprd_data->base);  
> >
> > Even though some elements of this (of_hwspin...) don't have generic firmware
> > interfaces, I would prefer to see those from linux/property.h used
> > wherever possible.  It will take us a long time to make that a subsystem
> > wide change, but good not to have more unnecessary instances of device tree
> > specific property reading.  
> 
> Sorry, I don't understand what needs to be modified. Can you provide
> more information or give an example?
> Do you mean that the "reg"  property reading is unnecessary?

No.  Where possibly use
	device_property_read_u32(dev, "reg".. etc
and similar functions from
include/linux/property.h rather than device tree specific ones.
The generic property handling deals with various different types of firmware
without needing drivers to be aware of it.

Some elements that you need here do not have generic property handling so
for those you will need to continue using the of_ variants.
Note that this is to support long term move of everything to the generic
firmware framework.  Even if we drivers in IIO etc that are really device
tree only there are benefits for maintenance in using one framework
for all drivers. As some IIO drivers do support other firmware types
(ACPI for example) the generic version is the preferred choice.

Thanks,

Jonathan